Repository: kubernetes-sigs/aws-alb-ingress-controller
Pull request #1162 (2020-02-11 12:34:34)

Detect unconditional redirects and ignore any rules defined afterwards
(_this is my first PR for GoLang code and for Kubernetes-related projects, so bear with me if I make mistakes or faux pas_)

ALBs only support 100 rules and we have hit the limit in our project -  we are looking for a way to reduce rules and have identified that the aws alb controller is creating some unneeded ALB rules.

We use a annotation to redirect port 80 requests to port 443. This is the annotation documentation: https://github.com/kubernetes-sigs/aws-alb-ingress-controller/blob/master/docs/guide/tasks/ssl_redirect.md

If a unconditional redirect rule is present in the configuration then all following rules within the same listener are not needed. This PR changes things so they are skipped thereby conserving rules.

For the biggest benefit, it is best to define the redirect rule first in the config. Because only subsequent rules are skipped. This is in line with the ssl-redirect annotation which [says](https://github.com/kubernetes-sigs/aws-alb-ingress-controller/blob/master/docs/guide/tasks/ssl_redirect.md):
> the ssl-redirect action must be be first rule(which will be evaluated first by ALB)

---

I have tested on our project, with success. But please let me know if I can make any improvements.

If this is not something that fits with the direction of the project, no problem, please close this PR. I only created it in case it's useful - I'm 100% happy to close if not the case.

---

Relvant GitHub issue https://github.com/kubernetes-sigs/aws-alb-ingress-controller/issues/853#issuecomment-578547468

k8s-ci-robot: Welcome @tomfotherby! <br><br>It looks like this is your first PR to <a href='https://github.com/kubernetes-sigs/aws-alb-ingress-controller'>kubernetes-sigs/aws-alb-ingress-controller</a> 🎉. Please refer to our [pull request process documentation](https://git.k8s.io/community/contributors/guide/pull-requests.md) to help your PR have a smooth ride to approval. <br><br>You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. [Here is the bot commands documentation](https://go.k8s.io/bot-commands). <br><br>You can also check if kubernetes-sigs/aws-alb-ingress-controller has [its own contribution guidelines](https://github.com/kubernetes-sigs/aws-alb-ingress-controller/tree/master/CONTRIBUTING.md). <br><br>You may want to refer to our [testing guide](https://git.k8s.io/community/contributors/devel/sig-testing/testing.md) if you run into trouble with your tests not passing. <br><br>If you are having difficulty getting your pull request seen, please follow the [recommended escalation practices](https://github.com/kubernetes/community/blob/master/contributors/guide/pull-requests.md#why-is-my-pull-request-not-getting-reviewed). Also, for tips and tricks in the contribution process you may want to read the [Kubernetes contributor cheat sheet](https://git.k8s.io/community/contributors/guide/contributor-cheatsheet/README.md). We want to make sure your contribution gets all the attention it needs! <br><br>Thank you, and welcome to Kubernetes. :smiley:
k8s-ci-robot: Hi @tomfotherby. Thanks for your PR.

I'm waiting for a [kubernetes-sigs](https://github.com/orgs/kubernetes-sigs/people) member to verify that this patch is reasonable to test. If it is, they should reply with `/ok-to-test` on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should [join the org](https://git.k8s.io/community/community-membership.md#member) to skip this step.

Once the patch is verified, the new status will be reflected by the `ok-to-test` label.

I understand the commands that are listed [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Faws-alb-ingress-controller).

<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.
</details>

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/aws-alb-ingress-controller/pull/1162#" title="Author self-approved">tomfotherby</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **bigkraig**
You can assign the PR to them by writing `/assign @bigkraig` 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=kubernetes-sigs%2Faws-alb-ingress-controller).

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

- **[OWNERS](https://github.com/kubernetes-sigs/aws-alb-ingress-controller/blob/master/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":["bigkraig"]} -->
tomfotherby: /assign @bigkraig
tomfotherby: We have been running a ALB ingress controller that uses the code in this PR at https://peopleperhour.com in production successfully. It freed up > 40 ALB rules for us. The ALB gets about 110 requests per second:

![Selection_311](https://user-images.githubusercontent.com/140179/74457639-75f51d80-4e80-11ea-9aed-a2717e7a951f.png)


M00nF1sh: /ok-to-test
k8s-ci-robot: @tomfotherby: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-aws-alb-ingress-controller-lint | 9274a84167b029284259fac29370724eeae98c03 | [link](https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-alb-ingress-controller/1162/pull-aws-alb-ingress-controller-lint/1228106163543871495) | `/test pull-aws-alb-ingress-controller-lint`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=aws-alb-ingress-controller&pr=1162). [Your PR dashboard](https://gubernator.k8s.io/pr/tomfotherby). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/aws-alb-ingress-controller/issues?q=is:issue+is:open) when you hit one in your PR.

<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 -->
k8s-ci-robot: @tomfotherby: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-aws-alb-ingress-controller-lint | 9274a84167b029284259fac29370724eeae98c03 | [link](https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-alb-ingress-controller/1162/pull-aws-alb-ingress-controller-lint/1228106163543871494) | `/test pull-aws-alb-ingress-controller-lint`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=aws-alb-ingress-controller&pr=1162). [Your PR dashboard](https://gubernator.k8s.io/pr/tomfotherby). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/aws-alb-ingress-controller/issues?q=is:issue+is:open) when you hit one in your PR.

<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 -->
k8s-ci-robot: @tomfotherby: The following tests **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-aws-alb-ingress-controller-lint | 9274a84167b029284259fac29370724eeae98c03 | [link](https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-alb-ingress-controller/1162/pull-aws-alb-ingress-controller-lint/1228106163543871494) | `/test pull-aws-alb-ingress-controller-lint`
pull-aws-alb-ingress-controller-e2e-test | 9274a84167b029284259fac29370724eeae98c03 | [link](https://prow.k8s.io/view/gcs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_aws-alb-ingress-controller/1162/pull-aws-alb-ingress-controller-e2e-test/1228106163548065793) | `/test pull-aws-alb-ingress-controller-e2e-test`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=aws-alb-ingress-controller&pr=1162). [Your PR dashboard](https://gubernator.k8s.io/pr/tomfotherby). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/aws-alb-ingress-controller/issues?q=is:issue+is:open) when you hit one in your PR.

<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 -->
M00nF1sh: would you help fix the lint..i'll clean up some vpcs in our test account to make e2e pass :D
tomfotherby: > would you help fix the lint..

Yes, I'll fix this. If I have understood it, I should have used `gofmt -w yourcode.go`. Leason learnt.

Patch:
diff --git a/internal/alb/ls/rules.go b/internal/alb/ls/rules.go
index f149b48f7..a66c1b026 100644
--- a/internal/alb/ls/rules.go
+++ b/internal/alb/ls/rules.go
@@ -128,7 +128,13 @@ func (c *rulesController) getDesiredRules(ctx context.Context, listener *elbv2.L
 			continue
 		}
 
+		seenUnconditionalRedirect := false
+
 		for _, path := range ingressRule.HTTP.Paths {
+			if seenUnconditionalRedirect {
+				// Ignore rules that follow a unconditional redirect, they are moot
+				continue
+			}
 			authCfg, err := c.authModule.NewConfig(ctx, ingress, path.Backend, aws.StringValue(listener.Protocol))
 			if err != nil {
 				return nil, err
@@ -146,6 +152,8 @@ func (c *rulesController) getDesiredRules(ctx context.Context, listener *elbv2.L
 			}
 			if createsRedirectLoop(listener, elbRule) {
 				continue
+			} else if isUnconditionalRedirect(listener, elbRule) {
+				seenUnconditionalRedirect = true
 			}
 			output = append(output, elbRule)
 			nextPriority++
@@ -529,3 +537,42 @@ func createsRedirectLoop(listener *elbv2.Listener, r elbv2.Rule) bool {
 	}
 	return false
 }
+
+// isUnconditionalRedirect checks whether specified rule always redirects
+// We consider the rule is a unconditional redirect if
+// 1) The Path condition is nil, or at least one Path condition is /*
+// 2) All other rule conditions are nil (ignoring the Host condition).
+// 3) RedirectConfig is not nil.
+func isUnconditionalRedirect(listener *elbv2.Listener, r elbv2.Rule) bool {
+	for _, action := range r.Actions {
+		rc := action.RedirectConfig
+		if rc == nil {
+			continue
+		}
+
+		var paths []string
+		for _, c := range r.Conditions {
+			switch aws.StringValue(c.Field) {
+			case conditions.FieldPathPattern:
+				paths = append(paths, aws.StringValueSlice(c.PathPatternConfig.Values)...)
+			case conditions.FieldHTTPRequestMethod, conditions.FieldSourceIP, conditions.FieldHTTPHeader, conditions.FieldQueryString:
+				// If there are any conditions, then the redirect is not unconditional
+				return false
+			}
+		}
+
+		if len(paths) != 0 {
+			// ALB path conditions are ORed, so if any of them are a wildcard, the redirect is unconditional
+			for _, path := range paths {
+				if path == "/*" {
+					return true
+				}
+			}
+			// The redirect isn't unconditional if none of the path conditions are a wildcard
+			return false
+		}
+
+		return true
+	}
+	return false
+}


Test patch:
diff --git a/internal/alb/ls/rules_test.go b/internal/alb/ls/rules_test.go
index 65bebfd5c..b2f564793 100644
--- a/internal/alb/ls/rules_test.go
+++ b/internal/alb/ls/rules_test.go
@@ -2878,6 +2878,200 @@ func Test_createsRedirectLoop(t *testing.T) {
 	}
 }
 
+func Test_isUnconditionalRedirect(t *testing.T) {
+	for _, tc := range []struct {
+		name     string
+		listener elbv2.Listener
+		rule     elbv2.Rule
+
+		expected bool
+	}{
+		{
+			name:     "No RedirectConfig",
+			listener: elbv2.Listener{Protocol: aws.String("HTTP"), Port: aws.Int64(80)},
+			rule:     elbv2.Rule{},
+			expected: false,
+		},
+		{
+			name:     "No conditions",
+			listener: elbv2.Listener{Protocol: aws.String("HTTP"), Port: aws.Int64(80)},
+			rule: elbv2.Rule{
+				Actions: []*elbv2.Action{
+					{
+						Type:           aws.String(elbv2.ActionTypeEnumRedirect),
+						RedirectConfig: redirectActionConfig(&elbv2.RedirectActionConfig{Path: aws.String("/#{path}")}),
+					},
+				},
+				Conditions: nil,
+			},
+			expected: true,
+		},
+		{
+			name:     "No Path conditions",
+			listener: elbv2.Listener{Protocol: aws.String("HTTP"), Port: aws.Int64(80)},
+			rule: elbv2.Rule{
+				Actions: []*elbv2.Action{
+					{
+						Type:           aws.String(elbv2.ActionTypeEnumRedirect),
+						RedirectConfig: redirectActionConfig(&elbv2.RedirectActionConfig{Path: aws.String("/#{path}")}),
+					},
+				},
+				Conditions: []*elbv2.RuleCondition{
+					{
+						Field: aws.String(conditions.FieldHostHeader),
+						HostHeaderConfig: &elbv2.HostHeaderConditionConfig{
+							Values: aws.StringSlice([]string{"www.example.com", "anno.example.com"}),
+						},
+					},
+				},
+			},
+			expected: true,
+		},
+		{
+			name:     "Path condition set to /*",
+			listener: elbv2.Listener{Protocol: aws.String("HTTP"), Port: aws.Int64(80)},
+			rule: elbv2.Rule{
+				Actions: []*elbv2.Action{
+					{
+						Type:           aws.String(elbv2.ActionTypeEnumRedirect),
+						RedirectConfig: redirectActionConfig(&elbv2.RedirectActionConfig{Path: aws.String("/#{path}")}),
+					},
+				},
+				Conditions: []*elbv2.RuleCondition{
+					{
+						Field: aws.String(conditions.FieldPathPattern),
+						PathPatternConfig: &elbv2.PathPatternConditionConfig{
+							Values: aws.StringSlice([]string{"/*"}),
+						},
+					},
+				},
+			},
+			expected: true,
+		},
+		{
+			name:     "Multiple Path conditions, one of which is /*",
+			listener: elbv2.Listener{Protocol: aws.String("HTTP"), Port: aws.Int64(80)},
+			rule: elbv2.Rule{
+				Actions: []*elbv2.Action{
+					{
+						Type:           aws.String(elbv2.ActionTypeEnumRedirect),
+						RedirectConfig: redirectActionConfig(&elbv2.RedirectActionConfig{Path: aws.String("/#{path}")}),
+					},
+				},
+				Conditions: []*elbv2.RuleCondition{
+					{
+						Field: aws.String(conditions.FieldPathPattern),
+						PathPatternConfig: &elbv2.PathPatternConditionConfig{
+							Values: aws.StringSlice([]string{"/*", "/test", "/annothertest"}),
+						},
+					},
+				},
+			},
+			expected: true,
+		},
+		{
+			name:     "Multiple Path conditions, one of which is /*, different ordering",
+			listener: elbv2.Listener{Protocol: aws.String("HTTP"), Port: aws.Int64(80)},
+			rule: elbv2.Rule{
+				Actions: []*elbv2.Action{
+					{
+						Type:           aws.String(elbv2.ActionTypeEnumRedirect),
+						RedirectConfig: redirectActionConfig(&elbv2.RedirectActionConfig{Path: aws.String("/#{path}")}),
+					},
+				},
+				Conditions: []*elbv2.RuleCondition{
+					{
+						Field: aws.String(conditions.FieldPathPattern),
+						PathPatternConfig: &elbv2.PathPatternConditionConfig{
+							Values: aws.StringSlice([]string{"/test", "/anothertest", "/*"}),
+						},
+					},
+				},
+			},
+			expected: true,
+		},
+		{
+			name:     "Multiple Path conditions, none of which is /*",
+			listener: elbv2.Listener{Protocol: aws.String("HTTP"), Port: aws.Int64(80)},
+			rule: elbv2.Rule{
+				Actions: []*elbv2.Action{
+					{
+						Type:           aws.String(elbv2.ActionTypeEnumRedirect),
+						RedirectConfig: redirectActionConfig(&elbv2.RedirectActionConfig{Path: aws.String("/#{path}")}),
+					},
+				},
+				Conditions: []*elbv2.RuleCondition{
+					{
+						Field: aws.String(conditions.FieldPathPattern),
+						PathPatternConfig: &elbv2.PathPatternConditionConfig{
+							Values: aws.StringSlice([]string{"/test", "/anothertest", "anothertest2"}),
+						},
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			name:     "Path condition set to /* and Host condition is set ",
+			listener: elbv2.Listener{Protocol: aws.String("HTTP"), Port: aws.Int64(80)},
+			rule: elbv2.Rule{
+				Actions: []*elbv2.Action{
+					{
+						Type:           aws.String(elbv2.ActionTypeEnumRedirect),
+						RedirectConfig: redirectActionConfig(&elbv2.RedirectActionConfig{Path: aws.String("/#{path}")}),
+					},
+				},
+				Conditions: []*elbv2.RuleCondition{
+					{
+						Field: aws.String(conditions.FieldPathPattern),
+						PathPatternConfig: &elbv2.PathPatternConditionConfig{
+							Values: aws.StringSlice([]string{"/*"}),
+						},
+					},
+					{
+						Field: aws.String(conditions.FieldHostHeader),
+						HostHeaderConfig: &elbv2.HostHeaderConditionConfig{
+							Values: aws.StringSlice([]string{"www.example.com", "anno.example.com"}),
+						},
+					},
+				},
+			},
+			expected: true,
+		},
+		{
+			name:     "Path condition set to /* but a SourceIP condition is also set",
+			listener: elbv2.Listener{Protocol: aws.String("HTTP"), Port: aws.Int64(80)},
+			rule: elbv2.Rule{
+				Actions: []*elbv2.Action{
+					{
+						Type:           aws.String(elbv2.ActionTypeEnumRedirect),
+						RedirectConfig: redirectActionConfig(&elbv2.RedirectActionConfig{Path: aws.String("/#{path}")}),
+					},
+				},
+				Conditions: []*elbv2.RuleCondition{
+					{
+						Field: aws.String(conditions.FieldPathPattern),
+						PathPatternConfig: &elbv2.PathPatternConditionConfig{
+							Values: aws.StringSlice([]string{"/*"}),
+						},
+					},
+					{
+						Field: aws.String(conditions.FieldSourceIP),
+						SourceIpConfig: &elbv2.SourceIpConditionConfig{
+							Values: aws.StringSlice([]string{"192.168.0.0/16"}),
+						},
+					},
+				},
+			},
+			expected: false,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			assert.Equal(t, tc.expected, isUnconditionalRedirect(&tc.listener, tc.rule))
+		})
+	}
+}
+
 func redirectActionConfig(override *elbv2.RedirectActionConfig) *elbv2.RedirectActionConfig {
 	r := &elbv2.RedirectActionConfig{
 		Host:       aws.String("#{host}"),