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

add TLS support for NLB / fix several NLB bugs #74910

Merged
merged 1 commit into from
May 1, 2019

Conversation

M00nF1sh
Copy link
Contributor

@M00nF1sh M00nF1sh commented Mar 4, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
Add TLS support for NLB
Fix several NLB bugs(around targetGroup naming/tagging)

Which issue(s) this PR fixes:

Fixes #73297
Fixes #69264
Fixes #75006

Special notes for your reviewer:

  1. new targetGroups will get name k8s-{namespace:8}-{name:8}-{uuid:10}.
  2. TLS is an upgrade version of SSL protocol, in CLB, both SSL/TLS is identified as SSL, however, in ALB/NLB, both SSL/TLS is identified as TLS. To avoid confusing and ease migration from CLB to NLB, service.beta.kubernetes.io/aws-load-balancer-backend-protocol:ssl is re-used for denoting backend SSL in NLB as well.
  3. Test done:
    • migration from TCP to TLS termination:

      1. create NLB service with TCP port(443), which forward to backend HTTPS port(443).
      2. access the NLB, observed TLS termination at backend works fine(cert by backend).
      3. migrate to NLB TLS termination by adding two annotation: service.beta.kubernetes.io/aws-load-balancer-ssl-cert: arnOfACMCert and service.beta.kubernetes.io/aws-load-balancer-backend-protocol: ssl
      4. access the NLB, observed TLS termination at NLB works fine(cert by ACM), and NLB still talks to backend in TLS 😄 .
      5. migrate to TCP backend by remove annotation service.beta.kubernetes.io/aws-load-balancer-backend-protocol: ssl, and change service targetPort to an HTTP port.
      6. access the NLB, observed TLS termination at NLB works fine(cert by ACM), and NLB still talks to backend in TCP(HTTP) 😄 .
      7. migrate back to TLS backend by add annotation service.beta.kubernetes.io/aws-load-balancer-backend-protocol: ssl, and change service targetPort to an TLS port
      8. access the NLB, observed TLS termination at NLB works fine(cert by ACM), and NLB still talks to backend in TCP(HTTP) 😄 .
    • create NLB service with multiple TLS/TCP port.

      1. create NLB service with TCP port(80), which forward to backend HTTP port(80), and TLS port(443) which forward to backend HTTP port(80), by adding annotation service.beta.kubernetes.io/aws-load-balancer-ssl-cert: arnOfACMCert, and service.beta.kubernetes.io/aws-load-balancer-ssl-ports: 443
      2. observed both port(80:TCP, 443:TLS) works as expected.
    • add TLS port to existing NLB service.

      1. create NLB service with TCP port(80), which forward to backend HTTP port(80).
      2. add an TLS port(443), which forward to backend HTTP port(80) by adding annotation service.beta.kubernetes.io/aws-load-balancer-ssl-cert: arnOfACMCert, and service.beta.kubernetes.io/aws-load-balancer-ssl-ports: 443
      3. observed both port(80:TCP, 443:TLS) works as expected.
    • modify SSL policy

      1. create NLB service with TLS port.
      2. modify SSL policy by adding annotation service.beta.kubernetes.io/aws-load-balancer-ssl-negotiation-policy: ELBSecurityPolicy-2016-08
      3. verify SSL policy updated.
    • targetGroup names/ tagging

      1. during the above testing, all new targetGroups are tagged properly, and named properly. targetGroups are recreated as need(while old targetGroup get deleted).

Does this PR introduce a user-facing change?:

Add TLS termination support for NLB

@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. kind/feature Categorizes issue or PR as related to a new feature. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Mar 4, 2019
@k8s-ci-robot k8s-ci-robot requested review from gnufied and jsafrane March 4, 2019 21:38
@k8s-ci-robot k8s-ci-robot added sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 4, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 5, 2019
@M00nF1sh M00nF1sh force-pushed the nlb_tls branch 4 times, most recently from b892012 to a30a909 Compare March 5, 2019 23:59
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 6, 2019
@M00nF1sh M00nF1sh changed the title [WIP]add TLS support for NLB add TLS support for NLB Mar 6, 2019
@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 Mar 6, 2019
@M00nF1sh M00nF1sh changed the title add TLS support for NLB add TLS support for NLB / Fix several NLB bugs Mar 6, 2019
@M00nF1sh M00nF1sh changed the title add TLS support for NLB / Fix several NLB bugs add TLS support for NLB / fix several NLB bugs Mar 6, 2019
@M00nF1sh
Copy link
Contributor Author

