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

Update AddressType definition to add domain-prefixed strings as an option #1178

Merged
merged 3 commits into from
Jun 7, 2022
Merged

Update AddressType definition to add domain-prefixed strings as an option #1178

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2589a92
Update AddressType definition to add domain-prefixed strings as an op…
Jun 1, 2022
8e3d6ba
Added webhook validation for AddressType fields
Jun 6, 2022
0d6eaff
Update with requested changes and regenerate
Jun 6, 2022
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
1 change: 0 additions & 1 deletion apis/v1alpha2/gateway_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,7 +450,6 @@ type GatewayAddress struct {
// Type of the address.
//
// +optional
// +kubebuilder:validation:Enum=IPAddress;Hostname;NamedAddress
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed this validation because it's moved to the underlying type.

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

Expand Down
21 changes: 14 additions & 7 deletions apis/v1alpha2/shared_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,20 @@ type AnnotationKey string
type AnnotationValue string

// AddressType defines how a network address is represented as a text string.
// This may take two possible forms:
//
// * A CamelCase string identifier (like `IPAddress` or `Hostname`)
youngnick marked this conversation as resolved.
Show resolved Hide resolved
// * A domain-prefixed string identifier (like `acme.io/CustomAddressType`)
//
// Values `IPAddress` and `Hostname` have Extended support.
//
// All other values, including domain-prefixed values have Custom support,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// All other values, including domain-prefixed values have Custom support,
// Domain-prefixed values have Custom support,

// and are expected to be used by implementations to implement custom behavior
// in a compatible way.
youngnick marked this conversation as resolved.
Show resolved Hide resolved
//
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
// +kubebuilder:validation:Pattern=`^([a-zA-Z0-9])+$|^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])\/[a-zA-Z0-9]+$`
Copy link
Member

Choose a reason for hiding this comment

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

I think we want a more limited regex here, maybe something like this:

^IPAddress$|^Hostname$|^(([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\-]*[a-zA-Z0-9])\.)*([A-Za-z0-9]|[A-Za-z0-9][A-Za-z0-9\-]*[A-Za-z0-9])\/[a-zA-Z0-9]+$

https://regex101.com/r/xr7aYg/1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm reluctant to do this, because it means that adding another constant will mean a validation change, which is technically a breaking API change. I'd rather have this validation here, with the constants, and add additional validation in the webhook if required.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed an update that shows what I mean. I think that doing things the way I have here will allow us to add more constants more easily later.

Copy link
Member

Choose a reason for hiding this comment

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

I definitely get the hesitancy to make a validation change, but I think the alternative would result in the following:

  • Lots of non domain-prefixed names being used without being defined centrally here
  • Lots of typos when writing out standard names (hostname instead of Hostname, or one of IPaddress or IPAdress instead of IPAddress)

I think the argument against stricter validation here could also be used to remove all uses of our enum validation throughout the API because we want to leave room for additional values in the future. I think this is the most relevant part of the API convention guidance:

If an API drives behavior that is implemented by external clients (like Ingress or NetworkPolicy), the enum field must explicitly indicate that additional values may be allowed in the future, and define how unrecognized values must be handled by clients. If this was not done in the first release containing the enum field, it is not safe to add new values that can break existing clients.

I think that matches what we're already doing with our other enum fields throughout the API. I'd personally prefer to just give that notice in advance and start with stricter validation here.

Copy link
Member

Choose a reason for hiding this comment

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

Discussed in our community meeting today. This seems to have the best possible balance. All the same validation, but leave the validation of the specific constants to the webhook so adding a constant value is easier in the future. I think my only nit here would be to explicitly call out in the godocs here that we may add additional supported values in the future.

type AddressType string

const (
Expand All @@ -502,11 +516,4 @@ const (
//
// Support: Extended
HostnameAddressType AddressType = "Hostname"

// A NamedAddress provides a way to reference a specific IP address by name.
// For example, this may be a name or other unique identifier that refers
// to a resource on a cloud provider such as a static IP.
//
// Support: Implementation-Specific
NamedAddressType AddressType = "NamedAddress"
)
6 changes: 6 additions & 0 deletions config/crd/experimental/gateway.networking.k8s.io_gateways.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions config/crd/stable/gateway.networking.k8s.io_gateways.yaml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.