New Linter: NonPointerStructs

[JoelSpeed]

Per conversation at the maintainer summit on Sunday, for built-in types, we must follow these rules

• Any non-pointer struct field that has a required field, must be marked required
• Any non-pointer struct field that has no required fields, must be marked optional
• Any non-point struct that validates a minimum number of properties (e.g. a non-discriminated union must have exactly one field set) must be marked as required
• To have a required struct with no required fields, the struct must be a pointer
• To have an optional struct with required fields, the struct must be a pointer

This new linter is enforcing the first half of this, I believe existing linters that we have will cover the second half already (see optionalfields and requiredfields)

/assign @thockin

@thockin Can you please confirm the above is what we agreed

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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 [JoelSpeed]

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

[thockin]

I concur with the description WRT non-pointer struct fields.

For builtin types with standard Go clients, the struct will be serialized as {} in JSON or the tag number in proto (unless we implement omitzero, which changes the serialization). For "required", validation will produce a "field is required" error on the interior required fields. For "optional", validation will see the absence of all interior optional fields.

For types which use other clients and implement something closer to the "everything is a pointer" ideal, serialization might not have the struct at all. For "required", validation will produce a "field is required" error on the struct key itself, and for "optional" it will see the absence of the struct key.

Slightly different errors, but same count and flavor, and sufficiently unambiguous (subjective).

Now, should we talk about non-pointer primitive fields WRT optional/required and the impact of "omitempty"? :)

We have fields that are required strings which use omitempty and which do not.

We have fields that are required ints, but none I could see use omitempty.

We have fields that are optional strings which use omitempty and which do not.

We have fields that are optional ints, but AFAICT they all use omitempty.

So we get to think about all those modes.

[lalitc375]

Any non-pointer struct field that has a required field, must be marked required
Any non-pointer struct field that has no required fields, must be marked optional

I don't understand the reasoning behind these two rules. It seems this would limit the evolution of CRDs.

For example, consider the following scenario:

type MySpec struct {
    // +required
    Foo Foo
}

type Foo struct {
    // +required
    field1 string
    // +optional
    field2 string
}

Now, let's say a developer wants to add a new field, Bar, to MySpec and use it in a union with the existing Foo field. To do this, they would likely want to make Foo optional.

type MySpec struct {
    // +required -> +optional  (This change would be blocked by the linter)
    Foo Foo
    // +optional
    Bar *Bar
}

type Foo struct {
    // +required
    field1 string
    // +optional
    field2 string
}

type Bar struct {
    // +required
    field3 string
    // +optional
    field4 string
}

The proposed linting rule would prevent changing Foo from +required to +optional. While converting Foo to a pointer type (*Foo) would be a backward incompatible change, this lint rule appears to restrict a valid evolution path for the API.

What is the recommended approach for developers in this situation?

[thockin]

While converting Foo to a pointer type (*Foo) would be a backward-compatible change

How is that compatible? Anyone using your typed client is broken.

Assuming you're OK with that, the next rule applies:

Any non-point struct that validates a minimum number of properties (e.g. a non-discriminated union must have exactly one field set) must be marked as required

If Foo and Bar represent a union, then fields of type MySpec (note: not *MySpec) must be +required. This ensures that an empty MySpec is never valid.

We can't even do tricks like decoding into FooApplyConfiguration because Go clients will send empty structs in some cases.

We could have different rules for builtins vs. CRDs but it seems minimally useful for the cost?

[lalitc375]

How is that compatible? Anyone using your typed client is broken.

Yes, this is backward incompatible change. I made a typo in my last comment.

We could have different rules for builtins vs. CRDs but it seems minimally useful for the cost?

I agree that CRDs should have constraints that are as consistent as possible with native types. CRDs are often published and consumed as libraries, where backward compatibility is just as critical. This is precisely why a linting rule that prevents this common and backward-compatible evolution pattern would be problematic for the CRD ecosystem. It would force developers into making breaking changes where they would otherwise not be necessary.

[thockin]

If you can't change Foo to a pointer, you can't really do a one-of without making your clients exceedingly wierd.

I think it's allowed to change a struct with one field into a one-of, assuming you knew that was coming. That would appear to violate "Any non-pointer struct field that has no required fields, must be marked optional" but "Any non-point(sic) struct that validates a minimum number of properties (e.g. a non-discriminated union must have exactly one field set) must be marked as required" also applies. Perhaps:

1. Any non-pointer struct field that has a required field, must be marked required.
2. Any non-pointer struct field that has no required fields, must be marked optional, unless there is something like a union where each field is optional but one of them must be specified, in which case the whole set of fields is considered required together. See rule 1.
3. To have a required struct with no required fields, the struct must be a pointer.
4. To have an optional struct with required fields, the struct must be a pointer.

[lalitc375]

If you can't change Foo to a pointer, you can't really do a one-of without making your clients exceedingly wierd.

It can be achieved like this. https://go.dev/play/p/Nabp1xS4UmV .

Are we sure all native types are conforming with these rules?

[thockin]

Discussed a bit IRL. The problem is that a non-pointer struct field which only has optional fuels, but is "required" is meaningless unless we do some validation on the serialized form, and even then is still ambiguous wrt Go clients. To go with the example above, suppose a client has:

MySpec { Bar: something() }

The serialized form will still have Foo in it (unless we overhaul all the omitempty / omitzeto logic in ALL serializations).

The same problem also exists in reverse. If I have an optional struct-type field with a required child field, I can't tell if you intended to invoke the optionalness or if you should get an error because of requiredness.

So it is simpler just say "you can't do that".

[lalitc375]

Now, I understand the reasoning behind these. Thank you for explaining this, @thockin . There will be a few exemptions, mostly for discriminated unions, that should be fine.