Repository: knative-sandbox/net-kourier
Pull request #619 (2021-08-29 13:51:26)

Support `httpProtocol: Redirected`
This patch supports `Spec.HTTPOption` in KIngress as well as
`httpProtocol: Redirected` in config-network.

Note about internal stuff as the redirect support needs some
internal changes:

Currently RouteConfig (`external_services`) is used by both HTTP
Listener and HTTPS Listener when TLS is enabled.
However, to add redirect support, we cannot share the RouteConfig as
HTTPListener needs Route with `RedirectAction`.

So, this patch introduces another RouteConfig
(`external_tls_services`) for the HTTPS Listener.

Fix https://github.com/knative/networking/issues/304 https://github.com/knative-sandbox/net-kourier/issues/428

**Release Note*

```release-note
net-kourier supports `httpProtocol: Redirected` in config-network.
```


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

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/net-kourier/pull/619#" 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-sandbox%2Fnet-kourier).

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:

- ~~[OWNERS](https://github.com/knative-sandbox/net-kourier/blob/main/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-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-sandbox-net-kourier-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/envoy/api/route.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/619/pull-knative-sandbox-net-kourier-go-coverage/1431977811765104640/artifacts/line-cov.html#file0) | 100.0% | 80.0% | -20.0
[pkg/generator/caches.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/619/pull-knative-sandbox-net-kourier-go-coverage/1431977811765104640/artifacts/line-cov.html#file1) | 65.1% | 68.9% | 3.8
[pkg/generator/ingress_translator.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/619/pull-knative-sandbox-net-kourier-go-coverage/1431977811765104640/artifacts/line-cov.html#file2) | 88.5% | 88.5% | -0.1

codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/619?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) Report
> Merging [#619](https://codecov.io/gh/knative-sandbox/net-kourier/pull/619?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (f3e4aaf) into [main](https://codecov.io/gh/knative-sandbox/net-kourier/commit/c722ae9333fc0b9b1a0af2c699be7fe094f3818c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (c722ae9) will **decrease** coverage by `1.17%`.
> The diff coverage is `59.09%`.

[![Impacted file tree graph](https://codecov.io/gh/knative-sandbox/net-kourier/pull/619/graphs/tree.svg?width=650&height=150&src=pr&token=KmlRo23zX4&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)](https://codecov.io/gh/knative-sandbox/net-kourier/pull/619?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)

```diff
@@            Coverage Diff             @@
##             main     #619      +/-   ##
==========================================
- Coverage   71.31%   70.13%   -1.18%     
==========================================
  Files          18       18              
  Lines         617      653      +36     
==========================================
+ Hits          440      458      +18     
- Misses        151      169      +18     
  Partials       26       26              
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/net-kourier/pull/619?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) | Coverage Δ | |
|---|---|---|
| [pkg/envoy/api/route.go](https://codecov.io/gh/knative-sandbox/net-kourier/pull/619/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-cGtnL2Vudm95L2FwaS9yb3V0ZS5nbw==) | `62.79% <0.00%> (-37.21%)` | :arrow_down: |
| [pkg/generator/caches.go](https://codecov.io/gh/knative-sandbox/net-kourier/pull/619/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-cGtnL2dlbmVyYXRvci9jYWNoZXMuZ28=) | `63.39% <90.00%> (+3.77%)` | :arrow_up: |
| [pkg/generator/ingress\_translator.go](https://codecov.io/gh/knative-sandbox/net-kourier/pull/619/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-cGtnL2dlbmVyYXRvci9pbmdyZXNzX3RyYW5zbGF0b3IuZ28=) | `82.99% <94.44%> (+0.77%)` | :arrow_up: |
| [pkg/generator/sni.go](https://codecov.io/gh/knative-sandbox/net-kourier/pull/619/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-cGtnL2dlbmVyYXRvci9zbmkuZ28=) | `86.66% <0.00%> (-13.34%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/619?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/619?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Last update [c722ae9...f3e4aaf](https://codecov.io/gh/knative-sandbox/net-kourier/pull/619?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox).

markusthoemmes: I've tried to review this 3 times now but struggle to gauge the impact :cry:. Do you think we can split this into 2 PRs, where the first one would just do the split between HTTP and HTTPS stuff and the second adds the redirect route? Or is that not possible?
nak3: Sure, no problem. I sent the part 1 (split between HTTP and HTTPS stuff ) https://github.com/knative-sandbox/net-kourier/pull/626
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-sandbox-net-kourier-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/envoy/api/route.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/619/pull-knative-sandbox-net-kourier-go-coverage/1435115132718944256/artifacts/line-cov.html#file0) | 100.0% | 80.0% | -20.0
[pkg/generator/caches.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/619/pull-knative-sandbox-net-kourier-go-coverage/1435115132718944256/artifacts/line-cov.html#file1) | 65.1% | 68.9% | 3.8
[pkg/generator/ingress_translator.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/619/pull-knative-sandbox-net-kourier-go-coverage/1435115132718944256/artifacts/line-cov.html#file2) | 88.5% | 88.0% | -0.6

knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-sandbox-net-kourier-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/envoy/api/route.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/619/pull-knative-sandbox-net-kourier-go-coverage/1435125671654330368/artifacts/line-cov.html#file0) | 100.0% | 80.0% | -20.0
[pkg/generator/caches.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/619/pull-knative-sandbox-net-kourier-go-coverage/1435125671654330368/artifacts/line-cov.html#file1) | 65.1% | 69.2% | 4.2
[pkg/generator/ingress_translator.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/619/pull-knative-sandbox-net-kourier-go-coverage/1435125671654330368/artifacts/line-cov.html#file2) | 88.5% | 88.0% | -0.6

knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-sandbox-net-kourier-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/envoy/api/route.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/619/pull-knative-sandbox-net-kourier-go-coverage/1435134888427655168/artifacts/line-cov.html#file0) | 100.0% | 80.0% | -20.0
[pkg/generator/ingress_translator.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/619/pull-knative-sandbox-net-kourier-go-coverage/1435134888427655168/artifacts/line-cov.html#file1) | 87.7% | 88.0% | 0.2

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

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/net-kourier/pull/619#pullrequestreview-747718651" title="Approved">markusthoemmes</a>*, *<a href="https://github.com/knative-sandbox/net-kourier/pull/619#" 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-sandbox%2Fnet-kourier).

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:

- ~~[OWNERS](https://github.com/knative-sandbox/net-kourier/blob/main/OWNERS)~~ [markusthoemmes,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":[]} -->

Patch:
diff --git a/pkg/envoy/api/route.go b/pkg/envoy/api/route.go
index 25b32a3c..54a511ae 100644
--- a/pkg/envoy/api/route.go
+++ b/pkg/envoy/api/route.go
@@ -66,3 +66,25 @@ func NewRoute(name string,
 		RequestHeadersToAdd: headersToAdd(headers),
 	}
 }
+
+func NewRedirectRoute(name string,
+	headersMatch []*route.HeaderMatcher,
+	path string,
+) *route.Route {
+	return &route.Route{
+		Name: name,
+		Match: &route.RouteMatch{
+			PathSpecifier: &route.RouteMatch_Prefix{
+				Prefix: path,
+			},
+			Headers: headersMatch,
+		},
+		Action: &route.Route_Redirect{
+			Redirect: &route.RedirectAction{
+				SchemeRewriteSpecifier: &route.RedirectAction_HttpsRedirect{
+					HttpsRedirect: true,
+				},
+			},
+		},
+	}
+}
diff --git a/pkg/generator/ingress_translator.go b/pkg/generator/ingress_translator.go
index 27d92725..3eb903d6 100644
--- a/pkg/generator/ingress_translator.go
+++ b/pkg/generator/ingress_translator.go
@@ -175,10 +175,16 @@ func (translator *IngressTranslator) translateIngress(ctx context.Context, ingre
 			}
 
 			if len(wrs) != 0 {
-				routes = append(routes, envoy.NewRoute(
-					pathName, matchHeadersFromHTTPPath(httpPath), path, wrs, 0, httpPath.AppendHeaders, httpPath.RewriteHost))
+				if ingress.Spec.HTTPOption == v1alpha1.HTTPOptionRedirected && rule.Visibility == v1alpha1.IngressVisibilityExternalIP {
+					routes = append(routes, envoy.NewRedirectRoute(
+						pathName, matchHeadersFromHTTPPath(httpPath), path))
+				} else {
+					routes = append(routes, envoy.NewRoute(
+						pathName, matchHeadersFromHTTPPath(httpPath), path, wrs, 0, httpPath.AppendHeaders, httpPath.RewriteHost))
+				}
 				if len(ingress.Spec.TLS) != 0 {
-					tlsRoutes = append(tlsRoutes, routes...)
+					tlsRoutes = append(tlsRoutes, envoy.NewRoute(
+						pathName, matchHeadersFromHTTPPath(httpPath), path, wrs, 0, httpPath.AppendHeaders, httpPath.RewriteHost))
 				}
 			}
 		}


Test patch:
diff --git a/pkg/generator/ingress_translator_test.go b/pkg/generator/ingress_translator_test.go
index fbf8f707..b2f8bee2 100644
--- a/pkg/generator/ingress_translator_test.go
+++ b/pkg/generator/ingress_translator_test.go
@@ -160,6 +160,158 @@ func TestIngressTranslator(t *testing.T) {
 				internalVirtualHosts:    vHosts,
 			}
 		}(),
+	}, {
+		name: "tls redirect",
+		in: ing("testspace", "testname", func(ing *v1alpha1.Ingress) {
+			ing.Spec.TLS = []v1alpha1.IngressTLS{{
+				Hosts:           []string{"foo.example.com"},
+				SecretNamespace: "secretns",
+				SecretName:      "secretname",
+			}}
+			ing.Spec.HTTPOption = v1alpha1.HTTPOptionRedirected
+		}),
+		state: []runtime.Object{
+			svc("servicens", "servicename"),
+			eps("servicens", "servicename"),
+			secret("secretns", "secretname"),
+		},
+		want: func() *translatedIngress {
+			vHosts := []*route.VirtualHost{
+				envoy.NewVirtualHost(
+					"(testspace/testname).Rules[0]",
+					[]string{"foo.example.com", "foo.example.com:*"},
+					[]*route.Route{envoy.NewRoute(
+						"(testspace/testname).Rules[0].Paths[/test]",
+						[]*route.HeaderMatcher{{
+							Name: "testheader",
+							HeaderMatchSpecifier: &route.HeaderMatcher_ExactMatch{
+								ExactMatch: "foo",
+							},
+						}},
+						"/test",
+						[]*route.WeightedCluster_ClusterWeight{
+							envoy.NewWeightedCluster("servicens/servicename", 100, map[string]string{"baz": "gna"}),
+						},
+						0,
+						map[string]string{"foo": "bar"},
+						"rewritten.example.com"),
+					},
+				),
+			}
+			vHostsRedirect := []*route.VirtualHost{
+				envoy.NewVirtualHost(
+					"(testspace/testname).Rules[0]",
+					[]string{"foo.example.com", "foo.example.com:*"},
+					[]*route.Route{envoy.NewRedirectRoute(
+						"(testspace/testname).Rules[0].Paths[/test]",
+						[]*route.HeaderMatcher{{
+							Name: "testheader",
+							HeaderMatchSpecifier: &route.HeaderMatcher_ExactMatch{
+								ExactMatch: "foo",
+							},
+						}},
+						"/test"),
+					},
+				),
+			}
+
+			return &translatedIngress{
+				name: types.NamespacedName{
+					Namespace: "testspace",
+					Name:      "testname",
+				},
+				sniMatches: []*envoy.SNIMatch{{
+					Hosts: []string{"foo.example.com"},
+					CertSource: types.NamespacedName{
+						Namespace: "secretns",
+						Name:      "secretname",
+					},
+					CertificateChain: cert,
+					PrivateKey:       privateKey,
+				}},
+				clusters: []*v3.Cluster{
+					envoy.NewCluster(
+						"servicens/servicename",
+						5*time.Second,
+						lbEndpoints,
+						false,
+						v3.Cluster_STATIC,
+					),
+				},
+				externalVirtualHosts:    vHostsRedirect,
+				externalTLSVirtualHosts: vHosts,
+				internalVirtualHosts:    vHostsRedirect,
+			}
+		}(),
+	}, {
+		// cluster local is not affected by HTTPOption.
+		name: "tls redirect cluster local",
+		in: ing("testspace", "testname", func(ing *v1alpha1.Ingress) {
+			ing.Spec.TLS = []v1alpha1.IngressTLS{{
+				Hosts:           []string{"foo.example.com"},
+				SecretNamespace: "secretns",
+				SecretName:      "secretname",
+			}}
+			ing.Spec.HTTPOption = v1alpha1.HTTPOptionRedirected
+			ing.Spec.Rules[0].Visibility = v1alpha1.IngressVisibilityClusterLocal
+		}),
+		state: []runtime.Object{
+			svc("servicens", "servicename"),
+			eps("servicens", "servicename"),
+			secret("secretns", "secretname"),
+		},
+		want: func() *translatedIngress {
+			vHosts := []*route.VirtualHost{
+				envoy.NewVirtualHost(
+					"(testspace/testname).Rules[0]",
+					[]string{"foo.example.com", "foo.example.com:*"},
+					[]*route.Route{envoy.NewRoute(
+						"(testspace/testname).Rules[0].Paths[/test]",
+						[]*route.HeaderMatcher{{
+							Name: "testheader",
+							HeaderMatchSpecifier: &route.HeaderMatcher_ExactMatch{
+								ExactMatch: "foo",
+							},
+						}},
+						"/test",
+						[]*route.WeightedCluster_ClusterWeight{
+							envoy.NewWeightedCluster("servicens/servicename", 100, map[string]string{"baz": "gna"}),
+						},
+						0,
+						map[string]string{"foo": "bar"},
+						"rewritten.example.com"),
+					},
+				),
+			}
+
+			return &translatedIngress{
+				name: types.NamespacedName{
+					Namespace: "testspace",
+					Name:      "testname",
+				},
+				sniMatches: []*envoy.SNIMatch{{
+					Hosts: []string{"foo.example.com"},
+					CertSource: types.NamespacedName{
+						Namespace: "secretns",
+						Name:      "secretname",
+					},
+					CertificateChain: cert,
+					PrivateKey:       privateKey,
+				}},
+				clusters: []*v3.Cluster{
+					envoy.NewCluster(
+						"servicens/servicename",
+						5*time.Second,
+						lbEndpoints,
+						false,
+						v3.Cluster_STATIC,
+					),
+				},
+				externalVirtualHosts:    []*route.VirtualHost{},
+				externalTLSVirtualHosts: []*route.VirtualHost{},
+				internalVirtualHosts:    vHosts,
+			}
+		}(),
 	}, {
 		name: "split",
 		in: ing("testspace", "testname", func(ing *v1alpha1.Ingress) {
diff --git a/test/e2e-kind.sh b/test/e2e-kind.sh
index 824a884d..e55e882f 100755
--- a/test/e2e-kind.sh
+++ b/test/e2e-kind.sh
@@ -35,7 +35,6 @@ export "GATEWAY_NAMESPACE_OVERRIDE=${KOURIER_GATEWAY_NAMESPACE}"
 echo ">> Running conformance tests"
 go test -count=1 -short -timeout=20m -tags=e2e ./test/conformance/... ./test/e2e/... \
   --enable-alpha --enable-beta \
-  --skip-tests="httpoption" \
   --ingressendpoint="${IPS[0]}" \
   --ingressClass=kourier.ingress.networking.knative.dev \
   --cluster-suffix="$CLUSTER_SUFFIX"
diff --git a/test/e2e-tests.sh b/test/e2e-tests.sh
index de861ede..8cc6b93f 100755
--- a/test/e2e-tests.sh
+++ b/test/e2e-tests.sh
@@ -25,7 +25,6 @@ go_test_e2e -timeout=20m -parallel=12 \
   ./test/conformance \
   ./test/e2e/ \
   --enable-alpha --enable-beta \
-  --skip-tests="httpoption" \
   --ingressClass=kourier.ingress.networking.knative.dev || failed=1
 
 # Give the controller time to sync with the rest of the system components.