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

Variety of fixes in response to feedback on #780 #796

Merged
merged 2 commits into from
Aug 25, 2021
+638 −616
Merged

Variety of fixes in response to feedback on #780 #796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffd6005
Variety of fixes in response to feedback on #780
robscott Aug 20, 2021
0fbc16b
Updates based on latest PR feedback
robscott Aug 23, 2021
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
188 changes: 87 additions & 101 deletions apis/v1alpha2/gateway_types.go
View file
Open in desktop

Large diffs are not rendered by default.

38 changes: 27 additions & 11 deletions apis/v1alpha2/gatewayclass_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,21 @@ import (
// +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`
// +kubebuilder:printcolumn:name="Description",type=string,JSONPath=`.spec.description`,priority=1

// GatewayClass describes a class of Gateways available to the user
// for creating Gateway resources.
// GatewayClass describes a class of Gateways available to the user for creating
// Gateway resources.
//
// It is recommended that this resource be used as a template for Gateways. This
// means that a Gateway is based on the state of the GatewayClass at the time it
// was created and changes to the GatewayClass or associated parameters are not
// propagated down to existing Gateways. This recommendation is intended to
// limit the blast radius of changes to GatewayClass or associated parameters.
// If implementations choose to propagate GatewayClass changes to existing
// Gateways, that MUST be clearly documented by the implementation.
//
// Whenever one or more Gateways are using a GatewayClass, implementations MUST
// add the `gateway-exists-finalizer.gateway.networking.k8s.io` finalizer on the
// associated GatewayClass. This ensures that a GatewayClass associated with a
// Gateway is not deleted while in use.
//
// GatewayClass is a Cluster level resource.
type GatewayClass struct {
Expand All @@ -47,12 +60,19 @@ type GatewayClass struct {
Status GatewayClassStatus `json:"status,omitempty"`
}

const (
// GatewayClassFinalizerGatewaysExist should be added as a finalizer to the
robscott marked this conversation as resolved.
Show resolved Hide resolved
// GatewayClass whenever there are provisioned Gateways using a
robscott marked this conversation as resolved.
Show resolved Hide resolved
// GatewayClass.
GatewayClassFinalizerGatewaysExist = "gateway-exists-finalizer.gateway.networking.k8s.io"
Copy link
Contributor

Choose a reason for hiding this comment

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

After reading your comments,

Suggested change
GatewayClassFinalizerGatewaysExist = "gateway-exists-finalizer.gateway.networking.k8s.io"
ProvisionedGatewayClassFinalizer = "provisioned-gateway-finalizer.gateway.networking.k8s.io"

is more aligned to what you mean ?

Copy link
Member Author

Choose a reason for hiding this comment

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

This probably could have a better name, but in this case I'm not actually changing the name, just moving it from gateway_types to gatewayclass_types. The idea is for it to indicate that at least one Gateway is using the GatewayClass and therefore the GatewayClass should not be deleted.

)

// GatewayClassSpec reflects the configuration of a class of Gateways.
type GatewayClassSpec struct {
// Controller is a domain/path string that indicates the
// controller that is managing Gateways of this class.
//
// Example: "acme.io/gateway-controller".
// Example: "example.net/gateway-controller".
robscott marked this conversation as resolved.
Show resolved Hide resolved
//
// This field is not mutable and cannot be empty.
//
Expand Down Expand Up @@ -133,15 +153,15 @@ const (
// This condition defaults to False, and MUST be set by a controller when it
// sees a GatewayClass using its controller string. The status of this
// condition MUST be set to true if the controller will support provisioning
// Gateways using this class. Otherwise, this status MUST be set to false.
// If the status is set to false, the controller SHOULD set a Message and
// Gateways using this class. Otherwise, this status MUST be set to False.
// If the status is set to False, the controller SHOULD set a Message and
// Reason as an explanation.
//
// Possible reasons for this condition to be true are:
//
// * "Admitted"
//
// Possible reasons for this condition to be false are:
// Possible reasons for this condition to be False are:
//
// * "InvalidParameters"
// * "Waiting"
Expand All @@ -162,12 +182,8 @@ const (
// This reason is used with the "Admitted" condition when the
// requested controller has not yet made a decision about whether
// to admit the GatewayClass. It is the default Reason on a new
// GatewayClass. It indicates
// GatewayClass.
GatewayClassReasonWaiting GatewayClassConditionReason = "Waiting"

// GatewayClassFinalizerGatewaysExist should be added as a finalizer to the
// GatewayClass whenever there are provisioned Gateways using a GatewayClass.
GatewayClassFinalizerGatewaysExist = "gateway-exists-finalizer.gateway.networking.k8s.io"
)

// GatewayClassStatus is the current status for the GatewayClass.
Expand Down
121 changes: 54 additions & 67 deletions apis/v1alpha2/httproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,44 +62,29 @@ type HTTPRouteSpec struct {
// 1. IPs are not allowed.
// 2. The `:` delimiter is not respected because ports are not allowed.
//
// Incoming requests are matched against the hostnames before the
// HTTPRoute rules. If no hostname is specified, traffic is routed
// based on the HTTPRouteRules.
//
// Requests will be matched against the Host field in the following order:
//
// 1. If Hostname is precise, the request matches this rule if
// the HTTP Host header is equal to the Hostname.
// 2. If Hostname is a wildcard, then the request matches this rule if
// the HTTP Host header is to equal to the suffix
// (removing the first label) of the wildcard rule.
// 3. If Hostname is unspecified, empty, or `*`, then any request will match
// this route.
//
// If a hostname is specified by the Listener that the HTTPRoute is bound
// to, at least one hostname specified here must match the Listener specified
// hostname as per the rules above. Other hostnames will not affect processing
// of the route in that case.
//
// If no hostname is specified by the Listener, then that value will be treated
// as '*', match any hostname, and so any hostname on this Route will match.
//
// If all hostnames do not match, then the HTTPRoute is not admitted, and
// the implementation must raise an 'Admitted' Condition with a status of
// `false` for that Listener.
//
// Examples:
// - A Listener with unspecified, empty, or `*` values for Hostname matches
// any HTTPRoute hostname.
// - A HTTPRoute with unspecified, empty, or `*` values for Hostname matches
// any Listener hostname.
// - A Listener with `test.foo.com` as the hostname matches *only*
// `test.foo.com` or `*.foo.com`. Any other hostnames present must be ignored.
// - A Listener with `*.foo.com` as hostname, all hostnames in the HTTPRoute
// must have any single label where the star is, and the rest of the hostname
// must match exactly. So, `test.foo.com`, `*.foo.com` or `blog.foo.com` match.
// `test.blog.foo.com`, `test.bar.com`, or `bar.com` do not. Hostnames that do
// not match will be ignored.
// If a hostname is specified by both the Listener and HTTPRoute, there
// must be at least one intersecting hostname for the HTTPRoute to be
// attached to the Listener. For example:
//
// * A Listener with `test.example.com` as the hostname matches HTTPRoutes
// that have either not specified any hostnames, or have specified at
// least one of `test.example.com` or `*.example.com`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// least one of `test.example.com` or `*.example.com`.
// least one of `test.example.com` or `*.example.com` or `*`.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually something that came out of API review, I can't remember which of the reviewers suggested it, but the recommendation was to remove "" as a possible value. An empty or unspecified value here would continue to be interpreted at "", but there was no point in differentiating between empty and "*" since they were interpreted the same way. Let me add that to the PR description.

// * A Listener with `*.example.com` as as the hostname matches HTTPRoutes
// that have either not specified any hostnames or have specified at least
// one hostname that matches the Listener hostname. For example,
// `test.example.com` and `*.example.com` would both match. On the other
// hand, `a.b.example.com`, `example.com`, and `test.example.net` would
// not match.
//
// If both the Listener and HTTPRoute have specified hostnames, any
// HTTPRoute hostnames that do not match the Listener hostname MUST be
// ignored. For example, if a Listener specified `*.example.com`, and the
// HTTPRoute specified `test.example.com` and `test.example.net`,
// `test.example.net` must not be considered for a match.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean as following ?
// If both the Listener and HTTPRoute have specified hostnames, only exact match
// will be considered.
// For example, if a Listener specified test.example.com, and the
// HTTPRoute specified test.example.com and test.example.net,
// test.example.net is not be considered for a match.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is trying to describe how wildcard hostname matching should work with hostnames specified on HTTPRoutes. I don't think removing the wildcard from the example would help here, but I may be misunderstanding what you're suggesting.

// If hostnames do not match with the criteria above, then the HTTPRoute is
// not admitted, and the implementation must raise an 'Admitted' Condition
// with a status of `False` for the target Listener(s).
//
// Support: Core
//
Expand Down Expand Up @@ -155,8 +140,10 @@ type HTTPRouteRule struct {
// of the following criteria, continuing on ties:
//
// * The longest matching hostname.
// * The longest matching non-wildcard hostname.
// * The longest matching path.
// * The largest number of header matches.
// * The largest number of query param matches.
//
// If ties still exist across multiple Routes, matching precedence MUST be
// determined in order of the following criteria, continuing on ties:
Expand Down Expand Up @@ -199,11 +186,14 @@ type HTTPRouteRule struct {
Filters []HTTPRouteFilter `json:"filters,omitempty"`

// BackendRefs defines the backend(s) where matching requests should be
// sent. If unspecified or invalid (refers to a non-existent resource or a Service with no endpoints),
// the rule performs no forwarding; if no filters are specified that would result in a
// response being sent, a HTTP 503 status code is returned. 503 responses must be sent so that the overall
// weight is respected; if an invalid backend is requested to have 80% of requests, then 80% of requests
// must get a 503 instead.
// sent.

// If unspecified or invalid (refers to a non-existent resource or a Service
Copy link
Contributor

@ccfishk ccfishk Aug 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

is there any k8s Service (Service with no endpoints) that does not have endpoints ?
maybe "resource" is more accurate here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think what we're trying to communicate here is a Service that does not have any endpoints, likely because there are no Pods running that have been selected by that service.

// with no endpoints), the rule performs no forwarding. If that are also no
// filters specified that would result in a response being sent, a HTTP 503
// status code is returned. 503 responses must be sent so that the overall
// weight is respected; if an invalid backend is requested to have 80% of
// requests, then 80% of requests must get a 503 instead.
//
// Support: Core for Kubernetes Service
// Support: Custom for any other resource
Expand Down Expand Up @@ -466,11 +456,9 @@ type HTTPRouteMatch struct {

// ExtensionRef is an optional, implementation-specific extension to the
// "match" behavior. For example, resource "myroutematcher" in group
// "networking.acme.io". If the referent cannot be found, the rule is not
// included in the route. The controller should raise the "ResolvedRefs"
// condition on the Gateway with the "DegradedRoutes" reason. The gateway
// status for this route should be updated with a condition that describes
// the error more specifically.
// "networking.example.net". If the referent cannot be found, the rule is
// not included in the route. The controller must ensure the "ResolvedRefs"
// condition on the Route status is set to `status: False`.
//
// Support: Custom
//
Expand Down Expand Up @@ -528,7 +516,8 @@ type HTTPRouteFilter struct {
// +optional
RequestMirror *HTTPRequestMirrorFilter `json:"requestMirror,omitempty"`

// RequestRedirect defines a schema for a filter that redirects request.
// RequestRedirect defines a schema for a filter that responds to the
// request with an HTTP redirection.
//
// Support: Core
//
Expand All @@ -537,7 +526,7 @@ type HTTPRouteFilter struct {

// ExtensionRef is an optional, implementation-specific extension to the
// "filter" behavior. For example, resource "myroutefilter" in group
// "networking.acme.io"). ExtensionRef MUST NOT be used for core and
// "networking.example.net"). ExtensionRef MUST NOT be used for core and
robscott marked this conversation as resolved.
Show resolved Hide resolved
// extended filters.
//
// Support: Implementation-specific
Expand Down Expand Up @@ -650,10 +639,10 @@ type HTTPRequestHeaderFilter struct {
// +kubebuilder:validation:MaxItems=16
Add []HTTPHeader `json:"add,omitempty"`

// Remove the given header(s) from the HTTP request before the
// action. The value of RemoveHeader is a list of HTTP header
// names. Note that the header names are case-insensitive
// (see https://datatracker.ietf.org/doc/html/rfc2616#section-4.2).
// Remove the given header(s) from the HTTP request before the action. The
// value of Remove is a list of HTTP header names. Note that the header
// names are case-insensitive (see
// https://datatracker.ietf.org/doc/html/rfc2616#section-4.2).
//
// Input:
// GET /foo HTTP/1.1
Expand Down Expand Up @@ -717,15 +706,14 @@ type HTTPRequestRedirect struct {
type HTTPRequestMirrorFilter struct {
// BackendRef references a resource where mirrored requests are sent.
//
// If the referent cannot be found, this HTTPBackendRef is invalid
// and must be dropped from the Gateway. The controller must ensure the
// "ResolvedRefs" condition on the Gateway is set to `status: true`
// with the "DegradedRoutes" reason, and not configure this backend in the
// underlying implemenation.
// If the referent cannot be found, this BackendRef is invalid and must be
// dropped from the Gateway. The controller must ensure the "ResolvedRefs"
// condition on the Route status is set to `status: False` and not configure
// this backend in the underlying implementation.
//
// If there is a cross-namespace reference to an *existing* object
// that is not allowed by a ReferencePolicy, the controller must ensure the
// "ResolvedRefs" condition on the Gateway is set to `status: true`,
// "ResolvedRefs" condition on the Route is set to `status: False`,
// with the "RefNotPermitted" reason and not configure this backend in the
// underlying implementation.
//
Expand All @@ -743,15 +731,14 @@ type HTTPRequestMirrorFilter struct {
type HTTPBackendRef struct {
// BackendRef is a reference to a backend to forward matched requests to.
//
// If the referent cannot be found, this HTTPBackendRef is invalid
// and must be dropped from the Gateway. The controller must ensure the
// "ResolvedRefs" condition on the Gateway is set to `status: true`
// with the "DegradedRoutes" reason, and not configure this backend in the
// underlying implemenation.
// If the referent cannot be found, this HTTPBackendRef is invalid and must
// be dropped from the Gateway. The controller must ensure the
// "ResolvedRefs" condition on the Route is set to `status: False` and not
// configure this backend in the underlying implementation.
//
// If there is a cross-namespace reference to an *existing* object
// that is not covered by a ReferencePolicy, the controller must ensure the
// "ResolvedRefs" condition on the Gateway is set to `status: true`,
// "ResolvedRefs" condition on the Route is set to `status: true`,
// with the "RefNotPermitted" reason and not configure this backend in the
// underlying implementation.
//
Expand All @@ -763,7 +750,7 @@ type HTTPBackendRef struct {
// +optional
BackendRef `json:",inline"`

// Filters defined at this-level should be executed if and only if the
// Filters defined at this level should be executed if and only if the
// request is being forwarded to the backend defined here.
//
// Support: Custom (For broader support of filters, use the Filters field
Expand Down
48 changes: 32 additions & 16 deletions apis/v1alpha2/shared_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,12 @@ type BackendRef struct {
// BackendObjectReference references a Kubernetes object.
BackendObjectReference `json:",inline"`

// Weight specifies the proportion of HTTP requests forwarded to the
// referenced backend. This is computed as
// weight/(sum of all weights in this ForwardTo list). For non-zero values,
// there may be some epsilon from the exact proportion defined here
// depending on the precision an implementation supports. Weight is not a
// percentage and the sum of weights does not need to equal 100.
// Weight specifies the proportion of requests forwarded to the referenced
// backend. This is computed as weight/(sum of all weights in this
// BackendRefs list). For non-zero values, there may be some epsilon from
// the exact proportion defined here depending on the precision an
// implementation supports. Weight is not a percentage and the sum of
// weights does not need to equal 100.
//
// If only one backend is specified and it has a weight greater than 0, 100%
// of the traffic is forwarded to that backend. If weight is set to 0, no
Expand All @@ -166,9 +166,13 @@ type BackendRef struct {
type RouteConditionType string

const (
// This condition indicates whether the route has been admitted
// or rejected by a Gateway, and why.
// This condition indicates whether the route has been admitted or rejected
// by a Gateway, and why.
robscott marked this conversation as resolved.
Show resolved Hide resolved
ConditionRouteAdmitted RouteConditionType = "Admitted"

// This condition indicates whether the controller was able to resolve all
// the object references for the Route.
ConditionRouteResolvedRefs RouteConditionType = "ResolvedRefs"
)

// RouteParentStatus describes the status of a route with respect to an
Expand All @@ -182,18 +186,32 @@ type RouteParentStatus struct {
// wrote this status. This corresponds with the controller field on
// GatewayClass.
//
// Example: "acme.io/gateway-controller".
// Example: "example.net/gateway-controller".
//
// The format of this field is DOMAIN "/" PATH, where DOMAIN and PATH are
// valid Kubernetes names
// (https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names).
Controller GatewayController `json:"controller"`

// Conditions describes the status of the route with respect to the
// Gateway. The "Admitted" condition must always be specified by controllers
// to indicate whether the route has been admitted or rejected by the Gateway,
// and why. Note that the route's availability is also subject to the Gateway's
// own status conditions and listener status.
// Conditions describes the status of the route with respect to the Gateway.
// Note that the route's availability is also subject to the Gateway's own
// status conditions and listener status.
//
// If the Route's ParentRef specifies an existing Gateway that supports
// Routes of this kind AND that Gateway's controller has sufficient access,
// then that Gateway's controller MUST set the "Admitted" condition on the
// Route, to indicate whether the route has been admitted or rejected by the
// Gateway, and why.
//
// A Route MUST be considered "Admitted" if at least one of the Route's
// rules is implemented by the Gateway.
//
// There are a number of cases where the "Admitted" condition may not be set
// due to lack of controller visibility, that includes when:
//
// * The Route refers to a non-existent parent.
// * The Route is of a type that the controller does not support.
// * The Route is in a namespace the the controller does not have access to.
//
// +listType=map
// +listMapKey=type
Expand Down Expand Up @@ -229,8 +247,6 @@ type RouteStatus struct {
// Hostname can be "precise" which is a domain name without the terminating
// dot of a network host (e.g. "foo.example.com") or "wildcard", which is a
// domain name prefixed with a single wildcard label (e.g. `*.example.com`).
// The wildcard character `*` must appear by itself as the first DNS label
// and matches only a single label.
//
// Note that as per RFC1035 and RFC1123, a *label* must consist of lower case
// alphanumeric characters or '-', and must start and end with an alphanumeric
Expand Down
22 changes: 9 additions & 13 deletions apis/v1alpha2/tcproute_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,13 +89,11 @@ type TCPRouteRule struct {
Matches []TCPRouteMatch `json:"matches,omitempty"`

// BackendRefs defines the backend(s) where matching requests should be
// sent. If unspecified or invalid (refers to a non-existent resource or
// a Service with no endpoints), the rule performs no forwarding; if no
// filters are specified that would result in a response being sent, the
// underlying implementation must actively reject connection attempts to
// this backend. Connection rejections must respect weight; if an invalid
// backend is requested to have 80% of connections, then 80% of connections
// must be rejected instead.
// sent. If unspecified or invalid (refers to a non-existent resource or a
// Service with no endpoints), the underlying implementation MUST actively
// reject connection attempts to this backend. Connection rejections must
// respect weight; if an invalid backend is requested to have 80% of
// connections, then 80% of connections must be rejected instead.
//
// Support: Core for Kubernetes Service
// Support: Custom for any other resource
Expand All @@ -111,12 +109,10 @@ type TCPRouteRule struct {
// given action.
type TCPRouteMatch struct {
// ExtensionRef is an optional, implementation-specific extension to the
// "match" behavior. For example, resource "mytcproutematcher" in group
// "networking.acme.io". If the referent cannot be found, the rule is not
// included in the route. The controller should raise the "ResolvedRefs"
// condition on the Gateway with the "DegradedRoutes" reason. The gateway
// status for this route should be updated with a condition that describes
// the error more specifically.
// "match" behavior. For example, resource "mytcproutematcher" in group
// "networking.example.net". If the referent cannot be found, the rule MUST
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// "networking.example.net". If the referent cannot be found, the rule MUST
// "networking.example.net". If the referent cannot be found, the rule must

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there's probably room for a broader discussion around the usability of these all caps RFC terms, but so far the guidance from other reviewers has been to move more into this capitalization style. We can add to the agenda for next community meeting and/or create an issue to discuss if that's helpful. I don't have a very strong opinion here other than trying to be consistent with what we've done elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely a thing we've copied from RFCs, where the increased emphasis is very purposeful - it's a guide for people implementing the spec that allows the reader to easily find the things they MUST do when reading what is often a long and dry specification. That's why I'm supportive of the all-caps.

// not be included in the route. The controller must ensure the
// "ResolvedRefs" condition on the Route status is set to `status: False`.
//
// Support: Custom
//
Expand Down
Loading