I don't understand what this is saying. The field appears to be annotated incorrectly; it claims kubebuilder:validation:Required, but the actual API validation does not require it. Why shouldn't we just remove the annotation?

(Also, why do you mention CRDs here?)

Anyone who is creating a CRD that imports these types somewhere into their API, would fall into this switch case in the CRD schema generator and would hit kubebuilder:validation:Required rather than optional.

This means that the CRD schema that is generated would say that the Error field for their API is required.

If we removed the kubebuilder:validation:Required marker, than the next time they generate their CRD, that field would change from required to optional, which is technically a breaking change, which we probably don't want to just blanketly force upon people.

Yes, I appreciate that this is a core type, but we do know that folks import core types and generate schemas based on the markers on those core types in the wild

creating a CRD that imports these types somewhere into their API

ah, can you clarify that a little better in the comment?

and maybe the API docs for that type should warn about this too? (Actually, why does the linter exception end up in an unrelated file somewhere else rather than being a //nolint-style comment at the point where it's needed?)

ah, can you clarify that a little better in the comment?

I can certainly try!

and maybe the API docs for that type should warn about this too?

I can also give that a go

//nolint-style comment at the point where it's needed?

Using an exceptions file is not new to K/K.

At present, the only way to ignore one of the linter rules is //nolint:kubeapilinter which would ignore every rule on that line/type, which is undesirable.

It would probably be beneficial to have a no lint for the individual rules but we don't have that yet. If the exceptions are not natural like this, and folks think it's important, I can look at prioritising a way to ignore the individual rules.

@danwinship If you can check the latest updates, is the documentation clearer now?

what's the history behind // +kubebuilder:validation:Required getting added to an optional field ... that seems like a bug at first glance

Seems it was added in c970a46#diff-5ffbebb40a29f8e491f4ff1e07c618eb3edcc0d5f00a1a71445272d6767d7883, which added the field as +optional, but also +kubebuilder:validation:Required

Scanning through the comments on the PR that added it, I cannot see anything that suggests it was talked about

remind me why a required field should not be a pointer? that seems orthogonal to omitempty

If you had a field that was a pointer, but not omitempty, assuming a structured client hasn't set it, so, something like

type A struct {
	Foo *string `json:"foo"`
	Bar *string `json:"bar,omitempty"`
}

a := A{}

aData, err := json.Marshal(a)
if err != nil {
	panic(err)
}

fmt.Printf("A: %s", string(aData))

The output would be A: {"foo":null}.

Now, I know fields can be +nullable, but my understanding was that this is the rarer case, and generally fields would not be nullable. I've not personally come across a use case for this before.

In the case that the structured client hasn't expressed an opinion on this required, the likelihood is that null would be rejected by the API server as an invalid value, but not for the right reason?

For example, a required string field, I would expect to have some sort of validation that asserts that when specified, the length is greater than zero. This certainly seems to be the case for all the field validations I did check as part of working up this PR.

I also believe that the more pointers we have in our APIs, the more likely folks are to create accidental dereferencing bugs, so we should encourage not to use pointers where it makes sense.

To quote the API conventions:

Required fields have the opposite properties, namely:

* They do not have an +optional comment tag.
* They do not have an omitempty struct tag.
* They are not a pointer type in the Go definition (e.g. AnotherFlag SomeFlag).
* The API server should not allow POSTing or PUTing a resource with this field unset.

So far I've only discovered 1 example where the field is a pointer and also required, and when I reviewed the validation for that field I couldn't see any reason why changing that field to remove the pointer would have made a difference to the validation from either structured or unstructured client perspectives.

Since not every field is marked up as either +optional or +required yet, there may be more exceptions out there to discover still.

Are there examples you're aware of where the fields are required and should absolutely be pointers?

Any required scalar field where the zero-value is a valid value (e.g. 0 for int fields, false for bool fields) must be a pointer to avoid implicitly defaulting to the zero-value client-side via a typed client.

Once it's a pointer, unless you want to make it nullable as well (which you probably don't), it would also need to be omitempty to keep spurious nulls out of the serialization.

So something like this seems fairly legitimate / necessary if you want a required scalar field where the zero-value of the scalar is valid:

