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

terraform upcloud: Added possibility to set up nodes with only private networks #20

Open
wants to merge 5 commits into
base: v2.25.0+k8s-node-fix+missing-tf-provider+upcloud-fixes-and-features+calico-upgrade
Choose a base branch
from

Conversation

Xartos
Copy link

@Xartos Xartos commented Oct 4, 2024

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This is just for review purpuse, I'll take it upstream when this is approved

This also changes the output variables master_ip and worker_ip from having the IPs in private_ip and public_ip for each node to be private and public. I tried to look and that doesn't seem to be used anywhere so it should be fine

Does this PR introduce a user-facing change?:


Copy link

@robinAwallace robinAwallace left a comment

Choose a reason for hiding this comment

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

LGTM, But I will try it later today 🙂

Comment on lines +113 to +114
* `force_public_ip`: If `use_public_ips` is set to false, this forces a public NIC on the machine. Useful if you're migrating from public nodes to only private. Defaults to `false`
* `force_no_user_data`: If `private_network_dns` is set existing nodes will be deleted. This forces this mahcine to not add the user_data. Useful if you're migrating from public nodes to only private. Defaults to `false`

Choose a reason for hiding this comment

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

Should there be some migration steps to move from public to private and vice verse?

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, yea that makes sense. I'll write some small section about that. But it's basically to just replace all nodes.

Choose a reason for hiding this comment

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

When trying to apply these changes on an existing cluster I'm seeing will be updated in-place, is this not true or what do you mean by "replace all nodes"?

Copy link

@robinAwallace robinAwallace left a comment

Choose a reason for hiding this comment

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

I tried to create vms with just private ips and a bastion host and it works 👍

Copy link

@simonklb simonklb left a comment

Choose a reason for hiding this comment

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

The plans after switching to this branch from the base branch of this PR looks like this:

With use_public_ips=true:

  ~ master_ip           = {
      ~ simonklb-control-plane-0 = {
          + private    = "172.16.123.3"
          - private_ip = "172.16.123.3"
          + public     = "185.26.51.68"
          - public_ip  = "185.26.51.68"
        }
    }

I would not have expected any diffs here if it was completely backwards compatible. I'm not sure if there is something relying on the output variables but if there are it might be Example of backwards incompatibility https://github.com/elastisys/ck8s-devbox/pull/535 which makes it worth noting that it's not backwards compatible.

With use_public_ips=false:

  ~ master_ip           = {
      ~ simonklb-control-plane-0 = {
          + private    = "185.26.51.68"
          - private_ip = "172.16.123.3"
          - public_ip  = "185.26.51.68"
        }
    }

This looks wrong or maybe I'm not understanding what "private IPs" means here. I would have assumed the private IP would have been the local address.

@@ -100,6 +100,8 @@ terraform destroy --var-file cluster-settings.tfvars \
* `template_name`: The name or UUID of a base image

Choose a reason for hiding this comment

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

The overview at the top of the README looks a bit out of date, maybe add the bastion to the ASCII diagram or just remove it?

It also mentions the network setup so maybe also mention the private only setup or get rid of that as well?

@@ -108,6 +110,8 @@ terraform destroy --var-file cluster-settings.tfvars \
* `cpu`: number of cpu cores
* `mem`: memory size in MB
* `disk_size`: The size of the storage in GB
* `force_public_ip`: If `use_public_ips` is set to false, this forces a public NIC on the machine. Useful if you're migrating from public nodes to only private. Defaults to `false`

Choose a reason for hiding this comment

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

Being able to both have use_public_ips=false and force_public_ip=true is a bit confusing.

Without digging into the code I wouldn't really get what this meant. It kind of reads as "create a public NIC but don't use it", is that the case?

Could the documentation be clearer somehow to explain why this is useful when migrating. Is it to keep some machines in the same state as before but recreating the rest?

Comment on lines +113 to +114
* `force_public_ip`: If `use_public_ips` is set to false, this forces a public NIC on the machine. Useful if you're migrating from public nodes to only private. Defaults to `false`
* `force_no_user_data`: If `private_network_dns` is set existing nodes will be deleted. This forces this mahcine to not add the user_data. Useful if you're migrating from public nodes to only private. Defaults to `false`

Choose a reason for hiding this comment

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

When trying to apply these changes on an existing cluster I'm seeing will be updated in-place, is this not true or what do you mean by "replace all nodes"?

@@ -108,6 +110,8 @@ terraform destroy --var-file cluster-settings.tfvars \
* `cpu`: number of cpu cores
* `mem`: memory size in MB
* `disk_size`: The size of the storage in GB
* `force_public_ip`: If `use_public_ips` is set to false, this forces a public NIC on the machine. Useful if you're migrating from public nodes to only private. Defaults to `false`
* `force_no_user_data`: If `private_network_dns` is set existing nodes will be deleted. This forces this mahcine to not add the user_data. Useful if you're migrating from public nodes to only private. Defaults to `false`

Choose a reason for hiding this comment

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

Suggested change
* `force_no_user_data`: If `private_network_dns` is set existing nodes will be deleted. This forces this mahcine to not add the user_data. Useful if you're migrating from public nodes to only private. Defaults to `false`
* `force_no_user_data`: If `private_network_dns` is set existing nodes will be deleted. This forces this machine to not add the user_data. Useful if you're migrating from public nodes to only private. Defaults to `false`

@@ -108,6 +110,8 @@ terraform destroy --var-file cluster-settings.tfvars \
* `cpu`: number of cpu cores
* `mem`: memory size in MB
* `disk_size`: The size of the storage in GB
* `force_public_ip`: If `use_public_ips` is set to false, this forces a public NIC on the machine. Useful if you're migrating from public nodes to only private. Defaults to `false`
* `force_no_user_data`: If `private_network_dns` is set existing nodes will be deleted. This forces this mahcine to not add the user_data. Useful if you're migrating from public nodes to only private. Defaults to `false`

Choose a reason for hiding this comment

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

Same with this. It's not clear to me why I would need to use this variable.

@davidumea davidumea force-pushed the esys/v2.25.0+k8s-node-fix+missing-tf-provider+upcloud-fixes-and-features+calico-upgrade+upcloud-private-ip branch from fb19888 to 685a617 Compare October 14, 2024 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants