Gateway::Status::Addresses has a new unique type []GatewayStatusAddress

[dprotaso]

/kind cleanup
/area api

What type of PR is this?

What this PR does / why we need it:

• This allows the status.addresses type to evolve separately from the spec type (eg. to support GEP-1651)
• This came up a bunch of times in meetings/PR reviews
• Notable changes from the GatewayAddress (used in spec)
  • Type field is required to be set (dropped +optional)

Does this PR introduce a user-facing change?:

v1beta1.Gateway Spec.Addresses is now a new Go type []GatewayStatusAddress

[dprotaso]

I'd like some input from the maintainers that dropping +optional on the Type field is what we want when listing addresses in the Status

As an end-user it would make sense to me that when consuming the status I would need to know the type prior to parsing the value.

[robscott]

I'd like some input from the maintainers that dropping +optional on the Type field is what we want when listing addresses in the Status

This is a bit of a strange detail in CRDs. For defaulting to work properly, a field has to be marked as optional. So although the type is technically optional, the end result should never be empty since a default will be applied:

gateway-api/apis/v1beta1/gateway_types.go

Lines 462 to 464 in f13c905

	// +optional
	// +kubebuilder:default=IPAddress
	Type *AddressType `json:"type,omitempty"`

[dprotaso]

For defaulting to work properly, a field has to be marked as optional.

Good point. I guess the question is when implementors update the status block with addresses do we want to require this field to be explicit - or like on GET default to 'IPAddress' if empty?

[robscott]

the question is when implementors update the status block with addresses do we want to require this field to be explicit - or like on GET default to 'IPAddress' if empty?

Unfortunately we can only loosen validation, we can't go in the other direction. So in this case, even if we wanted to make this require input from implementations, we can't in a backwards compatible way.

Potentially useful - k8s defaulting happens on writes, so it should be a bit more persistent/reliable than a read-based system.

[robscott]

Thanks @dprotaso! This PR makes sense to me, but I want to make sure more meaningful related changes make it in before merging this one. IE we should probably only split this out in the same release as we're introducing new fields to GatewayStatusAddress. Adding a temporary hold until we can get that lined up as well.

/hold

[dprotaso]

rebased

[robscott]

Thanks @dprotaso! Looks like the other pieces are coming together, will leave hold in place until everything is ready.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, robscott

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

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

[robscott]

Thanks @dprotaso! Everything for this GEP looks to be lined up well. All the other PRs LGTM and have other approvals or LGTMs, will merge in sequence.

/hold cancel
/lgtm