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 2 commits
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 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,

// 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"
)
35 changes: 34 additions & 1 deletion apis/v1alpha2/validation/gateway.go
View file
Open in desktop
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,27 @@ func validateListenerHostname(listeners []gatewayv1a2.Listener, path *field.Path
}
return errs
}

// 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
re := 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]+$`)

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 := re.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
}
Copy link
Member

@shaneutt shaneutt Jun 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I would like to suggest a micro-optimization here regarding the regex:

As written this will cause every call at runtime to validateAddresses to compile this regex. Being static however this regexp compiles to the same thing each time. This will cause work during runtime that doesn't need to be done, instead we can just make this var a glob above the function (and document it) and use it repeatedly.

For some illustration consider the following two programs:

package main

import "regexp"

func main() {
	for i := 0; i < 100000; i++ {
		match()
	}
}

func match() {
	re := 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]+$`)
	_ = re.MatchString("asdf")
}
package main

import "regexp"

func main() {
	for i := 0; i < 100000; i++ {
		match()
	}
}

var re = 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]+$`)

func match() {
	_ = re.MatchString("asdf")
}

When compiled to assembly (e.g. go tool compile -S main.go) you'll see that the first program will result in the regexp.MustCompile call being placed in .match and will be called repeatedly in the .main loop:

"".main STEXT size=71 args=0x0 locals=0x10 funcid=0x0 align=0x0
        ; e.t.c. (other instructions)
	0x0014 00020 (main.go:5)	XORL	AX, AX
	0x0016 00022 (main.go:6)	JMP	44
	0x0018 00024 (main.go:6)	MOVQ	AX, "".i(SP)
	0x001c 00028 (main.go:7)	PCDATA	$1, $0
	0x001c 00028 (main.go:7)	NOP
	0x0020 00032 (main.go:7)	CALL	"".match(SB)
	0x0025 00037 (main.go:6)	MOVQ	"".i(SP), AX
	0x0029 00041 (main.go:6)	INCQ	AX
	0x002c 00044 (main.go:6)	CMPQ	AX, $100000
	0x0032 00050 (main.go:6)	JLT	24
"".match STEXT size=103 args=0x0 locals=0x70 funcid=0x0 align=0x0
        ; e.t.c. (other instructions)
	rel 33+4 t=7 regexp.MustCompile+0

conversely, the second program will call it only once in .init:

"".init STEXT size=86 args=0x0 locals=0x18 funcid=0x0 align=0x0
        ; e.t.c. (other instructions)
	0x0020 00032 (main.go:11)	CALL	regexp.MustCompile(SB)

This is because go has no such optimization to comprehend that the input to MustCompile is actually static and then try to provide any optimization for that.

This may seem fairly negligible (which is why I call it a micro-optimization) but the timing results for these two variations have a fairly large gap:

$ time ./unoptimized

real	0m0.932s
user	0m1.006s
sys	0m0.038s

$ time ./optimized

real	0m0.073s
user	0m0.074s
sys	0m0.000s

Given our expectation that our validation code is going to run repeatedly all over the world for years to come, seems like a good a place as any to shave off some time (that would otherwise accumulate) for the very low price of moving the variable:

Suggested change
// 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
re := 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]+$`)
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 := re.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
}
// 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
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, that's a great explanation! To be honest, kind of annoyed at myself I didn't see it. 😄 I'll get that change in now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. I'll leave the comment unresolved though so others can easily see the explanation.

34 changes: 34 additions & 0 deletions apis/v1alpha2/validation/gateway_test.go
View file
Open in desktop
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
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.