Repository: knative/serving
Pull request #10023 (2020-11-03 13:34:44)

Validate domain name by IsFullyQualifiedDomainName()
This patch adds `IsFullyQualifiedDomainName()` validation when generating the domain.

For example, if the domain template has `domainTemplate:
'{{.Name}}-{{.Namespace}}.{{.Domain}}'`, 
then the `namespace` + `name` exceeds the 63 length limitation of url label (RFC 1123).

Here is an example event error by this PR:

```
34m         Warning   InternalError             route/hello-example                                      invalid domain name "hello-example.default.example.com.Foo": : Invalid value: "hello-example.default.example.com.Foo": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')
```

**Release Note**

```release-note
Domain is validated by k8s library IsFullyQualifiedDomainName().
```

/cc @markusthoemmes @vagababov


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

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10023#" title="Author self-approved">nak3</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/apis/OWNERS](https://github.com/knative/serving/blob/master/pkg/apis/OWNERS)~~ [nak3]

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/10023?src=pr&el=h1) Report
> Merging [#10023](https://codecov.io/gh/knative/serving/pull/10023?src=pr&el=desc) into [master](https://codecov.io/gh/knative/serving/commit/f6ea0d008c020a5e396930d2d30f46113dbaeb4f?el=desc) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative/serving/pull/10023/graphs/tree.svg?width=650&height=150&src=pr&token=MPpDfAj4Ui)](https://codecov.io/gh/knative/serving/pull/10023?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master   #10023      +/-   ##
==========================================
- Coverage   88.04%   88.02%   -0.02%     
==========================================
  Files         184      184              
  Lines        8657     8663       +6     
==========================================
+ Hits         7622     7626       +4     
- Misses        789      790       +1     
- Partials      246      247       +1     
```


| [Impacted Files](https://codecov.io/gh/knative/serving/pull/10023?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/apis/serving/v1/route\_validation.go](https://codecov.io/gh/knative/serving/pull/10023/diff?src=pr&el=tree#diff-cGtnL2FwaXMvc2VydmluZy92MS9yb3V0ZV92YWxpZGF0aW9uLmdv) | `100.00% <100.00%> (ø)` | |
| [pkg/reconciler/revision/reconcile\_resources.go](https://codecov.io/gh/knative/serving/pull/10023/diff?src=pr&el=tree#diff-cGtnL3JlY29uY2lsZXIvcmV2aXNpb24vcmVjb25jaWxlX3Jlc291cmNlcy5nbw==) | `80.72% <0.00%> (-2.41%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative/serving/pull/10023?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative/serving/pull/10023?src=pr&el=footer). Last update [f6ea0d0...3e4e6e5](https://codecov.io/gh/knative/serving/pull/10023?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

vagababov: I.e, can't we verify this on the inbound leg rather than after reconciliation?
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/serving/pull/10023#" title="Author self-approved">nak3</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/reconciler/route/OWNERS](https://github.com/knative/serving/blob/master/pkg/reconciler/route/OWNERS)~~ [nak3]

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":[]} -->
knative-prow-robot: @nak3: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-serving-unit-tests | 788de663fd64054033087d3f7758b7f2e13ad418 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative_serving/10023/pull-knative-serving-unit-tests/1323973774848036864) | `/test pull-knative-serving-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative_serving/10023). [Your PR dashboard](https://gubernator.knative.dev/pr/nak3).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
nak3: Thank you. Updated.

Patch:
diff --git a/pkg/reconciler/route/domains/domains.go b/pkg/reconciler/route/domains/domains.go
index a1cc7fa10..c67ee8a1f 100644
--- a/pkg/reconciler/route/domains/domains.go
+++ b/pkg/reconciler/route/domains/domains.go
@@ -24,6 +24,8 @@ import (
 	"text/template"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/validation"
+	"k8s.io/apimachinery/pkg/util/validation/field"
 	network "knative.dev/networking/pkg"
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
 	"knative.dev/pkg/apis"
@@ -95,6 +97,12 @@ func DomainNameFromTemplate(ctx context.Context, r metav1.ObjectMeta, name strin
 	if err := templ.Execute(&buf, data); err != nil {
 		return "", fmt.Errorf("error executing the DomainTemplate: %w", err)
 	}
+
+	urlErrs := validation.IsFullyQualifiedDomainName(field.NewPath("url"), buf.String())
+	if urlErrs != nil {
+		return "", fmt.Errorf("invalid domain name %q: %w", buf.String(), urlErrs.ToAggregate())
+	}
+
 	return buf.String(), nil
 }
 


Test patch:
diff --git a/pkg/reconciler/route/domains/domains_test.go b/pkg/reconciler/route/domains/domains_test.go
index ac83cf582..9ffc420a5 100644
--- a/pkg/reconciler/route/domains/domains_test.go
+++ b/pkg/reconciler/route/domains/domains_test.go
@@ -90,10 +90,10 @@ func TestDomainNameFromTemplate(t *testing.T) {
 		want:     "test-name.example.com",
 		local:    false,
 	}, {
-		name:     "SuperShort",
+		name:     "Too Short", // domain must be at least two segments separated by dots.
 		template: "{{.Name}}",
 		args:     args{name: "test-name"},
-		want:     "test-name",
+		wantErr:  true,
 		local:    false,
 	}, {
 		name:     "Annotations",
@@ -114,6 +114,12 @@ func TestDomainNameFromTemplate(t *testing.T) {
 		args:     args{name: "test-name"},
 		wantErr:  true,
 		local:    false,
+	}, {
+		name:     "Invalid domain name",
+		template: "{{.Name}}.{{.Namespace}}.{{.Domain}}.Foo",
+		args:     args{name: "test-name"},
+		wantErr:  true,
+		local:    false,
 	}}
 
 	meta := metav1.ObjectMeta{
@@ -222,15 +228,20 @@ func TestGetAllDomainsAndTags(t *testing.T) {
 			"myroute.default.example.com":              "",
 		},
 	}, {
-		name:           "bad template",
+		name:           "bad template in domain",
 		domainTemplate: "{{.NNName}}.{{.Namespace}}.{{.Domain}}",
 		tagTemplate:    "{{.Name}}-{{.Tag}}",
 		wantErr:        true,
 	}, {
-		name:           "bad template",
+		name:           "bad template in template",
 		domainTemplate: "{{.Name}}.{{.Namespace}}.{{.Domain}}",
 		tagTemplate:    "{{.NNName}}-{{.Tag}}",
 		wantErr:        true,
+	}, {
+		name:           "bad domain name",
+		domainTemplate: "{{.Name}}.{{.Namespace}}.{{.Domain}}",
+		tagTemplate:    "Foo.{{.Name}}-{{.Tag}}",
+		wantErr:        true,
 	}}
 
 	route := &v1.Route{
diff --git a/pkg/reconciler/route/route_test.go b/pkg/reconciler/route/route_test.go
index 5b7034c91..5c1fa8d20 100644
--- a/pkg/reconciler/route/route_test.go
+++ b/pkg/reconciler/route/route_test.go
@@ -1562,10 +1562,10 @@ func TestRouteDomain(t *testing.T) {
 		Pass:     true,
 		Expected: "myapp.example.com",
 	}, {
-		Name:     "SuperShort",
+		Name:     "Too short", // domain must be at least two segments separated by dots.
 		Template: "{{.Name}}",
-		Pass:     true,
-		Expected: "myapp",
+		Pass:     false,
+		Expected: "",
 	}, {
 		Name:     "Annotations",
 		Template: `{{.Name}}.{{ index .Annotations "sub"}}.{{.Domain}}`,