Additional subnet configuration options for AWS ELB #97431

kishorj · 2020-12-21T22:03:11Z

What type of PR is this?

/kind bug

What this PR does / why we need it:
This fix fulfills the security requirements of choosing specific set of subnets per load balancer on the same cluster.

We need this PR to support the following use-cases-

Ability to choose different or specific set of subnets per LB to fulfil security requirements. This will be supported via annotation.
Simplify VPC setup requirements since cluster name is no longer required on the tag key for subnet resources. This also helps easily provision k8s on an existing VPC setup.

Prior to this change, the AWS cloudprovider would auto discover subnets only with the cluster tags when provisioning NLB/CLB for service resources. We want to modify this behavior to include the subnets without any cluster tags, in addition to the ones previously matched by auto-discovery. After the changes, the auto-discovery will consider all subnets except the ones tagged explicitly for other cluster. If there are multiple subnets per AZ, the ties are broken in the following order

prefer the subnet with the correct role tag. kubernetes.io/role/elb for public and kubernetes.io/role/internal-elb for private access
prefer the subnet with the cluster tag kubernetes.io/cluster/<Cluster Name>
prefer the subnet that is first in lexicographic order

This PR also introduces an additional annotation to manually configure the subnets.

service.beta.kubernetes.io/aws-load-balancer-subnets: <CSV List of subnet IDs or names>

The default behavior is to auto-discover the subnets. The annotation is a comma separated list of subnet IDs or the Name tag of the subnets. The subnets specified on this annotation must exist on the same VPC and must be from different AZs.

For NLB, due to current limitations, subnet configuration is applied during load balancer creation only.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
Verified the following on a kops cluster -

subnets with the appropriate cluster tag continue to get auto-discovered
subnets without any cluster tag also gets auto-discovered
subnets with cluster tag not matching the current cluster is not used
in case there are multiple subnets on an AZ, role tag takes precedence over cluster tag, cluster tag over lexicographic order of subnets
subnets annotation support both subnet IDs and names
subnets annotation override auto-discovery. For NLB, the specified subnets are used during LB creation
for CLB, subnets annotation can be edited and the changes get reflected on the load balancer during the next reconcile
when nonexistent subnets were specified in the annotation, service-controller threw an error. Once the annotation got edited, lb got reconciled successfully
when multiple subnets from the same AZ were specified in the annotation, got InvalidConfigurationRequest from ELB api

Does this PR introduce a user-facing change?:

AWS cloudprovider supports auto-discovering subnets without any kubernetes.io/cluster/<clusterName> tags. It also supports additional service annotation service.beta.kubernetes.io/aws-load-balancer-subnets to manually configure the subnets.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2020-12-21T22:03:19Z

@kishorj: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 · 2020-12-21T22:03:19Z

Hi @kishorj. Thanks for your PR.

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

M00nF1sh · 2020-12-21T23:48:36Z

/ok-to-test

staging/src/k8s.io/legacy-cloud-providers/aws/aws.go

kishorj · 2020-12-22T01:47:45Z

/label tide/merge-method-squash

staging/src/k8s.io/legacy-cloud-providers/aws/aws.go

M00nF1sh

do we want to support this for ELB as well?
have we tested the annotation works with kops cluster?

M00nF1sh

/ok-to-test
/lgtm

andrewsykim · 2021-01-22T17:08:51Z

The default behavior can lead to insecure configurations since when there are multiple subnets tagged within a AZ, it chose one based on lexical order of subnetID and there is no way to override it.

I'm fine with this PR, given it improves security and avoids insecure configurations. Left a few comments.

* support subnets without cluster tag for auto-discovery * add support for manual subnet configuration via service annotation

andrewsykim

Minor question, lgtm otherwise

staging/src/k8s.io/legacy-cloud-providers/aws/tags.go

andrewsykim · 2021-02-09T15:04:05Z

/approve

Will leave lgtm for @nckturner or @M00nF1sh

k8s-ci-robot · 2021-02-09T15:04:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, kishorj, M00nF1sh

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:

~~staging/src/k8s.io/legacy-cloud-providers/OWNERS~~ [andrewsykim]

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

M00nF1sh · 2021-02-09T19:24:29Z

/lgtm

kishorj · 2021-02-09T21:03:51Z

/retest

kishorj · 2021-02-10T00:02:07Z

/retest

fejta-bot · 2021-02-10T03:01:03Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2021-02-10T06:52:05Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2021-02-10T10:43:06Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2021-02-10T13:31:04Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

…31-upstream-release-1.19 Automated cherry pick of #97431: additional subnet configuration for AWS ELB

…31-upstream-release-1.20 Automated cherry pick of #97431: additional subnet configuration for AWS ELB

In Kubernetes v1.21, tagging subnets is not required to create load balancers. Keights will still support tagging them. See kubernetes/kubernetes#97431.

Miciah · 2021-12-21T18:09:46Z

subnets without any cluster tag also gets auto-discovered