M00nF1sh commented Mar 6, 2019

/assign @micahhausler

@M00nF1sh
Copy link
Contributor Author

M00nF1sh commented Mar 6, 2019

/test pull-kubernetes-e2e-gce-100-performance

@usvgmani
Copy link

Do we know when this bug will be fixed? We are interested in installing Istio on our AWS EKS with multiple NLBs --> Fixes #69264

@lelabo-m
Copy link

@gnufied @jsafrane ?

@yurrriq
Copy link

yurrriq commented Apr 30, 2019

/bump

Copy link
Member

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 30, 2019
@M00nF1sh
Copy link
Contributor Author

/test pull-kubernetes-kubemark-e2e-gce-big

@k8s-ci-robot k8s-ci-robot merged commit 4f08ea9 into kubernetes:master May 1, 2019
@yurrriq
Copy link

yurrriq commented May 2, 2019

What are the chances this'll make its way into a 1.11.x release?

@matthewejohnson711
Copy link

Dragging in on the mail chain.

@M00nF1sh
Copy link
Contributor Author

M00nF1sh commented May 4, 2019

What are the chances this'll make its way into a 1.11.x release?

Hi, I'll check whether there are merge conflicts and cherry pick this back 😄

@micahhausler
Copy link
Member

Sorry, cherrypicks are only for bugfixes not features

@yurrriq
Copy link

yurrriq commented May 4, 2019

To me, this seems like a bit of both. So which version will have these changes then?

@tnachen
Copy link
Contributor

tnachen commented May 10, 2019

+1 What version is this going into?

@M00nF1sh
Copy link
Contributor Author

@tnachen Hi, this will go into v1.15 😄

@yurrriq
Copy link

yurrriq commented May 10, 2019

Thanks for the response, @M00nF1sh. I guess it's time to put the heat on the kops team to catch up then :)

@davidxjohnson
Copy link

Thanks @M00nF1sh for working on this.

@igorvpcleao
Copy link

Hi @M00nF1sh and @micahhausler!
Do you know why this has not been released?

@M00nF1sh
Copy link
Contributor Author

Hi @M00nF1sh and @micahhausler!
Do you know why this has not been released?

@igorvpcleao Hi, this is already available in k8s v1.15.

@yogeek
Copy link

yogeek commented Oct 23, 2019

Can someone confirm this has been released in 1.15 please ?
Because I am having the #69264 issue (DuplicateTargetGroupName) when trying to configure Istio NLB for 2 clusters in the same zone with the following versions :

$ kubectl version
Client Version: version.Info{Major:"1", Minor:"15", GitVersion:"v1.15.3", GitCommit:"2d3c76f9091b6bec110a5e63777c332469e0cba2", GitTreeState:"clean", BuildDate:"2019-08-19T11:13:54Z", GoVersion:"go1.12.9", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"15", GitVersion:"v1.15.5", GitCommit:"20c265fef0741dd71a66480e35bd69f18351daea", GitTreeState:"clean", BuildDate:"2019-10-15T19:07:57Z", GoVersion:"go1.12.10", Compiler:"gc", Platform:"linux/amd64"}

(cluster has been deployed on AWS by kubeadm 1.15.3)

@chris-mac
Copy link

Is anybody using a sane workaround for EKS + NLB + ACM until v1.15 + becomes available for EKS?

@lelabo-m
Copy link

Is anybody using a sane workaround for EKS + NLB + ACM until v1.15 + becomes available for EKS?

Also interested ...

@danielelisi
Copy link

Is anybody using a sane workaround for EKS + NLB + ACM until v1.15 + becomes available for EKS?

For now I'm adding the ACM certs manually via CLI/UI after the NLB is provisioned

@kswong-sg
Copy link

Is anybody using a sane workaround for EKS + NLB + ACM until v1.15 + becomes available for EKS?

For now I'm adding the ACM certs manually via CLI/UI after the NLB is provisioned

AWS EKS now is supports 1.15 for upgrade and can use this already

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/cloudprovider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet