kubernetes-sigs / cluster-api-provider-openstack

Cluster API implementation for OpenStack
https://cluster-api-openstack.sigs.k8s.io/
Apache License 2.0
275 stars 253 forks source link

User provided router is not reconciled #2129

Open MaysaMacedo opened 2 weeks ago

MaysaMacedo commented 2 weeks ago

/kind bug

What steps did you take and what happened: When an existent router is provided in the openstack cluster spec, that is not reconciled here. The router is only reconciled when created by CAPO here.

What did you expect to happen: The provided router would be reconciled and the ID of the router would be present on the status of the OpenStack cluster object.

EmilienM commented 2 weeks ago

Bleurk, this is a valid issue. Let me know if you need help on this one.

huxcrux commented 2 weeks ago

Isn't the point with a user provided router that it shouldn't be reconciled? The only thing that should be needed is a router interface in all subnets?

mdbooth commented 2 weeks ago

What's the impact of this? Can you describe a problem that this causes?

MaysaMacedo commented 2 weeks ago

@huxcrux @mdbooth I noticed the user provided network and subnet IDs are set in the status, but the ID of the user provided router in not the status. The only place where this is really a problem seems to be here. But, in general as a user I thought that all the resources would end up having the ID resolved in the status.

Here is the example:

  spec:
    controlPlaneEndpoint:
      host: x.x.x.x
      port: 6443
    externalNetwork:
      filter:
        name: test
    identityRef:
      cloudName: openstack
      name: openstack-credentials
    managedSecurityGroups:
      allowAllInClusterTraffic: false
    network:
      filter:
        tags:
        - test
    router:
      filter:
        tags:
        - test
    subnets:
    - filter:
        tags:
        - test
  status:
    controlPlaneSecurityGroup:
      id: 823b7ed4-76f3-4087-89c3-ab109cefda08
      name: k8s-cluster-clusters-openstack-openstack-secgroup-controlplane
    externalNetwork:
      id: 316eeb47-1498-46b4-b39e-00ddf73bd2a5
      name: provider_net_shared_3
    failureDomains:
      nova:
        controlPlane: true
    network:
      id: d57c6e63-4060-4f83-904d-1075da54aa18
      name: test
      subnets:
      - cidr: 10.200.0.0/24
        id: b58f80ad-e6b0-40d9-946a-23a4be3e8905
        name: test
        tags:
        - test
      tags:
      - test
    ready: true
    workerSecurityGroup:
      id: 389e230d-2cc4-4fc8-b542-b32dc8dd7264
      name: k8s-cluster-clusters-openstack-openstack-secgroup-worker
mdbooth commented 2 weeks ago

This is a weird situation. The purpose of specifying a router is to re-use an existing router when creating a network. If we're not creating a network we don't need a router specification. I'd almost be inclined to go the other way and emit a validation warning if the user specifies a router which isn't going to be used. The API remains a bit of a mess here, tbh.

If I'm honest I don't understand why we're adding router IPs to API allowed CIDRs. Still, resolving an unused router just for that use case seems a bit odd. I'd be happy to leave it ignored.