Skip to content

[Backport release-1.3] fix(harbor): drive bucket-secret.yaml from values, gate HR on BucketInfo - #2673

Merged
myasnikovdaniil merged 4 commits into
release-1.3from
backport-2528-to-release-1.3
May 18, 2026
Merged

myasnikovdaniil merged 4 commits into
release-1.3from
backport-2528-to-release-1.3

Conversation

@github-actions

Copy link
Copy Markdown

Description

Backport of #2528 to release-1.3.

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)
@dosubot dosubot Bot added area/release Issues or PRs related to release tooling (changelog, backport, release pipeline) area/storage Issues or PRs related to storage (linstor, seaweedfs, bucket, velero, harbor) kind/backport Categorizes issue or PR as requiring a backport to the current release line labels May 18, 2026
@myasnikovdaniil
myasnikovdaniil merged commit d71a2e9 into release-1.3 May 18, 2026
7 checks passed
@myasnikovdaniil
myasnikovdaniil deleted the backport-2528-to-release-1.3 branch May 18, 2026 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/release Issues or PRs related to release tooling (changelog, backport, release pipeline) area/storage Issues or PRs related to storage (linstor, seaweedfs, bucket, velero, harbor) kind/backport Categorizes issue or PR as requiring a backport to the current release line

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant