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

Add CEL for BackendTLSPolicy targetRefs #3496

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+267 −27
Open

Add CEL for BackendTLSPolicy targetRefs #3496

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3674f33
Add CEL for BackendTLSPolicy targetRefs
snorwin Dec 11, 2024
37da93e
Additional test cases with different groups and kinds for BackendTLSP…
snorwin Dec 12, 2024
94ef768
Increase number of targetRefs in BackenTLSPolicy CEL tests
snorwin Dec 12, 2024
76e172c
Add godoc for targetRef CEL validation in BackendTLSPolicy
snorwin Dec 13, 2024
60b7185
Apply PR suggestions
snorwin Dec 18, 2024
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
11 changes: 11 additions & 0 deletions apis/v1alpha3/backendtlspolicy_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,12 +65,23 @@ type BackendTLSPolicySpec struct {
// by default, but this default may change in the future to provide
// a more granular application of the policy.
//
// TargetRefs must be _distinct_. This means either that:
Copy link
Contributor

Choose a reason for hiding this comment

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

https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.SectionName shows targetRef section name would commonly be a port name on a Service, the expected type for BackendTLSPolicy. For LocalPolicyTargetReferenceWithSectionName, it says sectionName is optional and when not specified, refers to the entire resource.

After some thought, I wonder if there is any use case where one targetRef needs to be associated to a specific sectionName (port name in the Service), and another targetRef doesn't care which port it targets on that Service. In that case shouldn't it be okay to keep one sectionName blank, even though the other sectionName s would have to be specified in order to keep it distinct?

Also, I worry about the fact that sectionName is optional in LocalPolicyTargetReferenceWithSectionName. If LocalPolicyTargetReferenceWithSectionName is used by another resource, will the same validation for distinct-ness (distinction?) apply, or do we foresee a need to require sectionName for distinction here and not when we use LocalPolicyTargetReferenceWithSectionName in another object? And how confusing would the latter be?

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @youngnick @robscott, since we talked about this today in the community meeting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the current version of the CEL, it is valid to keep a sectionName blank for one target (e.g., a Service) and specify sectionName for other targets. However, allowing the same target with and without sectionName and applying the most specific target could be confusing for both implementations and users.

Regarding the second concern that upcoming policies might use different validations for the LocalPolicyTargetReferenceWithSectionName type, we could either extend the description of the type or introduce a CommonLocalPolicySpec which could be reused, similar to the CommonRouteSpec.

//
// * They select different objects. If this is the case, then targetRef
// entries are distinct. In terms of fields, this means that the
// multi-part key defined by `group`, `kind`, and `name` must
// be unique across all targetRef entries in the BackendTLSPolicy.
// * They do not select different objects, each TargetRef that selects
// the same object must set the sectionName to a different value.
snorwin marked this conversation as resolved.
Show resolved Hide resolved
//
// Support: Extended for Kubernetes Service
//
// Support: Implementation-specific for any other resource
//
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:MaxItems=16
// +kubebuilder:validation:XValidation:message="sectionName must be specified when targetRefs includes 2 or more references to the same target",rule="self.all(p1, self.all(p2, p1.group == p2.group && p1.kind == p2.kind && p1.name == p2.name ? ((!has(p1.sectionName) || p1.sectionName == '') == (!has(p2.sectionName) || p2.sectionName == '')) : true))"
snorwin marked this conversation as resolved.
Show resolved Hide resolved
// +kubebuilder:validation:XValidation:message="sectionName must be unique when targetRefs includes 2 or more references to the same target",rule="self.all(p1, self.exists_one(p2, p1.group == p2.group && p1.kind == p2.kind && p1.name == p2.name && (((!has(p1.sectionName) || p1.sectionName == '') && (!has(p2.sectionName) || p2.sectionName == '')) || (has(p1.sectionName) && has(p2.sectionName) && p1.sectionName == p2.sectionName))))"
TargetRefs []v1alpha2.LocalPolicyTargetReferenceWithSectionName `json:"targetRefs"`

// Validation contains backend TLS validation configuration.
Expand Down
23 changes: 23 additions & 0 deletions config/crd/experimental/gateway.networking.k8s.io_backendtlspolicies.yaml
View file
Open in desktop

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

2 changes: 1 addition & 1 deletion pkg/generated/openapi/zz_generated.openapi.go
View file
Open in desktop

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

Loading