feat: support labels/annotations on connection secrets#1009
Conversation
derbauer97
changed the title
|
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 (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThis PR extends connection secret publishing to support optional metadata (labels and annotations) via ChangesConnection Secret Metadata Support
🎯 3 (Moderate) | ⏱️ ~25 minutes Thanks for the clear changes — could you add a short note in the commit message indicating that metadata reconciliation is conditional on 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/reconciler/managed/api_test.go (1)
335-393: ⚡ Quick winGreat test coverage for the main metadata scenarios!
The two new test cases nicely cover:
- MetadataChanged: Verifies that metadata-only changes (labels/annotations) trigger secret publishing even when data hasn't changed
- MetadataOmittedPreservesExisting: Confirms that when metadata is nil in the reference, existing secret labels/annotations are preserved
This demonstrates the declarative vs. preserve behavior clearly.
One suggestion: consider adding a test case for partially-specified metadata, such as when labels are set but annotations are omitted from the metadata block (or vice versa). This would help verify the behavior when
ref.Metadatais non-nil but only one of Labels or Annotations is specified. It would clarify whether omitting a field within the metadata block means "don't touch existing values" or "clear existing values."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/reconciler/managed/api_test.go` around lines 335 - 393, Add a new table-driven test case in the same file that covers partially-specified metadata (e.g., ref.Metadata non-nil with Labels set and Annotations nil, and another variant with Annotations set and Labels nil) to assert whether labels are updated and annotations preserved/cleared; reuse the existing patterns (fields.secret using resource.ApplyFn, fake.ModernManaged, resource.LocalConnectionSecretFor, and LocalConnectionSecretWriterTo.Ref) and set expected want.published and want.err accordingly so the behavior when a metadata block is present but only one subfield is specified is explicitly validated.pkg/resource/resource.go (1)
98-117: ⚡ Quick winClarify nil vs. empty map semantics for declarative metadata reconciliation.
The metadata propagation logic looks solid overall! I have one clarifying question about the intended behavior:
When a user specifies
metadata: {}(non-nil but with nil Labels/Annotations),maps.Clone(nil)will producenilmaps in the Secret. When this Secret is applied via merge patch, willLabels: nilcorrectly clear existing labels on the server, or will the nil field be omitted from the patch (preserving existing values)?For true declarative reconciliation, setting
metadata: {labels: {}}should remove existing labels, but iflabelsis omitted withinmetadata(likemetadata: {annotations: {...}}), I want to confirm the expected behavior. Could you verify this works as intended with your patching strategy?Consider adding a test case covering partially-specified metadata (e.g., labels set but annotations omitted, or vice versa) to verify the patch semantics work correctly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/resource/resource.go` around lines 98 - 117, The current creation of the Secret in NewConnectionSecretFromObject uses maps.Clone(ref.Metadata.Labels) / Annotations which yields nil when the user provided an empty metadata block with nil labels/annotations; to ensure declarative behavior (explicit empty => clear on server) change the logic in the GetWriteConnectionSecretToReference handling path so that when ref.Metadata != nil you set Labels and Annotations to non-nil empty maps if the original slices are nil (e.g., coerce to map[string]string{} rather than leaving nil), while still cloning non-empty maps; update the code around GetWriteConnectionSecretToReference/ref.Metadata and the Secret construction (ObjectMeta.Labels/Annotations) and add unit tests exercising partial metadata cases (metadata:{} with nil labels/annotations, metadata:{labels:{}} only, metadata:{annotations:{}} only) to validate merge/patch semantics with your chosen patch strategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/reconciler/managed/api_test.go`:
- Around line 335-393: Add a new table-driven test case in the same file that
covers partially-specified metadata (e.g., ref.Metadata non-nil with Labels set
and Annotations nil, and another variant with Annotations set and Labels nil) to
assert whether labels are updated and annotations preserved/cleared; reuse the
existing patterns (fields.secret using resource.ApplyFn, fake.ModernManaged,
resource.LocalConnectionSecretFor, and LocalConnectionSecretWriterTo.Ref) and
set expected want.published and want.err accordingly so the behavior when a
metadata block is present but only one subfield is specified is explicitly
validated.
In `@pkg/resource/resource.go`:
- Around line 98-117: The current creation of the Secret in
NewConnectionSecretFromObject uses maps.Clone(ref.Metadata.Labels) / Annotations
which yields nil when the user provided an empty metadata block with nil
labels/annotations; to ensure declarative behavior (explicit empty => clear on
server) change the logic in the GetWriteConnectionSecretToReference handling
path so that when ref.Metadata != nil you set Labels and Annotations to non-nil
empty maps if the original slices are nil (e.g., coerce to map[string]string{}
rather than leaving nil), while still cloning non-empty maps; update the code
around GetWriteConnectionSecretToReference/ref.Metadata and the Secret
construction (ObjectMeta.Labels/Annotations) and add unit tests exercising
partial metadata cases (metadata:{} with nil labels/annotations,
metadata:{labels:{}} only, metadata:{annotations:{}} only) to validate
merge/patch semantics with your chosen patch strategy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0bbbb974-7635-481d-a832-95ad667e45a7
📒 Files selected for processing (6)
pkg/reconciler/managed/api.gopkg/reconciler/managed/api_test.gopkg/resource/resource.gopkg/resource/resource_test.gopkg/xcrd/crd_test.gopkg/xcrd/schemas.go
derbauer97
force-pushed
the
feat/connection-secret-metadata
branch
from
0e2d66d to
fbe2230
Compare
… and claims Add metadata.labels and metadata.annotations to writeConnectionSecretToRef for modern (namespaced) managed resources and claims via LocalSecretReference. - LocalConnectionSecretFor clones labels/annotations from ref.Metadata - connectionSecretNeedsUpdate detects metadata drift when manageMetadata=true - Omitted metadata preserves existing secret labels/annotations (merge patch) - Set metadata declaratively reconciles (merge patch replaces full map) - Shared publishConnectionSecret helper deduplicates both publishers - XRD schema includes metadata in claim writeConnectionSecretToRef validation Closes: crossplane#1008 Signed-off-by: Florian Fl Bauer <florian.fl.bauer@deutschebahn.com>
derbauer97
force-pushed
the
feat/connection-secret-metadata
branch
from
fbe2230 to
89fbad9
Compare
1 participant
Why
Crossplane 1.20's
publishConnectionDetailsTo.metadataallowed users to set labels and annotations on published connection secrets. After the migration towriteConnectionSecretToRef, this capability was lost. Users need it to integrate with secret consumers that select by labels (e.g. external-secrets, ArgoCD).Closes #1008
What
Adds
metadata.labelsandmetadata.annotationssupport towriteConnectionSecretToReffor modern (namespaced) managed resources and claims viaLocalSecretReference.Implementation
LocalConnectionSecretForclones labels/annotations fromref.Metadataonto the desired SecretconnectionSecretNeedsUpdatedetects metadata drift only whenmanageMetadata=true(i.e.spec.writeConnectionSecretToRef.metadata != nil)metadatais omitted, existing secret labels/annotations are preserved (merge patch omits nil fields)metadatais set, labels/annotations are declaratively reconciled (the merge patch replaces the full map)publishConnectionSecrethelper eliminates duplication between both publishersmetadatain claimwriteConnectionSecretToRefvalidationDependency: crossplane/crossplane
This PR depends on a change to
crossplane/crossplane/apis/v2/core/v2that adds:SecretReferenceMetadatastruct withLabelsandAnnotationsfieldsMetadata *SecretReferenceMetadatafield onLocalSecretReferenceChecklist
./nix.sh flake checkto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR.I can open this as well if this implementation is accepted