Skip to content
This repository has been archived by the owner on Apr 7, 2020. It is now read-only.

Enable cloud-nat for GCP #379

Merged
merged 1 commit into from
Oct 24, 2019
Merged

Conversation

zanetworker
Copy link
Contributor

@zanetworker zanetworker commented Oct 21, 2019

What this PR does / why we need it:
This PR adds CloudNat as a replacement to ephemeral public IPs for GCP, this is to be consistent with other cloud providers and to provide more security not to have instances exposed to the outside.

Which issue(s) this PR fixes:

Special notes for your reviewer:
The MCM PR should be merged first, otherwise, CloudNAT and public IPs will co-exist.

/cc @prashanth26 @hardikdr @schrodit @DockToFuture

Release note:

CloudNAT is now used instead of ephemeral public IPs for GCP instances, please don't use the public IP directly to access the VMs, instead use LoadBalancers to access applications, or bastion hosts to SSH (opening firewall rules won't be enough).

@zanetworker zanetworker requested a review from a team as a code owner October 21, 2019 05:15
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 added needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 21, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2019
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2019
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2019
@gardener-robot-ci-3 gardener-robot-ci-3 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2019
@gardener-robot-ci-2 gardener-robot-ci-2 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2019
@gardener-robot-ci-3 gardener-robot-ci-3 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Oct 21, 2019
@prashanth26
Copy link
Contributor

prashanth26 commented Oct 21, 2019

Hi @zanetworker ,

I apologize for the late changes, but we have inverted the logic for external IP attachment to maintain backward compatibility for existing MCM users. You can probably ignore the machineAPI changes to this PR. I shall create another PR on Gardener to adopt the new APIs and fix them. In the meanwhile this PR could just introduce the cloud NAT.

Refer - https://github.com/gardener/machine-controller-manager/pull/340/files#diff-f1905fa3f5fb0c5fde16ea315dcedc1fR1031

Copy link
Contributor

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

Generally, lgtm, let's wait for a new MCM release.

controllers/provider-gcp/pkg/controller/worker/machines.go Outdated Show resolved Hide resolved
@zanetworker
Copy link
Contributor Author

as discussed with @prashanth26, a small change needs to incorporated to use "disableExternalIP" instead of "attachExternalIP" as MCM needs to have it enabled by default for external users of MCM.

/cc @rfranzke

@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 21, 2019
@prashanth26
Copy link
Contributor

prashanth26 commented Oct 21, 2019

Also a quick reminder to the vendor in the new MachineAPIs into the Gardener-extensions with the disableExternalIP field. Also, handle the same in worker pool controller. This is post the new MCM release planned later this week.

@rfranzke
Copy link
Contributor

Re-vendoring is not needed I guess because we don't work with the MCM types but only use the YAML specification.

@prashanth26
Copy link
Contributor

Okay, then I maybe misunderstood the flow earlier. Now it makes sense. Re vendoring might not be required then.

@vlerenc
Copy link

vlerenc commented Oct 21, 2019

Do you have more details @zanetworker? Is the plan roughly gardener/machine-controller-manager#339 (comment) and will we basically "migrate" from external IPs to NAT GWs? If so and because of @rfranzke comment #379, please inform the end users in advance:

  • Your cluster will be migrated from external IPs to NAT GWs and any code/processes/solutions that depend on external IPs will fail (e.g. opening firewall rules won't cut it anymore)
  • Your worker nodes will be forcefully rolled outside your maintenance time window (unless we can prevent that from happening, which would be preferred, of course)

cc @zanetworker @DockToFuture @prashanth26 @hardikdr @rfranzke

@rfranzke rfranzke added the reviewed/do-not-merge Has no approval for merging, may not be merged as it may break things or be of poor quality label Oct 21, 2019
@zanetworker
Copy link
Contributor Author

zanetworker commented Oct 21, 2019

Do you have more details @zanetworker? Is the plan roughly gardener/machine-controller-manager#339 (comment) and will we basically "migrate" from external IPs to NAT GWs? If so and because of @rfranzke comment #379?

Yes, machines will be migrated from externalIPs in the next reconciliation, because the machine class will change which will eventually leads to having newer machines.

  • Your cluster will be migrated from external IPs to NAT GWs and any code/processes/solutions that depend on external IPs will fail (e.g. opening firewall rules won't cut it anymore)

That I already mentioned in the current release note but I can add that last bit (e.g. opening firewall rules won't cut it anymore) :)

  • Your worker nodes will be forcefully rolled outside your maintenance time window (unless we can prevent that from happening, which would be preferred, of course)

I don't think this is needed, simply because the external IPs currently assigned for the VMs are ephemeral by nature meaning they change anyways every VM restart and can not be guranteed. For this PR, a new machine would simple be like a VM restart since the IP changes anyways, except this time there will be no ephemeral IPs.

The currently assigned IPs are ephemeral and only meant by design to allow external access for VMs. Relying on an ephemeral IP means having downtime for the application for every VM restart / hibernation / interface attachement and so on. Therefore, I don't think there is any production workload that is actively using it, or what do you think?

@rfranzke
Copy link
Contributor

Your worker nodes will be forcefully rolled outside your maintenance time window (unless we can prevent that from happening, which would be preferred, of course)

The machines won't be rolled outside of the maintenance time window. This does only happen in case the bootstrap config changes, but here this is not the case.

Copy link
Contributor

@rfranzke rfranzke left a comment

Choose a reason for hiding this comment

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

Once #382 is merged we can also get this in, I guess :)

@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Oct 23, 2019
@rfranzke rfranzke removed the reviewed/do-not-merge Has no approval for merging, may not be merged as it may break things or be of poor quality label Oct 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove public IP's from gcp worker
7 participants