Detect unconditional redirects and ignore any rules defined afterwards

[tomfotherby]

(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:

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 #853 (comment)

[k8s-ci-robot]

Welcome @tomfotherby!

It looks like this is your first PR to kubernetes-sigs/aws-alb-ingress-controller 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

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.

You can also check if kubernetes-sigs/aws-alb-ingress-controller has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @tomfotherby. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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 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.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[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:

[M00nF1sh]

/ok-to-test

[M00nF1sh]

overall looks good to me, and should be able to handle cases like

host: a.example.com
   - path: /*
      backend: redirect-to-https
host: b.example.com
   - path: /*
      backend: redirect-to-https

Thanks for making this change 🤣

[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 correctly, I should have used gofmt -w yourcode.go. Leason learnt.

[tomfotherby]

I pushed a fix which I hope will pass the linter because I used gofmt -w myfiles.go (source) 🤞 . I also added more unit tests and refactored given the suggestion in #1162 (comment).

[M00nF1sh]

thanks 🤣
/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: M00nF1sh, tomfotherby

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [M00nF1sh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[tdmalone]

Thanks so much for this @tomfotherby and for merging it @M00nF1sh. We're really keen to use this and just wondering if there is a target date for the next release containing it? Thanks!

[AndrewBoklashko]

Hi @tomfotherby @M00nF1sh

I just tried v1.1.6 with this fix. Is this intended to work across multiple hosts? For example:

spec:
  rules:
    - http:
        paths:
          - path: /*
            backend:
              serviceName: ssl-redirect
              servicePort: use-annotation
    - host: hostA
      http:
        paths:
          - path: /pathA/*
            backend:
              serviceName: serviceA
              servicePort: 80
    - host: hostB
      http:
        paths:      
          - path: /*
            backend:
              serviceName: serviceB
              servicePort: 80

I believe that if the redirect rule does not have host specified, it should be considered as unconditional redirect as well. So in this case hostA and hostB rules should be skipped.

[tomfotherby]

@AndrewBoklashko - Good point. I agree with you. Alas, currently the controller processes each rule in turn and the short circuit mechanism that avoids unneeded rules only operates in the scope of the individual rule.

In my understanding, but please correct me if I'm wrong, the bug you found isn't causing active harm, i.e. it's not causing needed rules to go missing, but it's not operating as well as it could (i.e. it's not doing what it says on the tin). Is that correct?

---

I appologise for this mistake. I hadn't considered "hostless rules" even though I see the example does not have a host specified. Thanks for bringing it up - I will work on a patch because we would also benefit from this. I can't promise speed though, I think it will be a level-up in complexity from the current solution and there's rather a lot going on presently...

[AndrewBoklashko]

In my understanding, but please correct me if I'm wrong, the bug you found isn't causing active harm, i.e. it's not causing needed rules to go missing, but it's not operating as well as it could (i.e. it's not doing what it says on the tin). Is that correct?

That is correct.

I appologise for this mistake. I hadn't considered "hostless rules" even though I see the example does not have a host specified. Thanks for bringing it up - I will work on a patch because we would also benefit from this. I can't promise speed though, I think it will be a level-up in complexity from the current solution and there's rather a lot going on presently...

All good, thanks for the follow up. Will be looking to try a patch, but no rush - we can live with how it works now too.

[tdmalone]

hi @tomfotherby, @AndrewBoklashko, I just thought I'd follow up and see if either you got any further on the above ^^

Is this being tracked under #853? If not, would you like me to create a new issue for it?

[tomfotherby]

@tdmalone - At my company we don't currently use "hostless rules" in our ALB ingress spec. So to set expectations, I'm not going to be working on an enhancement for the foreseeable future. I would find it hard to test things without a real use-case in my aws account. My PR unfortunately had a bug (raised in #1274 and fixed in #1286) which revealed to me the sharp edges and how important it would be to test well. With any luck, other folk will get involved.

Is this being tracked under #853? If not, would you like me to create a new issue for it?

I would say #853 tracks it.

[tdmalone]

No worries @tomfotherby, thanks for the reply. I might see if I can refactor to hostless rules and/or try to dig into the code here to see if I can make something work (not a Go programmer, but I could take a stab!)

[gpolicante]

resolved to version 1.16 #1274