This is a breaking change for users who do have untagged subnets, don't want the cloud provider to use those untagged subnets, and upgrade to a version of Kubernetes with this change. Should this be behind a feature gate?

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Dec 21, 2020

k8s-ci-robot requested review from chrislovecnm and jsafrane December 21, 2020 22:04

k8s-ci-robot added area/cloudprovider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 21, 2020

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 Dec 21, 2020

M00nF1sh reviewed Dec 22, 2020

View reviewed changes

staging/src/k8s.io/legacy-cloud-providers/aws/aws.go Outdated Show resolved Hide resolved

k8s-ci-robot added tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. 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 Dec 22, 2020

kishorj changed the title ~~Support subnets without cluster tag for auto-discovery~~ Additional subnet configuration for AWS ELB Dec 22, 2020

kishorj changed the title ~~Additional subnet configuration for AWS ELB~~ Additional subnet configuration options for AWS ELB Dec 22, 2020

M00nF1sh reviewed Dec 22, 2020

View reviewed changes

staging/src/k8s.io/legacy-cloud-providers/aws/aws.go Show resolved Hide resolved

M00nF1sh reviewed Dec 22, 2020

View reviewed changes

k8s-ci-robot assigned M00nF1sh Dec 22, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 22, 2020

k8s-ci-robot assigned cheftako Jan 22, 2021

additional subnet configuration for AWS ELB

778e97d

* support subnets without cluster tag for auto-discovery * add support for manual subnet configuration via service annotation

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 22, 2021

andrewsykim reviewed Feb 8, 2021

View reviewed changes

staging/src/k8s.io/legacy-cloud-providers/aws/tags.go Show resolved Hide resolved

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

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2021

k8s-ci-robot merged commit f48972e into kubernetes:master Feb 10, 2021

k8s-ci-robot added this to the v1.21 milestone Feb 10, 2021

kishorj mentioned this pull request Feb 22, 2021

annotation "aws-load-balancer-subnets" not taken into account kubernetes-sigs/aws-load-balancer-controller#1821

Closed

This was referenced Mar 19, 2021

Automated cherry pick of #97431: additional subnet configuration for AWS ELB #100414

Merged

Automated cherry pick of #97431: additional subnet configuration for AWS ELB #100416

Merged

kishorj deleted the aws-subnet-tagging-update branch March 19, 2021 22:40

k8s-ci-robot added a commit that referenced this pull request Apr 8, 2021

Merge pull request #100416 from kishorj/automated-cherry-pick-of-#974…

6b1f5f1

…31-upstream-release-1.19 Automated cherry pick of #97431: additional subnet configuration for AWS ELB

k8s-ci-robot added a commit that referenced this pull request Apr 8, 2021

Merge pull request #100414 from kishorj/automated-cherry-pick-of-#974…

2ac4f20

…31-upstream-release-1.20 Automated cherry pick of #97431: additional subnet configuration for AWS ELB

This was referenced May 1, 2021

Cherry pick PRs from K/K kubernetes/cloud-provider-aws#209

Closed

Cherry-pick: additional subnet configuration for AWS ELB (#97431) kubernetes/cloud-provider-aws#210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Additional subnet configuration options for AWS ELB #97431

Additional subnet configuration options for AWS ELB #97431

kishorj commented Dec 21, 2020 •

edited

k8s-ci-robot commented Dec 21, 2020

k8s-ci-robot commented Dec 21, 2020

M00nF1sh commented Dec 21, 2020

kishorj commented Dec 22, 2020

M00nF1sh left a comment

M00nF1sh left a comment

andrewsykim commented Jan 22, 2021

andrewsykim left a comment

andrewsykim commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

M00nF1sh commented Feb 9, 2021

kishorj commented Feb 9, 2021

kishorj commented Feb 10, 2021

fejta-bot commented Feb 10, 2021

fejta-bot commented Feb 10, 2021

fejta-bot commented Feb 10, 2021

fejta-bot commented Feb 10, 2021

Miciah commented Dec 21, 2021

Additional subnet configuration options for AWS ELB #97431

Additional subnet configuration options for AWS ELB #97431

Conversation

kishorj commented Dec 21, 2020 • edited

k8s-ci-robot commented Dec 21, 2020

k8s-ci-robot commented Dec 21, 2020

M00nF1sh commented Dec 21, 2020

kishorj commented Dec 22, 2020

M00nF1sh left a comment

Choose a reason for hiding this comment

M00nF1sh left a comment

Choose a reason for hiding this comment

andrewsykim commented Jan 22, 2021

andrewsykim left a comment

Choose a reason for hiding this comment

andrewsykim commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

M00nF1sh commented Feb 9, 2021

kishorj commented Feb 9, 2021

kishorj commented Feb 10, 2021

fejta-bot commented Feb 10, 2021

fejta-bot commented Feb 10, 2021

fejta-bot commented Feb 10, 2021

fejta-bot commented Feb 10, 2021

Miciah commented Dec 21, 2021

kishorj commented Dec 21, 2020 •

edited