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

Update GEP-713 and add BackendTLSPolicy implementation #2448

Merged
merged 3 commits into from
Oct 12, 2023
Merged

Update GEP-713 and add BackendTLSPolicy implementation #2448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
02ddf1e
Update GEP-713 with status struct design
youngnick Sep 28, 2023
fa5cafb
Finish PolicyStatus struct design and add to the API
youngnick Oct 1, 2023
1419758
Add BackendTLSPolicy implementation and make generate
youngnick Oct 1, 2023
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
148 changes: 148 additions & 0 deletions apis/v1alpha2/backendtlspolicy_types.go
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that this file is basically copied verbatim from GEP-1897.

Now's the time to carefully review though and see if we missed anything there.

Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
/*
Copyright 2023 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package v1alpha2

import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"sigs.k8s.io/gateway-api/apis/v1beta1"
)

// +genclient
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
// +kubebuilder:storageversion
// +kubebuilder:resource:categories=gateway-api,shortName=btlspolicy
// +kubebuilder:printcolumn:name="Age",type=date,JSONPath=`.metadata.creationTimestamp`

// BackendTLSPolicy provides a way to configure how a Gateway
// connects to a Backend via TLS.
type BackendTLSPolicy struct {
youngnick marked this conversation as resolved.
Show resolved Hide resolved
metav1.TypeMeta `json:",inline"`
metav1.ObjectMeta `json:"metadata,omitempty"`

// Spec defines the desired state of BackendTLSPolicy.
Spec BackendTLSPolicySpec `json:"spec"`

// Status defines the current state of BackendTLSPolicy.
Status PolicyStatus `json:"status,omitempty"`
}

// +kubebuilder:object:root=true
// BackendTLSPolicyList contains a list of BackendTLSPolicies
type BackendTLSPolicyList struct {
metav1.TypeMeta `json:",inline"`
metav1.ListMeta `json:"metadata,omitempty"`
Items []BackendTLSPolicy `json:"items"`
}

// BackendTLSPolicySpec defines the desired state of
// BackendTLSPolicy.
// Note: BackendTLSPolicy is a Direct Attached Policy only.
//
// Support: Extended
type BackendTLSPolicySpec struct {
// TargetRef identifies an API object to apply the policy to.
// Only Services have Extended support. Implementations MAY support
// additional objects, with Implementation Specific support.
// Note that this config applies to the entire referenced resource
// by default, but this default may change in the future to provide
// a more granular application of the policy.
//
// Support: Extended for Kubernetes Service
//
// Support: Implementation-specific for any other resource
//
TargetRef PolicyTargetReferenceWithSectionName `json:"targetRef"`

// TLS contains backend TLS policy configuration.
TLS BackendTLSPolicyConfig `json:"tls"`
}

// BackendTLSPolicyConfig contains backend TLS policy configuration.
// +kubebuilder:validation:XValidation:message="must not contain both CertRefs and StandardCerts",rule="(has(self.caCertRefs) && size(self.caCertRefs) > 0 && has(self.wellKnownCACerts) && self.wellKnownCACerts != \"\")"
// +kubebuilder:validation:XValidation:message="must specify either CertRefs or StandardCerts",rule="!(has(self.caCertRefs) && size(self.caCertRefs) > 0 || has(self.wellKnownCACerts) && self.wellKnownCACerts != \"\")"
type BackendTLSPolicyConfig struct {
// CACertRefs contains one or more references to Kubernetes objects that
// contain a PEM-encoded TLS CA certificate bundle, which is used to
// validate a TLS handshake between the Gateway and backend Pod.
//
// If CACertRefs is empty or unspecified, then StandardCerts must
// be specified. Only one of CACertRefs or StandardCerts may be
// specified, not both.
//
// If CACertRefs is empty or unspecified, then system trusted
// certificates should be used. If there are none, or the
// implementation doesn't define system trusted certificates,
// then a TLS connection must fail.
//
// References to a resource in a different namespace are
// invalid for the moment, although we will revisit this
// in the future.
Comment on lines +93 to +95
Copy link
Contributor

Choose a reason for hiding this comment

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

No ReferenceGrant support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not for now - we wanted to get this MVP in, and we can add ReferenceGrant support later. I agree it seems reasonable to add for this field though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, at a high level is this the kind of decision an implementation could decide to implement on their own given the right documentation/reasoning, or would that be considered bad practice. Mostly just curious about how people should/have approach such a thing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've actually chosen the types such that you can't at the moment - but the LocalObjectReference type and the ObjectReference type are unmarshal-equivalent, so when we do migrate, anything stored as JSON/YAML in etcd will deserialize correctly into the ObjectReference type.

So, I guess that's a long way of saying that changing this right now will require forking BackendTLSPolicy, which I wouldn't recommend.

//
// A single CACertRef to a Kubernetes ConfigMap kind has "Core"
// support. Implementations MAY choose to support attaching
// multiple certificates to a backend, but this behavior is
// implementation-specific.
//
// Support: Core - An optional single reference to a Kubernetes
// ConfigMap, with the CA certificate in a key named `ca.crt`.
//
// Support: Implementation-specific (More than one reference, or other kinds
// of resources).
//
// +kubebuilder:validation:MaxItems=8
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: MinItems=1? otherwise you can say certRefs=[] (I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

MinItems=1 makes this not optional though, so I think we need to stick with this. In any case, we can add this later if we need it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that GEP-1897 states "CertRefs contains one or more (up to 64) references to Kubernetes objects..." and should be updated accordingly.

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 CEL validation should be able to prevent invalid inputs like this, but I've filled #2473 to ensure we have good test coverage of our validation here before releasing this.

// +optional
CACertRefs []v1beta1.ObjectReference `json:"caCertRefs,omitempty"`

// WellKnownCACerts specifies whether system CA certificates may
// be used in the TLS handshake between the gateway and
// backend pod.
//
// If WellKnownCACerts is unspecified or set to "", then CACertRefs must
// be specified with at least one entry for a valid configuration.
// Only one of CACertRefs or WellKnownCACerts may be specified, not both.
//
// WellKnownCACerts must be set to "System" when CACertRefs is unspecified.
//
//
// Support: Core for "System"
//
//
// +optional
WellKnownCACerts *WellKnownCACertType `json:"wellKnownCACerts,omitempty"`

// Hostname is used for two purposes in the connection between Gateways and
// backends:
//
// 1. Hostname MUST be used as the SNI to connect to the backend (RFC 6066).
// 2. Hostname MUST be used for authentication and MUST match the certificate
// served by the matching backend.
//
// Support: Core
Hostname v1beta1.PreciseHostname `json:"hostname"`
}

// WellKnownCACertType is the type of CA certificate that will be used when
// the TLS.caCertRefs is unspecified.
// +kubebuilder:validation:Enum=System
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to do like we did for address? Some core enum types, but allow vendor builtin prefix?

Seems less required here as you could use a certRefs to point to an arbitrary type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is essentially a boolean with room for later expansion if we need it. I think domain-prefix is not necessary in this case.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think domain-prefixed values might actually be useful in the future, but I'm fine with leaving that until later since I can't think of any immediate use cases. IMO, the most important thing at this point is that we have room for expansion here.

type WellKnownCACertType string

const (
// Indicates that standard system CA certificates should be used.
WellKnownCACertSystem WellKnownCACertType = "System"
)
93 changes: 93 additions & 0 deletions apis/v1alpha2/policy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ limitations under the License.

package v1alpha2

import metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

const (
// PolicyLabelKey is the label whose presence identifies a CRD that the
// Gateway API Policy attachment model. The value of the label SHOULD be one
Expand Down Expand Up @@ -120,3 +122,94 @@ const (
// policy is attached to an invalid target resource.
PolicyReasonTargetNotFound PolicyConditionReason = "TargetNotFound"
)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you update L79 inPolicyTargetReferenceWithSectionName : "a ResolvedRefs or similar Condition in the Policy's status." to what it should be, either TargetNotFound or add a new condition TargetSectionNotFound?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! I think this might actually be best for a follow up PR because we probably also want to have TargetSectionNotFound like you're suggesting, but I think it would be cleaner to add a new condition like that in a separate PR.

// PolicyAncestorStatus describes the status of a route with respect to an
// associated Ancestor.
//
// Ancestors refer to objects that are either the Target of a policy or above it
// in terms of object hierarchy. For example, if a policy targets a Service, the
// Policy's Ancestors are, in order, the Service, the HTTPRoute, the Gateway, and
// the GatewayClass. Almost always, in this hierarchy, the Gateway will be the most
// useful object to place Policy status on, so we recommend that implementations
// SHOULD use Gateway as the PolicyAncestorStatus object unless the designers
// have a _very_ good reason otherwise.
//
// In the context of policy attachment, the Ancestor is used to distinguish which
// resource results in a distinct application of this policy. For example, if a policy
// targets a Service, it may have a distinct result per attached Gateway.
//
// Policies targeting the same resource may have different effects depending on the
// ancestors of those resources. For example, different Gateways targeting the same
// Service may have different capabilities, especially if they have different underlying
// implementations.
//
// For example, in BackendTLSPolicy, the Policy attaches to a Service that is
// used as a backend in a HTTPRoute that is itself attached to a Gateway.
// In this case, the relevant object for status is the Gateway, and that is the
// ancestor object referred to in this status.
//
// Note that a parent is also an ancestor, so for objects where the parent is the
// relevant object for status, this struct SHOULD still be used.
//
// This struct is intended to be used in a slice that's effectively a map,
// with a composite key made up of the AncestorRef and the ControllerName.
type PolicyAncestorStatus struct {
// AncestorRef corresponds with a ParentRef in the spec that this
// PolicyAncestorStatus struct describes the status of.
AncestorRef ParentReference `json:"ancestorRef"`

// ControllerName is a domain/path string that indicates the name of the
// controller that wrote this status. This corresponds with the
// controllerName field on GatewayClass.
//
// 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).
//
// Controllers MUST populate this field when writing status. Controllers should ensure that
// entries to status populated with their ControllerName are cleaned up when they are no
// longer necessary.
ControllerName GatewayController `json:"controllerName"`
Copy link
Contributor

Choose a reason for hiding this comment

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

if it is possible that more than one controllerName can update a condition, e.g. controllerName A updates one condition and controllerName B updates another condition, it might make more sense to have a controllerName per condition.

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 whole point of PolicyAncestorStatus is that there must be one PolicyAncestorStatus per controller name + AncestorRef combination, the same as how there must be one RouteParentStatus per controller name and ParentRef combination. Those two fields are the key in a listTypeMap-style object.

I've added some information to the top of the godoc for the struct to this effect.


// Conditions describes the status of the Policy with respect to the given Ancestor.
//
// +listType=map
// +listMapKey=type
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=8
Conditions []metav1.Condition `json:"conditions,omitempty"`
}

// PolicyStatus defines the common attributes that all Policies should include within
// their status.
type PolicyStatus struct {
// Ancestors is a list of ancestor resources (usually Gateways) that are
// associated with the policy, and the status of the policy with respect to
// each ancestor. When this policy attaches to a parent, the controller that
// manages the parent and the ancestors MUST add an entry to this list when
// the controller first sees the policy and SHOULD update the entry as
// appropriate when the relevant ancestor is modified.
//
// Note that choosing the relevant ancestor is left to the Policy designers;
// an important part of Policy design is designing the right object level at
// which to namespace this status.
//
// Note also that implementations MUST ONLY populate ancestor status for
// the Ancestor resources they are responsible for. Implementations MUST
// use the ControllerName field to uniquely identify the entries in this list
// that they are responsible for.
//
// Note that to achieve this, the list of PolicyAncestorStatus structs
// MUST be treated as a map with a composite key, made up of the AncestorRef
// and ControllerName fields combined.
//
// A maximum of 16 ancestors will be represented in this list. An empty list
// means the Policy is not relevant for any ancestors.
//
// If this slice is full, implementations MUST NOT add further entries.
//
// +kubebuilder:validation:MaxItems=16
Ancestors []PolicyAncestorStatus `json:"ancestors"`
}
Loading