Repository navigation
[Backport release-1.3] fix(harbor): drive bucket-secret.yaml from values, gate HR on BucketInfo - #2673
Merged
Conversation
Two bugs caused the harbor E2E to fail (CI #25049445125): 1. cozy-harbor's bucket-secret.yaml called `index $existingSecret.data "BucketInfo"` unconditionally. During first-time install the COSI BucketAccess controller may create the credentials Secret as a placeholder before populating it, so `.data` is nil and the chart render crashes with `index of untyped nil`. 2. The downstream `<release>-system` HelmRelease starts reconciling immediately, in parallel with bucket provisioning, hitting bug #1 on every retry within the test's 5-minute window. The previous shape relied on `lookup` to read the Secret at render time. helm-controller's upgrade trigger is digest-based over composed values + chart artifact, so a `lookup` returning new data on a later reconcile is not enough on its own to force an upgrade — the rendered output may diverge but the digest does not. Switch to a values-driven shape: - `bucket-secret.yaml` now reads `.Values.bucket.bucketInfo` (a JSON string) and uses `dig` for safe access; if the value is empty or the expected nested fields are missing, no `*-registry-s3` Secret is rendered. - The `<release>-system` HR sources `BucketInfo` via `valuesFrom` with `targetPath: bucket.bucketInfo`. With the default `optional: false`, helm-controller will refuse to compose values until the key exists, which both gates initial reconciliation on the BucketAccess Secret being populated and forces a config-digest change (and thus a helm upgrade) when its contents change. - `bucket.secretName` is removed from the system chart's values and from the apps chart's `values:` block; the only consumer (the `lookup` call) is gone. Verified four scenarios via `helm template` against the system chart: unset bucketInfo, empty string, empty JSON object `{}`, and a fully-populated BucketInfo — only the last renders the registry-s3 Secret; the others render nothing without erroring. Signed-off-by: Myasnikov Daniil <[email protected]> (cherry picked from commit 8daf495)
The previous fix (a21c18f) sourced BucketInfo via Flux valuesFrom with `targetPath: bucket.bucketInfo`. Flux runs values with `targetPath` through Helm's `strvals.ParseInto`, which splits the value on commas as list separators. The COSI BucketInfo JSON is comma-rich, so values resolution bailed: could not resolve Secret chart values reference 'tenant-X/harbor-X-registry-bucket' with key 'BucketInfo': key "\"spec\":{\"bucketName\":\"bucket-1785...\"" has no value (cannot end with ,) Drop `targetPath`. With only `valuesKey: BucketInfo`, helm-controller unmarshals the value as YAML and merges at the chart's values root, so JSON commas stay nested instead of being split. The system chart's bucket-secret.yaml now reads `.Values.spec.bucketName` / `.Values.spec.secretS3.{accessKeyID,accessSecretKey,endpoint}`, guarded by nested `with` blocks so the registry-s3 Secret renders only when secretS3 has been populated. Gating semantics from the previous fix are preserved: with the default `optional: false`, helm-controller still refuses to compose values until the COSI BucketAccess controller writes `BucketInfo` into the Secret, and the value's content still drives the HR config-digest. Verified via `helm template` against the system chart with three scenarios: missing `spec` and `spec` without `secretS3` render nothing; full BucketInfo renders all five S3 env keys correctly. Signed-off-by: Myasnikov Daniil <[email protected]> Assisted-By: Claude <[email protected]> (cherry picked from commit 2555a9a)
The COSI BucketAccess controller is expected to populate `BucketInfo` atomically, but the prior guard (`with .Values.spec` / `with .secretS3`) only short-circuits on missing or empty inputs. A partially populated `secretS3` would still emit the registry-s3 Secret with empty `accessSecretKey`/`endpoint` values, and a missing `bucketName` was not guarded at all — silently breaking Harbor's S3 storage backend. Tighten the inner guard to require all four required fields (accessKeyID, accessSecretKey, endpoint, bucketName) before the Secret is emitted, so partial input falls into the no-render path alongside unset/empty. The body now uses dot-access on `.secretS3` for consistency with the new gate. Behaviour for fully populated input is unchanged. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]> (cherry picked from commit 5fb337d)
`hack/helm-unit-tests.sh` auto-discovers any package whose Makefile has a `test` target. `packages/system/harbor` had none, so the conditional rendering of `bucket-secret.yaml` (the path this PR actively reshapes) was un-asserted in CI and the bucket-secret guard would not be pinned against future refactors. Add the canonical `test: helm unittest .` target, matching every other `packages/system/*` package that ships a `tests/` directory (linstor-scheduler, linstor-gui, cozystack-scheduler, nfs-driver). Add `tests/bucket_secret_test.yaml` with seven cases covering: spec absent, spec empty, secretS3 absent, secretS3 empty, secretS3 partial (only one inner field), bucketName missing, and fully populated. The first six assert no Secret is rendered; the last asserts the four expected `stringData` entries plus the static region. 7/7 pass locally (`bash hack/helm-unit-tests.sh`). Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]> (cherry picked from commit 4ecb8fb)
2 of 3 tasks
myasnikovdaniil
approved these changes
May 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Backport of #2528 to
release-1.3.