Update GEP-713 and add BackendTLSPolicy implementation #2448

youngnick · 2023-10-01T04:16:38Z

What type of PR is this?
/kind feature
/kind gep

What this PR does / why we need it:

This updates GEP-713 with the information needed for BackendTLSPolicy's status (including making this a new recommendation for Policy status), and also implements BackendTLSPolicy.

Which issue(s) this PR fixes:
Updates #713
Updates #1897

Does this PR introduce a user-facing change?:

BackendTLSPolicy has been added to control TLS from Gateway to backends specified in HTTPRoutes.

youngnick · 2023-10-01T04:17:28Z

This needs a little more work to add tests for the BackendTLSPolicy CEL rules, but I haven't done that before, not sure how long it will take. This will let folks review the rest while I work on that.

/hold

youngnick · 2023-10-01T04:19:09Z

apis/v1alpha2/backendtlspolicy_types.go

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.

youngnick · 2023-10-01T04:28:55Z

BackendTLSPolicy also has the problem that support for it should be Extended, but it will be tricky to write conformance tests for. I probably also need to add something to GEP-1897 about how implementations should signal that they support the new object.

I'll work on that now.

Edit: This needs quite a lot of talking, and GRPCRoute is in a similar boat. I actually think we should hold off here and do both together.

robscott

Thanks for getting this one through @youngnick! I just focused on the TLS side of things, but the Policy side largely lines up with what I was hoping for, just did not have time to take a close look yet.

apis/v1alpha2/backendtlspolicy_types.go

robscott · 2023-10-01T18:19:03Z

apis/v1alpha2/backendtlspolicy_types.go

+// BackendTLSPolicyConfig contains backend TLS policy configuration.
+// +kubebuilder:validation:XValidation:message="must not contain both CertRefs and StandardCerts",rule="(has(self.certRefs) && size(self.certRefs) > 0 && has(self.standardCerts) && self.standardCerts != ”)"
+// +kubebuilder:validation:XValidation:message="must specify either CertRefs or StandardCerts",rule="!(has(self.certRefs) && size(self.certRefs) > 0 || has(self.standardCerts) && self.standardCerts != ”)"


I think these both look correct, but would appreciate new tests in pkg/test/cel to confirm that they're working as expected.

apis/v1alpha2/backendtlspolicy_types.go

robscott · 2023-10-01T18:48:23Z

apis/v1alpha2/backendtlspolicy_types.go

+	//
+	// +kubebuilder:validation:MaxItems=8
+	// +optional
+	CertRefs []v1beta1.ConfigMapObjectReference `json:"certRefs,omitempty"`


Nit: I'm not convinced that ConfigMaps are the best long term solution here, would prefer not starting with a type that defaults to them.

Okay, would you prefer no default here to start with then? That also doesn't seem ideal.

The idea we discussed was to make it easy to get started for now, and we can change the defaults later if we want - because they are defaults, they are added the first time the object is persisted, so existing resources will keep working even if we do.

My concern is that I think generally we're supposed to release a new API version when changing default values (https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md#static-defaults). We could theoretically say that it's experimental channel and anything goes, but in this particular case it seems very unlikely that ConfigMap as default is something we want to stick with through to GA, so I'd personally rather not start with it.

Yeah, okay, I'll pull out ConfigMapObjectReference for now then, can always bring it back later if need be. This will be moved to just a straight ObjectReference then, which will mean that people will need to specify that this is a configmap to begin with (which I agree is not a huge deal).

Paging @candita though for this one, this is a little different to the discussion we had on the GEP.

+1 to no defaults

I've updated this to use a new ObjectReference type that has no defaults - this means that group and kind are now required.

@youngnick we went in a few different directions in the original GEP, and first decided that ConfigMapReference would be the simplest at the time: #2113 (comment)

#2113 (comment) discusses why we went with ConfigMapReference.

ObjectReference seems even simpler, but perhaps we could add some examples for the Group and Kind if we don't default it (ClusterTrustBundled, ConfigMapReference)?

robscott

Thanks @youngnick! This review just covers the policy side of the PR. I think it all looks solid, just had some suggested tweaks + additions to spec here.

geps/gep-713.md

geps/gep-1897.md

apis/v1alpha2/backendtlspolicy_types.go

howardjohn · 2023-10-03T15:17:34Z

apis/v1alpha2/backendtlspolicy_types.go

+	// Support: Implementation-specific (More than one reference, or other kinds
+	// of resources).
+	//
+	// +kubebuilder:validation:MaxItems=8


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

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.

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

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.

apis/v1alpha2/backendtlspolicy_types.go

howardjohn · 2023-10-03T15:25:25Z

apis/v1alpha2/backendtlspolicy_types.go

