Repository: knative-sandbox/net-kourier
Pull request #706 (2021-11-16 11:58:45)

[release-1.0] Introduce `KOURIER_HTTPOPTION_DISABLED` env variable to disable redirect
This is an automated cherry-pick of #704

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

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/net-kourier/pull/706#" title="Author self-approved">knative-prow-robot</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **zhiminxiang** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @zhiminxiang` in a comment when ready.

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

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

- **[OWNERS](https://github.com/knative-sandbox/net-kourier/blob/release-1.0/OWNERS)**

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":["zhiminxiang"]} -->
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/706#" title="Author self-approved">knative-prow-robot</a>*, *<a href="https://github.com/knative-sandbox/net-kourier/pull/706#pullrequestreview-807204175" title="Approved">markusthoemmes</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/release-1.0/OWNERS)~~ [markusthoemmes]

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-sandbox/net-kourier/pull/706?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) Report
> Merging [#706](https://codecov.io/gh/knative-sandbox/net-kourier/pull/706?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (bad8e7f) into [release-1.0](https://codecov.io/gh/knative-sandbox/net-kourier/commit/1d908e173b0c4d01b5a62603e6e7e71eae1bcb07?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (1d908e1) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative-sandbox/net-kourier/pull/706/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/706?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)

```diff
@@             Coverage Diff              @@
##           release-1.0     #706   +/-   ##
============================================
  Coverage        70.53%   70.53%           
============================================
  Files               18       18           
  Lines              655      655           
============================================
  Hits               462      462           
  Misses             168      168           
  Partials            25       25           
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/net-kourier/pull/706?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) | Coverage Δ | |
|---|---|---|
| [pkg/generator/ingress\_translator.go](https://codecov.io/gh/knative-sandbox/net-kourier/pull/706/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-cGtnL2dlbmVyYXRvci9pbmdyZXNzX3RyYW5zbGF0b3IuZ28=) | `83.44% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/706?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/706?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Last update [1d908e1...bad8e7f](https://codecov.io/gh/knative-sandbox/net-kourier/pull/706?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).


Patch:
diff --git a/pkg/generator/ingress_translator.go b/pkg/generator/ingress_translator.go
index b416cc42..900ef406 100644
--- a/pkg/generator/ingress_translator.go
+++ b/pkg/generator/ingress_translator.go
@@ -19,6 +19,7 @@ package generator
 import (
 	"context"
 	"fmt"
+	"os"
 	"time"
 
 	v3 "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3"
@@ -175,7 +176,9 @@ func (translator *IngressTranslator) translateIngress(ctx context.Context, ingre
 			}
 
 			if len(wrs) != 0 {
-				if ingress.Spec.HTTPOption == v1alpha1.HTTPOptionRedirected && rule.Visibility == v1alpha1.IngressVisibilityExternalIP {
+				// Do not create redirect route when KOURIER_HTTPOPTION_DISABLED is set. This option is useful when front end proxy handles the redirection.
+				// e.g. Kourier on OpenShift handles HTTPOption by OpenShift Route so KOURIER_HTTPOPTION_DISABLED should be set.
+				if _, ok := os.LookupEnv("KOURIER_HTTPOPTION_DISABLED"); !ok && ingress.Spec.HTTPOption == v1alpha1.HTTPOptionRedirected && rule.Visibility == v1alpha1.IngressVisibilityExternalIP {
 					routes = append(routes, envoy.NewRedirectRoute(
 						pathName, matchHeadersFromHTTPPath(httpPath), path))
 				} else {


Test patch:
diff --git a/pkg/generator/ingress_translator_test.go b/pkg/generator/ingress_translator_test.go
index b2f8bee2..b9aa2bb5 100644
--- a/pkg/generator/ingress_translator_test.go
+++ b/pkg/generator/ingress_translator_test.go
@@ -106,7 +106,7 @@ func TestIngressTranslator(t *testing.T) {
 		state: []runtime.Object{
 			svc("servicens", "servicename"),
 			eps("servicens", "servicename"),
-			secret("secretns", "secretname"),
+			secret,
 		},
 		want: func() *translatedIngress {
 			vHosts := []*route.VirtualHost{
@@ -173,7 +173,7 @@ func TestIngressTranslator(t *testing.T) {
 		state: []runtime.Object{
 			svc("servicens", "servicename"),
 			eps("servicens", "servicename"),
-			secret("secretns", "secretname"),
+			secret,
 		},
 		want: func() *translatedIngress {
 			vHosts := []*route.VirtualHost{
@@ -258,7 +258,7 @@ func TestIngressTranslator(t *testing.T) {
 		state: []runtime.Object{
 			svc("servicens", "servicename"),
 			eps("servicens", "servicename"),
-			secret("secretns", "secretname"),
+			secret,
 		},
 		want: func() *translatedIngress {
 			vHosts := []*route.VirtualHost{
@@ -546,6 +546,179 @@ func TestIngressTranslator(t *testing.T) {
 	}
 }
 
+// TestIngressTranslatorWithHTTPOptionDisabled runs same redirect test in TestIngressTranslator with KOURIER_HTTPOPTION_DISABLED env value.
+func TestIngressTranslatorWithHTTPOptionDisabled(t *testing.T) {
+	tests := []struct {
+		name  string
+		in    *v1alpha1.Ingress
+		state []runtime.Object
+		want  *translatedIngress
+	}{{
+		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,
+		},
+		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:    vHosts,
+				externalTLSVirtualHosts: vHosts,
+				internalVirtualHosts:    vHosts,
+			}
+		}(),
+	}, {
+		// 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,
+		},
+		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,
+			}
+		}(),
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			t.Setenv("KOURIER_HTTPOPTION_DISABLED", "true")
+			ctx, _ := pkgtest.SetupFakeContext(t)
+			kubeclient := fake.NewSimpleClientset(test.state...)
+
+			translator := NewIngressTranslator(
+				func(ns, name string) (*corev1.Secret, error) {
+					return kubeclient.CoreV1().Secrets(ns).Get(ctx, name, metav1.GetOptions{})
+				},
+				func(ns, name string) (*corev1.Endpoints, error) {
+					return kubeclient.CoreV1().Endpoints(ns).Get(ctx, name, metav1.GetOptions{})
+				},
+				func(ns, name string) (*corev1.Service, error) {
+					return kubeclient.CoreV1().Services(ns).Get(ctx, name, metav1.GetOptions{})
+				},
+				&pkgtest.FakeTracker{},
+			)
+
+			got, err := translator.translateIngress(ctx, test.in, false)
+			assert.NilError(t, err)
+			assert.DeepEqual(t, got, test.want,
+				cmp.AllowUnexported(translatedIngress{}),
+				protocmp.Transform(),
+			)
+		})
+	}
+}
+
 func ing(ns, name string, opts ...func(*v1alpha1.Ingress)) *v1alpha1.Ingress {
 	ingress := &v1alpha1.Ingress{
 		ObjectMeta: metav1.ObjectMeta{
@@ -647,20 +820,17 @@ var lbEndpoints = []*endpoint.LbEndpoint{
 	envoy.NewLBEndpoint("5.5.5.5", 8080),
 }
 
-func secret(ns, name string) *corev1.Secret {
-	return &corev1.Secret{
+var (
+	cert       = []byte("cert")
+	privateKey = []byte("key")
+	secret     = &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			Namespace: ns,
-			Name:      name,
+			Namespace: "secretns",
+			Name:      "secretname",
 		},
 		Data: map[string][]byte{
 			"tls.crt": cert,
 			"tls.key": privateKey,
 		},
 	}
-}
-
-var (
-	cert       = []byte("cert")
-	privateKey = []byte("key")
 )