-
Notifications
You must be signed in to change notification settings - Fork 71
feat: add option to specify the clusterIP of OpenFGA service #264
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdded optional Service clusterIP templating in Helm and set default service.clusterIP to None in values, making the Service headless. Minor port name formatting adjustment. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1776c38 to
e5353b6
Compare
e5353b6 to
2b57cfc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/openfga/templates/service.yaml(1 hunks)charts/openfga/values.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: lint-test
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Anything blocking for merging this and release a new version of the helm chart? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Khayet - approving!
(sorry for the delays)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request adds support for configuring the clusterIP field of the OpenFGA Kubernetes service, enabling users to run OpenFGA as a headless service for client-side load balancing of gRPC connections. The implementation allows users to specify service.clusterIP in their values configuration, which is then applied to the service manifest.
Key Changes:
- Added conditional rendering of clusterIP field in service.yaml template
- Fixed trailing whitespace in the http port name definition
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| {{- if .Values.service.clusterIP }} | ||
| clusterIP: {{ .Values.service.clusterIP }} | ||
| {{- end }} |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The service template references .Values.service.clusterIP but this field has not been added to the values.yaml file. Users will not be able to configure this setting without adding it to the values.yaml file. Add a clusterIP field under the service section in values.yaml with appropriate documentation.
| {{- if .Values.service.clusterIP }} | ||
| clusterIP: {{ .Values.service.clusterIP }} | ||
| {{- end }} |
Copilot
AI
Dec 11, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new clusterIP field should be documented in the values.schema.json file to provide schema validation and documentation for users. Add a schema definition for the clusterIP property under the service object in values.schema.json, including a description that explains it can be set to "None" for headless services or a specific IP address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Khayet I agree with the bot here, it would be nice to add this to values.schema.json so that it is more visible to users - would you mind doing that?
Adds the option to specify the ClusterIP of the OpenFGA service. This allows running OpenFGA as a "headless service" (see https://kubernetes.io/docs/concepts/services-networking/service/#headless-services).
What problem is being solved?
We want to run OpenFGA as a headless service, so we can do client-side load balancing of our gRPC connection (which requires discovering the IPs of the individual replicas).
How is it being solved?
Adds
service.clusterIPentry to values.yaml and sets the service specification accordingly.What changes are made to solve it?
clusterIPsetting in values.yamlReferences
Review Checklist
mainSummary by CodeRabbit