+	//
+	// +kubebuilder:validation:MaxItems=8
+	// +optional
+	CertRefs []v1beta1.ConfigMapObjectReference `json:"certRefs,omitempty"`


+1 to no defaults

apis/v1alpha2/backendtlspolicy_types.go

howardjohn · 2023-10-03T15:26:44Z

apis/v1alpha2/backendtlspolicy_types.go

+
+// StandardCertType is the type of CA certificate that will be used when
+// the TLS.certRefs is unspecified.
+// +kubebuilder:validation:Enum=System


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

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.

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.

howardjohn · 2023-10-03T15:28:28Z

apis/v1alpha2/backendtlspolicy_types.go

+	// backends:
+	//
+	// 1. Hostname MUST be used as the SNI to connect to the backend (RFC 3546).
+	// 2. Hostname MUST be used for authentication and MUST be in the certificate


nit: This is trading off precising vs complexity, but technically it doesn't need to be in the certificate; the cert could be a wildcard. Maybe simply use "match"? Possibly tie to the TLS verification RFC if we want

"match" is a good idea, done.

Signed-off-by: Nick Young <nick@isovalent.com>

kflynn · 2023-10-11T02:39:10Z

I voiced concerns about this one at the v1.0 API review. Overall, those concerns kind of boil down to the backend TLS and ancestors feeling even more experimental than GEP-713 itself, which worries me: landing this will fold those into GEP-713, and no one reading the GEP later will know the gradations between bits of 713.

But perhaps I should trust the experimental process more. @youngnick, I'm OK landing this as experimental for v1.0. Probably sometime after that we should take a serious look at graduation criteria for GEP-713 -- it's significant enough that we might not want to leave it exempt.

robscott

Thanks @youngnick! A few nits but otherwise LGTM.

robscott · 2023-10-11T22:09:00Z

apis/v1alpha2/backendtlspolicy_types.go

+// +kubebuilder:validation:XValidation:message="must not contain both CertRefs and StandardCerts",rule="(has(self.certRefs) && size(self.certRefs) > 0 && has(self.standardCerts) && self.standardCerts != \"\")"
+// +kubebuilder:validation:XValidation:message="must specify either CertRefs or StandardCerts",rule="!(has(self.certRefs) && size(self.certRefs) > 0 || has(self.standardCerts) && self.standardCerts != \"\")"


Added #2473 to track adding CEL tests for this. Does not need to block this PR, but probably needs to block v1.0.

apis/v1alpha2/backendtlspolicy_types.go

robscott · 2023-10-11T22:10:52Z

apis/v1alpha2/backendtlspolicy_types.go

+	// Support: Implementation-specific (More than one reference, or other kinds
+	// of resources).
+	//
+	// +kubebuilder:validation:MaxItems=8


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.

apis/v1alpha2/backendtlspolicy_types.go

robscott · 2023-10-11T22:18:02Z

apis/v1alpha2/policy_types.go

@@ -120,3 +122,81 @@ const (
 	// policy is attached to an invalid target resource.
 	PolicyReasonTargetNotFound PolicyConditionReason = "TargetNotFound"
 )
+


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.

robscott · 2023-10-11T22:19:12Z

apis/v1alpha2/backendtlspolicy_types.go

+
+// StandardCertType is the type of CA certificate that will be used when
+// the TLS.certRefs is unspecified.
+// +kubebuilder:validation:Enum=System


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.

robscott

Thanks @youngnick! 3 non-blocking nits, feel free to remove hold if you'd rather cover them in a follow up.

/lgtm
/hold

robscott · 2023-10-12T01:30:07Z

apis/v1alpha2/policy_types.go

+	// A maximum of 8 ancestors will be represented in this list. An empty list
+	// means the Policy is not relevant for any ancestors.


Don't think it needs to block this PR, but we should address Tim's feedback here. We really should define what implementations should do when this is full. At a minimum we should clarify that they should not try to add to the list, but maybe we can think of some better options here longer term?

robscott · 2023-10-12T01:31:27Z

apis/v1beta1/httproute_types.go

+	//   is present that refers to the Service, and the implementation is unable
+	//   to meet the requirement. At the time of writing, BackendTLSPolicy is
+	//   experimental, but once it becomes standard, this will become a MUST
+	// requirement.


Non-blocking nit:

Suggested change

// requirement.

// requirement.

geps/gep-713.md

k8s-ci-robot · 2023-10-12T01:34:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: robscott, youngnick

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [robscott,youngnick]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Nick Young <nick@isovalent.com>

robscott · 2023-10-12T03:30:10Z

/lgtm

robscott · 2023-10-12T03:30:27Z

/hold cancel

EItanya · 2023-10-17T13:09:17Z

apis/v1alpha2/backendtlspolicy_types.go

