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.

Sign up for GitHub

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

Add exception to pointer guidance for structs that must be omitted #1411

Merged
merged 1 commit into from
Oct 9, 2024
Merged

Add exception to pointer guidance for structs that must be omitted #1411

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions dev-guide/api-conventions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,18 @@ This means, that any end user fetching the resource from the API can observe tha
This has the effect of helping our users to understand how they might be able to configure their cluster, without
having to search for features or review API schemas within the product docs.

##### Pointers to structs

An exception to this rule is when using a pointer to a struct in combination with an API validation that requires the field to be unset.

The JSON tag `omitempty` is not compatible with struct references. Meaning any struct will always, when empty, be marshalled to `{}`. If the struct **must** genuinely be omitted, it must be a pointer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

under what condition must the struct be omitted? Discriminated unions, yes. But anywhere else? I would expect the zero value to be exactly the same as nil and missing.

Copy link
Contributor Author

@JoelSpeed JoelSpeed Jun 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A concrete example is the API we are adding for the OpenStack failure domains to control plane machine set.

We don't want to allow an empty failure domain in the list, it must have at least one non-empty field.

- {} // invalid, no fields set in the failure domain
- rootVolume: {} // rootVolume is a struct, this entry in the list is still invalid
- availabilityZone: foo // valid, one field is non-empty
- rootVolume:
    availabilityZone: foo // valid, one field is non-empty
- availabilityZone: foo
  rootVolume:
    availabilityZone: foo // Also valid, more than one field is non-empty

To achieve this set of requirements, we use

// +kubebuilder:validation:MinProperties:=1
type OpenStackFailureDomain struct {
  // +optional
  AvailabilityZone string
  // +optional
  RootVolume *RootVolume
}

// +kubebuilder:validation:MinProperties:=1
type RootVolume struct {
  // +optional
  AvailabilityZone string
}

If RootVolume is not a pointer, when serialized from Go, the JSON library will serialize it as {}, so an empty OpenStackFailureDomain struct becomes {"rootVolume":{}} in JSON. Because the field is then "present", the validation runs and it is rejected because rootVolume does not have 1 property. That case is ok.

But, the issue comes if I want to create:

foo := OpenStackFailureDomain{
  AvailabilityZone: "foo",
}

With a pointer it marshals to {"availabilityZone": "foo"}, without a pointer it marshals to {"availabilityZone": "foo","rootVolume":{}}, which then fails validation because rootVolume does not have 1 property.

Is there some alternative way to achieve the validation we want?

A GoPlayground to demonstrate the behaviour of json.Marshal.

And the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Can you mention all of it in the document?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a specific part you think is particularly useful? I worry adding too many in depth examples will lead to the document being overwhelmingly long and complicated to read

Perhaps a link to this thread would make that easier?

Copy link
Member

@ingvagabund ingvagabund Jun 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link works as well (assuming the thread will not get removed by accident).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So while I understand that it makes the validation slightly easier, for a configuration resource, why shouldn't we expect a zero-value of rootVolume to behave the same as a nil rootVolume?

For a workload resource, I definitely agree it should be a pointer?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One example would be a discriminated union. We use those on configuration resources regularly. The DU needs a required discriminator, but, you cannot have a required field in a non-pointer struct. So any DU must be a pointer to struct independent of configuration or not.

In other examples where we want a required field in a struct, say for example an optional reference to another object with a namespace. If you specify the reference, we want the name and namespace field within to be required.

Is it better here to use CEL to check for "all required values are empty" and allow that case? Does that become a little confusing to the user?


When used in combination with a validation such as `minProperties` on the parent, or a required field within the struct, the struct itself must be
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Always useful to provide a simple example. I prefer to first check examples and then read in more detail all the limitations/constraints.

omitted to avoid validations being applied to the empty struct and returning
false positives.

For a concrete example, view this [thread](https://github.com/openshift/enhancements/pull/1411/files#r1212790070).

### Only one phrasing for each idea

Each idea should have a single possible expression in the API structures, without having multiple ways to say the same thing.
Expand Down