[Openstack] master foreach and fixes

[robinAwallace]

What type of PR is this?
/kind feature

What this PR does / why we need it:

1. Added possibility to toggle to use existing network with use_existing_network.
2. Added possibility to set port_security to null with force_null_port_security. Some clouds do not allow to set this variable.
3. Added depends_on for port when attaching floating ip. For some clouds the port needs a fixed_ip before attaching a floating_ip.
4. Fixed the dynamic inventory to find the floating ips with the new networking resource.
5. Added possibility to create master nodes using for_each. Easier to switch out a master.

Which issue(s) this PR fixes:

Fixes #8696

Special notes for your reviewer:

It started of with just adding for_each for master nodes but ended yup fixing some other issues.

Does this PR introduce a user-facing change?:

[OpenStack] Create master nodes with `for_each`  for openstack. Makes it easier to switch out master nodes via terraform.

[k8s-ci-robot]

Hi @robinAwallace. Thanks for your PR.

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

[cristicalin]

Nice work @robinAwallace !

/ok-to-test

[cristicalin]

@robinAwallace I see the elastix CI job is still failing: https://gitlab.com/kargo-ci/kubernetes-sigs-kubespray/-/jobs/2329389057 ; am I wrong in assuming that this change should have addressed that openstack failure as well ?

[robinAwallace]

@cristicalin Yes it should fix it. By setting use_existing_network=false will ensure that a new network is created.

[cristicalin]

I'm guessing this should have a default to false.

[cristicalin]

@cristicalin Yes it should fix it. By setting use_existing_network=false will ensure that a new network is created.

In this case I think this should be the default, no? I'm not sure how terraform initialises boolean variables without a default, maybe that explains the CI failure.

[floryut]

@cristicalin default to true here (regarding your question about use_existing_network, as terraform don't init var, but fail if no defaults)

[robinAwallace]

I can switch it so default false. If that will solve the pipeline issue.

[floryut]

I can switch it so default false. If that will solve the pipeline issue.

I'm not sure, default to true seems fine, but setting it to false for the elastx job maybe 😄
https://github.com/kubernetes-sigs/kubespray/blob/master/.gitlab-ci/terraform.yml#L161

[robinAwallace]

I can switch it so default false. If that will solve the pipeline issue.

I'm not sure, default to true seems fine, but setting it to false for the elastx job maybe smile https://github.com/kubernetes-sigs/kubespray/blob/master/.gitlab-ci/terraform.yml#L161

Okay, will update it 🙂

[cristicalin]

I can switch it so default false. If that will solve the pipeline issue.

I'm not sure, default to true seems fine, but setting it to false for the elastx job maybe smile https://github.com/kubernetes-sigs/kubespray/blob/master/.gitlab-ci/terraform.yml#L161

In public clouds I think the safest approach is false as they don't give you access directly to their provider networks. In private clouds it depends some may go with the simple approach of one provider but that is not the norm.

[robinAwallace]

I can switch it so default false. If that will solve the pipeline issue.

I'm not sure, default to true seems fine, but setting it to false for the elastx job maybe smile https://github.com/kubernetes-sigs/kubespray/blob/master/.gitlab-ci/terraform.yml#L161

In public clouds I think the safest approach is false as they don't give you access directly to their provider networks. In private clouds it depends some may go with the simple approach of one provider but that is not the norm.

@cristicalin Im open to have it false as default. It does not really matter for us.

[robinAwallace]

@cristicalin @floryut use_existing_network is now default false.

[cristicalin]

Thanks for fixing this @robinAwallace !

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cristicalin, robinAwallace

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:

• contrib/terraform/OWNERS [cristicalin]

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

[cristicalin]

/cc @floryut @oomichi

[oomichi]

/lgtm

[oomichi]

/lgtm

[rptaylor]

With a default of false it seems this has caused a change in behaviour. Previously you could just set use_neutron=0 and network_name to use a pre-existing network. Now this results in an error, so you also have to set use_existing_network = true.
It's fine but a release note would have been helpful to prepare for the change and avoid errors.

[rptaylor]

Also doesn't use_existing_network have effectively the same function as use_neutron already had?
@robinAwallace
Unfortunately use_neutron was added without documentation :( also it is weird that it takes 0 or 1 instead of true/false.
If use_neutron is redundant now perhaps it could be removed?

[robinAwallace]

Hi @rptaylor, that was unfortunate that neutron and network name was missed. As you said it was not documented 😞
But yes it looks like neutron is redundant now.
Hope you manged to use the new way of using an existing network 🙂

[rptaylor]

This seems to change the mapping of ports to nodes, which causes Terraform to destroy and rebuild all nodes of an existing cluster after upgrading to Kubespray 2.19. This is very disruptive. I won't be able to manage my clusters with Terraform anymore (e.g. scale up/down nodes, modify security groups etc.) unless I destroy and rebuild them all from scratch.
Is there any workaround possible for existing users to keep using kubespray after upgrading to 2.19 without destroying and rebuilding all clusters @cristicalin @floryut @robinAwallace ?

If I do terraform plan it shows a port change forces replacement of each node, including masters:

  # module.compute.openstack_compute_instance_v2.k8s_master_no_floating_ip[0] must be replaced
-/+ resource "openstack_compute_instance_v2" "k8s_master_no_floating_ip" {
      ~ access_ip_v4        = "192.168.243.200" -> (known after apply)
      + access_ip_v6        = (known after apply)
      ~ all_metadata        = {
          - "depends_on"       = " "
          - "kubespray_groups" = "etcd,kube_control_plane,,k8s_cluster,no_floating"
          - "ssh_user"         = "almalinux"
          - "use_access_ip"    = "1"
        } -> (known after apply)
      ~ all_tags            = [] -> (known after apply)
      ~ flavor_name         = "p2-3gb" -> (known after apply)
      ~ id                  = "83bf862c-51cb-4a3c-982b-8616d1926e48" -> (known after apply)
      ~ image_id            = "Attempt to boot from volume - no image supplied" -> (known after apply)
      + image_name          = (known after apply)
        name                = "cluster-dev-k8s-master-nf-1"
      ~ region              = "RegionOne" -> (known after apply)
      ~ security_groups     = [
          - "cluster-dev-k8s",
          - "cluster-dev-k8s-master",
          - "cluster-dev-k8s-master-rules",
        ] -> (known after apply)
      - tags                = [] -> null
        # (7 unchanged attributes hidden)

      ~ block_device {
            # (7 unchanged attributes hidden)
        }

      ~ network {
          ~ fixed_ip_v4    = "192.168.243.200" -> (known after apply)
          + fixed_ip_v6    = (known after apply)
          + floating_ip    = (known after apply)
          ~ mac            = "fa:16:3e:e9:8d:18" -> (known after apply)
          ~ name           = "VLAN300" -> (known after apply)
          + port           = (known after apply) # forces replacement
          ~ uuid           = "4e8d0dcf-4853-454c-9184-d2379f397a9b" -> (known after apply)
            # (1 unchanged attribute hidden)
        }
    }

As for use_neutron, it looks like that is used to determine whether to create a router and subnet etc (contrib/terraform/openstack/modules/network/main.tf) whereas use_existing_network mainly handles setting up the ports for the nodes, so I guess both are still needed - or the new use_existing_network variable could be expanded to replace the remaining functionality of use_neutron.

[robinAwallace]

Right, this version was really distributive for openstack. We really maintain our own wrapper around kubespray, with some migration scripts. You can find how we migrated our openstack terraform state to the new version here

Note that our migration script expects two clusters. But you could extract the bash script to work for one cluster.