User-facing migration to ClusterBundle#702
Conversation
ec6c107 to
551e847
Compare
551e847 to
d9dd750
Compare
0b8b584 to
9dac46d
Compare
2da597b to
f9e3118
Compare
2062dd2 to
f114776
Compare
f114776 to
b87a514
Compare
b87a514 to
688c870
Compare
9947942 to
b456e1d
Compare
b456e1d to
b87b744
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
255e0ab to
83d044c
Compare
83d044c to
b18814e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
pkg/bundle/internal/target/target.go:423
TrustBundleHashiterates overtarget.GetAnnotations()andtarget.GetLabels()maps directly. Go map iteration order is randomized, so this makes the computed hash nondeterministic when there are 2+ labels/annotations, potentially causing perpetual “needs update” churn. Consider hashing these maps in a deterministic order (e.g., sort keys first) before writing to the hasher.
// Add Target annotations and labels to the hash so it becomes aware of changes and triggers an update.
for k, v := range target.GetAnnotations() {
_, _ = hash.Write([]byte(k + v))
}
for k, v := range target.GetLabels() {
_, _ = hash.Write([]byte(k + v))
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
b18814e to
4a15924
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
4a15924 to
29415dc
Compare
29415dc to
0b406bb
Compare
0b406bb to
ab4112f
Compare
ab4112f to
307c211
Compare
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
c9982cf to
203f248
Compare
| // This is done to prevent mistakes. TLS Secrets should never include keys | ||
| // matching the private-key entry, including via wildcard patterns. | ||
| if secret.Type == corev1.SecretTypeTLS { | ||
| matchesPrivateKey, err := path.Match(b.ref.Key, corev1.TLSPrivateKeyKey) | ||
| if err != nil { |
SgtCoDFish
left a comment
There was a problem hiding this comment.
/lgtm
/approve
/hold
I think the test comment from AI is a legitimate find but if you tell me it's fine I'm willing to roll with it.
As such, LGTM with a hold. If you agree the test comment is legit, then you can fix it and ping me for a re-review. If you disagree, just unhold and merge.
🚀
|
|
||
| found := false | ||
| for k, v := range data { | ||
| ok, err := path.Match(b.ref.Key, k) |
There was a problem hiding this comment.
note: This will mean that Bundle resources could now set a key of *.crt and it would start matching, right?
I guess that's fine - probably worth calling out in the release notes.
| var bundleList trustmanagerapi.ClusterBundleList | ||
| if err := b.client.List(ctx, &bundleList); err != nil { | ||
| logf.FromContext(ctx).Error(err, "failed to list all Bundles, exiting error") | ||
| os.Exit(-1) |
There was a problem hiding this comment.
suggestion (non-blocking): For a different PR but seeing os.Exit here spooks me so much. Obviously not changed here, but I can't believe this is the best way to handle this.
| }, | ||
| expNeedsUpdate: true, | ||
| }, | ||
| "if object exists with owner but no data, expect update": { |
There was a problem hiding this comment.
suggestion(AI): target_test.go test fixtures still assert with old Kind: "Bundle" owner references
E.g. pkg/bundle/internal/target/target_test.go:130-138. The tests pass because metav1.IsControlledBy(obj, clusterBundle) returns false (mismatched kind), forcing needsUpdate=true — i.e. these tests pass "for the right outcome, the wrong reason." They're effectively also testing the migration-path side (old Bundle-owned target needs an update), which is useful, but a couple of explicit "object already owned by ClusterBundle" test cases would help guard the steady-state path.
I reckon this looks like a legit finding so I'm raising it as a comment!
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SgtCoDFish The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR introduces the new ClusterBundle CRD and enables the migration controller according to the accepted design.
The main controller is changed to reconcile ClusterBundle instead of Bundle.
I've tried to keep this PR as small as possible to make it easier to review. We will likely need to add additional tests for the new features in the ClusterBundle API (e.g., multiple keys) and consolidate the existing tests due to the more generic API. However, I suggest handling this in follow-up PRs. Let me know what you think!