Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for NLBs targeting ALBs #2379

Closed
wants to merge 11 commits into from
Closed

Conversation

jon-fearer
Copy link

@jon-fearer jon-fearer commented Nov 22, 2021

Issue

Support for NLBs targeting ALBs

Description

The goal of this pull request is to add support for targeting ALBs from NLBs by adding the "alb" value to the existing nlb target type annotation. It is currently a work in progress. If possible, it would be great to hear from the maintainers whether this is a feature that you are willing to support in this project. And if so, do you have any thoughts on what the service spec should look like for targeting an ALB? (see below). Thank you in advance!

Example:

# Application Load Balancer Ingress
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: echoserver-alb <-- The ingress can be selected by name from the nlb service using the "ingress" selector
  namespace: echoserver
  annotations:
    kubernetes.io/ingress.class: alb
    alb.ingress.kubernetes.io/scheme: internet-facing
    alb.ingress.kubernetes.io/tags: ...
    alb.ingress.kubernetes.io/subnets: ...
spec:
  defaultBackend:
    service:
      name: echoserver
      port:
        number: 80

# Network Load Balancer Service
apiVersion: v1
kind: Service
metadata:
  name: echoserver-nlb
  namespace: echoserver
  annotations:
    service.beta.kubernetes.io/aws-load-balancer-scheme: internet-facing
    service.beta.kubernetes.io/aws-load-balancer-type: "external"
    service.beta.kubernetes.io/aws-load-balancer-nlb-target-type: "alb" <-- New alb option
spec:
  # Not sure how everything below should look
  ports:
    - port: 80
      targetPort: 8080
      protocol: TCP
  type: LoadBalancer
  selector:
    ingress: echoserver-alb <-- Select the alb using "ingress" selector

Also, would it make sense to extend the TargetGroupBinding CRD to support something like an IngressRef for this scenario (since we are targeting an Ingress instead of a Service)?

Example:

apiVersion: elbv2.k8s.aws/v1beta1
kind: TargetGroupBinding
metadata:
  name: my-tgb
spec:
  ingressRef: <-- new option for ALB target type(?)
    name: awesome-ingress # route traffic to the awesome-ingress ALB
    port: 80
  targetGroupARN: <arn-to-targetGroup>

Checklist

  • Added tests that cover your change (if possible) IN PROGRESS
  • Added/modified documentation as required (such as the README.md, or the docs directory) TODO
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 22, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @jon-fearer!

It looks like this is your first PR to kubernetes-sigs/aws-load-balancer-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-load-balancer-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
Copy link
Contributor

Hi @jon-fearer. 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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 22, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jon-fearer
To complete the pull request process, please assign m00nf1sh after the PR has been reviewed.
You can assign the PR to them by writing /assign @m00nf1sh in a comment when ready.

The full list of commands accepted by this bot can be found 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

@pjaudiomv
Copy link

This would be amazing, def willing to test.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 24, 2021

CLA Signed

The committers are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 24, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 30, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 1, 2021
@jon-fearer jon-fearer marked this pull request as ready for review December 6, 2021 22:55
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 6, 2021
@jon-fearer
Copy link
Author

I've implemented the spec changes outlined above and tested manually. I'll hold off on any other changes for now

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 9, 2022
@pjaudiomv
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 9, 2022
@christianlafleur
Copy link

@jon-fearer any active work on thir pr ? This will be amazing.

@pjaudiomv
Copy link

@jon-fearer any active work on thir pr ? This will be amazing.

The lack of any maintainer feedback has stalled his work.

@erickfaustino
Copy link

Hi, folks!

This feature would be a life saver for us!

Can anyone unblock this, please?

Thank you very much!

@liad5h
Copy link

liad5h commented Sep 21, 2022

Would definitely use this.

@smcavallo
Copy link

@kishorj @M00nF1sh - any update? We have been waiting for this functionality. It is a common use case.
Another common use case is creating and managing both the NLB and associated backend ALB and targetgroup with this controller

@M00nF1sh
Copy link
Collaborator

M00nF1sh commented Jan 4, 2023

@jon-fearer
@smcavallo
For your use case, would there be multiple ALBs behind NLB or just a single ALB behind a NLB?

Personally i don't favor current approach as the Kubernetes Service object is designed to target pods only. Using Service objects to model AWS NLB in this scenario is not appropriate to me.

