chore: clarify how to get internal NLB #2356

froblesmartin · 2021-11-11T08:20:22Z

Issue

Description

Adding clarifications about the current annotation "service.beta.kubernetes.io/aws-load-balancer-type: external" as it sounds confusing and does not show that an "internal" NLB can be achieved using that type.

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

Refactored something and made the world a better place 🌟

Adding clarifications about the current annotation "service.beta.kubernetes.io/aws-load-balancer-type: external" as it sounds confusing and does not show that an "internal" NLB can be achieved using that type.

k8s-ci-robot · 2021-11-11T08:20:30Z

Welcome @froblesmartin!

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 · 2021-11-11T08:20:30Z

Hi @froblesmartin. 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.

M00nF1sh · 2021-11-12T00:38:03Z

docs/guide/service/annotations.md

-        - For `nlb-ip` type, controller will provision NLB with IP targets. This value is supported for backwards compatibility
-        - For `external` type, NLB target type depend on the annotation [nlb-target-type](#nlb-target-type)
+        - For `nlb-ip` type, controller will provision NLB with IP targets. This value is supported for backwards compatibility, but it is deprecated.
+        - For `external` type, NLB target type depend on the annotation [nlb-target-type](#nlb-target-type). If used in combination with [lb-scheme](#lb-scheme) set to internal, it will provision an internal NLB.


Thanks for contributing this.
I think maybe - [Deprecated] `nlb-ip` type, controller will provision NLB with IP targets. This value is supported for backwards compatibility

BTW, it feels a weird to mention lb-scheme here as it have it's own section. and internal NLB is the default mode if lb-scheme is not specified after all.

1st thing 👍

About the lb-scheme, I just wanted to highlight that "external" allows to use "internal" LBs. That's what happened to me, I did not try to use the external type because I thought that would only provision an internet-facing LB 😅

Maybe if not a note in the lb-scheme clarifying that it can be used with the "external" type?

@froblesmartin
I understand your confusing now. it's a valid point. I think maybe add a separate tip section like

!!!tip This annotation specifies the controller used to provision LoadBalancers. Refer to [lb-scheme](#lb-scheme) to specify whether the LoadBalancer is public-facing.

btw, this annotation itself will be deprecated soon as well, we'll use new spec.ServiceClass in the future instead of service.beta.kubernetes.io/aws-load-balancer-type

Good to know! 😄 I have changed it to the tip, and as well reduced the last part just to clarify that this controller can provision any NLB+Target Group configuration.

codecov-commenter · 2021-11-23T23:20:07Z

Codecov Report

Merging #2356 (4ed06a3) into main (c60d5e6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2356   +/-   ##
=======================================
  Coverage   54.19%   54.19%           
=======================================
  Files         136      136           
  Lines        7702     7702           
=======================================
  Hits         4174     4174           
  Misses       3212     3212           
  Partials      316      316

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c60d5e6...4ed06a3. Read the comment docs.

docs/guide/service/annotations.md

Co-authored-by: Steven E. Harris <[email protected]>

froblesmartin · 2021-11-30T08:44:47Z

@M00nF1sh what would be missing to approve and merge this? Should I do something else? Thank you!

kishorj · 2021-11-30T16:52:22Z

docs/guide/service/annotations.md

+
+!!!note ""
+    With the right combination of annotations any Network Load Balancer and Target Group configuration can be provisioned with this controller.


This note doesn't provide additional information. Lets remove it.

The AWS Load Balancer Controller manages Kubernetes Services in a compatible way with the legacy aws cloud provider. The annotation service.beta.kubernetes.io/aws-load-balancer-type is used to determine which controller reconciles the service. If the annotation value is nlb-ip or external, legacy cloud provider ignores the service resource (provided it has the correct patch) so that the AWS Load Balancer controller can take over. For all other values of the annotation, the legacy cloud provider will handle the service. Note that this annotation should be specified during service creation and not edited later.

As with the rest of the changes, right after reading that paragraph, I assumed that the new controller can not manage an internal load balancer with instance target type. If you are not okay adding that note, maybe I could rewrite that paragraph to make that clearer?

sounds better

docs/guide/service/annotations.md

kishorj · 2021-12-03T22:10:26Z

@froblesmartin, I will remove the note from the end of this document and commit the rest of the changes. Once we publish the new live docs, feel free to raise a separate PR for additional changes.

Co-authored-by: Steven E. Harris <[email protected]>

k8s-ci-robot · 2021-12-03T22:15:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: froblesmartin, kishorj

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~~ [kishorj]

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

* chore: specify how to get classic nlb type Adding clarifications about the current annotation "service.beta.kubernetes.io/aws-load-balancer-type: external" as it sounds confusing and does not show that an "internal" NLB can be achieved using that type. * Move deprecation note to line start * add tip about provisioning internal lb * fix: only nlb can be provisioned * chore: change to Internet-facing Co-authored-by: Steven E. Harris <[email protected]> * Update docs/guide/service/annotations.md * Update docs/guide/service/annotations.md * Update docs/guide/service/annotations.md Co-authored-by: Steven E. Harris <[email protected]> Co-authored-by: Steven E. Harris <[email protected]> Co-authored-by: Kishor Joshi <[email protected]>

chore: specify how to get classic nlb type

f66e0a9

Adding clarifications about the current annotation "service.beta.kubernetes.io/aws-load-balancer-type: external" as it sounds confusing and does not show that an "internal" NLB can be achieved using that type.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 11, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 11, 2021

k8s-ci-robot requested review from kishorj and M00nF1sh November 11, 2021 08:20

froblesmartin mentioned this pull request Nov 11, 2021

Force this controller to manage "nlb" type or any way to use instance Target Groups #2350

Closed

M00nF1sh reviewed Nov 12, 2021

View reviewed changes

froblesmartin added 3 commits November 12, 2021 10:19

Move deprecation note to line start

d3b8379

add tip about provisioning internal lb

142b3fd

fix: only nlb can be provisioned

4ed06a3

froblesmartin requested a review from M00nF1sh November 18, 2021 10:36

froblesmartin changed the title ~~chore: specify how to get classic nlb type~~ chore: clarify how to get internal NLB Nov 18, 2021

kishorj added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 23, 2021

seh reviewed Nov 26, 2021

View reviewed changes

docs/guide/service/annotations.md Outdated Show resolved Hide resolved

docs/guide/service/annotations.md Outdated Show resolved Hide resolved

chore: change to Internet-facing

b7e6fd2

Co-authored-by: Steven E. Harris <[email protected]>

kishorj reviewed Nov 30, 2021

View reviewed changes

froblesmartin requested a review from kishorj November 30, 2021 18:27

kishorj requested changes Dec 3, 2021

View reviewed changes

docs/guide/service/annotations.md Outdated Show resolved Hide resolved

docs/guide/service/annotations.md Outdated Show resolved Hide resolved

kishorj added 2 commits December 3, 2021 14:10

Update docs/guide/service/annotations.md

404e84d

Update docs/guide/service/annotations.md

4451034

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 3, 2021

Update docs/guide/service/annotations.md

d302074

Co-authored-by: Steven E. Harris <[email protected]>

kishorj self-requested a review December 3, 2021 22:14

kishorj approved these changes Dec 3, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2021

kishorj merged commit f736deb into kubernetes-sigs:main Dec 3, 2021

oliviassss mentioned this pull request Dec 16, 2021

🥳 aws-load-balancer-controller v2.3.1 Automated Release! 🥑 oliviassss/eks-charts#7

Closed

oliviassss mentioned this pull request Feb 9, 2022

🥳 aws-load-balancer-controller v2.3.1 Automated Release! 🥑 oliviassss/eks-charts#18

Closed


		!!!note ""
		With the right combination of annotations any Network Load Balancer and Target Group configuration can be provisioned with this controller.

chore: clarify how to get internal NLB #2356

chore: clarify how to get internal NLB #2356

Uh oh!

Conversation

froblesmartin commented Nov 11, 2021

Issue

Description

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

Uh oh!

k8s-ci-robot commented Nov 11, 2021

Uh oh!

k8s-ci-robot commented Nov 11, 2021

Uh oh!

M00nF1sh Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

froblesmartin Nov 12, 2021

Choose a reason for hiding this comment

Uh oh!

M00nF1sh Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

M00nF1sh Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

froblesmartin Nov 17, 2021

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 23, 2021

Codecov Report

Uh oh!

Uh oh!

Uh oh!

froblesmartin commented Nov 30, 2021

Uh oh!

kishorj Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

froblesmartin Nov 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kishorj Nov 30, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kishorj commented Dec 3, 2021

Uh oh!

k8s-ci-robot commented Dec 3, 2021

Uh oh!

Uh oh!

froblesmartin Nov 30, 2021 •

edited

Loading