Repository: knative-sandbox/net-kourier
Pull request #704 (2021-11-15 05:32:17)

Introduce `KOURIER_HTTPOPTION_DISABLED` env variable to disable redirect
This patch introduces `KOURIER_HTTPOPTION_DISABLED` env variable to stop creating redirect route when it is set.
This option is useful when front end proxy handles the redirection by `spec.HTTPOption`.
e.g. Kourier on OpenShift handles `HTTPOption` by OpenShift Route so `KOURIER_HTTPOPTION_DISABLED` should be set.

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/704#" 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":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/704?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) Report
> Merging [#704](https://codecov.io/gh/knative-sandbox/net-kourier/pull/704?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (cb9bace) into [main](https://codecov.io/gh/knative-sandbox/net-kourier/commit/d89a91a9ab5232f6585e4fb6442f28fde04f1614?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (d89a91a) will **not change** coverage.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative-sandbox/net-kourier/pull/704/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/704?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     #704   +/-   ##
=======================================
  Coverage   69.89%   69.89%           
=======================================
  Files          18       18           
  Lines         671      671           
=======================================
  Hits          469      469           
  Misses        177      177           
  Partials       25       25           
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/net-kourier/pull/704?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/704/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/704?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/704?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Last update [d89a91a...cb9bace](https://codecov.io/gh/knative-sandbox/net-kourier/pull/704?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).

nak3: /cc @markusthoemmes 

Could you please take a look? And do you have any better idea for https://issues.redhat.com/browse/SRVKS-840 ?
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/704#pullrequestreview-805935888" title="Approved">markusthoemmes</a>*, *<a href="https://github.com/knative-sandbox/net-kourier/pull/704#" 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/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")
 )