Possible alternatives in my mind:

  1. use Gateway API(https://gateway-api.sigs.k8s.io/references/spec/#gateway.networking.k8s.io/v1alpha2.TCPRouteRule) and backendRefs to reference ALBs. We have plans to support gateway APIs in upcoming major releases. (This feels better to me)
  2. use annotation on Ingress or field on IngressClass to specify a unique identifier representing a NLB(similar how ingressGroup's name works), and for each such identifier create a NLB and attach these NLBs to it.

any thoughts? @kishorj

@smcavallo
Copy link

smcavallo commented Jan 4, 2023

@M00nF1sh - this is our use case:
We accept both tcp and http traffic from customers.
We have a public facing NLB at the perimeter managed by aws-load-balancer-controller which accepts both tcp and http traffic

  • tcp traffic is routed to kubernetes ingress/pods just like any other use case of aws-load-balancer-controller
  • http traffic is routed to ALB target group (below)

we have an internal facing ALB managed by aws-load-balancer-controller with WAF and TLS/SNI for http traffic

  • http traffic is routed to kubernetes ingress/pods just like any other use case of aws-load-balancer-controller
    (we can't offload tls/sni to something inside the cluster - we need WAF, aws certificates, and other aws specific integrations and ALB functionality.)

Everything is out of the box aws-load-balancer-controller and working as it's designed to be used.
The only thing we can't do is wire up the ALB (managed by aws-load-balancer-controller) to sit behind the NLB (managed by the aws-load-balancer-controller)
Since aws-load-balancer-controller is managing BOTH the NLB and ALB it seems like it should be able to connect the 2.

The alternative workaround is for us to manage both the NLB and ALB via infrastructure-as-code (crossplane!) and then use the TargetGroup functionality of aws-load-balancer-controller to make sure the nodes/pods exist as part of the target groups.

It feels like overkill to use to manage the entire config some other way when aws-load-balancer-controller can create and maintain everything, it just can't connect the infrastructure it created.

image
https://aws.amazon.com/blogs/networking-and-content-delivery/application-load-balancer-type-target-group-for-network-load-balancer/
All the http targets and tcp targets exist within kubernetes. we just have to route the http traffic through an ALB.

@sftim
Copy link
Contributor

sftim commented Jan 4, 2023

We could make a very narrow implementation of the Gateway API that deploys an NLB and ALB (and, if we want, integrates with PrivateLink).

In Kubernetes, the Service API doesn't express a desired state that looks like that diagram @jon-fearer. That is why we shouldn't use Service to manage it.

If someone wants to add complexity to the AWS Load Balancer controller to add Gateway, they can - it's open source. However, that is a big ask.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 4, 2023
@matthewdaltonfanduel
Copy link

@M00nF1sh

I am wondering if you can expand upon why you find it inappropriate to model the nlb with the service object?

I am encountering the use case needing to inject an ALB for http to https redirection at the nlb. Load Balancers managed by aws-load-balancer-controller, this becomes quite the difficult ask. As @smcavallo pointed out earlier we could move this logic into IaC and manage with target groups but I find this to be a less desirable option as we prefer to manage the LB's with the controller rather than with IaC.

I think this functionality also opens additional feature functionality that does align with this controller and how it functions.
This feature would also allow implementations of a service defining an nlb targetting an ingress defining an alb. This functionality would be able to model any complex networking use-cases that load-balancers support.

I think this change can only be positive. I think that the aws-load-balancer-controller should be able to model and implement any of the features and capacities of load balancers in aws. Allowing the controller to be agnostic of implementation details allows it to remain in feature parity and be in lock step with additional aws features released down the road.

@matthewdaltonfanduel
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 4, 2023
@M00nF1sh
Copy link
Collaborator

@matthewdaltonfanduel
The main reason is Kubernetes Service object is designed to target pods instead of targeting other objects like ALB. We are just abusing Kubernetes Service spec if we implement this way.
Personally i still prefer we implement this by the Gateway API, https://gateway-api.sigs.k8s.io/, which is more flexible.

There is a on-going Gateway API implementation from community folks: #3146, we can consider implement this use case with that.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 12, 2023
@pjaudiomv
Copy link

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 12, 2023
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 20, 2024
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 19, 2024
@jon-fearer
Copy link
Author

Looks like @M00nF1sh has suggested a better solution here, so I'm going to close this. Thanks all

@jon-fearer jon-fearer closed this Mar 7, 2024
@scila1996
Copy link

@matthewdaltonfanduel The main reason is Kubernetes Service object is designed to target pods instead of targeting other objects like ALB. We are just abusing Kubernetes Service spec if we implement this way. Personally i still prefer we implement this by the Gateway API, https://gateway-api.sigs.k8s.io/, which is more flexible.

There is a on-going Gateway API implementation from community folks: #3146, we can consider implement this use case with that.

Just create a normal ingress with the class: alb. However, we should add a feature to support an annotation called 'static IP.' The ALB will automatically create an additional NLB and target the ALB. We should not create an additional service of type LoadBalancer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.