Extra parameter to force usage of only one ALB per cluster

[marcosdiez]

This PR makes the server support one extra command line parameter: --force-alb-name=XXXXXX

When this is used, the server will always use this ALB all the time, no matter what.
That means it will create it if necessary and it will use the existing one otherwise. If one combines with the annotation to keep the ALB, then it will just live forever.

Also, it only cares about the rules it needs to create and to modify in the ALB.
That means one can create other rules manually and as long as they don't conflict with your kubernetes annotations, they will be untouched.

Currently it has two known issues:

• it will probably break if one adds more 50 rules in a ALB ( aws alb limitation )
• it will NOT delete any rule, even if it created in the past and the rule became unnecessary/obsolete.

I do plan to address the above issue in the future, but I am pretty sure even with them this is a step forward.

One can try it using the following docker image: docker.io/marcosdiez/aws-alb-ingress-controller:v1.1.0-single-alb

Fixes #724 and #688 and #298 and #228

Extra explanation on how to use it:

kubectl apply -f https://raw.githubusercontent.com/kubernetes-sigs/aws-alb-ingress-controller/v1.1.0/docs/examples/rbac-role.yaml

Download https://raw.githubusercontent.com/kubernetes-sigs/aws-alb-ingress-controller/v1.1.0/docs/examples/alb-ingress-controller.yaml

• change the image to
  image: docker.io/marcosdiez/aws-alb-ingress-controller:v1.1.0-single-alb

• on the args section, add a new line with the load balancer name. something like - --force-alb-name=k8s-alb

• follow the rest of the instructions here: https://kubernetes-sigs.github.io/aws-alb-ingress-controller/guide/walkthrough/echoserver/

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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. I understand the commands that are listed here.

[k8s-ci-robot]

Hi @marcosdiez. Thanks for your PR.

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

[marcosdiez]

Just for the record, I just signed the CLA

[christopherhein]

@marcosdiez - this is very cool.

it will NOT delete any rule, even if it created in the past and the rule became unnecessary/obsolete.

Does this mean if there a conflict the new rule will take precedence or does the old one?

[M00nF1sh]

@marcosdiez
Thanks so much for contributing 👍 .

However, i'm a little opposed to this approach given it's limitations.(nice work though 😄 )
Actually, this will cause more troubles, like when you have listeners with different listen ports.

BTW, i'm currently working on a different solution called ingress group to solve this problem based a different approach.

The basic idea is add another annotation called ingressGroup, and ingress belongs to same group are merged and reconciled together(result in single alb)
This approach give you the flexibility on how to merge(group) ingresses. e.g.

• you can set default ingressGroup at controller level, to use single ALB for all ingress cluster-wise.(the same use case as your --force-alb-name solution 😄 )
• you can set default ingressGroup at namespace level, to use single ALB for all ingress within namespace.
• you can set same ingressGroup for all ingresses of your micro-service application across namespaces, to use single ALB.
• you can also use old way one ALB per ingress.

Based on this, we can have admission controllers to controller "who" can create ingress to join "which" ingressGroup for k8s envrionments that are shared.

Also, this helps solve the pain-point of path / port based configurations. e.g.

• different ingress rule for different port on same alb.
• different auth setting for different path.(cannot achieved for now if your service have multiple ports)

[marcosdiez]

@ChristopherHeim my code adds new rules and modifies existing ones, no matter if it created it or not (it does not track which rules it created). So conflicting rules will be overwritten.

[marcosdiez]

@M00nF1sh your proposal is clearly better then mine. It's much more flexible without any added downsides. It's properly designed and now that I know about it, I also believe merging this PR is not a good idea.

The problem is that your ETA is middle march and I can't wait for it.
So I suggest we leave this PR open until yours is ready, so people who can't wait can have an alternative.

Porting from one container to the other is trivial. Just adding the annotations you described and removing the --force-alb-name parameter.

The only issue with this approach is that it has the same disadvantages as mine:

• it's not clear what happens after the 50th ALB rule
• it's not clear in your description if it deletes rules it has created and are not needed anymore

Both are solvable. Do you have any plans on them ?

By the way, nice design. I couldn't do better.

[M00nF1sh]

@marcosdiez
Thanks a lot for the comments 🤣 .
I can speed up my implementation and deliver a draft version before that(without unit test cases, which is the most time consuming part =.=)
BTW,

1. the rules limit should be able to be increased via aws support ticket. Also, if users can manually split ingress into groups to sastify the default rule limit. (smarter variants exists like auto-split into groups, but i don't think users will want them since how the split happens is un-predictable from user's pov).
2. Yes, the rules that are not needed will be automatically deleted.(the code base are been refactored to support it 😄 )

[marcosdiez]

Small status update: I just found a bug that was causing issues with two ingress rules with the same name on different namespaces. It has been fixed. The container image has been updated as well with the same name. The old broken on has been renamed to broken.

This is clearly not a best practice but assigning versions to a fork that has it's days counted does not worth the trouble.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marcosdiez

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:

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

[marcosdiez]

Due to reasons beyond my control, I had to withdraw my pull request. If they get solved, I'll put it back when possible. Sorry.

[oba11]

Was looking forward to this PR being merged 😢 . @marcosdiez please can I ask why the pr was withdrawn even after it has been approved?

[tonidas]

What happened @marcosdiez ? It was already approved :(

Do you authorize anyone to use your PR to merge it?

[Jinkxed]

Do either of these solutions solve the problem of if you have target groups that have different health endpoints?

Right now the healthcheck path is in the annotation, what would take this a HUGE step forward is to move it to the backend parameter. So we could do something like:

spec:
  rules:
  - http:
      paths:
      - backend:
          serviceName: redirect
          servicePort: use-annotation
        path: /*
  - host: myhost.example.com
    http:
      paths:
      - path: /*
        backend:
           serviceName: "some-web"
           servicePort: 80
           healthcheck: /health

With this I could specify multiple hosts using different target groups. Is this something you are accounting for @M00nF1sh ?

[M00nF1sh]

@sc-chad ,
Currently you can already add the healthcheck related annotations on your service objects(which can achieve your seetings), which only affects settings for corresponding backend(and override settings on ingress objects)

[Jinkxed]

@M00nF1sh do you have an example of this? Would love to be able to do that.

[Jinkxed]

Nevermind, figured out what you were saying. In case anyone else comes across this. You need to add:

  annotations:
    alb.ingress.kubernetes.io/healthcheck-path: /

To your service that you want to attach to the ALB. Thanks @M00nF1sh for the awesome module!

[frederiksf]

When will this feature be documented and generally available?

[yanivpaz]

@sc-chad it will be great if you can document it here :-)

[Jinkxed]

I'll see what I can do!

[Jinkxed]

#861

[benderillo]

@M00nF1sh Where can we track the work you mentioned for the IngressGroups? What is general status and ETA?

[M00nF1sh]

@benderillo
I'm going through a internal design review today, and will release an working alpha version soon.

[rverma-nikiai]

@M00nF1sh this is great news. Awaiting eagerly.

[tdmalone]

Just in case anyone didn't know - the Ingress Groups work is being discussed at #914