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

add api changes according to gep-3155 #3304

Merged
merged 1 commit into from
Aug 31, 2024
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
39 changes: 39 additions & 0 deletions apis/applyconfiguration/apis/v1/gatewaybackendtls.go

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

9 changes: 9 additions & 0 deletions apis/applyconfiguration/apis/v1/gatewayspec.go

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

17 changes: 17 additions & 0 deletions apis/applyconfiguration/apis/v1alpha3/backendtlspolicyspec.go

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

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

62 changes: 62 additions & 0 deletions apis/applyconfiguration/apis/v1alpha3/subjectaltname.go

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

33 changes: 33 additions & 0 deletions apis/applyconfiguration/internal/internal.go

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

4 changes: 4 additions & 0 deletions apis/applyconfiguration/utils.go

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

32 changes: 32 additions & 0 deletions apis/v1/gateway_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,15 @@ type GatewaySpec struct {
// <gateway:experimental>
// +optional
Infrastructure *GatewayInfrastructure `json:"infrastructure,omitempty"`

// BackendTLS configures TLS settings for when this Gateway is connecting to
// backends with TLS.
//
// Support: Core
//
// +optional
// <gateway:experimental>
Copy link
Member

Choose a reason for hiding this comment

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

I was kind of confused when I saw this new experimental feature to be set as core. This can be problematic, as when we graduate this feature to GA, this will be a breaking change: all implementation will have to support it, otherwise conformance won't be given. I think this is a bit problematic, but I'd like to ask others' opinion (@robscott @youngnick @shaneutt).

I just found out we already have a precedent, with the infrastructure Gateway feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair. I think before graduating this to GA we should have at least (3?) implementations implementing it, right?

I am not sure if there are guidelines on whether a feature should have to graduate as extended, I think support level and channel are two different things. I believe if we move any feature to Core support, it would mean some implementations would loose conformance.

Copy link
Member

Choose a reason for hiding this comment

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

🤔 This actually might be sufficiently portable that everyone could implement it. I know we default to "extended" for so many things, that it's rare to add a new core feature, but this particular one seems like it could be worth aiming for. In general, I'd say the support level that gets merged in experimental is more of a target than a reality since it's still locked behind experimental CRDs. So I'm actually in favor of starting with "core" as a signal that we hope and expect this to graduate at that level, so implementations have a lot of time (2 releases) to call out issues with this goal. I think it would be less disruptive to move from "core" to "extended" in the future than the other way around.

Copy link
Member

Choose a reason for hiding this comment

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

I'm in favor of starting in extended and then we can start communicating a desire to move it to core as soon as we like, giving some soak time for implementations to think about that.

Copy link
Member

Choose a reason for hiding this comment

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

My thinking is that nothing in experimental channel can really be "core" anyways, so the support level here is more aspirational than anything. So at this point we're signaling that we think this can be "core", and thus giving the maximum possible notice that that's what it may graduate to standard as. Any alternative seems like it would either result in less notice to implementations or graduating the feature as "extended" and then having to manage a second transition to "core". I'd rather just start with a target of what we think we can achieve and then accept pushback to a lower level before this feature graduates if we think that's necessary.

Copy link
Member

Choose a reason for hiding this comment

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

+1

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with having this targeted as core, and agree when you say that in case we'll change it in the future, the core->extended transition is less problematic than the extended->core. One thing we should be very careful about is clearly stating to implementations that when an experimental core feature graduates, that's a breaking change (i.e., all the implementations MUST implement it). We need to communicate this ahead of time, to avoid that implementation suddenly and inexpectedly see their conformance broken.

BackendTLS *GatewayBackendTLS `json:"backendTLS,omitempty"`
}

// Listener embodies the concept of a logical endpoint where a Gateway accepts
Expand Down Expand Up @@ -374,6 +383,29 @@ const (
UDPProtocolType ProtocolType = "UDP"
)

// GatewayBackendTLS describes backend TLS configuration for gateway.
type GatewayBackendTLS struct {
// ClientCertificateRef is a reference to an object that contains a Client
// Certificate and the associated private key.
//
// References to a resource in different namespace are invalid UNLESS there
// is a ReferenceGrant in the target namespace that allows the certificate
// to be attached. If a ReferenceGrant does not allow this reference, the
// "ResolvedRefs" condition MUST be set to False for this listener with the
// "RefNotPermitted" reason.
//
// ClientCertificateRef can reference to standard Kubernetes resources, i.e.
// Secret, or implementation-specific custom resources.
//
// This setting can be overridden on the service level by use of BackendTLSPolicy.
//
// Support: Core
//
// +optional
// <gateway:experimental>
ClientCertificateRef *SecretObjectReference `json:"clientCertificateRef,omitempty"`
}

// GatewayTLSConfig describes a TLS configuration.
//
// +kubebuilder:validation:XValidation:message="certificateRefs or options must be specified when mode is Terminate",rule="self.mode == 'Terminate' ? size(self.certificateRefs) > 0 || size(self.options) > 0 : true"
Expand Down
13 changes: 13 additions & 0 deletions apis/v1/shared_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -535,6 +535,19 @@ type Hostname string
// +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*$`
type PreciseHostname string

// AbsoluteURI represents a Uniform Resource Identifier (URI) as defined by RFC3986.

// The AbsoluteURI MUST NOT be a relative URI, and it MUST follow the URI syntax and
// encoding rules specified in RFC3986. The AbsoluteURI MUST include both a
// scheme (e.g., "http" or "spiffe") and a scheme-specific-part. URIs that
// include an authority MUST include a fully qualified domain name or
// IP address as the host.
Comment on lines +538 to +544
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure how we should approach for case-sensitivity, scheme and host aren't but other parts can be. Let me know if you want to add any language to indicate what we expect

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe "implementations SHOULD treat this string case insensitively" or something? Feels like it's better to have some stance on case sensitivity rather than none.

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 I'd leave case sensitivity out of this as it's potentially different depending on the scheme. Per RFC-3986:

When a URI uses components of the generic syntax, the component syntax equivalence rules always apply; namely, that the scheme and host are case-insensitive and therefore should be normalized to lowercase. For example, the URI HTTP://www.EXAMPLE.com/ is equivalent to http://www.example.com/. The other generic syntax components are assumed to be case-sensitive unless specifically defined otherwise by the scheme

https://datatracker.ietf.org/doc/html/rfc3986#section-6.2.2.1


// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=253
// +kubebuilder:validation:Pattern=`^(([^:/?#]+):)(//([^/?#]*))([^?#]*)(\?([^#]*))?(#(.*))?`
Copy link
Member

Choose a reason for hiding this comment

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

Non-blocking nit: Would be nice to have a way to track the source of this RegEx for future readers. Can you leave a comment in #3306 with the source of this validation?

type AbsoluteURI string

// Group refers to a Kubernetes Group. It must either be an empty string or a
// RFC 1123 subdomain.
//
Expand Down
25 changes: 25 additions & 0 deletions apis/v1/zz_generated.deepcopy.go

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

Loading