Add nginx-ingress-controller option #1022
Conversation
WalkthroughThe changes involve updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IngressController
participant BackendService
User->>IngressController: Request Ingress
IngressController->>BackendService: Forward Request
BackendService-->>IngressController: Response
IngressController-->>User: Return Response
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
build/charts/yorkie-cluster/values.yaml (1)
63-65: LGTM! Consider adding more nginx-specific configuration options.The addition of the
nginxconfiguration block aligns well with the PR objective of introducing nginx-ingress-controller support. Setting it toenabled: falseby default is a good practice.Consider expanding the
nginxblock with additional configuration options that might be specific to nginx-ingress-controller. This could include settings like:nginx: enabled: false annotations: {} config: {} # Add any other nginx-specific settings hereThis would provide more flexibility for users who choose to enable the nginx ingress controller.
build/charts/yorkie-cluster/templates/istio/ingress.yaml (3)
34-38: LGTM! Consider adding a comment for clarityThe addition of NGINX-specific annotations is correct and aligns with the PR objective. The configuration looks good.
Consider adding a brief comment explaining the purpose of the rewrite-target annotation for better maintainability. For example:
annotations: # Rewrite all incoming requests to the root path nginx.ingress.kubernetes.io/rewrite-target: / kubernetes.io/ingress.class: "nginx"
44-47: LGTM! Consider adding a comment for clarityThe addition of conditional logic for handling ingress hosts is a good improvement, providing flexibility in different deployment scenarios.
Consider adding a brief comment explaining the purpose of this conditional block for better maintainability. For example:
{{ if .Values.ingress.hosts.enabled }} - host: {{ .Values.ingress.hosts.apiHost }} http: {{ else }} # Default configuration when no specific host is set - http: {{ end }}
Line range hint
1-56: Overall, the changes look good and align with the PR objectivesThe modifications to the ingress.yaml file successfully introduce support for the NGINX ingress controller while maintaining compatibility with existing AWS ALB and NCP ALB configurations. The conditional logic for naming and annotations provides the necessary flexibility for different deployment scenarios.
A few minor suggestions were made to improve code clarity and fix indentation issues. These changes will enhance the maintainability of the file.
If you'd like assistance implementing any of the suggested improvements or have any questions about the review, please let me know. I'm here to help!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- build/charts/yorkie-cluster/templates/istio/ingress.yaml (2 hunks)
- build/charts/yorkie-cluster/values.yaml (1 hunks)
🧰 Additional context used
🪛 yamllint
build/charts/yorkie-cluster/templates/istio/ingress.yaml
[error] 5-5: syntax error: expected , but found ''
(syntax)
🔇 Additional comments (2)
build/charts/yorkie-cluster/values.yaml (2)
Line range hint
1-74: Overall, the changes align well with the PR objectives.The addition of the
nginxconfiguration block in theingresssection successfully introduces support for the nginx-ingress-controller, which was the main goal of this PR. The changes maintain the existing structure of the file while adding the necessary flexibility for users to choose between different ingress controllers.A few points to consider:
- The nginx configuration could be expanded to include more specific options when enabled.
- The purpose of the new
namefield in theratelimitsection needs clarification.These changes enhance the ingress configuration options available in the Yorkie cluster, providing more flexibility as intended. Good job on maintaining backwards compatibility by keeping the existing configurations intact.
Line range hint
69-69: Please clarify the purpose of the newnamefield in theratelimitsection.The addition of
name: ratelimitin theratelimitsection is noted. While this provides a way to identify the ratelimit configuration, it's not immediately clear how this name will be used or why it was added.Could you please explain the intended use of this new
namefield? Is it required for the nginx-ingress-controller configuration, or does it serve another purpose?To help understand the context, let's check if this
namefield is used elsewhere in the chart:✅ Verification successful
Verified:
name: ratelimitis correctly implemented.The
name: ratelimitfield is consistently referenced across multiple templates, ensuring uniform configuration for the ratelimit component.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of 'ratelimit' as a name or reference rg --type yaml 'name:\s*ratelimit' build/charts/yorkie-cluster rg --type yaml 'ratelimit\.name' build/charts/yorkie-clusterLength of output: 1620
| {{ if .Values.ingress.nginx.enabled }} | ||
| name: ingress-nginx | ||
| {{ else }} | ||
| name: {{ .Values.yorkie.name }} | ||
| {{ end }} |
There was a problem hiding this comment.
Fix indentation for consistent YAML structure
The conditional block for naming has inconsistent indentation, which could lead to YAML parsing errors. Please adjust the indentation to ensure all lines are properly aligned.
Apply this diff to fix the indentation:
{{ if .Values.ingress.nginx.enabled }}
- name: ingress-nginx
- {{ else }}
+name: ingress-nginx
+{{ else }}
name: {{ .Values.yorkie.name }}
- {{ end }}
+{{ end }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{ if .Values.ingress.nginx.enabled }} | |
| name: ingress-nginx | |
| {{ else }} | |
| name: {{ .Values.yorkie.name }} | |
| {{ end }} | |
| {{ if .Values.ingress.nginx.enabled }} | |
| name: ingress-nginx | |
| {{ else }} | |
| name: {{ .Values.yorkie.name }} | |
| {{ end }} |
🧰 Tools
🪛 yamllint
[error] 5-5: syntax error: expected , but found ''
(syntax)
hackerwins
changed the title
| @@ -1,7 +1,11 @@ | |||
| apiVersion: networking.k8s.io/v1 | |||
| kind: Ingress | |||
| metadata: | |||
| {{ if .Values.ingress.nginx.enabled }} | |||
| name: ingress-nginx | |||
There was a problem hiding this comment.
Looks like ingress name is just ingress-nginx while we uses {{ .Values.yorkie.name }} for previous settings.
What this PR does / why we need it:
Previously, our AWS-based setup only supported ALB ingresses via the
alb-ingress-controller. This change enables the use of Nginx ingress
controller as an alternative, particularly beneficial for self-hosted
clusters. Users can now choose between ALB and Nginx when configuring
ingresses, providing more flexibility in ingress management.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
nginxconfiguration block in the ingress settings.namefield.Bug Fixes