-
Notifications
You must be signed in to change notification settings - Fork 50
Conversation
99630f4
to
52acd4d
Compare
52acd4d
to
b9dcd08
Compare
b9dcd08
to
8334e3b
Compare
8334e3b
to
bbb86bd
Compare
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. |
There was a problem hiding this 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.
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 |
bbb86bd
to
ff5aae3
Compare
Also a quick reminder to the vendor in the new MachineAPIs into the |
Re-vendoring is not needed I guess because we don't work with the MCM types but only use the YAML specification. |
Okay, then I maybe misunderstood the flow earlier. Now it makes sense. Re vendoring might not be required then. |
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:
cc @zanetworker @DockToFuture @prashanth26 @hardikdr @rfranzke |
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.
That I already mentioned in the current release note but I can add that last bit
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? |
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. |
There was a problem hiding this 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 :)
controllers/provider-gcp/charts/internal/machineclass/templates/machineclass.yaml
Outdated
Show resolved
Hide resolved
ff5aae3
to
0e8545f
Compare
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: