Repository: knative/serving
Pull request #13637 (2023-01-23 18:37:37)

Allow challenges for hosts that don't match the route's host
This allows certificate plugins (ie. net-cert-manager) to deal
with domain names that are really long.

Part of https://github.com/knative-sandbox/net-certmanager/issues/214#issuecomment-1400611756

Was testing it via https://github.com/knative/serving/pull/13621

knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/13637#" title="Author self-approved">dprotaso</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Fserving).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[pkg/OWNERS](https://github.com/knative/serving/blob/main/pkg/OWNERS)~~ [dprotaso]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative/serving/pull/13637?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
Base: **86.17**% // Head: **86.17**% // Decreases project coverage by **`-0.01%`** :warning:
> Coverage data is based on head [(`b4c6b6c`)](https://codecov.io/gh/knative/serving/pull/13637?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) compared to base [(`87f5b62`)](https://codecov.io/gh/knative/serving/commit/87f5b626a5814cf6741b0d95323a032b1955f670?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).
> Patch coverage: 88.23% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main   #13637      +/-   ##
==========================================
- Coverage   86.17%   86.17%   -0.01%     
==========================================
  Files         197      197              
  Lines       14722    14727       +5     
==========================================
+ Hits        12687    12691       +4     
+ Misses       1734     1733       -1     
- Partials      301      303       +2     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/13637?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Coverage Δ | |
|---|---|---|
| [pkg/reconciler/route/resources/ingress.go](https://codecov.io/gh/knative/serving/pull/13637?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcmVzb3VyY2VzL2luZ3Jlc3MuZ28=) | `95.00% <85.71%> (-1.19%)` | :arrow_down: |
| [pkg/reconciler/domainmapping/resources/ingress.go](https://codecov.io/gh/knative/serving/pull/13637?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3JlY29uY2lsZXIvZG9tYWlubWFwcGluZy9yZXNvdXJjZXMvaW5ncmVzcy5nbw==) | `100.00% <100.00%> (ø)` | |
| [pkg/autoscaler/statserver/server.go](https://codecov.io/gh/knative/serving/pull/13637?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL2F1dG9zY2FsZXIvc3RhdHNlcnZlci9zZXJ2ZXIuZ28=) | `73.72% <0.00%> (-2.19%)` | :arrow_down: |
| [pkg/reconciler/route/route.go](https://codecov.io/gh/knative/serving/pull/13637?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3JlY29uY2lsZXIvcm91dGUvcm91dGUuZ28=) | `81.23% <0.00%> (+0.92%)` | :arrow_up: |
| [pkg/autoscaler/scaling/multiscaler.go](https://codecov.io/gh/knative/serving/pull/13637?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL2F1dG9zY2FsZXIvc2NhbGluZy9tdWx0aXNjYWxlci5nbw==) | `88.59% <0.00%> (+1.34%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/knative/serving/pull/13637?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).


Patch:
diff --git a/pkg/reconciler/domainmapping/resources/ingress.go b/pkg/reconciler/domainmapping/resources/ingress.go
index d8903c9aa..f9f3b2874 100644
--- a/pkg/reconciler/domainmapping/resources/ingress.go
+++ b/pkg/reconciler/domainmapping/resources/ingress.go
@@ -19,6 +19,7 @@ package resources
 import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/apimachinery/pkg/util/sets"
 
 	netapi "knative.dev/networking/pkg/apis/networking"
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
@@ -35,6 +36,7 @@ import (
 // KIngress).  The created ingress will contain a RewriteHost rule to cause the
 // given hostName to be used as the host.
 func MakeIngress(dm *servingv1alpha1.DomainMapping, backendServiceName, hostName, ingressClass string, httpOption netv1alpha1.HTTPOption, tls []netv1alpha1.IngressTLS, acmeChallenges ...netv1alpha1.HTTP01Challenge) *netv1alpha1.Ingress {
+	paths, hosts := routeresources.MakeACMEIngressPaths(acmeChallenges, sets.NewString(dm.GetName()))
 	return &netv1alpha1.Ingress{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      kmeta.ChildName(dm.GetName(), ""),
@@ -52,11 +54,11 @@ func MakeIngress(dm *servingv1alpha1.DomainMapping, backendServiceName, hostName
 			HTTPOption: httpOption,
 			TLS:        tls,
 			Rules: []netv1alpha1.IngressRule{{
-				Hosts:      []string{dm.Name},
+				Hosts:      append(hosts, dm.Name),
 				Visibility: netv1alpha1.IngressVisibilityExternalIP,
 				HTTP: &netv1alpha1.HTTPIngressRuleValue{
 					// The order of the paths is sensitive, always put tls challenge first
-					Paths: append(routeresources.MakeACMEIngressPaths(acmeChallenges, dm.GetName()),
+					Paths: append(paths,
 						[]netv1alpha1.HTTPIngressPath{{
 							RewriteHost: hostName,
 							Splits: []netv1alpha1.IngressBackendSplit{{
diff --git a/pkg/reconciler/route/resources/ingress.go b/pkg/reconciler/route/resources/ingress.go
index f6cf82618..5046b4861 100644
--- a/pkg/reconciler/route/resources/ingress.go
+++ b/pkg/reconciler/route/resources/ingress.go
@@ -185,8 +185,9 @@ func makeIngressSpec(
 			}
 			// If this is a public rule, we need to configure ACME challenge paths.
 			if visibility == netv1alpha1.IngressVisibilityExternalIP {
-				rule.HTTP.Paths = append(
-					MakeACMEIngressPaths(acmeChallenges, domains...), rule.HTTP.Paths...)
+				paths, hosts := MakeACMEIngressPaths(acmeChallenges, domains)
+				rule.Hosts = append(hosts, rule.Hosts...)
+				rule.HTTP.Paths = append(paths, rule.HTTP.Paths...)
 			}
 			rules = append(rules, rule)
 		}
@@ -214,7 +215,7 @@ func getChallengeHosts(challenges []netv1alpha1.HTTP01Challenge) map[string]netv
 	return c
 }
 
-func routeDomain(ctx context.Context, targetName string, r *servingv1.Route, visibility netv1alpha1.IngressVisibility) ([]string, error) {
+func routeDomain(ctx context.Context, targetName string, r *servingv1.Route, visibility netv1alpha1.IngressVisibility) (sets.String, error) {
 	hostname, err := domains.HostnameFromTemplate(ctx, r.Name, targetName)
 	if err != nil {
 		return nil, err
@@ -232,18 +233,20 @@ func routeDomain(ctx context.Context, targetName string, r *servingv1.Route, vis
 	if isClusterLocal {
 		domains = ingress.ExpandedHosts(sets.NewString(domains...)).List()
 	}
-	return domains, err
+	return sets.NewString(domains...), err
 }
 
 // MakeACMEIngressPaths returns a set of netv1alpha1.HTTPIngressPath
 // that can be used to perform ACME challenges.
-func MakeACMEIngressPaths(acmeChallenges []netv1alpha1.HTTP01Challenge, domains ...string) []netv1alpha1.HTTPIngressPath {
+func MakeACMEIngressPaths(acmeChallenges []netv1alpha1.HTTP01Challenge, domains sets.String) ([]netv1alpha1.HTTPIngressPath, []string) {
 	challenges := getChallengeHosts(acmeChallenges)
+
 	paths := make([]netv1alpha1.HTTPIngressPath, 0, len(challenges))
-	for _, domain := range domains {
-		challenge, ok := challenges[domain]
-		if !ok {
-			continue
+	var extraHosts []string
+
+	for _, challenge := range challenges {
+		if !domains.Has(challenge.URL.Host) {
+			extraHosts = append(extraHosts, challenge.URL.Host)
 		}
 
 		paths = append(paths, netv1alpha1.HTTPIngressPath{
@@ -258,16 +261,16 @@ func MakeACMEIngressPaths(acmeChallenges []netv1alpha1.HTTP01Challenge, domains
 			Path: challenge.URL.Path,
 		})
 	}
-	return paths
+	return paths, extraHosts
 }
 
-func makeIngressRule(domains []string, ns string,
+func makeIngressRule(domains sets.String, ns string,
 	visibility netv1alpha1.IngressVisibility,
 	targets traffic.RevisionTargets,
 	roCfgs []*traffic.ConfigurationRollout,
 	encryption bool) netv1alpha1.IngressRule {
 	return netv1alpha1.IngressRule{
-		Hosts:      domains,
+		Hosts:      domains.List(),
 		Visibility: visibility,
 		HTTP: &netv1alpha1.HTTPIngressRuleValue{
 			Paths: []netv1alpha1.HTTPIngressPath{


Test patch:
diff --git a/pkg/reconciler/route/resources/ingress_test.go b/pkg/reconciler/route/resources/ingress_test.go
index cc6516a3e..89dd75eac 100644
--- a/pkg/reconciler/route/resources/ingress_test.go
+++ b/pkg/reconciler/route/resources/ingress_test.go
@@ -26,6 +26,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/intstr"
+	"k8s.io/apimachinery/pkg/util/sets"
 
 	"knative.dev/networking/pkg/apis/networking"
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
@@ -853,7 +854,7 @@ func TestMakeIngressSpecCorrectRulesWithTagBasedRouting(t *testing.T) {
 
 // One active target.
 func TestMakeIngressRuleVanilla(t *testing.T) {
-	domains := []string{"a.com", "b.org"}
+	domains := sets.NewString("a.com", "b.org")
 	targets := traffic.RevisionTargets{{
 		TrafficTarget: v1.TrafficTarget{
 			ConfigurationName: "config",
@@ -913,7 +914,7 @@ func TestMakeIngressRuleZeroPercentTarget(t *testing.T) {
 			Percent:           ptr.Int64(0),
 		},
 	}}
-	domains := []string{"test.org"}
+	domains := sets.NewString("test.org")
 	tc := &traffic.Config{
 		Targets: map[string]traffic.RevisionTargets{
 			traffic.DefaultTarget: targets,
@@ -969,7 +970,7 @@ func TestMakeIngressRuleTwoTargets(t *testing.T) {
 		},
 	}
 	ro := tc.BuildRollout()
-	domains := []string{"test.org"}
+	domains := sets.NewString("test.org")
 	rule := makeIngressRule(domains, ns, netv1alpha1.IngressVisibilityExternalIP,
 		targets, ro.RolloutsByTag("a-tag"), false /* internal encryption */)
 	expected := netv1alpha1.IngressRule{
diff --git a/pkg/reconciler/route/table_test.go b/pkg/reconciler/route/table_test.go
index eff5454be..accca01c3 100644
--- a/pkg/reconciler/route/table_test.go
+++ b/pkg/reconciler/route/table_test.go
@@ -2758,6 +2758,15 @@ func TestReconcileEnableAutoTLS(t *testing.T) {
 						ServiceName:      "cm-solver",
 						ServicePort:      intstr.FromInt(8090),
 						ServiceNamespace: "default",
+					}, {
+						URL: &apis.URL{
+							Scheme: "http",
+							Host:   "k.example.com",
+							Path:   "/.well-known/acme-challenge/challengeToken2",
+						},
+						ServiceName:      "cm-solver",
+						ServicePort:      intstr.FromInt(8090),
+						ServiceNamespace: "default",
 					}},
 				},
 			},
@@ -2788,6 +2797,15 @@ func TestReconcileEnableAutoTLS(t *testing.T) {
 					ServiceName:      "cm-solver",
 					ServicePort:      intstr.FromInt(8090),
 					ServiceNamespace: "default",
+				}, {
+					URL: &apis.URL{
+						Scheme: "http",
+						Host:   "k.example.com",
+						Path:   "/.well-known/acme-challenge/challengeToken2",
+					},
+					ServiceName:      "cm-solver",
+					ServicePort:      intstr.FromInt(8090),
+					ServiceNamespace: "default",
 				}},
 			),
 			simpleK8sService(