+	// References to a resource in a different namespace are
+	// invalid for the moment, although we will revisit this
+	// in the future.


No ReferenceGrant support?

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.

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

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.

k8s-ci-robot requested review from keithmattix and robscott October 1, 2023 04:16

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2023

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2023

youngnick force-pushed the backend-policy-update branch from b22773a to 23444a4 Compare October 1, 2023 04:18

youngnick commented Oct 1, 2023

View reviewed changes

youngnick force-pushed the backend-policy-update branch 2 times, most recently from a49826e to 91a98bd Compare October 1, 2023 04:25

youngnick force-pushed the backend-policy-update branch 4 times, most recently from 5852aca to b417cf7 Compare October 1, 2023 05:04

robscott reviewed Oct 1, 2023

View reviewed changes

robscott reviewed Oct 3, 2023

View reviewed changes

youngnick force-pushed the backend-policy-update branch 5 times, most recently from 18af989 to ad9d57c Compare October 3, 2023 06:21

gcs278 reviewed Oct 3, 2023

View reviewed changes

geps/gep-1897.md Outdated Show resolved Hide resolved

howardjohn reviewed Oct 3, 2023

View reviewed changes

youngnick force-pushed the backend-policy-update branch from ad9d57c to e7fa9a3 Compare October 5, 2023 03:59

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2023

youngnick force-pushed the backend-policy-update branch from 195e7eb to bff8619 Compare October 10, 2023 23:45

youngnick added 2 commits October 11, 2023 00:06

Update GEP-713 with status struct design

02ddf1e

Signed-off-by: Nick Young <nick@isovalent.com>

Finish PolicyStatus struct design and add to the API

fa5cafb

Signed-off-by: Nick Young <nick@isovalent.com>

youngnick force-pushed the backend-policy-update branch from bff8619 to dc577d6 Compare October 11, 2023 00:06

youngnick force-pushed the backend-policy-update branch 2 times, most recently from 68515ff to d8bc3df Compare October 11, 2023 04:56

robscott mentioned this pull request Oct 11, 2023

BackendTLSPolicy: Add CEL Tests #2473

Closed

robscott reviewed Oct 11, 2023

View reviewed changes

youngnick force-pushed the backend-policy-update branch 2 times, most recently from 995d310 to fe6a88d Compare October 12, 2023 01:13

robscott approved these changes Oct 12, 2023

View reviewed changes

k8s-ci-robot assigned robscott Oct 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2023

Add BackendTLSPolicy implementation and make generate

1419758

Signed-off-by: Nick Young <nick@isovalent.com>

youngnick force-pushed the backend-policy-update branch from fe6a88d to 1419758 Compare October 12, 2023 02:55

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 12, 2023

k8s-ci-robot merged commit 5d96cf2 into kubernetes-sigs:main Oct 12, 2023
8 checks passed

youngnick mentioned this pull request Oct 17, 2023

CFP: Support TLS (HTTPS) backends and TLS passthrough on Cilium Ingress cilium/cilium#20960

Open

EItanya reviewed Oct 17, 2023

View reviewed changes

sunjayBhatia mentioned this pull request Nov 1, 2023

Gateway API: Implement BackendTLSPolicy projectcontour/contour#5929

Closed

sunjayBhatia mentioned this pull request Jan 19, 2024

BackendTLSPolicy wellKnownCACerts field "System" value should not be core #2726

Closed

robscott mentioned this pull request Feb 7, 2024

Clarifying policy TargetNotFound condition responsibility #2755

Closed

This was referenced Apr 4, 2024

Split GEP-713 into a Memorandum and separate GEPs for Direct and Inherited Policy #2813

Merged

Define standard PolicyAncestor status type for Policy objects #2923

Open

		// +kubebuilder:validation:XValidation:message="must not contain both CertRefs and StandardCerts",rule="(has(self.certRefs) && size(self.certRefs) > 0 && has(self.standardCerts) && self.standardCerts != \"\")"
		// +kubebuilder:validation:XValidation:message="must specify either CertRefs or StandardCerts",rule="!(has(self.certRefs) && size(self.certRefs) > 0 \|\| has(self.standardCerts) && self.standardCerts != \"\")"

		// A maximum of 8 ancestors will be represented in this list. An empty list
		// means the Policy is not relevant for any ancestors.

Update GEP-713 and add BackendTLSPolicy implementation #2448

Update GEP-713 and add BackendTLSPolicy implementation #2448

Conversation

youngnick commented Oct 1, 2023

youngnick commented Oct 1, 2023

Choose a reason for hiding this comment

youngnick commented Oct 1, 2023 • edited Loading

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kflynn commented Oct 11, 2023

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 12, 2023

robscott commented Oct 12, 2023

robscott commented Oct 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youngnick commented Oct 1, 2023 •

edited

Loading