add docs for NLB TLS termination #2680

geoffcline · 2022-06-08T03:51:19Z

Issue

none.

Description

add docs for NLB TLS termination

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the docs directory)
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 · 2022-06-08T03:51:27Z

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

codecov-commenter · 2022-06-08T03:57:49Z

Codecov Report

Merging #2680 (ad9af82) into main (9c79e45) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #2680   +/-   ##
=======================================
  Coverage   53.99%   53.99%           
=======================================
  Files         144      144           
  Lines        8214     8214           
=======================================
  Hits         4435     4435           
  Misses       3461     3461           
  Partials      318      318

Impacted Files	Coverage Δ
pkg/targetgroupbinding/networking_manager.go	`34.54% <0.00%> (ø)`

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 9c79e45...ad9af82. Read the comment docs.

geoffcline · 2022-06-13T02:09:42Z

@M00nF1sh can I get a review for this addition to the docs?

geoffcline · 2022-06-25T06:05:08Z

/ok-to-test

kishorj

For fig.jpg, the "Source IP Address is Maintained" note is not always true. It depends on the target group attributes from the pov of the NLB, and the spec.externalTrafficPolicy for the pod in case of instance target.

kishorj · 2022-07-08T19:33:17Z

docs/guide/use_cases/nlb_tls_termination.md

+  type: LoadBalancer
+```
+
+!!! warning


TLS configuration should not cause duplicate NLB resources. Did you encounter any issues modifying existing service resources?

Yes. I tried adding TLS termination to an existing NLB. It didn't show the cert in the NLB console until I deleted the NLB, and the controller recreated it.

kishorj · 2022-07-08T19:36:04Z

docs/guide/use_cases/nlb_tls_termination.md

+
+```
+NAME    TYPE           CLUSTER-IP      EXTERNAL-IP                                                                     PORT(S)        AGE
+envoy   LoadBalancer   10.100.24.154   a7ea2bbde8a164036a7e4c1ed5700cdf-154fb911d990bb1f.elb.us-east-2.amazonaws.com   443:31606/TCP   40d


By default, an NLB provisioned by this controller has the name format k8s-<namespace>-<name>-<hash>. Looking at the name it appears to be provisioned by kcm. Lets provide an example from a service provisioned by this controller to avoid confusion.

excellent point thank you

geoffcline · 2022-07-19T05:17:25Z

For fig.jpg, the "Source IP Address is Maintained" note is not always true. It depends on the target group attributes from the pov of the NLB, and the spec.externalTrafficPolicy for the pod in case of instance target.

Do you think I should remove this from the figure, or just mention these requirements?

geoffcline · 2022-07-20T04:16:21Z

revised, thanks again @kishorj !!

geoffcline · 2022-07-27T18:04:51Z

I did testing. This time updating the annotations worked. I think I misunderstood what annotations can/can't be updated after creation.

I revised and this should be good to go @kishorj :)

kishorj

I have some minor comments, looks good otherwise.

docs/guide/use_cases/nlb_tls_termination/index.md

geoffcline · 2022-07-29T18:56:02Z

squashed, rebased, reworded, and revised

kishorj · 2022-08-04T20:22:47Z

docs/guide/use_cases/nlb_tls_termination/index.md

+
+The NLB decrypts the request, and transmits it on to your cluster on port 80. It follows the standard request routing configured within the cluster. Notably, the request received within the cluster includes the actual origin IP address of the external client. 
+
+Alternate ports may be configured. End-to-end encryption technically requires the segment between the NLB and cluster pods be encrypted also. A follow-up post will describe the NLB originating TLS based on a cluster certificate. 


You can configure backend encryption via the annotation service.beta.kubernetes.io/aws-load-balancer-backend-protocol.

Lets also not mention a follow-up-post. We can add the new live doc page once we have it.

good point, I revised.

kishorj · 2022-08-04T20:47:45Z

/lgtm

k8s-ci-robot · 2022-08-05T17:12:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: geoffcline, 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

* add use case documentation for NLB TLS termination * fixup

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 8, 2022

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 Jun 8, 2022

k8s-ci-robot requested review from kishorj and M00nF1sh June 8, 2022 03:51

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 25, 2022

kishorj reviewed Jul 8, 2022

View reviewed changes

geoffcline mentioned this pull request Jul 20, 2022

add information on load balancer lifecycle #2703

Closed

6 tasks

kishorj added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Jul 29, 2022

kishorj reviewed Jul 29, 2022

View reviewed changes

docs/guide/use_cases/nlb_tls_termination/index.md Outdated Show resolved Hide resolved

add use case documentation for NLB TLS termination

979555a

geoffcline force-pushed the gdc-nlb-tls branch from d77f631 to 979555a Compare July 29, 2022 18:55

kishorj reviewed Aug 4, 2022

View reviewed changes

fixup

045fc3f

k8s-ci-robot assigned kishorj Aug 4, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 4, 2022

kishorj approved these changes Aug 5, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2022

k8s-ci-robot merged commit fb7d7e9 into kubernetes-sigs:main Aug 5, 2022

kishorj mentioned this pull request Aug 10, 2022

🥳 aws-load-balancer-controller v2.4.3 Automated Release! 🥑 aws/eks-charts#797

Merged

Timothy-Dougherty pushed a commit to adammw/aws-load-balancer-controller that referenced this pull request Nov 9, 2023

add docs for NLB TLS termination (kubernetes-sigs#2680)

9a8eb46

* add use case documentation for NLB TLS termination * fixup


		The NLB decrypts the request, and transmits it on to your cluster on port 80. It follows the standard request routing configured within the cluster. Notably, the request received within the cluster includes the actual origin IP address of the external client.

		Alternate ports may be configured. End-to-end encryption technically requires the segment between the NLB and cluster pods be encrypted also. A follow-up post will describe the NLB originating TLS based on a cluster certificate.

add docs for NLB TLS termination #2680

add docs for NLB TLS termination #2680

Uh oh!

Conversation

geoffcline commented Jun 8, 2022

Issue

Description

Checklist

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

Uh oh!

k8s-ci-robot commented Jun 8, 2022

Uh oh!

codecov-commenter commented Jun 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

geoffcline commented Jun 13, 2022

Uh oh!

geoffcline commented Jun 25, 2022

Uh oh!

kishorj left a comment

Choose a reason for hiding this comment

Uh oh!

kishorj Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

geoffcline Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

geoffcline Jul 20, 2022

Choose a reason for hiding this comment

Uh oh!

kishorj Jul 8, 2022

Choose a reason for hiding this comment

Uh oh!

geoffcline Jul 19, 2022

Choose a reason for hiding this comment

Uh oh!

geoffcline commented Jul 19, 2022

Uh oh!

geoffcline commented Jul 20, 2022

Uh oh!

geoffcline commented Jul 27, 2022

Uh oh!

kishorj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

geoffcline commented Jul 29, 2022

Uh oh!

kishorj Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

geoffcline Aug 4, 2022

Choose a reason for hiding this comment

Uh oh!

kishorj commented Aug 4, 2022

Uh oh!

k8s-ci-robot commented Aug 5, 2022

Uh oh!

Uh oh!

codecov-commenter commented Jun 8, 2022 •

edited

Loading