Repository navigation
[harbor] Add managed Harbor container registry - #2058
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Harbor application package, Helm charts and values, system templates for Postgres/Redis, application templates and bundle integration, and an end-to-end test that deploys and validates a Harbor installation and cleans it up. Changes
Sequence Diagram(s)sequenceDiagram
participant CI as Dev/CI
participant Flux as Flux/Helm operator
participant K8s as Kubernetes API
participant CNPG as CNPG (Postgres)
participant RedisRF as RedisFailover
participant HarborCore as Harbor Core
participant Registry as Harbor Registry
participant Portal as Harbor Portal
participant Ingress as Ingress/Gateway
CI->>Flux: push harbor package / HelmRelease
Flux->>K8s: apply HelmRelease / Chart resources
K8s->>CNPG: create CNPG Cluster (database)
K8s->>RedisRF: create RedisFailover (redis)
CNPG-->>K8s: DB Ready
RedisRF-->>K8s: Redis Ready
K8s->>K8s: create Secrets, ConfigMaps, PVCs
K8s->>HarborCore: create Deployment (core)
K8s->>Registry: create Deployment (registry)
K8s->>Portal: create Deployment (portal)
HarborCore-->>K8s: Core ready
Registry-->>K8s: Registry ready
Portal-->>K8s: Portal ready
K8s->>Ingress: create Ingress/HTTPRoute + TLS Secret
Ingress-->>K8s: route traffic to services
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
dosubot
Bot
added
size/XXL
Summary of ChangesHello Aleksei Sviridkin (@lexfrei), 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 integrates Harbor, a robust cloud-native container registry, into the platform as a new managed service. It provides all the necessary infrastructure for tenants to deploy and operate their own Harbor instances, complete with automated setup, configurable resources, secure access, and persistent storage. The changes ensure a seamless and scalable way to manage container images and artifacts within the ecosystem. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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 (@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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces Harbor as a managed tenant-level container registry service. The changes are comprehensive, including a wrapper Helm chart, vendoring of the upstream Harbor chart, ApplicationDefinition for dynamic CRD registration, and an end-to-end test. The implementation looks solid. I have a couple of suggestions to improve the robustness of the E2E test and the remediation strategy for the HelmRelease to make it safer.
| resources: {} | ||
| resourcesPreset: "nano" | ||
| EOF | ||
| sleep 5 |
There was a problem hiding this comment.
Using a fixed sleep can introduce flakiness into the test suite, as the required wait time might vary. A more robust approach is to actively wait for the necessary resource to be created before proceeding. This ensures the test is more reliable and less prone to timing-related failures.
timeout 30s bash -c 'until kubectl -n tenant-test get hr "$name" >/dev/null 2>&1; do sleep 1; done'
There was a problem hiding this comment.
This is a standard pattern across all Cozystack e2e tests — sleep 5 allows the controller to pick up the resource before waiting on conditions.
| remediation: | ||
| retries: -1 |
There was a problem hiding this comment.
Configuring infinite retries (retries: -1) for install remediation can be risky. If there's a persistent issue preventing the Helm release from installing, this could cause the Flux controller to enter a resource-intensive loop and flood the logs. It's generally safer to use a finite number of retries (e.g., 3 or 5) to handle transient errors while still failing definitively on persistent problems. This same concern applies to the upgrade.remediation configuration on line 40.
remediation:
retries: 3There was a problem hiding this comment.
retries: -1 is a standard pattern across all Cozystack HelmRelease templates. It ensures eventual convergence when dependencies (database secrets, COSI buckets, etc.) are provisioned asynchronously.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@hack/e2e-apps/harbor.bats`:
- Around line 3-48: The test "Create Harbor" can leave orphaned resources if any
earlier kubectl command fails; add a Bash trap at the top of the test to always
run the existing cleanup (the kubectl -n tenant-test delete
harbor.apps.cozystack.io $name command that currently appears at the end) on
EXIT or ERR, or implement an inline teardown function and register it with trap;
locate the test block (`@test` "Create Harbor") and update it so that the trap
references the same variable name 'name' and runs the exact delete command used
today to guarantee cleanup regardless of intermediate failures.
In `@packages/extra/harbor/README.md`:
- Line 3: Update the README sentence "Harbor is an open source trusted cloud
native registry project that stores, signs, and scans content." to hyphenate
compound adjectives: change "open source" to "open-source" and "cloud native" to
"cloud-native" so it reads "Harbor is an open-source trusted cloud-native
registry project that stores, signs, and scans content."
In `@packages/extra/harbor/templates/harbor.yaml`:
- Around line 85-86: The resources block is nested one level too high — change
the YAML so the include "cozy-lib.resources.defaultingSanitize" output is placed
under registry.registry.resources and registry.controller.resources (not
registry.resources). Specifically, create two keys under registry: "registry:
resources: {{ include "cozy-lib.resources.defaultingSanitize" (list
.Values.registry.resourcesPreset .Values.registry.resources $) }}" and
"controller: resources: {{ include "cozy-lib.resources.defaultingSanitize" (list
.Values.registry.controller.resourcesPreset
.Values.registry.controller.resources $) }}", so the vendored chart will apply
limits/requests to the registry and registryctl containers.
In
`@packages/system/harbor/charts/harbor/templates/core/core-pre-upgrade-job.yaml`:
- Around line 4-14: The migration Job named "migration-job" lacks a
hook-delete-policy and uses a hardcoded name; patch it via your wrapper chart or
values override to (1) add the annotation "helm.sh/hook-delete-policy" with a
value like "before-hook-creation,hook-succeeded" to ensure the old Job is
removed, and (2) make the Job name release-scoped (e.g., prefix/suffix the
literal "migration-job" with the release identity or use the chart's fullname
helper) so the resource name isn't global and won't collide across releases;
target the Job resource with metadata.name "migration-job" and its existing
"helm.sh/hook" annotation when applying the override.
In `@packages/system/harbor/charts/harbor/templates/core/core-secret.yaml`:
- Around line 17-21: The wrapper chart (packages/extra/harbor) is not persisting
Harbor token signing keys so the vendored template's genCA "harbor-token-ca" 365
in core-secret.yaml regenerates keys on each upgrade; fix by implementing the
same persistence pattern used for harborAdminPassword: either generate and
persist a stable core.secretName in the wrapper chart and set core.secretName in
the HelmRelease, or store and pass stable core.tokenKey and core.tokenCert
values in the HelmRelease values so the template does not call genCA; ensure the
chosen approach reliably writes/looks up the secret across upgrades and
references core.secretName, core.tokenKey and core.tokenCert accordingly.
🧹 Nitpick comments (9)
packages/system/harbor/charts/harbor/.helmignore (1)
6-6: Add trailing newline.The file is missing a trailing newline, which is a common convention in text files.
📝 Proposed fix to add trailing newline
test/* +packages/system/harbor/Makefile (1)
6-10: Pin the upstream chart version inhelm pullfor reproducible vendoring.The
helm pullcommand does not specify--version, so runningmake updatein the future will silently pull whatever the latest chart version is, making builds non-reproducible. The PR targets Harbor chart v1.18.2 — pin it explicitly.♻️ Proposed fix
update: rm -rf charts helm repo add harbor https://helm.goharbor.io helm repo update harbor - helm pull harbor/harbor --untar --untardir charts + helm pull harbor/harbor --version 1.18.2 --untar --untardir chartspackages/system/harbor/charts/harbor/templates/internal/auto-tls.yaml (1)
68-68: Redundantandwith a single argument.
{{- if and .Values.trivy.enabled}}— theandis unnecessary here;{{- if .Values.trivy.enabled}}is equivalent. This is likely from upstream, so not worth modifying in the vendored chart.packages/system/harbor/charts/harbor/templates/nginx/configmap-http.yaml (1)
131-133: Cosmetic: inconsistent indentation for/service/notificationslocation block.This location block is indented 2 spaces less than sibling blocks (e.g., lines 67, 78, 108). Functionally correct since nginx uses brace-delimited scoping, but it hurts readability. Since this is a vendored chart, not worth fixing here.
packages/system/harbor/charts/harbor/templates/nginx/configmap-https.yaml (1)
158-160: Same cosmetic indentation inconsistency as the HTTP config.
/service/notificationsis indented 2 spaces less than sibling location blocks. Functionally correct; cosmetic only. Same pattern asconfigmap-http.yaml.packages/system/harbor/charts/harbor/templates/exporter/exporter-dpl.yaml (1)
119-122: Unusedconfigvolume in exporter deployment (upstream).The volume
config(Line 120-122) is defined referencing the exporter secret, but it is never mounted in the exporter container'svolumeMounts(Lines 109-118). The container already receives this secret's data viasecretRefat Line 80-81, so this is functionally harmless but dead configuration. Since this is a vendored upstream chart, no direct edit is expected — just noting for awareness.packages/system/harbor/charts/harbor/templates/trivy/trivy-sts.yaml (1)
214-214: Trailing tab character on volumeClaimTemplate line (upstream).Line 214 contains a trailing tab character after
v1. While most YAML parsers tolerate this, it's a whitespace artifact from the vendored upstream chart. Noting for awareness — no direct edit per repository guidelines.packages/extra/harbor/templates/harbor.yaml (2)
37-40:upgrade.force: truemay cause unnecessary downtime for a stateful service.With
force: true, Flux will delete and recreate Kubernetes resources that cannot be patched during upgrades. For a stateful service like Harbor (with PVCs for database, registry, and Redis), this can cause brief but avoidable downtime on every upgrade. Consider whether this is necessary or if it was added to work around a specific issue.
109-153: WorkloadMonitors cover core, registry, and portal — consider adding jobservice.Database and Redis are StatefulSets with single replicas and likely don't need monitors, but jobservice is a Deployment and a key component for background tasks (replication, garbage collection, scanning). If it goes down, asynchronous operations silently stop. Is the omission intentional?
| @test "Create Harbor" { | ||
| name='harbor' | ||
| kubectl apply -f- <<EOF | ||
| apiVersion: apps.cozystack.io/v1alpha1 | ||
| kind: Harbor | ||
| metadata: | ||
| name: $name | ||
| namespace: tenant-test | ||
| spec: | ||
| host: "" | ||
| storageClass: "" | ||
| core: | ||
| resources: {} | ||
| resourcesPreset: "nano" | ||
| registry: | ||
| size: 5Gi | ||
| resources: {} | ||
| resourcesPreset: "nano" | ||
| jobservice: | ||
| resources: {} | ||
| resourcesPreset: "nano" | ||
| trivy: | ||
| enabled: false | ||
| size: 2Gi | ||
| resources: {} | ||
| resourcesPreset: "nano" | ||
| database: | ||
| size: 2Gi | ||
| resources: {} | ||
| resourcesPreset: "nano" | ||
| redis: | ||
| size: 1Gi | ||
| resources: {} | ||
| resourcesPreset: "nano" | ||
| EOF | ||
| sleep 5 | ||
| kubectl -n tenant-test wait hr $name --timeout=60s --for=condition=ready | ||
| kubectl -n tenant-test wait hr $name-system --timeout=300s --for=condition=ready | ||
| kubectl -n tenant-test wait deploy $name-core --timeout=120s --for=condition=available | ||
| kubectl -n tenant-test wait deploy $name-registry --timeout=120s --for=condition=available | ||
| kubectl -n tenant-test wait deploy $name-portal --timeout=120s --for=condition=available | ||
| kubectl -n tenant-test get secret $name-credentials -o jsonpath='{.data.admin-password}' | base64 --decode | grep -q '.' | ||
| kubectl -n tenant-test get secret $name-credentials -o jsonpath='{.data.url}' | base64 --decode | grep -q 'https://' | ||
| kubectl -n tenant-test get svc $name -o jsonpath='{.spec.ports[0].port}' | grep -q '80' | ||
| kubectl -n tenant-test delete harbor.apps.cozystack.io $name | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if other e2e bats tests use teardown for cleanup
rg -n 'teardown' --type-add 'bats:*.bats' --type=bats hack/e2e-apps/Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
# List all bats files in the e2e-apps directory
fd -e bats hack/e2e-apps/ | head -20Repository: cozystack/cozystack
Length of output: 424
🏁 Script executed:
# Get an overview of test structure and teardown patterns from a few bats files
for file in $(fd -e bats hack/e2e-apps/ | head -5); do
echo "=== $file ==="
head -30 "$file"
echo ""
rg 'teardown|setup' "$file" -A 3
echo ""
doneRepository: cozystack/cozystack
Length of output: 424
🏁 Script executed:
# List all bats files in the e2e-apps directory using correct syntax
fd . hack/e2e-apps/ -e bats | head -20Repository: cozystack/cozystack
Length of output: 440
🏁 Script executed:
# Get an overview of test structure and cleanup patterns from bats files
fd . hack/e2e-apps/ -e bats | while read file; do
echo "=== $file ==="
head -50 "$file"
echo ""
done | head -200Repository: cozystack/cozystack
Length of output: 6621
🏁 Script executed:
# Specifically search for cleanup patterns: delete, teardown, setup, etc.
rg 'delete|cleanup|teardown|setup' hack/e2e-apps/ --type-add 'bats:*.bats' -i -A 2 | head -100Repository: cozystack/cozystack
Length of output: 6151
🏁 Script executed:
# Search for trap, ||, or error handling patterns in bats files
rg 'trap|set -e|set -o||| true' hack/e2e-apps/*.bats | head -30Repository: cozystack/cozystack
Length of output: 1757
Cleanup won't run if any preceding command fails — consider wrapping the delete in a Bash trap or adding error handling.
If any kubectl command before the delete on line 47 fails (e.g., a wait timeout or assertion), execution halts and the Harbor resource is never removed, leaving orphaned resources in the namespace. While most E2E test suites use a teardown() function for guaranteed cleanup, this codebase consistently applies inline cleanup at the end of each test. To stay consistent with that pattern (bucket.bats, postgres.bats, etc.), either add explicit error handling around critical commands or wrap the test body in a trap to ensure cleanup always runs.
🤖 Prompt for AI Agents
In `@hack/e2e-apps/harbor.bats` around lines 3 - 48, The test "Create Harbor" can
leave orphaned resources if any earlier kubectl command fails; add a Bash trap
at the top of the test to always run the existing cleanup (the kubectl -n
tenant-test delete harbor.apps.cozystack.io $name command that currently appears
at the end) on EXIT or ERR, or implement an inline teardown function and
register it with trap; locate the test block (`@test` "Create Harbor") and update
it so that the trap references the same variable name 'name' and runs the exact
delete command used today to guarantee cleanup regardless of intermediate
failures.
There was a problem hiding this comment.
This follows the existing project-wide pattern — all E2E bats tests (bucket, postgres, redis, kafka, etc.) use inline cleanup at the end of each test without traps or teardown functions. The E2E framework runs each test in isolation and the namespace is cleaned up between runs.
There was a problem hiding this comment.
@lexfrei, thank you for clarifying! I understand now—the E2E framework handles namespace isolation and cleanup between test runs, so inline cleanup at the end of each test is sufficient and matches the established pattern across all E2E bats tests in this project. I appreciate the context about how the test infrastructure works.
✏️ Learnings added
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2058
File: hack/e2e-apps/harbor.bats:3-45
Timestamp: 2026-02-16T18:50:59.859Z
Learning: In the cozystack project, E2E bats tests in hack/e2e-apps/ use inline cleanup (e.g., `kubectl delete` at the end of the test) without traps or teardown functions. The E2E framework runs each test in isolation and cleans up the namespace between runs, so framework-level cleanup is preferred over test-level error handling with traps.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| @@ -0,0 +1,54 @@ | |||
| # Managed Harbor Container Registry | |||
|
|
|||
| Harbor is an open source trusted cloud native registry project that stores, signs, and scans content. | |||
There was a problem hiding this comment.
Minor grammar: hyphenate compound adjectives.
"open source" → "open-source" and "cloud native" → "cloud-native" when used as adjectives before a noun.
Proposed fix
-Harbor is an open source trusted cloud native registry project that stores, signs, and scans content.
+Harbor is an open-source trusted cloud-native registry project that stores, signs, and scans content.📝 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.
| Harbor is an open source trusted cloud native registry project that stores, signs, and scans content. | |
| Harbor is an open-source trusted cloud-native registry project that stores, signs, and scans content. |
🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: Use a hyphen to join words.
Context: ...or Container Registry Harbor is an open source trusted cloud native registry pro...
(QB_NEW_EN_HYPHEN)
[grammar] ~3-~3: Use a hyphen to join words.
Context: ... Harbor is an open source trusted cloud native registry project that stores, sig...
(QB_NEW_EN_HYPHEN)
🤖 Prompt for AI Agents
In `@packages/extra/harbor/README.md` at line 3, Update the README sentence
"Harbor is an open source trusted cloud native registry project that stores,
signs, and scans content." to hyphenate compound adjectives: change "open
source" to "open-source" and "cloud native" to "cloud-native" so it reads
"Harbor is an open-source trusted cloud-native registry project that stores,
signs, and scans content."
There was a problem hiding this comment.
README.md is auto-generated by cozyvalues-gen. The description comes from the upstream Harbor project.
There was a problem hiding this comment.
@lexfrei, thank you for the clarification! Since the README.md is auto-generated by cozyvalues-gen and the description comes directly from upstream Harbor, it makes sense not to modify it locally. Changes to the description would need to be made either upstream in the Harbor project or in the source configuration that feeds the generator.
I appreciate you explaining the generation process—this helps ensure future suggestions are more contextually appropriate.
✏️ Learnings added
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2058
File: packages/apps/harbor/README.md:3-3
Timestamp: 2026-02-16T18:38:50.741Z
Learning: README.md files in packages that are auto-generated by cozyvalues-gen should not be manually edited. These files are generated from upstream sources or configuration files.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| metadata: | ||
| name: migration-job | ||
| namespace: {{ .Release.Namespace | quote }} | ||
| labels: | ||
| {{ include "harbor.labels" . | indent 4 }} | ||
| component: migrator | ||
| annotations: | ||
| # This is what defines this resource as a hook. Without this line, the | ||
| # job is considered part of the release. | ||
| "helm.sh/hook": pre-upgrade | ||
| "helm.sh/hook-weight": "-5" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the upstream Harbor chart v1.18.2 has a hook-delete-policy in the pre-upgrade job
# Look for hook-delete-policy in the vendored chart templates
rg -n "hook-delete-policy" packages/system/harbor/charts/harbor/templates/Repository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check for any other pre-upgrade jobs or migration jobs in the chart
rg -n "migration-job\|pre-upgrade" packages/system/harbor/charts/harbor/templates/ -lRepository: cozystack/cozystack
Length of output: 45
🏁 Script executed:
#!/bin/bash
# Check if the chart directory exists and list its contents
ls -la packages/system/harbor/charts/harbor/templates/core/ 2>/dev/null || echo "Directory not found"Repository: cozystack/cozystack
Length of output: 574
🏁 Script executed:
#!/bin/bash
# List all yaml files in the harbor chart templates
find packages/system/harbor/charts/harbor/templates/ -name "*.yaml" -type f 2>/dev/null | head -20Repository: cozystack/cozystack
Length of output: 1510
🏁 Script executed:
#!/bin/bash
# Check if the specific file exists
if [ -f "packages/system/harbor/charts/harbor/templates/core/core-pre-upgrade-job.yaml" ]; then
echo "File exists. Showing content:"
cat -n "packages/system/harbor/charts/harbor/templates/core/core-pre-upgrade-job.yaml"
else
echo "File does not exist at that path"
fiRepository: cozystack/cozystack
Length of output: 3031
🏁 Script executed:
#!/bin/bash
# Check for vendor markers or documentation about the harbor chart source
find packages/system/harbor -name "Chart.yaml" -o -name "Chart.lock" | xargs cat 2>/dev/null | head -30Repository: cozystack/cozystack
Length of output: 780
🏁 Script executed:
#!/bin/bash
# Check if there's any git submodule or vendor information
if [ -f ".gitmodules" ]; then
grep -A 3 "harbor" .gitmodules
fi
# Also check for any upstream reference files
find packages/system/harbor -name "*.md" -o -name "*.txt" | xargs grep -l "upstream\|vendor" 2>/dev/nullRepository: cozystack/cozystack
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check the Harbor Chart.yaml to see the version
cat packages/system/harbor/charts/harbor/Chart.yaml | grep -E "^(name|version|description)" | head -10Repository: cozystack/cozystack
Length of output: 173
🏁 Script executed:
#!/bin/bash
# Check if this file has been modified locally or is original upstream
git log --oneline -n 10 packages/system/harbor/charts/harbor/templates/core/core-pre-upgrade-job.yaml 2>/dev/null || echo "No git history available"Repository: cozystack/cozystack
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Check if there's a wrapper chart or values overrides that address this
find packages/system/harbor -name "values*.yaml" | xargs grep -l "hook-delete-policy\|enableMigrateHelmHook" 2>/dev/nullRepository: cozystack/cozystack
Length of output: 111
🏁 Script executed:
#!/bin/bash
# Search for any Harbor Helm customization or wrapper documentation
find packages/system/harbor -type f \( -name "*.md" -o -name "*.txt" -o -name "*.yaml" \) | xargs grep -l "upstream\|vendored\|override" 2>/dev/null | head -10Repository: cozystack/cozystack
Length of output: 305
🏁 Script executed:
#!/bin/bash
# Check values.yaml for hook-delete-policy or migration hook settings
cat packages/system/harbor/charts/harbor/values.yaml | grep -A 5 -B 5 "hook-delete-policy\|enableMigrateHelmHook" || echo "Not found in values"Repository: cozystack/cozystack
Length of output: 366
🏁 Script executed:
#!/bin/bash
# Check the README for documentation about this chart
cat packages/system/harbor/charts/harbor/README.md | head -100Repository: cozystack/cozystack
Length of output: 16295
🏁 Script executed:
#!/bin/bash
# Search for hook-delete-policy usage anywhere in the harbor package
rg "hook-delete-policy" packages/system/harbor/ -r 2>/dev/null || echo "No hook-delete-policy found anywhere in harbor package"Repository: cozystack/cozystack
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Check what the recent commit actually changed
git show 5be346b0 --stat 2>/dev/null | head -30Repository: cozystack/cozystack
Length of output: 1607
🌐 Web query:
Harbor Helm chart v1.18.2 core-pre-upgrade-job.yaml hook-delete-policy
💡 Result:
In Harbor Helm chart v1.18.2, templates/core/core-pre-upgrade-job.yaml does not set the helm.sh/hook-delete-policy annotation (it only sets helm.sh/hook: pre-upgrade and helm.sh/hook-weight: "-5"). (raw.githubusercontent.com)
Per Helm’s hook docs, if no hook-delete-policy is specified, Helm defaults to before-hook-creation. (helm.sh)
Citations:
- 1: https://raw.githubusercontent.com/goharbor/harbor-helm/v1.18.2/templates/core/core-pre-upgrade-job.yaml
- 2: https://helm.sh/docs/v3/topics/charts_hooks/
helm.sh/hook-delete-policy missing — Job may block subsequent upgrades (upstream issue).
This limitation is inherited from upstream Harbor Helm chart v1.18.2. Without an explicit hook-delete-policy annotation, Helm defaults to before-hook-creation, meaning the migration Job persists after upgrade. On the next helm upgrade, this will cause issues if the old Job still exists with the same name migration-job. Additionally, the hardcoded job name is not release-scoped, risking collisions if multiple Harbor releases exist in the same namespace.
Since this is a vendored chart, do not edit it directly. Consider addressing this via a wrapper chart or values override to set a proper hook deletion policy (e.g., before-hook-creation,hook-succeeded).
🤖 Prompt for AI Agents
In
`@packages/system/harbor/charts/harbor/templates/core/core-pre-upgrade-job.yaml`
around lines 4 - 14, The migration Job named "migration-job" lacks a
hook-delete-policy and uses a hardcoded name; patch it via your wrapper chart or
values override to (1) add the annotation "helm.sh/hook-delete-policy" with a
value like "before-hook-creation,hook-succeeded" to ensure the old Job is
removed, and (2) make the Job name release-scoped (e.g., prefix/suffix the
literal "migration-job" with the release identity or use the chart's fullname
helper) so the resource name isn't global and won't collide across releases;
target the Job resource with metadata.name "migration-job" and its existing
"helm.sh/hook" annotation when applying the override.
There was a problem hiding this comment.
This is in the vendored upstream chart, not our modification.
There was a problem hiding this comment.
@lexfrei, you're absolutely right—thank you for the clarification. Since this is vendored upstream code from Harbor Helm chart v1.18.2 and the project follows the principle of not modifying vendored charts directly, my suggestion doesn't apply here.
If this limitation becomes an issue in practice (blocking upgrades due to Job persistence), you could consider working around it in your wrapper chart at packages/apps/harbor/ or reporting it upstream to the Harbor project. For now, no action needed on your part.
🧠 Learnings used
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to **/*.yaml : Do NOT directly edit upstream Helm charts in vendored charts directory; use proper chart vendoring mechanisms
Learnt from: lllamnyp
Repo: cozystack/cozystack PR: 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.
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to packages/core/** : Use Helm Chart umbrella pattern with vendored upstream charts in `charts/` directory
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to packages/system/** : Use Helm Chart umbrella pattern with vendored upstream charts in `charts/` directory
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2010
File: packages/core/installer/values.yaml:10-26
Timestamp: 2026-02-10T10:44:34.963Z
Learning: In the cozystack/cozystack repository, the Makefile uses `helm template -s` to selectively render templates for different modes (talos, generic, hosted). Because Helm evaluates all templates regardless of `-s` filtering, using `required` checks in mode-specific templates (e.g., generic-mode templates) would break the build for other modes. Therefore, avoid suggesting `required` validation for mode-specific values in Helm templates in this repository.
Learnt from: CR
Repo: cozystack/cozystack PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-25T09:45:26.524Z
Learning: Applies to packages/apps/** : Use Helm Chart umbrella pattern with vendored upstream charts in `charts/` directory
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.trivy.resourcesPreset .Values.trivy.resources $) | nindent 10 }} | ||
| {{- end }} | ||
| database: | ||
| type: internal |
There was a problem hiding this comment.
we should not use internal databases, as they get deployed as helm charts they have no stable day2 operations, please follow same pattern as for monitoring or seaweedf and provision database using CPNG as part of this application.
There was a problem hiding this comment.
Fixed. Switched to CloudNativePG (CPNG) operator, following the seaweedfs pattern.
| internal: | ||
| resources: {{- include "cozy-lib.resources.defaultingSanitize" (list .Values.database.resourcesPreset .Values.database.resources $) | nindent 12 }} | ||
| redis: | ||
| type: internal |
There was a problem hiding this comment.
While redis is usually just a cache it is not critical to keep it internal, but still better to use redis-operator to provision reliable redis HA database
There was a problem hiding this comment.
Fixed. Switched to redis-operator (RedisFailover), following the redis app pattern.
| size: {{ .Values.registry.size }} | ||
| {{- with .Values.storageClass }} | ||
| storageClass: {{ . }} |
There was a problem hiding this comment.
We probably don't need persistentVolumeClaim here, registry should be configured to store data in s3 provisioned via BucketClaim and BucketAccess from automatically targeted seaweedfs from namespace values.
There was a problem hiding this comment.
Fixed. Registry now uses S3 via BucketClaim/BucketAccess from the namespace's SeaweedFS. S3 credentials are injected into the HelmRelease via valuesFrom with targetPath.
| - name: harbor-system | ||
| path: system/harbor | ||
| - name: harbor | ||
| path: extra/harbor |
There was a problem hiding this comment.
extra is usually uised for tenant modules. Which are singleton dependencies for the other apps. From my understaing Harbor is more likely self-sufficient end-user application which can be deployed in multiple instances per tenant, and should be placed into apps
There was a problem hiding this comment.
Fixed. Moved Harbor from packages/extra/ to packages/apps/ and updated the bundle entry from system.yaml to paas.yaml.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/system/harbor/templates/redis.yaml (1)
62-64:tcp-keepalive 0disables TCP keepalive — verify this is intentional.Disabling TCP keepalive can lead to stale connections accumulating, especially in cloud/NAT environments where idle connections get silently dropped. The Redis default is 300 seconds. Consider keeping a non-zero value unless there's a specific reason to disable it.
packages/extra/harbor/templates/harbor.yaml (1)
115-123: Database connection usessslmode: disable.While internal cluster communication without TLS is common in many Cozystack deployments, this means the Harbor-to-PostgreSQL connection is unencrypted. If the cluster enforces network policies or runs in a multi-tenant environment where in-transit encryption is expected, consider supporting
sslmode: requireorverify-fullwith CNPG's auto-generated TLS certificates.packages/system/harbor/templates/database.yaml (1)
1-24: Consider whether version pinning for PostgreSQL is needed—note that no CNPG Clusters in the codebase currently pin versions.The CNPG operator will use its default PostgreSQL version when
imageNameandimageCatalogRefare omitted. While pinning versions is a best practice to prevent unexpected major version changes during operator upgrades, this pattern is consistent across all system packages (seaweedfs, keycloak, grafana, alerta). If version pinning is adopted, it should be applied systematically across all CNPG Clusters rather than Harbor alone for consistency. Otherwise, relying on operator defaults is an acceptable choice given the current codebase approach.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@packages/apps/harbor/templates/harbor.yaml`:
- Around line 11-21: The template assigns $tokenKey and $tokenCert separately,
but later renders token fields guarded only by $tokenKey which can cause an
empty cert to be emitted; update the conditional logic so the token certificate
is only emitted when both $tokenKey and $tokenCert are present (i.e., require
hasKey for "tls.crt" and check both $tokenKey and $tokenCert non-empty before
rendering the cert), or alternatively gate the emission block on the presence of
$tokenCert as well, referencing the existing variables $existingCoreSecret,
$tokenKey, $tokenCert and the hasKey checks.
- Around line 53-67: The valuesFrom + targetPath approach for injecting S3
credentials may not be supported by the Cozystack HelmRelease controller; either
verify that valuesFrom entries (the block using valuesFrom, kind: Secret, name:
{{ .Release.Name }}-registry-bucket and targetPath entries for
harbor.persistence.imageChartStorage.s3.accesskey, .secretkey, .regionendpoint)
are actually propagated end-to-end by the controller, or refactor the template
to set those Harbor values inline by looking up the bucket Secret (use the same
pattern as the admin password lookup) and assign the secret fields directly into
values.harbor.persistence.imageChartStorage.s3.accesskey / .secretkey /
.regionendpoint so credentials are present at render time.
In `@packages/apps/harbor/templates/ingress.yaml`:
- Line 16: The ingress template hardcodes cert-manager.io/cluster-issuer as
letsencrypt-prod; add a configurable value (e.g., ingress.clusterIssuer) in
values.yaml with default "letsencrypt-prod" and update the ingress.yaml template
to render that value (use {{ .Values.ingress.clusterIssuer }}), optionally
surrounding it with a conditional so the annotation is only emitted when
non-empty. Ensure you reference the same key (ingress.clusterIssuer) in both
values.yaml and the template so environments can override the issuer (staging,
custom, etc.).
- Around line 21-24: Update the Ingress template to quote the host values:
replace the unquoted occurrences of {{ $harborHost }} used for the host (both in
the tls.host entry and in the rules.host entry) with the quoted form {{
$harborHost | quote }} so YAML parsing is defensive and consistent with the
other ingress template patterns.
🧹 Nitpick comments (3)
packages/system/harbor-rd/cozyrds/harbor.yaml (1)
10-11: Consider multi-line or block-scalar format for the openAPISchema.The entire OpenAPI schema is a single ~4 KB JSON string on one line, making it nearly impossible to review diffs, spot typos, or resolve merge conflicts. Using a YAML block scalar (
|) with pretty-printed JSON (or a folded scalar) would significantly improve maintainability.That said, if this single-line format is a project convention for
ApplicationDefinitionresources, feel free to disregard.packages/apps/harbor/templates/harbor.yaml (2)
129-143: Security: database and Redis connections without encryption.
- Line 136:
sslmode: disablefor PostgreSQL — data in transit between Harbor core and the CNPG cluster is unencrypted.- Line 141: Sentinel connection (
rfs-...:26379) without TLS.For intra-cluster traffic these may be acceptable, but CNPG clusters provision TLS certificates by default. Consider using
sslmode: require(orverify-ca) to leverage the existing CNPG cert infrastructure, which would harden the deployment against network-level attacks within the cluster.
91-108: Hardcodedregion: us-east-1for SeaweedFS S3 — consider documenting or templating.The S3 region is hardcoded to
us-east-1(line 97), which is a common placeholder for S3-compatible stores like SeaweedFS. This is functionally fine, but a brief inline comment would prevent future confusion (e.g., someone thinking it targets AWS).
| {{- $existingCoreSecret := lookup "v1" "Secret" .Release.Namespace (printf "%s-core" .Release.Name) }} | ||
| {{- $tokenKey := "" }} | ||
| {{- $tokenCert := "" }} | ||
| {{- if $existingCoreSecret }} | ||
| {{- if hasKey $existingCoreSecret.data "tls.key" }} | ||
| {{- $tokenKey = index $existingCoreSecret.data "tls.key" | b64dec }} | ||
| {{- end }} | ||
| {{- if hasKey $existingCoreSecret.data "tls.crt" }} | ||
| {{- $tokenCert = index $existingCoreSecret.data "tls.crt" | b64dec }} | ||
| {{- end }} | ||
| {{- end }} |
There was a problem hiding this comment.
Minor: tokenCert could be set to empty when only tokenKey exists.
Lines 112–115 gate on $tokenKey alone but unconditionally emit tokenCert. If the core secret has tls.key but is missing tls.crt (e.g., corruption or partial restore), Harbor core will receive an empty certificate string, which would break token authentication.
Consider gating on both values:
Proposed fix
- {{- if $tokenKey }}
+ {{- if and $tokenKey $tokenCert }}
tokenKey: {{ $tokenKey | quote }}
tokenCert: {{ $tokenCert | quote }}
{{- end }}🤖 Prompt for AI Agents
In `@packages/apps/harbor/templates/harbor.yaml` around lines 11 - 21, The
template assigns $tokenKey and $tokenCert separately, but later renders token
fields guarded only by $tokenKey which can cause an empty cert to be emitted;
update the conditional logic so the token certificate is only emitted when both
$tokenKey and $tokenCert are present (i.e., require hasKey for "tls.crt" and
check both $tokenKey and $tokenCert non-empty before rendering the cert), or
alternatively gate the emission block on the presence of $tokenCert as well,
referencing the existing variables $existingCoreSecret, $tokenKey, $tokenCert
and the hasKey checks.
There was a problem hiding this comment.
Both tls.key and tls.crt are generated together by the vendored chart's core-secret.yaml template via genCA. A scenario where one exists without the other would require manual corruption of the secret. The hasKey guards on both fields independently handle this defensively.
There was a problem hiding this comment.
@lexfrei, thank you for the clarification. You're absolutely right that the vendored chart's genCA logic ensures both tls.key and tls.crt are created atomically, and the independent hasKey checks already provide defensive handling for the edge case of manual secret corruption.
Your implementation is sound—the separate guards ensure that if one field is missing, the corresponding variable simply remains empty rather than causing a template error, and Harbor will handle the absence of token credentials appropriately.
🧠 Learnings used
Learnt from: mattia-eleuteri
Repo: cozystack/cozystack PR: 1988
File: packages/extra/external-dns/templates/external-dns.yaml:22-31
Timestamp: 2026-02-12T11:03:15.183Z
Learning: In the cozystack/cozystack repository, `valuesFrom` is not correctly supported with Cozystack's HelmRelease implementation. Inline credential values in HelmRelease specs are the expected pattern, and suggestions to move credentials to `valuesFrom` with Secret references should be avoided.
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2010
File: packages/core/installer/values.yaml:10-26
Timestamp: 2026-02-10T10:44:34.963Z
Learning: In the cozystack/cozystack repository, the Makefile uses `helm template -s` to selectively render templates for different modes (talos, generic, hosted). Because Helm evaluates all templates regardless of `-s` filtering, using `required` checks in mode-specific templates (e.g., generic-mode templates) would break the build for other modes. Therefore, avoid suggesting `required` validation for mode-specific values in Helm templates in this repository.
| valuesFrom: | ||
| - kind: Secret | ||
| name: cozystack-values | ||
| - kind: Secret | ||
| name: {{ .Release.Name }}-registry-bucket | ||
| valuesKey: accessKey | ||
| targetPath: harbor.persistence.imageChartStorage.s3.accesskey | ||
| - kind: Secret | ||
| name: {{ .Release.Name }}-registry-bucket | ||
| valuesKey: secretKey | ||
| targetPath: harbor.persistence.imageChartStorage.s3.secretkey | ||
| - kind: Secret | ||
| name: {{ .Release.Name }}-registry-bucket | ||
| valuesKey: endpoint | ||
| targetPath: harbor.persistence.imageChartStorage.s3.regionendpoint |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
echo "=== Search for valuesFrom with targetPath usage in packages/apps HelmRelease templates ==="
rg -n 'targetPath' --type yaml -g 'packages/apps/**/templates/*.yaml' -B3 -A1 2>/dev/null | head -100
echo ""
echo "=== Check harbor/templates directory structure and key files ==="
find packages/apps/harbor/templates -type f -name "*.yaml" 2>/dev/null | sort
echo ""
echo "=== Look for BucketClaim or COSI bucket-related resources in harbor ==="
rg -n 'BucketClaim\|BucketAccess\|registry-bucket' --type yaml packages/apps/harbor/ 2>/dev/null | head -50
echo ""
echo "=== Check for lookup() usage pattern in harbor templates for credential injection ==="
rg -n 'lookup\(' --type yaml packages/apps/harbor/templates/ 2>/dev/null | head -50
echo ""
echo "=== Examine the actual harbor.yaml file around lines 53-67 ==="
sed -n '40,75p' packages/apps/harbor/templates/harbor.yamlRepository: cozystack/cozystack
Length of output: 2695
🏁 Script executed:
#!/bin/bash
echo "=== Read bucket.yaml to understand registry-bucket secret creation ==="
cat -n packages/apps/harbor/templates/bucket.yaml
echo ""
echo "=== Search for any conditional valuesFrom handling or special configuration ==="
rg -n 'valuesFrom\|targetPath' --type yaml packages/apps/ -B2 -A2 2>/dev/null | grep -E '(harbor|targetPath|valuesFrom)' | head -50
echo ""
echo "=== Check if bucket secret uses COSI standard or custom approach ==="
rg -n 'BucketClaim\|ObjectBucketClaim' --type yaml packages/ 2>/dev/null
echo ""
echo "=== Search for any HelmRelease controller or flux documentation references ==="
rg -n 'valuesFrom' --type md packages/apps/harbor/ 2>/dev/null
echo ""
echo "=== Check git log for recent changes to harbor.yaml and comments ==="
git log --oneline --follow -n 20 -- packages/apps/harbor/templates/harbor.yaml 2>/dev/null | head -20Repository: cozystack/cozystack
Length of output: 1149
Confirm valuesFrom with targetPath compatibility or switch to inline credentials.
The learnings from this repository establish that valuesFrom is not correctly supported with Cozystack's HelmRelease implementation. This template uses valuesFrom with targetPath to inject S3 credentials (accesskey, secretkey, endpoint) from the COSI bucket secret into the Harbor Helm values. If the Cozystack controller does not propagate these valuesFrom entries correctly, the S3 credentials will be missing at runtime, causing registry storage to fail.
Either confirm this pattern works end-to-end with the Cozystack HelmRelease controller, or refactor to use inline credential values (e.g., by using lookup to directly reference the bucket secret in the values section, similar to how the admin password is handled elsewhere).
🤖 Prompt for AI Agents
In `@packages/apps/harbor/templates/harbor.yaml` around lines 53 - 67, The
valuesFrom + targetPath approach for injecting S3 credentials may not be
supported by the Cozystack HelmRelease controller; either verify that valuesFrom
entries (the block using valuesFrom, kind: Secret, name: {{ .Release.Name
}}-registry-bucket and targetPath entries for
harbor.persistence.imageChartStorage.s3.accesskey, .secretkey, .regionendpoint)
are actually propagated end-to-end by the controller, or refactor the template
to set those Harbor values inline by looking up the bucket Secret (use the same
pattern as the admin password lookup) and assign the secret fields directly into
values.harbor.persistence.imageChartStorage.s3.accesskey / .secretkey /
.regionendpoint so credentials are present at render time.
There was a problem hiding this comment.
This is a standard Flux HelmRelease v2 feature. The valuesFrom with targetPath is processed by Flux's helm-controller, not by a Cozystack-specific controller. On first install, the BucketAccess secret may not exist yet — Flux will fail to resolve valuesFrom and retry (retries: -1) until COSI provisions the bucket and creates the secret.
There was a problem hiding this comment.
@lexfrei, thank you for the clarification! You're absolutely right—this uses standard Flux HelmRelease v2 where valuesFrom with targetPath is a core feature processed by Flux's helm-controller. The retry mechanism (retries: -1) will handle the eventual consistency when COSI provisions the bucket secret.
I appreciate you explaining the distinction between Cozystack-specific implementations and standard Flux features. This approach is correct for this use case.
✏️ Learnings added
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2058
File: packages/apps/harbor/templates/harbor.yaml:53-67
Timestamp: 2026-02-16T18:38:53.224Z
Learning: In the cozystack/cozystack repository, standard Flux HelmRelease v2 resources correctly support `valuesFrom` with `targetPath` for injecting Secret values. This is processed by Flux's helm-controller. The retry mechanism (`retries: -1`) handles cases where secrets don't exist yet on first install (e.g., COSI-provisioned bucket secrets).
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mattia-eleuteri
Repo: cozystack/cozystack PR: 1988
File: packages/extra/external-dns/templates/external-dns.yaml:22-31
Timestamp: 2026-02-12T11:03:15.183Z
Learning: In the cozystack/cozystack repository, `valuesFrom` is not correctly supported with Cozystack's HelmRelease implementation. Inline credential values in HelmRelease specs are the expected pattern, and suggestions to move credentials to `valuesFrom` with Secret references should be avoided.
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2010
File: packages/core/installer/values.yaml:10-26
Timestamp: 2026-02-10T10:44:34.963Z
Learning: In the cozystack/cozystack repository, the Makefile uses `helm template -s` to selectively render templates for different modes (talos, generic, hosted). Because Helm evaluates all templates regardless of `-s` filtering, using `required` checks in mode-specific templates (e.g., generic-mode templates) would break the build for other modes. Therefore, avoid suggesting `required` validation for mode-specific values in Helm templates in this repository.
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Before applying any fix, first verify the finding against the current code and
decide whether a code change is actually needed. If the finding is not valid or
no change is required, do not modify code for that item and briefly explain why
it was skipped.
In `@hack/e2e-apps/harbor.bats`:
- Line 44: The test uses a substring grep on the kubectl output (`kubectl -n
tenant-test get svc $release -o jsonpath='{.spec.ports[0].port}' | grep -q
'80'`) which can match ports like 8080; update the assertion to use an
exact-line match (e.g., replace `grep -q '80'` with `grep -qx '80'` or `grep -xq
'80'`) so the port value returned by the jsonpath for `$release` is exactly 80.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@hack/e2e-apps/harbor.bats`: - Line 44: The test uses a substring grep on the kubectl output (`kubectl -n tenant-test get svc $release -o jsonpath='{.spec.ports[0].port}' | grep -q '80'`) which can match ports like 8080; update the assertion to use an exact-line match (e.g., replace `grep -q '80'` with `grep -qx '80'` or `grep -xq '80'`) so the port value returned by the jsonpath for `$release` is exactly 80.hack/e2e-apps/harbor.bats (1)
44-44: Nit: Use exact match for port assertion.
grep -q '80'is a substring match — it would also pass for ports like8080or800. Usinggrep -qx '80'(exact line match) makes the assertion more precise.Proposed fix
- kubectl -n tenant-test get svc $release -o jsonpath='{.spec.ports[0].port}' | grep -q '80' + kubectl -n tenant-test get svc $release -o jsonpath='{.spec.ports[0].port}' | grep -qx '80'🤖 Prompt for AI Agents
Before applying any fix, first verify the finding against the current code and decide whether a code change is actually needed. If the finding is not valid or no change is required, do not modify code for that item and briefly explain why it was skipped. In `@hack/e2e-apps/harbor.bats` at line 44, The test uses a substring grep on the kubectl output (`kubectl -n tenant-test get svc $release -o jsonpath='{.spec.ports[0].port}' | grep -q '80'`) which can match ports like 8080; update the assertion to use an exact-line match (e.g., replace `grep -q '80'` with `grep -qx '80'` or `grep -xq '80'`) so the port value returned by the jsonpath for `$release` is exactly 80.
| @@ -0,0 +1,224 @@ | |||
| {{- $ingress := .Values._namespace.ingress }} | |||
| {{- $host := .Values._namespace.host }} | |||
| {{- $harborHost := .Values.host | default (printf "harbor.%s" $host) }} | |||
There was a problem hiding this comment.
This is probably kept since chart was moving from extra to apps.
This migth conflict in case of multiple harbors installed in a namespace
| {{- $harborHost := .Values.host | default (printf "harbor.%s" $host) }} | |
| {{- $harborHost := .Values.host | default (printf "harbor.%s" .Release.Name) }} |
There was a problem hiding this comment.
Fixed in efb9bc7 — default host now uses .Release.Name instead of namespace host.
| @@ -0,0 +1,36 @@ | |||
| {{- $ingress := .Values._namespace.ingress }} | |||
| {{- $host := .Values._namespace.host }} | |||
| {{- $harborHost := .Values.host | default (printf "harbor.%s" $host) }} | |||
There was a problem hiding this comment.
Same here
| {{- $harborHost := .Values.host | default (printf "harbor.%s" $host) }} | |
| {{- $harborHost := .Values.host | default (printf "harbor.%s" .Release.Name) }} |
|
|
||
| ## @param {Registry} registry - Container image registry configuration. | ||
| registry: | ||
| storageType: "pvc" |
There was a problem hiding this comment.
it would be nice to have s3 as an option by default, we need to use it for e2e at least
There was a problem hiding this comment.
Fixed in e7ffc21. S3 is now the only storage backend for the registry — storageType option removed entirely. BucketClaim/BucketAccess are always created. E2E test updated to use S3 via inherited BucketClass from tenant-root. Added objectstorage-controller to PackageSource dependencies.
| - kind: Secret | ||
| name: {{ .Release.Name }}-registry-bucket | ||
| valuesKey: accessKey | ||
| targetPath: harbor.persistence.imageChartStorage.s3.accesskey | ||
| - kind: Secret | ||
| name: {{ .Release.Name }}-registry-bucket | ||
| valuesKey: secretKey | ||
| targetPath: harbor.persistence.imageChartStorage.s3.secretkey | ||
| - kind: Secret | ||
| name: {{ .Release.Name }}-registry-bucket | ||
| valuesKey: endpoint | ||
| targetPath: harbor.persistence.imageChartStorage.s3.regionendpoint |
There was a problem hiding this comment.
Unfortunately it is not going to work, BucketAccess creates very weird secret eg:
apiVersion: v1
data:
BucketInfo: eyJtZXRhZGF0YSI6eyJuYW1lIjoiYmMtODE5MjhhNjAtY2EyOS00ODgwLTgxNTYtNjc5YTgwOWJhNjRmIiwiY3JlYXRpb25UaW1lc3RhbXAiOm51bGx9LCJzcGVjIjp7ImJ1Y2tldE5hbWUiOiJidWNrZXQtZGJhZWRiNTAtZWRiNC00YTdjLWI0MmUtYzNhYzZhZDc5YzFjIiwiYXV0aGVudGljYXRpb25UeXBlIjoiS0VZIiwic2VjcmV0UzMiOnsiZW5kcG9pbnQiOiJodHRwczovL3MzLmRldjUuaW5mcmEuYWVuaXgub3JnIiwicmVnaW9uIjoiIiwiYWNjZXNzS2V5SUQiOiJQTjNUVzMwTDdSMlZaTEVDS1dRQSIsImFjY2Vzc1NlY3JldEtleSI6InRPWlpyQS92VmlXcUhQNjU1U0JZbFdORllTUjFFMXlTME5xNFpLaWcifSwic2VjcmV0QXp1cmUiOm51bGwsInByb3RvY29scyI6WyJzMyJdfX0=
kind: Secret
metadata:
creationTimestamp: "2026-02-17T21:41:30Z"
finalizers:
- cosi.objectstorage.k8s.io/secret-protection
labels:
internal.cozystack.io/managed-by-cozystack: "false"
name: bucket-test
namespace: tenant-test
resourceVersion: "11800176"
uid: 051ba4d5-ac8f-4fe3-99da-14e6a0394839
type: OpaqueThere was a problem hiding this comment.
Fixed in e7ffc21. Switched to the same pattern used by packages/system/bucket/templates/secret.yaml — the system chart does lookup + fromJson(b64dec(...)) to parse BucketInfo and creates a separate Secret with REGISTRY_STORAGE_S3_* env var keys. Harbor's existingSecret support loads these via envFrom.secretRef, overriding the ConfigMap values. On first install, the lookup intentionally fails (BucketInfo not ready yet), and Flux retries until COSI provisions the bucket.
Add Harbor v2.14.2 as a tenant-level managed service with per-component resource configuration, ingress with TLS termination, and internal PostgreSQL/Redis. Includes: - extra/harbor wrapper chart with HelmRelease, WorkloadMonitors, Ingress - system/harbor with vendored upstream chart (helm.goharbor.io v1.18.2) - harbor-rd ApplicationDefinition for dynamic CRD registration - PackageSource and system.yaml bundle entry - E2E test with Secret and Service verification Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…tion
Quote resourceNames values starting with {{ to prevent YAML parser
from interpreting them as flow mappings.
Co-Authored-By: Claude <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
Replace Harbor's internal PostgreSQL with CloudNativePG operator and internal Redis with redis-operator (RedisFailover), following established Cozystack patterns from seaweedfs and redis apps. Additional fixes from code review: - Fix registry resources nesting level (registry.registry/controller) - Persist token CA across upgrades to prevent JWT invalidation - Update values schema and ApplicationDefinition Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Move Harbor from packages/extra/ to packages/apps/ as it is a self-sufficient end-user application, not a singleton tenant module. Update bundle entry from system to paas accordingly. Replace registry PVC storage with S3 via COSI BucketClaim/BucketAccess, provisioned from the namespace's SeaweedFS instance. S3 credentials are injected into the HelmRelease via valuesFrom with targetPath. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add | quote to host values in ingress for proper YAML escaping. Add cloudflare issuer type handling following bucket/dashboard pattern. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
ApplicationDefinition has prefix "harbor-", so CR name "harbor" produces HelmRelease "harbor-harbor". Use name="test" and release="harbor-test" to correctly reference all resources. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add registry.storageType parameter (pvc/s3) to let users choose between PVC storage and S3 via COSI BucketClaim. Default is pvc, which works without SeaweedFS in the tenant namespace. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…crease E2E timeout Add postgres-operator and redis-operator to PackageSource dependsOn to ensure CRDs are available before Harbor system chart deploys. Make persistentVolumeClaim conditional to avoid empty YAML mapping when using S3 storage without Trivy. Increase E2E system HelmRelease timeout from 300s to 600s to account for CPNG + Redis + Harbor bootstrap time on QEMU. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Dump HelmRelease status, pods, events, and ExternalArtifact info when harbor-test-system fails to become ready, to diagnose the root cause of the persistent timeout. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
The vendored Harbor chart does an unsafe `lookup` of the Redis auth Secret at template rendering time to extract the password. On first install, the Secret doesn't exist yet (created by the same chart), causing a nil pointer error. Failed installs are rolled back, deleting the Secret, so retries also fail — creating an infinite failure loop. Fix by generating the Redis password in the wrapper chart (same pattern as admin password), storing it in the credentials Secret, and injecting it via HelmRelease valuesFrom with targetPath. This bypasses the vendored chart's lookup entirely — it uses the password value directly instead of looking up the Secret. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Multiple Harbor instances in the same namespace would get the same default hostname when derived from namespace host. Use Release.Name instead for unique hostnames per instance. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
a6237cf to
efb9bc7
Compare
Replace PVC-based registry storage with S3 via COSI BucketClaim/BucketAccess. The system chart parses BucketInfo secret and creates a registry-s3 Secret with REGISTRY_STORAGE_S3_* env vars that override Harbor's ConfigMap values. - Add bucket-secret.yaml to system chart (BucketInfo parser) - Remove storageType/size from registry config (S3 is now the only option) - Use Harbor's existingSecret support for S3 credentials injection - Add objectstorage-controller to PackageSource dependencies - Update E2E test with COSI bucket provisioning waits and diagnostics Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
…eanup Use tenant base domain in default hostname construction (harbor.RELEASE.DOMAIN) to match the pattern used by other apps (kubernetes, vpn). Remove unused $ingress variable from harbor.yaml. Add cleanup of stale resources from previous failed E2E runs. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Change sslmode from disable to require for CNPG PostgreSQL connection, as CNPG supports TLS out of the box. Fix token key/cert preservation to verify both values are present before passing to Harbor core. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Follow the same hostname pattern as kubernetes and vpn apps: use Release.Name directly (which already includes the harbor- prefix) instead of adding an extra harbor. subdomain. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Specify initdb bootstrap with database and owner names explicitly instead of relying on CNPG defaults which may change between versions. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Add encoding, localeCollate, and localeCType to initdb bootstrap configuration to ensure fulltext search (ilike) works correctly. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
3 participants
What this PR does
Adds Harbor v2.14.2 as a managed tenant-level container registry service in the PaaS bundle.
Architecture:
apps/harbor) — HelmRelease, Ingress, WorkloadMonitors, BucketClaim, dashboard RBACsystem/harbor) from helm.goharbor.io v1.18.2system/harbor) provisions PostgreSQL via CloudNativePG and Redis via redis-operatorsystem/harbor-rd) for dynamicHarborCRD registrationKey design decisions:
E2E test: Creates Harbor instance, verifies HelmRelease readiness, deployment availability, credentials secret, service port, then cleans up.
Release note
Summary by CodeRabbit
New Features
Tests
Chores