allow multiple SANS in upstream validation

[KauzClay]

Fixes #5520

release-note/minor

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4871aaf) 78.77% compared to head (b291479) 78.81%.
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5849      +/-   ##
==========================================
+ Coverage   78.77%   78.81%   +0.03%     
==========================================
  Files         138      138              
  Lines       19747    19765      +18     
==========================================
+ Hits        15555    15577      +22     
+ Misses       3888     3885       -3     
+ Partials      304      303       -1     

Files	Coverage Δ
internal/dag/cache.go	96.67% <100.00%> (+0.92%)	⬆️
internal/dag/dag.go	98.70% <100.00%> (ø)
internal/dag/extension_processor.go	95.90% <100.00%> (ø)
internal/envoy/cluster.go	100.00% <100.00%> (ø)
internal/envoy/v3/auth.go	100.00% <100.00%> (ø)
internal/featuretests/v3/envoy.go	99.08% <100.00%> (ø)

... and 3 files with indirect coverage changes

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[KauzClay]

hmm it seems like the rule I'm trying to add costs too much, just barely though :/

spec.validation.openAPIV3Schema.properties[spec].properties[routes].items.properties[services].items.properties[validation].x-kubernetes-validations[0].rule: Forbidden: estimated rule cost exceeds budget by factor of 1.017731x (try simplifying the rule, or adding maxItems, maxProperties, and maxLength where arrays, maps, and strings are declared)

// +kubebuilder:validation:XValidation:message="subjectNames[0] must equal subjectName if set",rule="has(self.subjectNames) ? self.subjectNames[0] == self.subjectName : true"

I don't quite understand why the operation is too expensive. Maybe it is because we are checking equality on string with no max length? I don't know if you can set maxlength on the strings within the array...

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[sunjayBhatia]

hmm it seems like the rule I'm trying to add costs too much, just barely though :/

spec.validation.openAPIV3Schema.properties[spec].properties[routes].items.properties[services].items.properties[validation].x-kubernetes-validations[0].rule: Forbidden: estimated rule cost exceeds budget by factor of 1.017731x (try simplifying the rule, or adding maxItems, maxProperties, and maxLength where arrays, maps, and strings are declared)

// +kubebuilder:validation:XValidation:message="subjectNames[0] must equal subjectName if set",rule="has(self.subjectNames) ? self.subjectNames[0] == self.subjectName : true"

I don't quite understand why the operation is too expensive. Maybe it is because we are checking equality on string with no max length? I don't know if you can set maxlength on the strings within the array...

Hm, adding a new type alias for string with a min/max length and using that doesn't seem to reduce complexity

