Fix crd validation: Key should not be required anymore in bundle source resources

[juliocamarero]

Hi folks,

in my last contribution (#460) I made a mistake.

I didn't realize the "key" when specifying a source secret with "includeAllKeys" was still required in the CRD.
This causes the webhook to reject bundles which use the new field:

validatingwebhookconfiguration.admissionregistration.k8s.io/trust-manager configured
The Bundle "ecp-internal-cas" is invalid: spec.sources[1].secret.key: Required value

[cert-manager-prow]

Hi @juliocamarero. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[erikgb]

/ok-to-test

[erikgb]

Thanks for coming back to fix this, @juliocamarero! I was expecting a minor fix, so my main question is why you are suggesting to rename a (Go) field.

[erikgb]

Suggested change

	Key *string `json:"key"`
	Key *string `json:"key,omitempty"`

What's your motivation for renaming this Go field? I don't think it's required to do, and I assume there was a reason for the extra abstraction when this field was introduced.

[juliocamarero]

Thanks a lot for having a look so fast @erikgb 🙇

What's your motivation for renaming this Go field?

It's not just a renaming. I am using a string pointer directly instead of using a struct which embeds a string.
If you look at the autogenerated changes in the CRD, the only change is that now the key field is not required anymore, there isn't any field renamed there.

Let me explain the problem as I see it:

• The struct SourceObjectKeySelector doest not have an explicit Keyfield, instead, it has a KeySelector field of type struct which contains a Keyfield, and the KeySelector field is annotated with json:inline which makes the field keyto be implicetly a field the SourceObjectKeySelector in json.
• We want the key field of the struct SourceObjectKeySelector in json to be optional (because it shouldn't be used when includeAllKeys: true
• The field KeySelector in the SourceObjectKeySelector was already marked as optional. However, since Key is required inside the KeySelector struct, when the CRD is autogenerated, the field key is always marked as required.

These are the different approaches I tried:

• changing the KeySelector to *KeySelectorin the SourceObjectKeySelector, this didn't change anything in the CRD, key was still required
• changing the Key inside the KeySelector struct to be optional. This change made the key field optional in the CRD, but not only for the SourceObjectKeySelector, but also for several other models for which it shouldn't be optional.

I was expecting a minor fix

I was expecting a minor fix too 😅 Happy to try any other suggestion you may have :) (hopefully there is a secret trick to make this field not required in the CRD which I am not aware of 🪄 )

I only could think of another approach, which is to introduce a new struct called OptionalKeySelector to be used instead of the existing KeySelectorin the SourceObjectKeySelector. It would just contain the string pointer Key. I couldn't think of any benefit of having this additional struct for wrapping a string pointer, so that's why I used the Key directly.

Thanks again Erik!! 🙇

[erikgb]

Ok, thanks for the explanation. This makes a lot more sense now. 👍 Since the KeySelector struct is reused multiple places, I agree this must change somehow. But do we need to make the Key field a string pointer - if we set omitEmpty?

Suggested change

	Key *string `json:"key"`
	Key string `json:"key,omitEmpty"`

[juliocamarero]

You are absolutely right! I hadn't thought of this 🙈
This simplifies the logic and reduces the amount of changes need 👌

[erikgb]

Looks great @juliocamarero! Just a minor suggestion to the field description. Let me know what you think. Can you update the PR title and squash everything into a single commit before we merge?

[erikgb]

Thanks for this important fix @juliocamarero! It was required to change more code than anticipated, but I think the changes represented a simplification. 🚀

/lgtm
/approve

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erikgb

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:

• OWNERS [erikgb]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[juliocamarero]

Thanks a lot @erikgb! 🙇