[platform] Separate assets server into dedicated deployment #1705
Conversation
|
Caution Review failedThe pull request is closed. 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 PR refactors the assets server from a sidecar container within the main cozystack deployment to a standalone StatefulSet. It removes assets-related build steps from the cozystack Dockerfile, creates a dedicated assets service and image, and updates repository and dashboard URLs to reference the new cozystack-assets service endpoint. Changes
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes
Possibly Related PRs
Suggested Reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
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 |
Summary of ChangesHello @kvaps, 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 significantly refactors the Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 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-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
f677d58 to
f674837
Compare
There was a problem hiding this comment.
Code Review
This pull request effectively separates the assets server into its own dedicated workload, which is a good step towards modularity. The changes involve creating a new StatefulSet for the assets server, updating Helm repository URLs to use the Kubernetes API proxy, and adding a script to generate the necessary client certificates for Flux.
My review has identified a few issues that should be addressed:
- A critical issue in the new Helm helper template that could prevent Flux from syncing repositories.
- A bug in the certificate generation script that prevents cleanup of temporary files.
- A duplicated and misleadingly named Kubernetes manifest.
- Some opportunities for code cleanup and to improve the robustness of the Kubernetes resource definitions.
Overall, the direction is good, but the implementation needs some refinement.
| {{- define "cozystack.kubernetesAPIEndpoint" -}} | ||
| {{- $cozyDeployment := lookup "apps/v1" "Deployment" "cozy-system" "cozystack" }} | ||
| {{- $cozyContainers := dig "spec" "template" "spec" "containers" list $cozyDeployment }} | ||
| {{- $kubernetesServiceHost := "" }} | ||
| {{- $kubernetesServicePort := "" }} | ||
| {{- range $cozyContainers }} | ||
| {{- if eq .name "cozystack" }} | ||
| {{- range .env }} | ||
| {{- if eq .name "KUBERNETES_SERVICE_HOST" }} | ||
| {{- $kubernetesServiceHost = .value }} | ||
| {{- end }} | ||
| {{- if eq .name "KUBERNETES_SERVICE_PORT" }} | ||
| {{- $kubernetesServicePort = .value }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- end }} | ||
| {{- if eq $kubernetesServiceHost "" }} | ||
| {{- $kubernetesServiceHost = "kubernetes.default.svc" }} | ||
| {{- end }} | ||
| {{- if eq $kubernetesServicePort "" }} | ||
| {{- $kubernetesServicePort = "443" }} | ||
| {{- end }} | ||
| {{- printf "%s:%s" $kubernetesServiceHost $kubernetesServicePort }} | ||
| {{- end -}} |
There was a problem hiding this comment.
The logic in this helper to determine the Kubernetes API endpoint is overly complex and likely incorrect. It attempts to read the KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT environment variables from the cozystack deployment, which are set to localhost and 7445. This will result in Flux's source-controller trying to connect to localhost:7445 to reach the API server, which will fail.
The standard way to access the API server from within a cluster is via the kubernetes.default.svc service on port 443. The helper should be simplified to use these values directly.
I suggest replacing the entire cozystack.kubernetesAPIEndpoint definition with:
{{- define "cozystack.kubernetesAPIEndpoint" -}}
{{- printf "kubernetes.default.svc:443" -}}
{{- end -}}
scripts/issue-flux-certificates.sh
Outdated
|
|
||
| # make temp directory and cleanup handler | ||
| TMPDIR=$(mktemp -d) | ||
| trap "echo rm -rf $TMPDIR" EXIT |
There was a problem hiding this comment.
The trap command is configured to echo the cleanup command instead of executing it. This means the temporary directory created by mktemp will not be removed when the script exits, leaving orphaned files on the system. The echo should be removed.
| trap "echo rm -rf $TMPDIR" EXIT | |
| trap "rm -rf $TMPDIR" EXIT |
packages/core/installer/Makefile
Outdated
| IMAGE="$(REGISTRY)/installer:$(call settag,$(TAG))@$$(yq e '."containerimage.digest"' images/installer.json -o json -r)" \ | ||
| yq -i '.cozystack.image = strenv(IMAGE)' values.yaml | ||
| IMAGE="$(REGISTRY)/installer:$(call settag,$(TAG))@$$(yq e '."containerimage.digest"' images/installer.json -o json -r)" \ | ||
| yq -i '.cozystack.image = strenv(IMAGE)' ../platform/values.yaml |
There was a problem hiding this comment.
There is code duplication here. The IMAGE variable is being constructed twice, and the yq command is run in two separate steps. This can be simplified to make it more readable and maintainable by calculating the image string once and applying it to both files in a more concise way. You could combine these into a single command since yq can operate on multiple files.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
packages/core/installer/Makefile (1)
32-35: Consider consolidating the duplicate image update commands.The same
IMAGEvariable construction andyqupdate pattern is repeated for two different files. This duplication could be simplified for better maintainability.packages/core/platform/templates/_helpers.tpl (1)
24-48: Reconsider the API endpoint resolution logic.Reading
KUBERNETES_SERVICE_HOSTandKUBERNETES_SERVICE_PORTfrom the cozystack Deployment's environment variables may return values configured for that specific pod (e.g.,localhost:7445), which will not be reachable from other pods like Flux'ssource-controller. The standard approach is to usekubernetes.default.svc:443directly.packages/core/platform/templates/daemonset-assets-server.yaml (1)
1-76: Address file duplication and misleading filename.This file appears to duplicate resources from
packages/core/platform/templates/cozystack-assets.yaml, and the filenamedaemonset-assets-server.yamlis misleading since it defines aStatefulSet, not aDaemonSet.scripts/issue-flux-certificates.sh (1)
14-14: Trap statement echoes instead of executing the cleanup command.The
echokeyword causes the cleanup command to be printed rather than executed, preventing the temporary directory from being removed on script exit. This will leave orphaned files on the system.Apply this diff to fix the trap statement:
-trap "echo rm -rf $TMPDIR" EXIT +trap "rm -rf $TMPDIR" EXITpackages/core/platform/templates/cozystack-assets.yaml (1)
31-32: Wildcard toleration permits unnecessary node taints.The toleration
operator: Existswithout akeyacts as a wildcard that makes the pod tolerate all taints on all nodes. This can result in unwanted scheduling on nodes with undesirable taints.Specify tolerations explicitly for only the necessary taints, for example:
- tolerations: - - operator: Exists + tolerations: + - key: "node.kubernetes.io/not-ready" + operator: "Exists" + effect: "NoSchedule" + - key: "node.cilium.io/agent-not-ready" + operator: "Exists" + effect: "NoSchedule"
🧹 Nitpick comments (2)
packages/core/platform/templates/daemonset-assets-server.yaml (2)
40-48: Tightly coupled RBAC to specific pod name.The RBAC role grants permissions specifically to
cozystack-assets-0(line 44), which tightly couples the authorization to StatefulSet naming conventions. If the StatefulSet is recreated or scaled, this could break.Consider whether this tight coupling is intentional for security isolation or if a label selector approach would be more flexible.
20-30: Consider adding resource limits and requests.The container specification lacks resource limits and requests. This can lead to resource contention and unpredictable behavior under load.
Apply this diff to add resource constraints:
- name: assets-server image: "{{ .Values.cozystack.image }}" command: - /usr/bin/cozystack-assets-server - "-dir=/cozystack/assets" - "-address=:8123" + resources: + requests: + cpu: 100m + memory: 128Mi + limits: + cpu: 500m + memory: 512Mi ports: - name: http containerPort: 8123 hostPort: 8123
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/core/installer/Makefile(1 hunks)packages/core/installer/images/cozystack/Dockerfile(1 hunks)packages/core/installer/templates/cozystack.yaml(0 hunks)packages/core/platform/templates/_helpers.tpl(1 hunks)packages/core/platform/templates/cozystack-assets.yaml(1 hunks)packages/core/platform/templates/daemonset-assets-server.yaml(1 hunks)packages/core/platform/templates/helmrepos.yaml(3 hunks)packages/core/platform/values.yaml(1 hunks)packages/extra/monitoring/templates/dashboards.yaml(1 hunks)scripts/installer.sh(1 hunks)scripts/issue-flux-certificates.sh(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/installer/templates/cozystack.yaml
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-07-14T16:23:12.803Z
Learnt from: NickVolynkin
Repo: cozystack/cozystack PR: 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/core/installer/Makefile
📚 Learning: 2025-07-26T18:26:01.447Z
Learnt from: NickVolynkin
Repo: cozystack/cozystack PR: 1216
File: packages/extra/monitoring/README.md:30-31
Timestamp: 2025-07-26T18:26:01.447Z
Learning: The cozyvalues-gen tool has a known issue (https://github.com/cozystack/cozyvalues-gen/issues/10) where it incorrectly places Grafana configuration entries under the wrong documentation sections, specifically placing grafana.resources.requests.* entries in the Metrics storage configuration table instead of the Grafana configuration table.
Applied to files:
packages/extra/monitoring/templates/dashboards.yaml
📚 Learning: 2025-07-11T06:11:25.438Z
Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 1130
File: hack/e2e-apps/kubernetes.bats:101-101
Timestamp: 2025-07-11T06:11:25.438Z
Learning: In cozystack, the plural form for the Kubernetes custom resource is `kuberneteses.apps.cozystack.io`, not `kubernetes.apps.cozystack.io`. This is defined in the API schema even though it's not grammatically perfect.
Applied to files:
packages/core/platform/templates/_helpers.tpl
📚 Learning: 2025-06-26T04:29:24.830Z
Learnt from: NickVolynkin
Repo: cozystack/cozystack PR: 1117
File: packages/apps/mysql/Makefile:8-8
Timestamp: 2025-06-26T04:29:24.830Z
Learning: The cozystack project uses yq v4+ on their CI runner, so yq v4 syntax (-o json --indent 4) is compatible and version checks are not needed.
Applied to files:
packages/core/platform/values.yaml
🪛 Shellcheck (0.11.0)
scripts/issue-flux-certificates.sh
[warning] 14-14: Use single quotes, otherwise this expands now rather than when signalled.
(SC2064)
⏰ 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: Build
🔇 Additional comments (5)
packages/core/installer/images/cozystack/Dockerfile (1)
37-37: LGTM!The addition of
opensslis appropriate for the TLS certificate provisioning workflow introduced in this PR.packages/core/platform/values.yaml (1)
1-2: LGTM!The image reference with SHA256 digest pinning provides immutability and security. The value is correctly propagated from the Makefile.
packages/extra/monitoring/templates/dashboards.yaml (1)
14-14: Service endpoint is correctly configured.The
cozystack-assets.cozy-system.svcendpoint aligns with the properly defined StatefulSet and Service inpackages/core/platform/templates/cozystack-assets.yaml. The Service (name:cozystack-assets, namespace:cozy-system) correctly selects pods with the labelapp: cozystack-assetsand maps port 80 to the container port 8123 where the assets server listens. The endpoint is reachable within the cluster.packages/core/platform/templates/helmrepos.yaml (1)
12-13: TLS secret provisioning order is correctly implemented in the installation sequence.The
issue-flux-certificates.shscript is called inscripts/installer.sh(line 73) before the platform chart is deployed (line 76), ensuring that thecozystack-assets-tlssecrets are created in bothcozy-systemandcozy-publicnamespaces before the HelmRepository resources are reconciled by Flux. The script creates the secrets with the required TLS certificate and key files that the HelmRepository resources reference viacertSecretRef.packages/core/platform/templates/cozystack-assets.yaml (1)
22-22: > Likely an incorrect or invalid review comment.
99a3152 to
3c48f6a
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/core/platform/templates/cozystack-assets.yaml (1)
31-32: Overly permissive toleration. The tolerationoperator: Existswithout akeyacts as a wildcard that allows the pod to tolerate all taints, including undesirable ones. Prior review feedback recommended explicit tolerations.Consider using specific tolerations that align with the target environment:
tolerations: - key: "node.kubernetes.io/not-ready" operator: "Exists" effect: "NoSchedule" - key: "node.cilium.io/agent-not-ready" operator: "Exists" effect: "NoSchedule"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Makefile(1 hunks)packages/core/installer/images/cozystack/Dockerfile(1 hunks)packages/core/installer/templates/cozystack.yaml(0 hunks)packages/core/platform/Makefile(2 hunks)packages/core/platform/images/cozystack-assets/Dockerfile(1 hunks)packages/core/platform/templates/assets-server.yaml(1 hunks)packages/core/platform/templates/cozystack-assets.yaml(1 hunks)packages/core/platform/templates/helmrepos.yaml(3 hunks)packages/core/platform/values.yaml(1 hunks)packages/extra/monitoring/templates/dashboards.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- packages/core/installer/templates/cozystack.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/core/platform/values.yaml
- packages/core/platform/templates/helmrepos.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-26T18:26:01.447Z
Learnt from: NickVolynkin
Repo: cozystack/cozystack PR: 1216
File: packages/extra/monitoring/README.md:30-31
Timestamp: 2025-07-26T18:26:01.447Z
Learning: The cozyvalues-gen tool has a known issue (https://github.com/cozystack/cozyvalues-gen/issues/10) where it incorrectly places Grafana configuration entries under the wrong documentation sections, specifically placing grafana.resources.requests.* entries in the Metrics storage configuration table instead of the Grafana configuration table.
Applied to files:
packages/extra/monitoring/templates/dashboards.yaml
🪛 checkmake (0.2.2)
packages/core/platform/Makefile
[warning] 24-24: Target "image" should be declared PHONY.
(phonydeclared)
🔇 Additional comments (5)
Makefile (1)
29-29: LGTM. The addition of the platform image build is correctly integrated into the main build target.packages/extra/monitoring/templates/dashboards.yaml (1)
14-14: LGTM. The dashboard backend URL correctly points to the new assets service endpoint.packages/core/installer/images/cozystack/Dockerfile (1)
33-33: LGTM. Addition ofopensslis appropriate for the installer, and removal of assets-related artifacts aligns with the separation of concerns.packages/core/platform/images/cozystack-assets/Dockerfile (1)
1-25: LGTM. The multistage Dockerfile correctly builds the assets server static binary and packages assets/dashboards for the final image.packages/core/platform/Makefile (1)
25-34: LGTM. Theimage-assetstarget correctly orchestrates the Docker build, captures the image digest, and updatesvalues.yamlwith the new image reference, enabling the assets server deployment to reference the freshly built image.
61161dc to
4f95309
Compare
| RUN apk add --no-cache make git | ||
| RUN apk add helm --repository=https://dl-cdn.alpinelinux.org/alpine/edge/community |
There was a problem hiding this comment.
All this does is server static assets, right? Do we need these packages?
There was a problem hiding this comment.
Yes, we need.
Helm and make are used to generate index
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
b5d1fb2 to
b1ba1f2
Compare
This change is extracted from - #1641 and reworked to work standalone requires: - #1705 ## What this PR does Adds a new `flux-aio` module and migration script to upgrade FluxCD to version 22. This introduces a new modular approach to FluxCD installation using the flux-aio OCI module. Changes: - Created new `flux-aio` package with Chart.yaml, Makefile, and CUE configuration - Added flux-aio module configuration using OCI module from `ghcr.io/stefanprodan/modules/flux-aio` - Generated large fluxcd.yaml template (11956+ lines) for FluxCD resources - Added migration script (migrations/21) to handle upgrade from version 21 to 22 - Updated installer to include flux-aio module - Added script `issue-flux-certificates.sh` for managing TLS certificates for cozystack-assets - Updated platform templates to support flux-aio module - Updated cozystack-assets service references ### Release note ```release-note [fluxcd] Add flux-aio module and migration ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Added TLS certificate support for Helm package repositories with automatic certificate provisioning. * **Chores** * Refactored FluxCD integration using Helm chart-based deployment. * Updated system to version 22 with automatic migration support. * Enhanced security dependencies (OpenSSL). <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Separates the assets server from the main cozystack installer into a dedicated StatefulSet deployment. This improves separation of concerns and allows the assets server to run independently from the installer. Changes: - Created new `cozystack-assets` StatefulSet in the platform package - Added dedicated Dockerfile for assets server image (`packages/core/platform/images/cozystack-assets/Dockerfile`) - Removed assets server container and Service from installer deployment - Updated HelmRepository URLs to point to new `cozystack-assets` service - Updated dashboard URLs in monitoring package to use new service - Added image build target to platform Makefile - Configured assets server with hostNetwork and proper RBAC permissions ```release-note [platform] Separate assets server into dedicated StatefulSet deployment ```
This change is extracted from - #1641 and reworked to work standalone requires: - #1705 Adds a new `flux-aio` module and migration script to upgrade FluxCD to version 22. This introduces a new modular approach to FluxCD installation using the flux-aio OCI module. Changes: - Created new `flux-aio` package with Chart.yaml, Makefile, and CUE configuration - Added flux-aio module configuration using OCI module from `ghcr.io/stefanprodan/modules/flux-aio` - Generated large fluxcd.yaml template (11956+ lines) for FluxCD resources - Added migration script (migrations/21) to handle upgrade from version 21 to 22 - Updated installer to include flux-aio module - Added script `issue-flux-certificates.sh` for managing TLS certificates for cozystack-assets - Updated platform templates to support flux-aio module - Updated cozystack-assets service references ```release-note [fluxcd] Add flux-aio module and migration ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> * **New Features** * Added TLS certificate support for Helm package repositories with automatic certificate provisioning. * **Chores** * Refactored FluxCD integration using Helm chart-based deployment. * Updated system to version 22 with automatic migration support. * Enhanced security dependencies (OpenSSL). <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## What this PR does Separates the assets server from the main cozystack installer into a dedicated StatefulSet deployment. This improves separation of concerns and allows the assets server to run independently from the installer. Changes: - Created new `cozystack-assets` StatefulSet in the platform package - Added dedicated Dockerfile for assets server image (`packages/core/platform/images/cozystack-assets/Dockerfile`) - Removed assets server container and Service from installer deployment - Updated HelmRepository URLs to point to new `cozystack-assets` service - Updated dashboard URLs in monitoring package to use new service - Added image build target to platform Makefile - Configured assets server with hostNetwork and proper RBAC permissions ### Release note ```release-note [platform] Separate assets server into dedicated StatefulSet deployment ```
This change is extracted from - #1641 and reworked to work standalone requires: - #1705 ## What this PR does Adds a new `flux-aio` module and migration script to upgrade FluxCD to version 22. This introduces a new modular approach to FluxCD installation using the flux-aio OCI module. Changes: - Created new `flux-aio` package with Chart.yaml, Makefile, and CUE configuration - Added flux-aio module configuration using OCI module from `ghcr.io/stefanprodan/modules/flux-aio` - Generated large fluxcd.yaml template (11956+ lines) for FluxCD resources - Added migration script (migrations/21) to handle upgrade from version 21 to 22 - Updated installer to include flux-aio module - Added script `issue-flux-certificates.sh` for managing TLS certificates for cozystack-assets - Updated platform templates to support flux-aio module - Updated cozystack-assets service references ### Release note ```release-note [fluxcd] Add flux-aio module and migration ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Added TLS certificate support for Helm package repositories with automatic certificate provisioning. * **Chores** * Refactored FluxCD integration using Helm chart-based deployment. * Updated system to version 22 with automatic migration support. * Enhanced security dependencies (OpenSSL). <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
Separates the assets server from the main cozystack installer into a dedicated StatefulSet deployment. This improves separation of concerns and allows the assets server to run independently from the installer.
Changes:
cozystack-assetsStatefulSet in the platform packagepackages/core/platform/images/cozystack-assets/Dockerfile)cozystack-assetsserviceRelease note
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.