Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ReferencePolicy.Spec.From.Namespace is Optional #962

Closed
robscott opened this issue Dec 9, 2021 · 2 comments · Fixed by #964
Closed

ReferencePolicy.Spec.From.Namespace is Optional #962

robscott opened this issue Dec 9, 2021 · 2 comments · Fixed by #964
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@robscott
Copy link
Member

robscott commented Dec 9, 2021

What happened:
ReferencePolicy.Spec.From.Namespace is Optional.

What you expected to happen:
It should be required.

Anything else we need to know?:
It looks like the accidental inclusion of omitempty on that field resulted in an optional field. Thanks to @howardjohn for catching this.

I'm not actually sure what the appropriate mitigation is here. Our current versioning guidelines do not allow us to tighten validation. To me this seems to very clearly be a bug, and making this field required could be considered a bug fix, but I think that would also break API conversion. We could consider a new v1alpha2 release just to fix this, but that would also require an update to our versioning guidelines. If we want to fix this, we should probably do it before we hit beta. Alternatively, we could leave this indefinitely and update documentation to state that the lack of a namespace is invalid.

@robscott robscott added the kind/bug Categorizes issue or PR as related to a bug. label Dec 9, 2021
@youngnick
Copy link
Contributor

I don't feel like we have any good options here. Issuing a v1alpaha3 just for this feels bad, updating our guidelines feels bad, leaving this indefinitely feels even worse.

I agree that this is a bug, and we should fix it before v1beta1.

The least worst option seems to me to be updating the versioning guidelines to say "in alpha only, we can tighten validation if there's a bug in the validation, only". That is, we allow fixes to this kind of bug only, and then shout loudly about it in all our comms channels.

@jpeach
Copy link
Contributor

jpeach commented Dec 13, 2021

// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$`
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=63
type Namespace string

Does this validation (esp. the minimum length) make the field required in practice? Or is the omitempty handled before the validation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants