-
Notifications
You must be signed in to change notification settings - Fork 468
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.
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
Update GEP-713 and add BackendTLSPolicy implementation #2448
Update GEP-713 and add BackendTLSPolicy implementation #2448
Conversation
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 |
b22773a
to
23444a4
Compare
There was a problem hiding this comment.
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.
a49826e
to
91a98bd
Compare
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. |
5852aca
to
b417cf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// 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 != ”)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these both look correct, but would appreciate new tests in pkg/test/cel
to confirm that they're working as expected.
// | ||
// +kubebuilder:validation:MaxItems=8 | ||
// +optional | ||
CertRefs []v1beta1.ConfigMapObjectReference `json:"certRefs,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Paging @candita though for this one, this is a little different to the discussion we had on the GEP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to no defaults
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated this to use a new ObjectReference
type that has no defaults - this means that group
and kind
are now required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
18af989
to
ad9d57c
Compare
// Support: Implementation-specific (More than one reference, or other kinds | ||
// of resources). | ||
// | ||
// +kubebuilder:validation:MaxItems=8 |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
// | ||
// +kubebuilder:validation:MaxItems=8 | ||
// +optional | ||
CertRefs []v1beta1.ConfigMapObjectReference `json:"certRefs,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to no defaults
|
||
// StandardCertType is the type of CA certificate that will be used when | ||
// the TLS.certRefs is unspecified. | ||
// +kubebuilder:validation:Enum=System |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"match" is a good idea, done.
ad9d57c
to
e7fa9a3
Compare
195e7eb
to
bff8619
Compare
Signed-off-by: Nick Young <nick@isovalent.com>
Signed-off-by: Nick Young <nick@isovalent.com>
bff8619
to
dc577d6
Compare
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. |
68515ff
to
d8bc3df
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @youngnick! A few nits but otherwise LGTM.
// +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 != \"\")" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added #2473 to track adding CEL tests for this. Does not need to block this PR, but probably needs to block v1.0.
// Support: Implementation-specific (More than one reference, or other kinds | ||
// of resources). | ||
// | ||
// +kubebuilder:validation:MaxItems=8 |
There was a problem hiding this comment.
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.
@@ -120,3 +122,81 @@ const ( | |||
// policy is attached to an invalid target resource. | |||
PolicyReasonTargetNotFound PolicyConditionReason = "TargetNotFound" | |||
) | |||
|
There was a problem hiding this comment.
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.
|
||
// StandardCertType is the type of CA certificate that will be used when | ||
// the TLS.certRefs is unspecified. | ||
// +kubebuilder:validation:Enum=System |
There was a problem hiding this comment.
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.
995d310
to
fe6a88d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @youngnick! 3 non-blocking nits, feel free to remove hold if you'd rather cover them in a follow up.
/lgtm
/hold
apis/v1alpha2/policy_types.go
Outdated
// A maximum of 8 ancestors will be represented in this list. An empty list | ||
// means the Policy is not relevant for any ancestors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
apis/v1beta1/httproute_types.go
Outdated
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking nit:
// requirement. | |
// requirement. |
[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:
Approvers can indicate their approval by writing |
Signed-off-by: Nick Young <nick@isovalent.com>
fe6a88d
to
1419758
Compare
/lgtm |
/hold cancel |
// References to a resource in a different namespace are | ||
// invalid for the moment, although we will revisit this | ||
// in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No ReferenceGrant
support?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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?: