Skip to content

Addressing v0.8.0 API Review feedback #2261

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

Merged
merged 1 commit into from
Aug 7, 2023
Merged

Addressing v0.8.0 API Review feedback #2261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion apis/v1alpha2/grpcroute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ type GRPCRouteRule struct {
// Specifying the same filter multiple times is not supported unless explicitly
// indicated in the filter.
//
// If an implementation can not support a combinations of filters, they must clearly
// If an implementation can not support a combination of filters, it must clearly
// document that limitation. In cases where incompatible or unsupported
// filters are specified and cause the `Accepted` condition to be set to status
// `False`, implementations may use the `IncompatibleFilters` reason to specify
Expand Down
21 changes: 14 additions & 7 deletions apis/v1beta1/gateway_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,20 @@ type GatewaySpec struct {
// At least one Listener MUST be specified.
//
// Each listener in a Gateway must have a unique combination of Hostname,
// Port, and Protocol. Below combinations are considered Core and MUST be
// supported:
// Port, and Protocol.
//
// Within the HTTP Conformance Profile, the below combinations of port and
// protocol are considered Core and MUST be supported:
//
// 1. Port: 80, Protocol: HTTP
// 2. Port: 443, Protocol: HTTPS
//
// Port and protocol combinations not in this list are considered Extended.
// Within the TLS Conformance Profile, the below combinations of port and
// protocol are considered Core and MUST be supported:
//
// 1. Port: 443, Protocol: TLS
//
// Port and protocol combinations not listed above are considered Extended.
Comment on lines -73 to +86
Copy link
Member Author

@robscott robscott Aug 4, 2023

Choose a reason for hiding this comment

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

Does this update make sense?

/cc @shaneutt @mlavacca

Copy link
Contributor

@youngnick youngnick Aug 7, 2023

Choose a reason for hiding this comment

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

That makes sense to me.

//
// An implementation MAY group Listeners by Port and then collapse each
// group of Listeners into a single Listener if the implementation
Expand Down Expand Up @@ -117,9 +124,9 @@ type GatewaySpec struct {
// +listMapKey=name
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=64
// +kubebuilder:validation:XValidation:message="tls must be set for protocols ['HTTPS', 'TLS']",rule="self.all(l, l.protocol in ['HTTPS', 'TLS'] ? has(l.tls) : true)"
// +kubebuilder:validation:XValidation:message="tls must be empty for protocols ['HTTP', 'TCP', 'UDP']",rule="self.all(l, l.protocol in ['HTTP', 'TCP', 'UDP'] ? !has(l.tls) : true)"
// +kubebuilder:validation:XValidation:message="hostname must be empty for protocols ['TCP', 'UDP']",rule="self.all(l, l.protocol in ['TCP', 'UDP'] ? (!has(l.hostname) || l.hostname == '') : true)"
// +kubebuilder:validation:XValidation:message="tls must be specified for protocols ['HTTPS', 'TLS']",rule="self.all(l, l.protocol in ['HTTPS', 'TLS'] ? has(l.tls) : true)"
// +kubebuilder:validation:XValidation:message="tls must not be specified for protocols ['HTTP', 'TCP', 'UDP']",rule="self.all(l, l.protocol in ['HTTP', 'TCP', 'UDP'] ? !has(l.tls) : true)"
// +kubebuilder:validation:XValidation:message="hostname must not be specified for protocols ['TCP', 'UDP']",rule="self.all(l, l.protocol in ['TCP', 'UDP'] ? (!has(l.hostname) || l.hostname == '') : true)"
// +kubebuilder:validation:XValidation:message="Listener name must be unique within the Gateway",rule="self.all(l1, self.exists_one(l2, l1.name == l2.name))"
// +kubebuilder:validation:XValidation:message="Combination of port, protocol and hostname must be unique for each listener",rule="self.all(l1, self.exists_one(l2, l1.port == l2.port && l1.protocol == l2.protocol && (has(l1.hostname) && has(l2.hostname) ? l1.hostname == l2.hostname : true)))"
Listeners []Listener `json:"listeners"`
Expand Down Expand Up @@ -301,7 +308,7 @@ const (

// GatewayTLSConfig describes a TLS configuration.
//
// +kubebuilder:validation:XValidation:message="certificateRefs must be set and not empty when TLSModeType is Terminate",rule="self.mode == 'Terminate' ? size(self.certificateRefs) > 0 : true"
// +kubebuilder:validation:XValidation:message="certificateRefs must be specified when TLSModeType is Terminate",rule="self.mode == 'Terminate' ? size(self.certificateRefs) > 0 : true"
type GatewayTLSConfig struct {
// Mode defines the TLS behavior for the TLS session initiated by the client.
// There are two possible modes:
Expand Down
6 changes: 3 additions & 3 deletions apis/v1beta1/httproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ type HTTPRouteList struct {
type HTTPRouteSpec struct {
CommonRouteSpec `json:",inline"`

// Hostnames defines a set of hostname that should match against the HTTP Host
// Hostnames defines a set of hostnames that should match against the HTTP Host
// header to select a HTTPRoute used to process the request. Implementations
// MUST ignore any port value specified in the HTTP Host header while
// performing a match and (absent of any applicable header modification
Expand Down Expand Up @@ -866,9 +866,9 @@ const (

// HTTPPathModifier defines configuration for path modifiers.
//
// +kubebuilder:validation:XValidation:message="replaceFullPath must be set when type is set to 'ReplaceFullPath'",rule="self.type == 'ReplaceFullPath' ? has(self.replaceFullPath) : true"
// +kubebuilder:validation:XValidation:message="replaceFullPath must be specified when type is set to 'ReplaceFullPath'",rule="self.type == 'ReplaceFullPath' ? has(self.replaceFullPath) : true"
// +kubebuilder:validation:XValidation:message="type must be 'ReplaceFullPath' when replaceFullPath is set",rule="has(self.replaceFullPath) ? self.type == 'ReplaceFullPath' : true"
// +kubebuilder:validation:XValidation:message="replacePrefixMatch must be set when type is set to 'ReplacePrefixMatch'",rule="self.type == 'ReplacePrefixMatch' ? has(self.replacePrefixMatch) : true"
// +kubebuilder:validation:XValidation:message="replacePrefixMatch must be specified when type is set to 'ReplacePrefixMatch'",rule="self.type == 'ReplacePrefixMatch' ? has(self.replacePrefixMatch) : true"
// +kubebuilder:validation:XValidation:message="type must be 'ReplacePrefixMatch' when replacePrefixMatch is set",rule="has(self.replacePrefixMatch) ? self.type == 'ReplacePrefixMatch' : true"
type HTTPPathModifier struct {
// Type defines the type of path modifier. Additional types may be
Expand Down
11 changes: 8 additions & 3 deletions apis/v1beta1/shared_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ import (
// with "Core" support:
//
// * Gateway (Gateway conformance profile)
// * Service (Mesh conformance profile)
// * Service (Mesh conformance profile, experimental, ClusterIP Services only)
Copy link
Member Author

@robscott robscott Aug 4, 2023

Choose a reason for hiding this comment

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

Does this update line up with what we're targeting for GAMMA?

/cc @howardjohn @keithmattix @kflynn @mikemorris

Copy link
Contributor

@kflynn kflynn Aug 5, 2023

Choose a reason for hiding this comment

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

I believe so, yes.

//
// This API may be extended in the future to support additional kinds of parent
// resources.
Expand All @@ -49,7 +49,7 @@ type ParentReference struct {
// There are two kinds of parent resources with "Core" support:
//
// * Gateway (Gateway conformance profile)
// * Service (Mesh conformance profile)
// * Service (Mesh conformance profile, experimental, ClusterIP Services only)
//
// Support for other resources is Implementation-Specific.
//
Expand Down Expand Up @@ -92,6 +92,11 @@ type ParentReference struct {
// * Gateway: Listener Name. When both Port (experimental) and SectionName
// are specified, the name and port of the selected listener must match
// both specified values.
// * Service: Port Name. When both Port (experimental) and SectionName
// are specified, the name and port of the selected listener must match
// both specified values. Note that attaching Routes to Services as Parents
// is part of experimental Mesh support and is not supported for any other
// purpose.
//
// Implementations MAY choose to support attaching Routes to other resources.
// If that is the case, they MUST clearly document how SectionName is
Expand Down Expand Up @@ -162,7 +167,7 @@ type CommonRouteSpec struct {
// There are two kinds of parent resources with "Core" support:
//
// * Gateway (Gateway conformance profile)
// * Service (Mesh conformance profile)
// * Service (Mesh conformance profile, experimental, ClusterIP Services only)
//
// This API may be extended in the future to support additional kinds of parent
// resources.
Expand Down
Loading