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

Cannot add SG rule with `remoteIPPrefix` #2075

Closed jnummelin closed 1 month ago

jnummelin commented 1 month ago

/kind bug

What steps did you take and what happened: Adding SG rules for e.g. SSH fails:

  managedSecurityGroups:
    allowAllInClusterTraffic: true
    allNodesSecurityGroupRules:
      - description: Allow SSH to cluster all nodes
        direction: ingress
        etherType: IPv4
        name: SSH
        portRangeMax: 22
        portRangeMin: 22
        protocol: tcp
        remoteIPPrefix: "0.0.0.0/0"

This fails to reconcile:

"failureMessage": "failed to reconcile security groups: remoteManagedGroups is required",

The docs say that I should be able to provide only the prefix, right:

You can specify either remoteGroupID or remoteIPPrefix or remoteManagedGroups

What did you expect to happen: I was expecting to have a rule in the managed SG to allow SSH

Anything else you would like to add: [Miscellaneous information that will assist in solving the issue.]

Environment:

mdbooth commented 1 month ago

@EmilienM Do you remember why we have this in validateRemoteManagedGroups? https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/64242f4d91a6b8dff8d85ef24cb2307935470753/pkg/cloud/services/networking/securitygroups.go#L299-L311

I think empty remote managed groups is fine, right? They just need to be valid if specified.

mdbooth commented 1 month ago

Hmm, we have a test that this produces an error: https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/64242f4d91a6b8dff8d85ef24cb2307935470753/pkg/cloud/services/networking/securitygroups_test.go#L51-L65

Now I'm worried I'm forgetting something important.

mdbooth commented 1 month ago

I suspect we've just over-encoded an assumption that there will always be a remote group because the 'upgrade' rules all had one. I suspect we can just remove it, but I'd like to get a second opinion.

EmilienM commented 1 month ago

/assign EmilienM