// +required
// +k8s:minimum=0
Foo *int32 `json:"foo,omitempty"`

So that example breaks two of the four points from the API conventions. There are always exceptions I guess.

Does this example also mean that we shouldn't be enforcing no omitempty on required fields?

For the reverse of this linter, the optionalfields, we implemented bounds checking, and the linter recommends the best approach it can based on the bounds it has available. I'm wondering if something similar could be implemented for cases like this integer one you mention.

Until we have declarative validation on fields, being smart in the linter is harder for built in types though. I could certainly go ahead and implement it, but we would have to find exceptions and mark them with the declarative validation syntax as we find them I think? As mentioned, so far there are no fields marked explicitly as required that have had this issue so far

Would you agree with:

• Required fields should not have omitempty, and should not be pointers UNLESS their zero value is a valid and accepted value, and the distinction between unset and the zero value matters

This would mean then that a string with a +k8s:minLength of 0 would also need to be pointer and omitempty

So that example breaks two of the four points from the API conventions. There are always exceptions I guess.

Hmm... I didn't remember the conventions talking about required fields not being pointers and not using omitempty. That's worth an update there... those two conventions in that direction are less hard and fast.

Something like:

Required fields have the following properties:

- They do not have an `+optional` comment tag.
- They mark themselves as required explicitly with a `+required` comment tag, or implicitly by not setting an `omitempty` struct tag.
- The API server should not allow POSTing or PUTing a resource with this field unset.
- They _typically_ do not use pointer types in the Go definition (e.g. `AnotherFlag SomeFlag`), though required fields where the zero value is a valid value may use pointer types, likely paired with an `omitempty` struct tag to avoid spurious null serializations.

Using the `+optional` comment tag, or the `omitempty` struct tag without a `+required` comment tag, causes OpenAPI documentation to reflect that the field is optional.

Yeah that makes sense to me. I can raise a PR.

So I think in terms of moving this forward, I can look at adding zero value detection based on declarative markers to this linter, and then we would have the following:

• If a field is marked required, and has incomplete +k8s markers, it should not be a pointer, and should not have omitempty
• If a field is marked required, and has +k8s markers that suggest the zero value is valid, then it should be a pointer, and should have omitempty or +nullable

This would mean that as we start marking more fields as +required during the validation gen project, we would have a way to express to this linter "it's ok for this field to be a pointer" without adding exceptions everywhere, and would help towards the generated validations in the long term

Does that sound like a reasonable plan forward to make this a useful linter rule?

• If a field is marked required, and has incomplete +k8s markers, it should not be a pointer, and should not have omitempty
• If a field is marked required, and has +k8s markers that suggest the zero value is valid, then it should be a pointer, and should have omitempty or +nullable

I'm not quite sure what "incomplete +k8s markers" means... with 0 +k8s markers, the zero value would be considered valid, right?

I think I'd phrase it something like the following:

If a field is marked required, and the zero value appears to be valid (either because of missing +k8s validation markers or validation that allows the zero value), then it should (or maybe could?) be a pointer, and if so, should either have omitempty or +nullable

I'm not quite sure what "incomplete +k8s markers" means... with 0 +k8s markers, the zero value would be considered valid, right?

Yes, sorry, I meant "doesn't have enough markers to know if the zero value is valid or not", so yes, if the markers are incomplete, the 0 value could be valid. In which case yes, the safest thing would be to pointer+omitempty it.

I think I'm coming to the realisation that while I thought this rule was low hanging fruit, it actually will rely on having complete validation on every required field across all of our types before we could really leverage it safely if we implement it correctly.

I'll work on getting the changes on the linter side and see how many exceptions it pulls up here.

And will need to catch up with the validation-gen folks to see what progress there is on markers that would allow this to be useful (minLength, minimum, maximum etc)

If a field is marked required, and the zero value appears to be valid (either because of missing +k8s validation markers or validation that allows the zero value), then it should (or maybe could?) be a pointer, and if so, should either have omitempty or +nullable

I think "should be a pointer is correct" based on the earlier comments in this thread. If the zero value is a valid choice, and the field is required, then without it being a pointer, it cannot have omitempty, and an empty marshalled object would implicitly marshal a value, removing the value of the field being required