fix: should encode additional target format just once per bundle reconcile #241
Conversation
jetstack-bot
added
dco-signoff: yes
|
/retest |
|
@aidy - this might be interesting to review. |
pkg/bundle/sync.go
Outdated
| resolvedBundle.data = strings.Join(bundles, "\n") + "\n" | ||
|
|
||
| resolvedBundle.binaryData = make(map[string][]byte) | ||
| err := populateAdditionalFormatData(resolvedBundle.data, bundle.Spec.Target, resolvedBundle.binaryData) | ||
| if err != nil { | ||
| return bundleData{}, fmt.Errorf("failed encoding additional formats: %w", err) | ||
| } |
There was a problem hiding this comment.
nitpick (non-blocking): This could be refactored a little more - maybe as a method on bundleData. It's a no-op though so I'm definitely not going to block over it!
func (b *bundleData) populateData(pemData []byte, target trustapi.BundleTarget) error {
b.data = pemData
b.binaryData = ...
}There was a problem hiding this comment.
Yeah, I didn't want to change too much to make it easier for #244. That was also noted in the PR description. But I'll take a look and see if this can be slightly more refactored now.
There was a problem hiding this comment.
Ok, I refactored some more. Hope this is closer to what you had in mind. PTAL!
There was a problem hiding this comment.
Oh totally my bad - I missed that in the description. I would've totally been fine with you skipping this with that in mind! Thank you for doing it though 😁
jetstack-bot
added
do-not-merge/hold
…ncile Signed-off-by: Erik Godding Boye <egboye@gmail.com>
erikgb
force-pushed
the
refactor-additional-formats
branch
from
459895c
to
efc3c6a
Compare
|
/test pull-trust-manager-verify |
1 similar comment
|
/test pull-trust-manager-verify |
|
[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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/unhold |
jetstack-bot
removed
the
do-not-merge/hold
While working on something else, I discovered that if a bundle specifies any additional format in targets, the JKS/PKCS12 is encoded for each and every target configmap/secret. This seems wrong to me, as encoding in these binary trust store formats is a quite heavy operation.
This PR moves the encoding of any additional target formats into
buildSourceBundlemethod - which is executed once per bundle reconciliation. I tried to make this fix/refactoring as minimal as possible to not affect the WIP on #235 too much. We could eventually improve this even more in a follow-up PR - especially the tests, which have a lot of duplication at present.