Update Flux Operator (v0.26.0) - #1302
Kingdon Barrett (kingdonb) wants to merge 2 commits into
Conversation
Signed-off-by: Kingdon B <[email protected]>
Signed-off-by: Kingdon B <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello Kingdon Barrett (@kingdonb), I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily focuses on upgrading the Flux Operator and its associated Flux Instance Helm charts to version v0.26.0. Key changes include updating chart metadata, adjusting default resource allocations for the operator, enhancing schema validation for chart values, and introducing a new healthcheck mechanism for Flux instances to ensure successful deployments.
Highlights
- Flux Operator Chart Update: The
flux-operatorHelm chart has been updated to versionv0.26.0, including a bump in theappVersionandversionfields inChart.yamlandREADME.md. - Flux Operator Resource Limits Adjustment: The default CPU resource limit for the Flux Operator has been increased from
1000mto2000minvalues.yaml. - Flux Operator Helm Chart Schema Enhancements: The
values.schema.jsonfor theflux-operatorchart has been significantly updated to include more explicit type definitions (type: object,type: array) andrequiredfields for various parameters, improving schema validation. - Flux Instance Chart Update: The
flux-instanceHelm chart has also been updated to versionv0.26.0, with corresponding version bumps inChart.yamlandREADME.md. - New Healthcheck Feature for Flux Instance: A new healthcheck feature has been introduced for the
flux-instancechart. This includes new Helm templates (healthcheck.yaml,serviceaccount.yaml) to deploy aJobthat performs post-install and post-upgrade health checks, along with comprehensive configuration options invalues.yamlandvalues.schema.json.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | Gemini (@gemini-code-assist) | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request updates the Flux Operator and related charts to version v0.26.0. The changes include version bumps, updated resource limits, and schema improvements. A new healthcheck feature is introduced for the flux-instance chart.
My review focuses on the new healthcheck implementation. I've identified a critical issue where the healthcheck will fail due to missing RBAC permissions if a new service account is created. I've also found a high-severity issue with the default service account naming which could cause conflicts, and a medium-severity issue with inconsistent labels in the healthcheck job template.
I have provided detailed suggestions to address these points to ensure the new feature is robust and secure.
| {{- if .Values.healthcheck.serviceAccount.create }} | ||
| apiVersion: v1 | ||
| kind: ServiceAccount | ||
| metadata: | ||
| name: {{ .Values.healthcheck.serviceAccount.name }} | ||
| namespace: {{ .Release.Namespace }} | ||
| labels: | ||
| {{- include "flux-instance.labels" . | nindent 4 }} | ||
| {{- with .Values.commonLabels }} | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| {{- with .Values.commonAnnotations }} | ||
| annotations: | ||
| {{- toYaml . | nindent 4 }} | ||
| {{- end }} | ||
| automountServiceAccountToken: {{ .Values.healthcheck.serviceAccount.automount }} | ||
| {{- end }} |
There was a problem hiding this comment.
When healthcheck.serviceAccount.create is true, a new ServiceAccount is created for the healthcheck Job. However, this ServiceAccount lacks the necessary RBAC permissions to check the status of the FluxInstance resource, which will cause the healthcheck Job to fail with permission errors.
You should also create a Role and RoleBinding to grant the required permissions. These resources should also be conditional on healthcheck.serviceAccount.create.
Here are the suggested resources to add (e.g., in a new rbac.yaml template or appended to this file):
{{- if .Values.healthcheck.serviceAccount.create }}
---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: {{ .Values.healthcheck.serviceAccount.name | default (printf "%s-healthcheck" (include "flux-instance.fullname" .)) }}
namespace: {{ .Release.Namespace }}
labels:
{{- include "flux-instance.labels" . | nindent 4 }}
rules:
- apiGroups: ["fluxcd.controlplane.io"]
resources: ["fluxinstances"]
verbs: ["get", "list", "watch"]
---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: {{ .Values.healthcheck.serviceAccount.name | default (printf "%s-healthcheck" (include "flux-instance.fullname" .)) }}
namespace: {{ .Release.Namespace }}
labels:
{{- include "flux-instance.labels" . | nindent 4 }}
roleRef:
apiGroup: rbac.authorization.k8s.io
kind: Role
name: {{ .Values.healthcheck.serviceAccount.name | default (printf "%s-healthcheck" (include "flux-instance.fullname" .)) }}
subjects:
- kind: ServiceAccount
name: {{ .Values.healthcheck.serviceAccount.name | default (printf "%s-healthcheck" (include "flux-instance.fullname" .)) }}
namespace: {{ .Release.Namespace }}
{{- end }}| imagePullSecrets: | ||
| {{- toYaml . | nindent 8 }} | ||
| {{- end }} | ||
| serviceAccountName: {{ .Values.healthcheck.serviceAccount.name }} |
There was a problem hiding this comment.
To ensure the job uses the correct service account (especially when one is created dynamically), a default, release-specific name should be used if no name is provided.
serviceAccountName: {{ .Values.healthcheck.serviceAccount.name | default (printf "%s-healthcheck" (include "flux-instance.fullname" .)) }}
| apiVersion: v1 | ||
| kind: ServiceAccount | ||
| metadata: | ||
| name: {{ .Values.healthcheck.serviceAccount.name }} |
There was a problem hiding this comment.
To avoid conflicts, a default, release-specific name should be used for the service account if no name is provided.
name: {{ .Values.healthcheck.serviceAccount.name | default (printf "%s-healthcheck" (include "flux-instance.fullname" .)) }}
| "default": { | ||
| "automount": true, | ||
| "create": false, | ||
| "name": "flux-operator" |
There was a problem hiding this comment.
To be consistent with the change in values.yaml, the default name in the schema should also be an empty string.
"name": ""
| serviceAccount: # @schema default: {"create":false,"automount":true,"name":"flux-operator"} | ||
| create: false | ||
| automount: true | ||
| name: "flux-operator" |
There was a problem hiding this comment.
The default service account name flux-operator is problematic when create is set to true, as it can conflict with the service account used by the Flux Operator itself. This could lead to deployment failures or unintended permissions.
It's better to use a release-specific name for the created service account. I recommend changing the default name to an empty string and using a default in the templates.
serviceAccount: # @schema default: {"create":false,"automount":true,"name":""}
create: false
automount: true
name: ""
| labels: | ||
| app.kubernetes.io/managed-by: {{ .Release.Service | quote }} | ||
| app.kubernetes.io/instance: {{ .Release.Name | quote }} | ||
| helm.sh/chart: "{{ .Chart.Name }}-{{ .Chart.Version }}" |
There was a problem hiding this comment.
The pod template labels are missing the app.kubernetes.io/version label, which is present on the Job metadata. For consistency and to follow Kubernetes recommended labels, it's good to include it here as well.
labels:
app.kubernetes.io/managed-by: {{ .Release.Service | quote }}
app.kubernetes.io/instance: {{ .Release.Name | quote }}
app.kubernetes.io/version: {{ .Chart.AppVersion }}
helm.sh/chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis update advances both the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant Kubernetes
participant Healthcheck Job
User->>Helm: Install/Upgrade flux-instance chart (with healthcheck.enabled=true)
Helm->>Kubernetes: Deploy resources (including Healthcheck Job as a hook)
Kubernetes->>Healthcheck Job: Create Job (post-install/post-upgrade)
Healthcheck Job->>Kubernetes: Run healthcheck container
Healthcheck Job->>Kubernetes: Wait for instance readiness (with timeout)
Healthcheck Job-->>Kubernetes: Complete and cleanup Job (per hook policy)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
♻️ Duplicate comments (6)
packages/system/fluxcd/charts/flux-instance/templates/serviceaccount.yaml (2)
1-17: ServiceAccount still lacks RBAC and release-specific default name
Same concerns raised previously (missing Role/RoleBinding and fallback to a release-scoped default name) remain unresolved.
5-5: Hard-coded name may collide with existing SA
Reuse the earlier suggestion to default to(<release>-healthcheck)when.Values.healthcheck.serviceAccount.nameis empty.packages/system/fluxcd/charts/flux-instance/templates/healthcheck.yaml (2)
20-22: Pod-template labels still missingapp.kubernetes.io/version
Previous feedback on adding the version label has not been applied.
29-30:serviceAccountNameshould fall back to a release-scoped default
Please apply the recommended default expression to avoid name collisions.packages/system/fluxcd/charts/flux-instance/values.yaml (1)
88-92: Default SA name still set toflux-operator
Leaving this non-empty invites conflicts with the operator’s own SA. Prior comment suggested defaulting to an empty string and handling the fallback in the template.packages/system/fluxcd/charts/flux-instance/values.schema.json (1)
219-223: Schema default forserviceAccount.nameshould be empty
Mirrors the unresolved values.yaml feedback; default should be""to prevent collisions.
🧹 Nitpick comments (3)
packages/system/fluxcd-operator/charts/flux-operator/values.schema.json (2)
146-151: *image.pullSecretsis typed as “array of object”, but the usual Helm idiom is list of stringsMost users pass pull secrets like:
image: pullSecrets: - regcredValidating against
type: objectwill reject this common pattern and requires:image: pullSecrets: - name: regcredIf deliberate, document the deviation clearly in the README. Otherwise, switch the schema to
type: string.- "pullSecrets": { - "type": "array", - "uniqueItems": true, - "items": { - "type": "object" - } + "pullSecrets": { + "type": "array", + "uniqueItems": true, + "items": { + "type": "string" + } },
330-373: Good move enforcing a hardenedsecurityContext– tiny nitpickSince the object is now required, consider adding
"additionalProperties": falseto lock the surface area and prevent typos in security-sensitive fields.packages/system/fluxcd-operator/charts/flux-operator/Chart.yaml (1)
11-12: Chart minor version may not reflect the breaking-change introduced in the schemaIf the
values.schema.jsonrequirement change stays, consider bumpingversion:to the next major (or at least note the break in the release notes) to follow Helm semver expectations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/system/fluxcd-operator/charts/flux-operator/Chart.yaml(2 hunks)packages/system/fluxcd-operator/charts/flux-operator/README.md(2 hunks)packages/system/fluxcd-operator/charts/flux-operator/values.schema.json(12 hunks)packages/system/fluxcd-operator/charts/flux-operator/values.yaml(1 hunks)packages/system/fluxcd/charts/flux-instance/Chart.yaml(2 hunks)packages/system/fluxcd/charts/flux-instance/README.md(2 hunks)packages/system/fluxcd/charts/flux-instance/templates/healthcheck.yaml(1 hunks)packages/system/fluxcd/charts/flux-instance/templates/serviceaccount.yaml(1 hunks)packages/system/fluxcd/charts/flux-instance/values.schema.json(4 hunks)packages/system/fluxcd/charts/flux-instance/values.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: the `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the tea...
Learnt from: lllamnyp
PR: cozystack/cozystack#1233
File: packages/system/hetzner-robotlb/charts/robotlb/templates/deployment.yaml:33-35
Timestamp: 2025-07-23T09:15:09.658Z
Learning: The `./charts/robotlb/` directory in the hetzner-robotlb package contains vendored code, and the team generally avoids modifying vendored code to maintain clean separation from upstream dependencies.
Applied to files:
packages/system/fluxcd/charts/flux-instance/Chart.yaml
📚 Learning: in the cozystack repository, the `readme-generator` tool removes enum contents from values.schema.js...
Learnt from: NickVolynkin
PR: cozystack/cozystack#1196
File: packages/apps/http-cache/Makefile:24-27
Timestamp: 2025-07-14T16:23:12.803Z
Learning: In the cozystack repository, the `readme-generator` tool removes enum contents from values.schema.json files during its operation. Therefore, when using readme-generator in Makefiles, any enum values need to be injected back into the schema using yq commands after readme-generator has run, not before.
Applied to files:
packages/system/fluxcd-operator/charts/flux-operator/values.schema.json
📚 Learning: in cosi (container object storage interface), bucketaccessclass resources have fields like drivernam...
Learnt from: lllamnyp
PR: cozystack/cozystack#1239
File: packages/extra/seaweedfs/templates/external/cosi-bucket-class.yaml:3-15
Timestamp: 2025-07-24T13:21:17.388Z
Learning: In COSI (Container Object Storage Interface), BucketAccessClass resources have fields like driverName and authenticationType at the top level, not under a spec block. Only BucketClass resources require a spec block for fields like driverName and deletionPolicy.
Applied to files:
packages/system/fluxcd/charts/flux-instance/values.schema.json
🪛 YAMLlint (1.37.1)
packages/system/fluxcd/charts/flux-instance/templates/serviceaccount.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
[warning] 10-10: wrong indentation: expected 2 but found 4
(indentation)
[warning] 11-11: wrong indentation: expected 2 but found 4
(indentation)
packages/system/fluxcd/charts/flux-instance/templates/healthcheck.yaml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (5)
packages/system/fluxcd-operator/charts/flux-operator/values.yaml (1)
48-50: CPU limit doubled to2000m– verify this is intentional for all default installsThe default overhead jumps from 1 vCPU to 2 vCPU per operator pod. On small clusters (e.g., single-node dev clusters) this may prevent scheduling or trigger cluster-autoscaler scale-ups.
If the increase is only needed for large environments, consider lowering the default and documenting how to raise it instead.
No code change required – just confirm the operational impact.
packages/system/fluxcd-operator/charts/flux-operator/README.md (1)
59-60: Table entry updated – 👍The README now reflects the new CPU limit; memory figures remain unchanged and consistent with
values.yaml.packages/system/fluxcd-operator/charts/flux-operator/Chart.yaml (1)
28-28: LGTM on metadata bump – version and appVersion are aligned with upstream 0.26.0 tag.packages/system/fluxcd/charts/flux-instance/Chart.yaml (1)
11-12: Version alignment looks good –flux-instancenow tracks the samev0.26.0release as the operator.packages/system/fluxcd/charts/flux-instance/README.md (1)
3-4: Docs accurately updated for 0.26.0 — LGTM
Version badges and new health-check options are reflected correctly.Also applies to: 40-41
| "type": "object", | ||
| "required": [ | ||
| "resources", | ||
| "securityContext" | ||
| ], |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
resources and securityContext added to the global required list – this is a (silent) breaking-change
Requiring these keys means all user-supplied values.yaml files that omit either block will now fail chart validation.
Helm chart ≃ semver best-practice states that any breaking change requires a major version bump (see “Versioning” in Helm docs). The chart was only bumped from 0.24.x → 0.26.x, so consumers may upgrade expecting backwards compatibility and hit an error.
Please either
- drop these two entries from the
requiredarray, or - bump the chart version to
1.0.0(or at least0.27.0with a release note that it is breaking).
- "required": [
- "resources",
- "securityContext"
- ],
+ // keep validation strict by providing defaults, but avoid a breaking change
+ "required": [],🏁 Script executed:
#!/bin/bash
# Inspect the required fields in the values.schema.json
echo "---- values.schema.json (lines 1–20) ----"
sed -n '1,20p' packages/system/fluxcd-operator/charts/flux-operator/values.schema.json
# Inspect the chart version in Chart.yaml
echo
echo "---- Chart.yaml version ----"
grep -E '^version:' -n packages/system/fluxcd-operator/charts/flux-operator/Chart.yamlLength of output: 1092
Address the breaking change in values.schema.json
Requiring resources and securityContext at the top level now turns into a silent break for any consumer that omits those blocks. The chart’s version is currently 0.26.0 (Chart.yaml: line 28), so you must either:
• Remove those entries from the required array to preserve backward compatibility
--- a/packages/system/fluxcd-operator/charts/flux-operator/values.schema.json
+++ b/packages/system/fluxcd-operator/charts/flux-operator/values.schema.json
@@ -3,7 +3,7 @@
"$schema": "https://json-schema.org/draft/2019-09/schema",
"type": "object",
- "required": [
- "resources",
- "securityContext"
- ],
+ "required": [],
"properties": {
"affinity": {
…• Or bump the chart version in Chart.yaml to 0.27.0 (or 1.0.0) and clearly document the breaking change in your release notes.
Files to update:
- packages/system/fluxcd-operator/charts/flux-operator/values.schema.json (lines 3–7)
- packages/system/fluxcd-operator/charts/flux-operator/Chart.yaml (version: 0.26.0 → 0.27.0/1.0.0)
📝 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.
| "type": "object", | |
| "required": [ | |
| "resources", | |
| "securityContext" | |
| ], | |
| "$schema": "https://json-schema.org/draft/2019-09/schema", | |
| "type": "object", | |
| "required": [], | |
| "properties": { | |
| "affinity": { | |
| … |
🤖 Prompt for AI Agents
In packages/system/fluxcd-operator/charts/flux-operator/values.schema.json lines
3 to 7, the "resources" and "securityContext" fields are marked as required,
causing a breaking change for consumers omitting these fields. To fix this,
either remove these fields from the "required" array to maintain backward
compatibility or update the chart version in
packages/system/fluxcd-operator/charts/flux-operator/Chart.yaml from 0.26.0 to
0.27.0 or 1.0.0 and document the breaking change in the release notes
accordingly.
1 participant
Changelog:
https://github.com/controlplaneio-fluxcd/flux-operator/releases/tag/v0.26.0
(and https://github.com/controlplaneio-fluxcd/flux-operator/releases/tag/v0.25.0)
Summary by CodeRabbit
New Features
Improvements
Documentation