Repository navigation
fix(harbor): mark credentials Secret with user-secret label - #2541
Conversation
The Helm-managed harbor-{name}-credentials Secret was not visible
through the TenantSecrets API because the lineage webhook compared the
real Secret name against the rendered template "{{ .name }}-credentials"
(which becomes "{name}-credentials" without the "harbor-" HelmRelease
prefix), failed to match, and labelled the Secret as
internal.cozystack.io/tenantresource=false.
Adopt the same pattern Bucket and RabbitMQ already use: stamp the
user-facing Secret with apps.cozystack.io/user-secret=true and add a
matchLabels rule alongside the existing resourceNames entry in the
ApplicationDefinition. The webhook now matches via labels instead of
relying on Secret-name templating, so the Secret is properly marked
tenantresource=true and surfaces in tenant-aware clients.
Signed-off-by: IvanHunters <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe pull request adds label-based secret discovery to Harbor's configuration. A Kubernetes label is added to credentials secrets, and the Harbor ApplicationDefinition is updated to discover secrets using this label alongside existing name-based matching. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
Summary of ChangesHello, 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 addresses an issue where Harbor credentials were not appearing in the TenantSecrets API because the lineage webhook failed to match the Secret name due to Helm naming conventions. By adopting the standard labeling pattern used by other system components, the webhook can now reliably identify and expose these credentials to tenant-aware clients. 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. 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a label-based approach for identifying Harbor credential secrets by adding the apps.cozystack.io/user-secret: "true" label to the secret and updating the Harbor RDS configuration to match it. Feedback suggests removing the redundant and broken resourceNames entry in the RDS configuration to improve maintainability and avoid confusion.
| - matchLabels: | ||
| apps.cozystack.io/user-secret: "true" |
There was a problem hiding this comment.
The addition of matchLabels correctly addresses the visibility issue for the credentials secret. However, as noted in the PR description, the existing resourceNames entry (line 35) is currently broken due to the harbor- prefix mismatch. Since the label-based approach is more robust and aligns with the pattern used in other packages like Bucket and RabbitMQ, the redundant resourceNames entry could be removed in a future cleanup to keep the configuration clean and avoid confusion.
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
APPROVE — fixes the stated bug correctly. The matchLabels approach makes the credentials Secret discoverable independently of Helm's release-name prefix and matches the Bucket/RabbitMQ pattern.
Three non-blocking follow-ups for separate PRs (do not gate this one):
-
The existing
resourceNames: ["{{ .name }}-credentials"]entry inharbor-rd/cozyrds/harbor.yamlis now dead code —release.prefix: "harbor-"makes the chart-managed Secretharbor-{name}-credentials, which never matches that template. Bucket and RabbitMQ include the prefix in their resourceNames (bucket-{{ .name }}-credentials,rabbitmq-{{ .name }}-default-user) and pair it with matchLabels for defense in depth. Either fix this line toharbor-{{ .name }}-credentialsto mirror that, or drop the dead entry. -
The same prefix mismatch likely affects
services.include({{ .name }}) andingresses.include({{ .name }}-ingress) in the same file. The Helm chart setsharbor.fullnameOverride: {{ .Release.Name }}so the rendered names areharbor-{name}*. If the lineage webhook is not prefix-aware for those resource kinds either, Harbor's TenantServices and TenantIngresses are silently broken too. Worth a follow-up. -
Pre-existing docs gap (predates this PR, do not gate): the Harbor docs page does not document how to retrieve the admin password. Now that this PR makes the credentials Secret reachable via the TenantSecrets API, the docs can finally describe that workflow — tracked in cozystack/website#522.
…4170) <!-- Thank you for making a contribution! Here are some tips for you: - Use Conventional Commits for the PR title: `type(scope): description` - Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore - Scopes are not an exhaustive list — pick the most specific scope for the change and extend the list when a genuinely new area appears. Examples: - System components: dashboard, platform, operator, cilium, kube-ovn, linstor, fluxcd, cluster-api - Managed apps: postgres, mariadb, redis, kafka, clickhouse, virtual-machine, kubernetes - Development and maintenance: api, hack, tests, ci, docs, maintenance - Breaking changes: append `!` after type/scope (`feat(api)!: ...`) or add a `BREAKING CHANGE:` footer - If it's a work in progress, consider creating this PR as a draft. - Don't hesistate to ask for opinion and review in the community chats, even if it's still a draft. - Add the label `kind/backport` if it's a bugfix that needs to be backported to a previous version. --> ## What this PR does `harbor` declares `release.prefix: "harbor-"` and then selects its Secret, Service and Ingress without it, so all three `resourceNames` selectors match nothing. This corrects the three names and adds a guard so the next omission fails a test instead of passing silently. ### Why they never matched Release names are `<prefix><app name>`, but the lineage webhook evaluates selectors against `{kind, name, namespace}`, where `name` is the custom resource's name and carries no prefix. Every definition therefore spells the prefix out itself, and the others do — `postgres-{{ .name }}-credentials`, `bucket-{{ .name }}-credentials`, and so on. `harbor` is the only one that does not. A selector that matches nothing raises no error: the webhook labels the object `internal.cozystack.io/tenantresource=false` and continues, which is indistinguishable from a selector that legitimately has nothing to match. That is why this survived. ### What actually changes at runtime Close to nothing, and that is the point — the manifest was wrong, not the behaviour: - the Secret already reaches tenants, because #2541 worked around this exact mismatch by stamping `apps.cozystack.io/user-secret: "true"` on it and matching by label. That workaround is deliberately left in place; - the Service and Ingress labels flip from `false` to `true`, which nothing consumes: `spec.services` and `spec.ingresses` are read only by the webhook that computes the label, and the label's only consumer is the TenantSecret registry, which handles Secrets. The dashboard reaches both through the per-release Role, which names them correctly. So this closes a latent defect before it bites, rather than fixing an outage. ### The guard `hack/check-rd-release-prefix.bats` asserts that every `resourceNames` entry in `packages/system/*-rd/cozyrds/*.yaml` contains `<prefix>{{ .name }}`. `hack/*.bats` is auto-discovered by `BATS_UNIT_FILES`, so it runs under `make unit-tests` with no registration. The assertion is "contains", not "starts with", because operator-created objects legitimately carry leading words of their own: mongodb excludes `internal-mongodb-{{ .name }}-users`, which is correct and stays green. Definitions with an empty prefix are skipped, since `hack/update-crd.sh` generates the definitions for `packages/extra` charts with `PREFIX=""`. A selector that is correct and still fails the check can opt out on its own with a trailing `# cozy-lint:ignore rd-release-prefix -- <reason>` comment. A directive without a reason does not count. Verified by mutation: the guard fails on the three harbor selectors before the fix and passes after. Across the rest of the tree `harbor` was the only violation. ### What this PR does not do It does not settle whether the prefix belongs in the template context instead. That would remove the duplication, but it requires migrating the definitions that spell the prefix out today, or they would double it. The guard is the cheaper half and closes the class either way. ### Screenshots <!-- REQUIRED for UI changes: attach screenshots or screen recordings demonstrating the visual impact of your changes. PRs with UI changes without screenshots will not be merged. --> ### Downstream repositories <!-- Cozystack is upstream for repositories that are NOT kept in sync with it automatically, and nothing in CI compares the two sides, so a change here can break them silently. Which change forces what, and which file to touch, is the trigger map in docs/agents/contributing.md under "Downstream Repositories". Walk it before you tick anything. IF YOU ARE AN AI AGENT OR AN LLM, THIS IS PART OF THE TASK, NOT A FORM TO FILL IN: 1. Walk the trigger map against the actual diff of this PR, file by file. Do not decide from the PR title or from your memory of what you changed. 2. For every repository you tick, open the follow-up PR there and link it on the same line. A ticked box with no link claims work that does not exist, which is worse than an empty box. Search that repository for an open PR or issue covering it first, and link that instead of filing a duplicate. 3. When the follow-up is out of scope, or needs a decision that is not yours to make, open an issue in that repository instead, link that, and say so in the PR body. Do not push a speculative PR into another repository just to fill a line here. 4. Do not tick "No downstream repository is affected" to make the checklist go away. If you are unsure, leave every box empty and say so in the PR body, so a human decides. --> - [x] No downstream repository is affected by this change - [ ] [cozystack/website](https://github.com/cozystack/website) - follow-up: - [ ] [cozystack/terraform-provider-cozystack](https://github.com/cozystack/terraform-provider-cozystack) - follow-up: - [ ] [cozystack/ansible-cozystack](https://github.com/cozystack/ansible-cozystack) - follow-up: - [ ] [cozystack/ccp](https://github.com/cozystack/ccp) - follow-up: - [ ] [cozystack/talm](https://github.com/cozystack/talm) - follow-up: - [ ] [cozystack/cozyhr](https://github.com/cozystack/cozyhr) - follow-up: - [ ] [cozystack/cozy-proxy](https://github.com/cozystack/cozy-proxy) - follow-up: - [ ] [cozystack/cozystack-telemetry-server](https://github.com/cozystack/cozystack-telemetry-server) - follow-up: - [ ] [cozystack/external-apps-example](https://github.com/cozystack/external-apps-example) - follow-up: - [ ] [cozystack/examples](https://github.com/cozystack/examples) - follow-up: - [ ] [cozystack/community](https://github.com/cozystack/community) - follow-up: Trigger map walked against the diff, which touches `packages/system/harbor-rd/cozyrds/harbor.yaml` and adds `hack/check-rd-release-prefix.bats`. The two nearest entries were checked and neither fires: - **terraform-provider-cozystack** lists "change `release.prefix`, or rename an output Secret or Service". Neither happens here: `release.prefix` is untouched and no object is renamed. The provider concatenates the prefix itself to derive names, so it was already computing the correct ones — it is this definition's selectors that were wrong. - **ccp** lists "rename `packages/system/<name>-rd/cozyrds/<name>.yaml`" and "move or rename anything under `hack/`". The file is edited, not renamed, and the new bats file is an addition; no make target changes behaviour. **website** would fire on a change to `ApplicationDefinition` semantics. Semantics are unchanged — one definition's data is corrected — and `content/en/docs/next/cozystack-api/application-definitions.md` documents neither `resourceNames` nor the prefix rule, so there is nothing there to bring in step. ### Release note <!-- Write a release note: - Explain what has changed internally and for users. - Start with the same `type(scope):` prefix as in the PR title - Follow the guidelines at https://github.com/kubernetes/community/blob/master/contributors/guide/release-notes.md. --> ```release-note fix(harbor-rd): the ApplicationDefinition selectors for Harbor's Secret, Service and Ingress now carry the release prefix, so they match the objects the chart creates. No user-visible change — the Secret already reached tenants through a label-based workaround, and nothing consumes the Service and Ingress labels. ``` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Updated Harbor resource naming patterns to consistently include the `harbor-` prefix for credentials, services, and ingress resources. - Improved alignment between Harbor resource names and their configured release prefixes. - **Tests** - Added validation to detect missing release prefixes in secret, service, and ingress resource selectors across system application definitions. - Validation reports all affected selectors and supports documented exceptions where necessary. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
What this PR does
{{ .Release.Name }}-credentialsSecret withapps.cozystack.io/user-secret: "true".matchLabelsrule tosecrets.includein the HarborApplicationDefinition, alongside the existingresourceNamesentry.The lineage webhook decides whether a Secret is tenant-visible by matching its name against the
secrets.include[].resourceNamestemplate in the correspondingApplicationDefinition. For Harbor the template{{ .name }}-credentialsrenders to{name}-credentialsand does not account for therelease.prefix: "harbor-"set in the same definition, while the Helm chart actually createsharbor-{name}-credentials. The names never match, so the Secret ends up labelledinternal.cozystack.io/tenantresource=falseand never appears in the TenantSecrets API.This change follows the existing
apps.cozystack.io/user-secret: "true"pattern already used by Bucket (packages/system/bucket/templates/user-credentials.yaml) and RabbitMQ (packages/apps/rabbitmq/templates/rabbitmq.yaml). The webhook then matches via labels and is independent of how Helm names the Secret.Screenshots
Not applicable — no UI changes in this PR.
Release note
Summary by CodeRabbit