feat(base-cluster/ingress-nginx): use risk-level Critical when annotations are enabled#1417
Conversation
WalkthroughThe changes modify the NGINX ingress configuration in the Helm chart by adding a conditional setting named Changes
Sequence Diagram(s)sequenceDiagram
participant V as .Values
participant R as Helm Renderer
participant I as NGINX Controller
V->>R: Provide ingress.allowNginxConfigurationSnippets flag
alt Flag is True
R->>I: Render annotations-risk-level = "Critical"
else Flag is False
R->>I: Omit annotations-risk-level
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
charts/base-cluster/values.schema.json (1)
1338-1342: New Annotations Risk Level Property Added.The addition of the
annotationsRiskLevelproperty—with its type set to string, enumerated values ["Critical", "High", "Medium", "Low"], and an informative description referencing the Kubernetes documentation—is clear and directly aligns with the PR objectives. This will allow users to control annotation risk levels in a granular way.charts/base-cluster/templates/ingress/nginx.yaml (1)
43-45: Conditional Rendering of Annotations-Risk-Level in Template.The conditional block correctly checks for the presence of
.Values.ingress.annotationsRiskLevelbefore rendering theannotations-risk-levelconfiguration. For improved consistency and to guard against potential YAML parsing issues (e.g., if a future risk level value contains special characters or spaces), consider applying thequotefunction, as in:annotations-risk-level: {{ . | quote }}This is a minor nitpick that ensures robust output formatting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/base-cluster/templates/ingress/nginx.yaml(1 hunks)charts/base-cluster/values.schema.json(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check licenses
- GitHub Check: generateDiffCommentBody
- GitHub Check: lint helm chart (base-cluster)
🤖 I have diffed this beep boop"/$namespace/$kind/$name.yaml" for normal resources
|
94d468b to
b43ab74
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
charts/base-cluster/templates/ingress/nginx.yaml (2)
22-22: Clarify Double Negation in allowSnippetAnnotationsThe expression using
| not | noton.Values.ingress.annotationsRiskLevelis effectively coercing its value to a boolean. While this pattern ensures that a defined (non-empty) risk level becomestrue, its intent may not be immediately clear. Consider adding an inline comment to explain that this conversion enables snippet annotations when any valid risk level is provided.
43-45: Verify and Document Conditional Rendering for annotations-risk-levelThe
withblock that conditionally rendersannotations-risk-levelis straightforward and correctly limits the configuration to cases where.Values.ingress.annotationsRiskLevelis set. For added clarity, it may be beneficial to note that the acceptable string values are defined in the JSON schema (i.e. "Critical", "High", "Medium", "Low"). This can help maintain consistency and aid future maintainers in understanding the linkage between configuration and schema.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
charts/base-cluster/templates/ingress/nginx.yaml(2 hunks)charts/base-cluster/values.schema.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/base-cluster/values.schema.json
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: check licenses
- GitHub Check: generateDiffCommentBody
- GitHub Check: lint helm chart (base-cluster)
…tions are enabled
b43ab74 to
075ab4e
Compare
🤖 I have created a release *beep* *boop* --- ## [8.0.0](base-cluster-v7.2.1...base-cluster-v8.0.0) (2025-05-27) ### ⚠ BREAKING CHANGES * **base-cluster/ingress:** add option traefik for ingress controller and make it default ([#1420](#1420)) * **base-cluster/monitoring:** migrate promtail to alloy ([#1347](#1347)) ### Features * **base-cluster/ingress-nginx:** use risk-level Critical when annotations are enabled ([#1417](#1417)) ([a9d8ef2](a9d8ef2)) * **base-cluster/ingress:** add option traefik for ingress controller and make it default ([#1420](#1420)) ([f62b197](f62b197)) * **base-cluster/ingress:** rename oauth-proxies to have a clean name ([#1434](#1434)) ([27a28d5](27a28d5)) * **base-cluster/monitoring:** migrate promtail to alloy ([#1347](#1347)) ([24db445](24db445)) * **base-cluster/monitoring:** rename alloy to be a generic name ([#1433](#1433)) ([3f5826a](3f5826a)) ### Bug Fixes * **base-cluster/cert-manager:** metrics collection ([#1397](#1397)) ([71e1189](71e1189)) * **base-cluster/rbac:** *RoleBindings should always be prefixed to avoid collision ([#1484](#1484)) ([75de246](75de246)) ### Miscellaneous Chores * **base-cluster/monitoring:** remove deprecated plugin ([#1478](#1478)) ([ee22df5](ee22df5)) * **base-cluster:** formatting ([#1424](#1424)) ([853f146](853f146)) * **base-cluster:** pin all versions ([#1447](#1447)) ([ec8a430](ec8a430)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Added support for the Traefik ingress controller, now recommended and available as an alternative to NGINX. - Updated ingress configuration to use a new provider option, allowing selection between "nginx", "traefik", or "none". - Expanded documentation with detailed migration guides and ingress controller comparisons. - **Breaking Changes** - Traefik is now the default ingress controller; configuration for disabling ingress has changed. - Monitoring stack migrated from promtail to alloy; Loki deployment updated and namespace changed. - Deprecated properties and configuration options removed or replaced. - **Bug Fixes** - Improved metrics collection in cert-manager. - Resolved RBAC role binding name collisions. - **Chores** - Updated container image versions and removed deprecated plugins. - Reformatted and reorganized chart metadata and documentation for clarity. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
This sets
annotations-risk-levelto Critical when nginx configuration snippets are enabled, restoring the default setting from before ingress-nginx release 1.12.Summary by CodeRabbit