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.

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
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
20 changes: 13 additions & 7 deletions apis/v1alpha2/shared_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,19 @@ 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 predefined CamelCase string identifier (currently limited to `IPAddress` or `Hostname`)
// * 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,

// which are used in implementation-specific behaviors.
//
// +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 +515,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"
)
39 changes: 38 additions & 1 deletion apis/v1alpha2/validation/gateway.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package validation

import (
"fmt"
"regexp"

"k8s.io/apimachinery/pkg/util/validation/field"

Expand All @@ -36,6 +37,11 @@ var (
gatewayv1a2.UDPProtocolType: {},
gatewayv1a2.TCPProtocolType: {},
}

addressTypesValid = map[gatewayv1a2.AddressType]struct{}{
gatewayv1a2.HostnameAddressType: {},
gatewayv1a2.IPAddressType: {},
}
)

// ValidateGateway validates gw according to the Gateway API specification.
Expand All @@ -51,7 +57,10 @@ func ValidateGateway(gw *gatewayv1a2.Gateway) field.ErrorList {
// validateGatewaySpec validates whether required fields of spec are set according to the
// Gateway API specification.
func validateGatewaySpec(spec *gatewayv1a2.GatewaySpec, path *field.Path) field.ErrorList {
return validateGatewayListeners(spec.Listeners, path.Child("listeners"))
var errs field.ErrorList
errs = append(errs, validateGatewayListeners(spec.Listeners, path.Child("listeners"))...)
errs = append(errs, validateAddresses(spec.Addresses, path.Child("addresses"))...)
return errs
}

// validateGatewayListeners validates whether required fields of listeners are set according
Expand Down Expand Up @@ -89,3 +98,31 @@ func validateListenerHostname(listeners []gatewayv1a2.Listener, path *field.Path
}
return errs
}

// domainPrefixedStringRegex is a regex used in validation to determine whether
// a provided string is a domain-prefixed string. Domain-prefixed strings are used
// to indicate custom (implementation-specific) address types.
var domainPrefixedStringRegex = regexp.MustCompile(`^([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]+$`)

// validateAddresses validates each listener address
// if there are addresses set. Otherwise, returns no error.
func validateAddresses(addresses []gatewayv1a2.GatewayAddress, path *field.Path) field.ErrorList {
var errs field.ErrorList

for i, a := range addresses {
if a.Type == nil {
continue
}
_, ok := addressTypesValid[*a.Type]
if !ok {
// Found something that's not one of the upstream AddressTypes
// Next, check for a domain-prefixed string
match := domainPrefixedStringRegex.Match([]byte(*a.Type))
if !match {
errs = append(errs, field.Invalid(path.Index(i).Child("type"), a.Type, "should either be a defined constant or a domain-prefixed string (example.com/Type)"))
}
}

}
return errs
}
34 changes: 34 additions & 0 deletions apis/v1alpha2/validation/gateway_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,11 @@ func TestValidateGateway(t *testing.T) {
Hostname: nil,
},
}
addresses := []gatewayv1a2.GatewayAddress{
{
Type: nil,
},
}
baseGateway := gatewayv1a2.Gateway{
ObjectMeta: metav1.ObjectMeta{
Name: "foo",
Expand All @@ -38,6 +43,7 @@ func TestValidateGateway(t *testing.T) {
Spec: gatewayv1a2.GatewaySpec{
GatewayClassName: "foo",
Listeners: listeners,
Addresses: addresses,
},
}
tlsConfig := gatewayv1a2.GatewayTLSConfig{}
Expand Down Expand Up @@ -76,6 +82,34 @@ func TestValidateGateway(t *testing.T) {
},
expectErrsOnFields: []string{"spec.listeners[0].hostname"},
},
"Address present with IPAddress": {
mutate: func(gw *gatewayv1a2.Gateway) {
ip := gatewayv1a2.IPAddressType
gw.Spec.Addresses[0].Type = &ip
},
expectErrsOnFields: []string{},
},
"Address present with Hostname": {
mutate: func(gw *gatewayv1a2.Gateway) {
host := gatewayv1a2.HostnameAddressType
gw.Spec.Addresses[0].Type = &host
},
expectErrsOnFields: []string{},
},
"Address present with example.com/CustomAddress": {
mutate: func(gw *gatewayv1a2.Gateway) {
customAddress := gatewayv1a2.AddressType("example.com/CustomAddress")
gw.Spec.Addresses[0].Type = &customAddress
},
expectErrsOnFields: []string{},
},
"Address present with invalid Type": {
mutate: func(gw *gatewayv1a2.Gateway) {
customAddress := gatewayv1a2.AddressType("CustomAddress")
gw.Spec.Addresses[0].Type = &customAddress
},
expectErrsOnFields: []string{"spec.addresses[0].type"},
},
}

for name, tc := range testCases {
Expand Down
14 changes: 6 additions & 8 deletions config/crd/experimental/gateway.networking.k8s.io_gateways.yaml

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

14 changes: 6 additions & 8 deletions config/crd/stable/gateway.networking.k8s.io_gateways.yaml

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