Allow to control which subnets and IPs get used for the API loadbalancer #10741

codablock · 2021-02-05T12:46:14Z

This PR implements the new field subnets for the api loadbalancer in the cluster spec:

spec:
  api:
    loadBalancer:
      type: Internal
      subnets:
        - name: subnet-a
          privateIPv4Address: 172.16.1.10

It allows to control which subnets are used by the api load balancer. This is useful when VPCs and subnets are managed externally and a more granular separation between subnets is desired, e.g. when you have subnets that offer VPN access to on-prem networks, subnets which offer public IPs and subnets which are completely private.

In such case, the subnets must be carefully chosen to not expose the API in the wrong place.

Review can be done on a per-commit basis as I've tried to separate changes.

Move checks for valid subnet operations into CheckChanges. This also fixes a bug where changes would cause immutable field errors while it's actually perfectly fine to add new subnets (only detaching is forbidden). This also commit changes the actualSubnets and expectedSubnets lists to be maps of *string. This is in preparation for the next commit that then relies on it being a map.

k8s-ci-robot · 2021-02-05T12:46:22Z

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

codablock · 2021-02-05T12:48:10Z

FYI, I've tested this on the 1.19 branch and would be really happy if this could be backported/cherry-picked into 1.19 :)

SubnetMappings allow to explicitely set the private IPv4 address that must be used for the NLB. SubnetMappings and Subnets in the AWS API are compatible as long as the address settings are not changes, making this commit backwards compatible.

…alancer

timothyclarke · 2021-02-08T20:20:00Z

docs/cluster_spec.md

+It is only allowed to add more subnets and forbidden to remove existing ones. This is due to limitations on AWS
+ELBs and NLBs.


Is there the possibility to remove existing ones if you accept that the consequence is destroying the loadbalancer and creating a new one? eg --force-recreate-loadbalancer
I'd consider losing access to the loadbalancer acceptible during a maintenance window.

I'd assume that this would be possible. But this is not a new problem and thus should be part of another PR. Currently, when you change your set of shared subnets, you'd run into the same issue already as the automatically chosen subnets would differ then.

My question was primarily around the validation side of things. If I wanted to change those values and accepted that I'd have to (manually) remove the loadbalancer, would your changes to kops reject my edits?

It would accept the edits and only fail when running the update. The failure is a little bit unfortunate as it keeps retrying but then of course never succeeds.

A workaround for the above behavior is to manually delete the load balancer. A kops update cluster should then report that it will (re)create the load balancer which should succeed with --yes. You'll run into certificate issues with gossip clusters though since the load balancer's DNS name is used in various certs.

Ah I see you mentioned this a few lines below. I believe you'll also need to kops rolling-update cluster --instance-group-roles=Master --cloudonly after kops update cluster --yes recreates the NLB in order to pick up new certificates.

@rifelpet I did not observe the need for rolling-update while doing my testing, which I did on a semi-live (not prod, but a lot of stuff deployed) cluster.

It may only be gossip clusters that need the rolling update (cluster names ending in .k8s.local, was yours gossip or using route53 ? Now that I think about it more, I think the nodes themselves might need to be rolled too to pick up the new load balancer dns name which is pretty invasive. We could say "It is not recommended to make this change on gossip clusters due to the requirement of performing a --cloudonly rolling update on the entire cluster which results in temporary downtime of all workloads. It is recommended to create a new cluster instead."

pkg/model/awsmodel/api_loadbalancer.go

upup/pkg/fi/cloudup/awstasks/network_load_balancer.go

codablock · 2021-02-10T09:05:42Z

@rifelpet Thanks for your review. I've gone through it and fixed things where necessary. You can review the changes per-commit.

…rmation

pkg/apis/kops/validation/aws.go

upup/pkg/fi/cloudup/awstasks/network_load_balancer.go

codablock · 2021-02-10T14:44:36Z

@rifelpet I pushed a few more commits to handle your latest comments.

rifelpet · 2021-02-10T15:41:42Z

this looks good to me, thanks for implementing this! I'll have someone else take a pass at it too

/ok-to-test

codablock · 2021-02-10T16:37:35Z

@rifelpet Can you help me find the logs of the test failures that appeared now?

rifelpet · 2021-02-10T16:48:42Z

Yea you can click the Details button in the list of checks at the bottom of the PR. here are the two failures:

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kops/10741/pull-kops-verify-staticcheck/1359527983353696256
looks like a pretty trivial fix in the validation test file.

https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kops/10741/pull-kops-verify-cloudformation/1359527985023029248

is a bit strange, i think the line numbers are off but its essentially complaining that the allocation ID and private IP v4 address fields in the cloudformation JSON file are null. We should probably not include them at all (omitempty field tag) if that is the case.

codablock · 2021-02-10T17:03:35Z

@rifelpet Thanks, that helped me :) Should be fixed now.

rifelpet · 2021-02-10T23:44:02Z

/lgtm

I'll give others a chance for input otherwise we can get this merged soon. I'll bring it up at kOps office hours on friday too. Excited to get this in :)

codablock · 2021-02-11T09:56:52Z

@rifelpet What do you think about the chances of this being cherry-picked into 1.19?

rifelpet · 2021-02-11T16:33:50Z

1.19.X releases will contain bug fixes only. Since this is a new feature it likely wont get backported to 1.19 though we are aiming for an accelerated 1.20.0 release so the wait shouldn't be too long.

codablock · 2021-02-11T16:35:55Z

ok, having this in 1.20.0 would already be very cool.

rifelpet · 2021-02-14T19:01:55Z