adding a check for existence of self.subjectName increases complexity slightly somehow (rule="has(self.subjectNames) && has(self.subjectName)...)

The CustomResourceDefinition "httpproxies.projectcontour.io" is invalid:
* spec.validation.openAPIV3Schema.properties[spec].properties[routes].items.properties[services].items.properties[validation].x-kubernetes-validations[0].rule: Forbidden: estimated rule cost exceeds budget by factor of 1.036235x (try simplifying the rule, or adding maxItems, maxProperties, and maxLength where arrays, maps, and strings are declared)

[sunjayBhatia]

hmm it seems like the rule I'm trying to add costs too much, just barely though :/

spec.validation.openAPIV3Schema.properties[spec].properties[routes].items.properties[services].items.properties[validation].x-kubernetes-validations[0].rule: Forbidden: estimated rule cost exceeds budget by factor of 1.017731x (try simplifying the rule, or adding maxItems, maxProperties, and maxLength where arrays, maps, and strings are declared)

// +kubebuilder:validation:XValidation:message="subjectNames[0] must equal subjectName if set",rule="has(self.subjectNames) ? self.subjectNames[0] == self.subjectName : true"

I don't quite understand why the operation is too expensive. Maybe it is because we are checking equality on string with no max length? I don't know if you can set maxlength on the strings within the array...

Hm, adding a new type alias for string with a min/max length and using that doesn't seem to reduce complexity

adding a check for existence of self.subjectName increases complexity slightly somehow (rule="has(self.subjectNames) && has(self.subjectName)...)

The CustomResourceDefinition "httpproxies.projectcontour.io" is invalid:
* spec.validation.openAPIV3Schema.properties[spec].properties[routes].items.properties[services].items.properties[validation].x-kubernetes-validations[0].rule: Forbidden: estimated rule cost exceeds budget by factor of 1.036235x (try simplifying the rule, or adding maxItems, maxProperties, and maxLength where arrays, maps, and strings are declared)

Ah, reducing the max length of the subjectName field actually gets things working (e.g. to 100 vs 256)

diff --git a/apis/projectcontour/v1/httpproxy.go b/apis/projectcontour/v1/httpproxy.go
index e6bb66d9..d8a563b5 100644
--- a/apis/projectcontour/v1/httpproxy.go
+++ b/apis/projectcontour/v1/httpproxy.go
@@ -1318,7 +1318,7 @@ type UpstreamValidation struct {
        // Key which is expected to be present in the 'subjectAltName' of the presented certificate.
        // Deprecated: migrate to using the plural field subjectNames.
        // +kubebuilder:validation:MinLength=1
-       // +kubebuilder:validation:MaxLength=256
+       // +kubebuilder:validation:MaxLength=100
        SubjectName string `json:"subjectName"`
        // List of keys, of which at least one is expected to be present in the 'subjectAltName of the
        // presented certificate.

I'm not sure in practice if there is a limit in any RFC here on the length of an DNS name or other identifier in a cert: https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.6

[github-actions]

The Contour project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 14d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, the PR is closed

You can:

• Ensure your PR is passing all CI checks. PRs that are fully green are more likely to be reviewed. If you are having trouble with CI checks, reach out to the #contour channel in the Kubernetes Slack workspace.
• Mark this PR as fresh by commenting or pushing a commit
• Close this PR
• Offer to help out with triage

Please send feedback to the #contour channel in the Kubernetes Slack

[sunjayBhatia]

Can we remove the WIP from the title here @KauzClay

This looks like it should work, only minor issue would be that we're adding a length limit to a field that did not used to have one, something to call out in the release note just in case. Technically this could be a breaking change but I would think this is a rare case? Thoughts @projectcontour/maintainers @projectcontour/contour-reviewers?

[sunjayBhatia]

Approving but will wait for some more thoughts from @projectcontour/maintainers and @projectcontour/contour-reviewers

[skriss]

Do we know what the highest possible max we could use here is? The higher it is, the less likely it is to break a user, and given that this is effectively a breaking API change, I'd rather go as high as we possibly can to try to minimize the set of affected users.

[KauzClay]

good point, I will try to find the max possible value

[KauzClay]

Okay, it seems to accept a max length of 250.

Aside, do you have any idea on how to better test this?

I've tried to apply the CRD to my own cluster, and I don't get an error, but then when I push and the PR tests run, it fails after installing giving me the warning about the CEL score.

[skriss]

hmm, not sure off the top of my head, @sunjayBhatia any thoughts here since you were playing around with this earlier?

I do feel much better about 250 vs. 100 as a limit, seems much less likely to have any actual user impact.

[sunjayBhatia]

what version of k8s is your cluster @KauzClay? You don't happen to have the CustomResourceValidationExpressions feature gate off?

Also TIL https://playcel.undistro.io/ exists which might be useful

[sunjayBhatia]

If we want to test the CEL validation itself works as expected, we could add an e2e test here too:

contour/test/e2e/httpproxy/httpproxy_test.go

Lines 57 to 66 in 0b8ab80

	// Contains specs that test that kubebuilder API validations
	// work as expected, and do not require a Contour instance to
	// be running.
	var _ = Describe("HTTPProxy API validation", func() {
	f.NamespacedTest("httpproxy-required-field-validation", testRequiredFieldValidation)
	f.NamespacedTest("httpproxy-invalid-wildcard-fqdn", testWildcardFQDN)
	f.NamespacedTest("invalid-cookie-rewrite-fields", testInvalidCookieRewriteFields)
	})

e.g. try to create an HTTPProxy with upstreamvalidation set such that the resource should be rejected, and appropriate assertions etc.

[KauzClay]

I was using a 1.27.7 from GKE, but I didn't know about CustomResourceValidationExpressions feature gate though, maybe that is turned off somehow?

[sunjayBhatia]

According to https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/#feature-gates-for-graduated-or-deprecated-features that feature gate should be on by default in 1.27, but maybe GKE is doing something weird?

[skriss]

hmm, not sure off the top of my head, @sunjayBhatia any thoughts here since you were playing around with this earlier?

I do feel much better about 250 vs. 100 as a limit, seems much less likely to have any actual user impact.

[skriss]

Couple little things but otherwise looks about ready to me

[skriss]

LGTM, thanks @KauzClay!

[skriss]

@sunjayBhatia leaving this one for you to merge when ready

[skriss]

So many CI flakes today..