Fix conversion Bundle->ClusterBundle#844
Conversation
cert-manager-prow
Bot
added
the
dco-signoff: yes
cert-manager-prow
Bot
added
the
size/L
erikgb
force-pushed
the
fix-conversion
branch
2 times, most recently
from
ebd4aed to
cb975d7
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes the conversion logic from Bundle to ClusterBundle to support user-facing migration. The changes address two main issues: handling multiple inline CA certificate sources and preserving the JKS encoder annotation for legacy support.
Changes:
- Concatenates multiple inline CA sources in Bundle into a single
inLineCAsfield in ClusterBundle - Exports the JKS annotation constant and ensures it's copied during conversion
- Adds test coverage for multi-source inline CA concatenation and JKS format annotation
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/apis/trust/v1alpha1/conversion.go | Exports AnnotationKeyJKSKey constant and implements inline CA concatenation logic with newline handling |
| pkg/bundle/controller/bundle_controller.go | Moves APIVersion/Kind assignment after conversion and adds annotation copying logic |
| pkg/apis/trust/v1alpha1/conversion_test.go | Adds unit test for multiple inline source concatenation and updates import aliases |
| test/integration/clusterbundle/migration_test.go | Adds integration test for JKS target conversion with annotation verification |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| clusterBundle := &trustmanagerapi.ClusterBundle{} | ||
| clusterBundle.APIVersion = "trust-manager.io/v1alpha2" | ||
| clusterBundle.Kind = "ClusterBundle" | ||
| clusterBundle.Name = bundle.Name | ||
| clusterBundle.Name = cb.Name | ||
| if jksKey, ok := cb.Annotations[trustapi.AnnotationKeyJKSKey]; ok { | ||
| clusterBundle.Annotations = map[string]string{trustapi.AnnotationKeyJKSKey: jksKey} | ||
| } | ||
| clusterBundle.Spec = cb.Spec | ||
| return clusterBundle, nil |
There was a problem hiding this comment.
The variable 'clusterBundle' is being created unnecessarily here, and then all fields are manually copied from 'cb'. This creates redundant code and potentially wastes resources. Instead, you should directly use and modify 'cb', then return it. The current implementation creates two ClusterBundle objects when only one is needed.
There was a problem hiding this comment.
Well, that's not entirely true, Copilot. I am NOT copying ALL fields, and I don't want most of the metadata converted from Bundle.
erikgb
force-pushed
the
fix-conversion
branch
from
cb975d7 to
cf64c7b
Compare
|
/cc @SgtCoDFish @inteon |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| clusterBundle := &trustmanagerapi.ClusterBundle{} | ||
| clusterBundle.APIVersion = "trust-manager.io/v1alpha2" | ||
| clusterBundle.Kind = "ClusterBundle" | ||
| clusterBundle.Name = bundle.Name | ||
| clusterBundle.Name = cb.Name | ||
| if jksKey, ok := cb.Annotations[trustapi.AnnotationKeyJKSKey]; ok { | ||
| clusterBundle.Annotations = map[string]string{trustapi.AnnotationKeyJKSKey: jksKey} | ||
| } | ||
| clusterBundle.Spec = cb.Spec | ||
| return clusterBundle, nil |
There was a problem hiding this comment.
The conversion result from bundle.ConvertTo(cb) includes complete ObjectMeta (all annotations, labels, etc.), but then a new ClusterBundle object is created with only selective fields copied (Name, JKS annotation, and Spec). This means any user-added labels or annotations on the Bundle (other than the JKS annotation) will not be present on the ClusterBundle. If this is intentional for Server-Side Apply semantics (to only manage specific fields), it should be documented with a comment. If user metadata should be preserved during migration, consider copying the full ObjectMeta or at least all annotations and labels from cb.
erikgb
force-pushed
the
fix-conversion
branch
from
cf64c7b to
862fd5b
Compare
SgtCoDFish
left a comment
SgtCoDFish
left a comment
There was a problem hiding this comment.
Few suggestions and comments - what do you think?
| // The following logic is not pretty, but is required as we allow multiple inline sources | ||
| // in the Bundle sources array. | ||
| // It breaks the round-trippable conversion Bundle->ClusterBundle->Bundle, | ||
| // but works for converting Bundle->ClusterBundle, and that's what we need for the migration. | ||
| cas := strings.TrimSuffix(*obj.Spec.InLineCAs, "\n") + "\n" + *in.InLine | ||
| obj.Spec.InLineCAs = &cas |
There was a problem hiding this comment.
note: I'm not massively against breaking strict round-trippability here (this should probably be a pretty rare case). Plus, I can't really see a way for this to lead to actual data loss, just a change of format. This seems OK to me!
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
erikgb
force-pushed
the
fix-conversion
branch
from
862fd5b to
fd24f40
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
SgtCoDFish
left a comment
SgtCoDFish
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Thank you for this 😁
|
[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 |
cert-manager-prow
Bot
added
the
approved
This change is extracted from #702 to make that PR smaller and easier to review.
While working on the user-facing migration, I struggled to get some of the tests to pass. After investigation, I detected some bugs that need to be fixed:
Bundlesources array, whileClusterBundleonly provides a single field for this. This mismatch (many-to-one) is a bit problematic; I considered several ways to fix this, but ended up with the simplest solution: concatenate all in-line source CA certificates into theClusterBundleinLineCAsfield when converting. This breaks round-trip conversion, but we only need conversion fromBundletoClusterBundle.