/lgtm
/approve

k8s-ci-robot · 2021-02-14T19:02:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: codablock, rifelpet

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

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

fejta-bot · 2021-02-14T21:45: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.

codablock · 2021-02-15T07:27:41Z

Just seen that this got merged, thanks :) Any steps required from my side to get this cherry-picked into 1.20?

rifelpet · 2021-02-15T17:11:58Z

@codablock I took care of it in #10829

…41-origin-release-1.20 Automated cherry pick of #10741: Refactor and fix NLB subnet change checks

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

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 5, 2021

k8s-ci-robot requested review from johngmyers and olemarkus February 5, 2021 12:46

k8s-ci-robot added area/api area/documentation area/provider/aws Issues or PRs related to aws provider size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 5, 2021

codablock force-pushed the nlb-subnets branch from 862e708 to 1700161 Compare February 5, 2021 14:03

codablock added 3 commits February 5, 2021 17:53

Allow to explicitely choose subnets and private IPs for the API loadb…

6facd1b

…alancer

Add documentation for new subnet fields

6d35423

codablock force-pushed the nlb-subnets branch from a89a31b to 6d35423 Compare February 5, 2021 16:54

bharath-123 mentioned this pull request Feb 8, 2021

[Question / Feature Request] AWS NLB - EIP Allocations #10706

Closed

timothyclarke reviewed Feb 8, 2021

View reviewed changes

rifelpet reviewed Feb 10, 2021

View reviewed changes

codablock added 4 commits February 10, 2021 08:21

Fix check for empty privateIPv4Address

c6eca9d

Move validation of ClusterSubnetSpec into pkg/apis/kops/validation

2c0f980

Better readable modification assigning of PrivateIPv4Address

295fb11

Abort in case a NLB has more then one PrivateIPv4Address

76dda4d

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 Feb 10, 2021

Add omitempty to Subnets and SubnetMappings for terraform and cloudfo…

091a18a

…rmation

codablock force-pushed the nlb-subnets branch from 7fd35dd to 091a18a Compare February 10, 2021 09:30

rifelpet reviewed Feb 10, 2021

View reviewed changes

Use proper errors for subnet validation

4ae47cc

codablock added 2 commits February 10, 2021 15:43

Remove Subnets fields from cloudformation and terraform

16c89e1

Add notice about it being a bug if len(PrivateIPv4Address) = 1

277b917

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 Feb 10, 2021

codablock added 2 commits February 10, 2021 17:56

Fix unnecessary copying of list

cc4aca6

Add back "omitempty" to cloudformation SubnetMapping

684ff34

k8s-ci-robot assigned rifelpet Feb 10, 2021

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

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

k8s-ci-robot merged commit cd10383 into kubernetes:master Feb 14, 2021

k8s-ci-robot added this to the v1.20 milestone Feb 14, 2021

rifelpet mentioned this pull request Feb 15, 2021

Automated cherry pick of #10741: Refactor and fix NLB subnet change checks #10829

Merged

k8s-ci-robot added a commit that referenced this pull request Feb 15, 2021

Merge pull request #10829 from rifelpet/automated-cherry-pick-of-#107…

e65b999

…41-origin-release-1.20 Automated cherry pick of #10741: Refactor and fix NLB subnet change checks

timothyclarke mentioned this pull request Feb 18, 2021

Adding Elastic IP Allocations to NLB API #10872

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to control which subnets and IPs get used for the API loadbalancer #10741

Allow to control which subnets and IPs get used for the API loadbalancer #10741

codablock commented Feb 5, 2021

k8s-ci-robot commented Feb 5, 2021

codablock commented Feb 5, 2021

timothyclarke Feb 8, 2021

codablock Feb 9, 2021

timothyclarke Feb 9, 2021

codablock Feb 9, 2021

rifelpet Feb 9, 2021

rifelpet Feb 10, 2021

codablock Feb 10, 2021

rifelpet Feb 10, 2021

codablock commented Feb 10, 2021

codablock commented Feb 10, 2021

rifelpet commented Feb 10, 2021

codablock commented Feb 10, 2021

rifelpet commented Feb 10, 2021

codablock commented Feb 10, 2021

rifelpet commented Feb 10, 2021

codablock commented Feb 11, 2021

rifelpet commented Feb 11, 2021

codablock commented Feb 11, 2021

rifelpet commented Feb 14, 2021

k8s-ci-robot commented Feb 14, 2021

fejta-bot commented Feb 14, 2021

codablock commented Feb 15, 2021

rifelpet commented Feb 15, 2021

		It is only allowed to add more subnets and forbidden to remove existing ones. This is due to limitations on AWS
		ELBs and NLBs.

Allow to control which subnets and IPs get used for the API loadbalancer #10741

Allow to control which subnets and IPs get used for the API loadbalancer #10741

Conversation

codablock commented Feb 5, 2021

k8s-ci-robot commented Feb 5, 2021

codablock commented Feb 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codablock commented Feb 10, 2021

codablock commented Feb 10, 2021

rifelpet commented Feb 10, 2021

codablock commented Feb 10, 2021

rifelpet commented Feb 10, 2021

codablock commented Feb 10, 2021

rifelpet commented Feb 10, 2021

codablock commented Feb 11, 2021

rifelpet commented Feb 11, 2021

codablock commented Feb 11, 2021

rifelpet commented Feb 14, 2021

k8s-ci-robot commented Feb 14, 2021

fejta-bot commented Feb 14, 2021

codablock commented Feb 15, 2021

rifelpet commented Feb 15, 2021