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

Jump to bottom

Adds Per Route TLS Config to Gateway API Type #71

Closed
wants to merge 1 commit into from
Closed

Adds Per Route TLS Config to Gateway API Type #71

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
96 changes: 82 additions & 14 deletions api/v1alpha1/gateway_types.go
View file
Open in desktop
Expand Up @@ -56,9 +56,12 @@ type GatewaySpec struct {
// Listeners associated with this Gateway. Listeners define what addresses,
// ports, protocols are bound on this Gateway.
Listeners []Listener `json:"listeners"`
// Routes associated with this Gateway. Routes define
// protocol-specific routing to backends (e.g. Services).
Routes []core.TypedLocalObjectReference `json:"routes"`
// Routes defines routes to associate with the Gateway.
//
// Support: Core
//
// +optional
Routes []Route `json:"routes,omitempty"`
}

const (
Expand All @@ -68,6 +71,25 @@ const (
HTTPSProcotol = "HTTPS"
)

// Route defines the schema for a route.
type Route struct {
// RouteRef is a reference to an object to associate with the Gateway.
// RouteRef defines protocol-specific routing to back-ends (e.g. Services).
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this really what a Route is? Looking at the current spec, TcpRoute and HTTPRoute are defined. To my mind these have more in common with endpoints (since we are in a reverse-proxy configuration, they will appear to be endpoints from the perspective of the client).

I realize that this terminology is already in play, but maybe we can clarify here. The description of "routing to back-ends" seems more applicable to HTTPRouteAction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you define more what "endpoint" and "client" you mention in the above.
Endpoint e.g. k8s Endpoint? Or something else?
Client as in the HTTP-client coming to the Gateway (or something else)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of the Route type is to group attributes with the route reference. Currently, we're focused on TLS, but I can foresee other attributes a Cluster Operator may want to associate to a route, e.g. a traffic shaping policy.

//
// If unspecified, no routes will be associated to the Gateway.
//
// Support: Core
//
// +optional
RouteRef core.ObjectReference `json:"routeRef"`
// TLS is the configuration used for establishing a TLS connection.
//
// Support: Core
//
// +optional
TLS *TLSConfig `json:"tls,omitempty"`
}

// Listener defines a
type Listener struct {
// Name is the listener's name and should be specified as an
Expand All @@ -87,7 +109,7 @@ type Listener struct {
// the request address is invalid, the GatewayClass MUST indicate
// this in the associated entry in GatewayStatus.Listeners.
//
// Support:
// Support: Core
//
// +optional
Address *ListenerAddress `json:"address,omitempty"`
Expand All @@ -107,7 +129,7 @@ type Listener struct {
// Support: Core
//
// +optional
TLS *ListenerTLS `json:"tls,omitempty"`
TLS *TLSConfig `json:"tls,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we need to step back from attaching TLS to the listener. The HTTPRouteHost has the hostname, maybe that's the right place to put a TLS configuration? That would strongly bind TLS configuration and virtual hosts in a way that's currently only implicit (even with the ListenerCertificates changes below).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jpeach IMO we need to get #95 resolved to move forward with this or any of the other PR's that address TLS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the App Dev is responsible for managing TLS config, then I agree that xRoute is the right place for all TLS config management.

// Extension for this Listener.
//
// Support: custom.
Expand Down Expand Up @@ -149,7 +171,7 @@ const (
TLS1_3 = "TLS1_3"
)

// ListenerTLS describes the TLS configuration for a given port.
// TLSConfig describes configuration for establishing a TLS connection.
Copy link
Contributor

Choose a reason for hiding this comment

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

To me, "establishing a TLS connection" implies pretty strongly that it is being used by a network client.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe TLSConfig => TLSServerConfig will be less ambiguous/

(the server also is part of connection establishment)

Copy link
Contributor

Choose a reason for hiding this comment

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

If this is intended to be specific to accepting a TLS session, then I agree TLSServerConfig is less ambiguous.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bowei I considered TLSServerConfig but TLSConfig contains client-side properties such as CACertificates used by the Gateway for establishing a TLS connection to a backend Service.

//
// References
// - nginx: https://nginx.org/en/docs/http/configuring_https_servers.html
Expand All @@ -158,20 +180,41 @@ const (
// - gcp: https://cloud.google.com/load-balancing/docs/use-ssl-policies#creating_an_ssl_policy_with_a_custom_profile
// - aws: https://docs.aws.amazon.com/elasticloadbalancing/latest/application/create-https-listener.html#describe-ssl-policies
// - azure: https://docs.microsoft.com/en-us/azure/app-service/configure-ssl-bindings#enforce-tls-1112
type ListenerTLS struct {
type TLSConfig struct {
// Certificates is a reference to one or more Kubernetes objects each containing
// an identity certificate that is bound to the listener. The hostname in a TLS
// an identity certificate that is bound to a listener. The hostname in a TLS
// SNI client hello message is used for certificate matching and route hostname
// selection. The SNI server_name must match a route hostname for the Gateway to
// route the TLS request.
// selection.
//
// If apiGroup and kind are empty, will default to Kubernetes Secrets resources.
//
// Support: Core (Kubernetes Secrets)
// Support: Implementation-specific (Other resource types)
//
// +required
Certificates []core.TypedLocalObjectReference `json:"certificates"`
// +optional
ListenerCertificates []core.TypedLocalObjectReference `json:"listenerCertificates,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this relates back to my comment on Listener.TLS. A collection of listener certificates makes sense when you are modeling a single port that can terminate different TLS names. However, further down, TLSConfig is used in HTTPRouteAction, and in that context it's not clear to me whether this is a pinned set of certificates that the client will verify and accept, or simply not used. Attaching the TLS server configuration to the HTTPHostRoute could help to resolve this ambiguity.

Copy link
Contributor

Choose a reason for hiding this comment

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

My current understand is that the certs associated with the Gateway here are for downstream connection and the TLSConfig in HostRouteAction is for the upstream hop.

// CACertificates is a reference to one or more Kubernetes objects
// each containing a CA certificate used by the TLS client for
// establishing a connection with a server.
//
// Here is a ConfigMap example (in yaml):
//
// apiVersion: v1
// kind: ConfigMap
// metadata:
// name: my-dest-svc-ca
// namespace: my-dest-svc-namespace
// data:
// ca-bundle.crt: |
// -----BEGIN CERTIFICATE-----
// Destination Service CA Certificate Bundle.
// -----END CERTIFICATE-----
//
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 a nice improvement. Should we add a reference to #74, which hopefully will fully define the requirements around certificates?

// Support: Core (Kubernetes ConfigMap)
// Support: Implementation-specific (For other resource types)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/Implementation-specific/Custom to be consistent with how this is used in other places.
Just a note: #56

//
// +optional
CACertificates []core.TypedLocalObjectReference `json:"caCertificates,omitempty"`
// MinimumVersion of TLS allowed. It is recommended to use one of
// the TLS_* constants above. Note: this is not strongly
// typed to allow implementation-specific versions to be used without
Expand All @@ -182,7 +225,15 @@ type ListenerTLS struct {
// values.
//
// +optional
MinimumVersion *string `json:"minimumVersion"`
MinimumVersion *string `json:"minimumVersion,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, are there any requirements on implementations if the minimum version is omitted? Maybe track a separate issue to discuss?

// TLSTermination defines how to terminate TLS connections.
//
// If unspecified, TLS termination type "Edge" will be used.
//
// Support: Core
//
// +optional
TLSTermination TLSTerminationType `json:"tlsTermination,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This gets interesting. Sometimes one can be in a situation where the termination is decided based on the SNI. For some SNIs, the proxy terminates TLS while for other it is passthrough. It has come up a few times but I'm not sure if it should be supported by our API.

Leaving a note here but I think it is okay to keep this as is.

// Options are a list of key/value pairs to give extended options
// to the provider.
//
Expand All @@ -192,9 +243,26 @@ type ListenerTLS struct {
// construct.
//
// Support: Implementation-specific.
Options map[string]string `json:"options"`
//
// +optional
Options map[string]string `json:"options,omitempty"`
}

// TLSTerminationType specifies where TLS connections will terminate.
type TLSTerminationType string

const (
// TLSTerminationEdge terminates the TLS connection at the gateway.
TLSTerminationEdge TLSTerminationType = "Edge"

// TLSTerminationPassthrough terminates the TLS connection at the
// destination service. The destination service is responsible for
// decrypting data from the connection. The Gateway listener must be
// configured for the HTTPS protocol. SNI is used by the Gateway to
// perform route selection.
TLSTerminationPassthrough TLSTerminationType = "Passthrough"
)

// GatewayStatus defines the observed state of Gateway.
type GatewayStatus struct {
// Conditions describe the current conditions of the Gateway.
Expand Down
1 change: 0 additions & 1 deletion api/v1alpha1/httproute_types.go
View file
Open in desktop
Expand Up @@ -180,7 +180,6 @@ type HTTPHeaderFilter struct {
type HTTPRouteAction struct {
// ForwardTo sends requests to the referenced object.
ForwardTo *core.TypedLocalObjectReference `json:"forwardTo"`

// Extension is an optional, implementation-specific extension
// to the "action" behavior.
//
Expand Down
48 changes: 37 additions & 11 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

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