kubernetes-sigs / cluster-api-provider-openstack

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

⚠️ Remove security group rules from status #1957

Closed mdbooth closed 6 months ago

mdbooth commented 6 months ago

We were writing the full set of generated security rules to the status. They dominated the size of cluster object and we weren't reading them: we always fetch the current rules from OpenStack during reconciliation.

This also tweaks the rule generation code slightly, which was previously reading security group rules multiple times during a reconciliation. We now do this first and pass the result around.

Fixes: #1956

k8s-ci-robot commented 6 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mdbooth

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: - ~~[OWNERS](https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/OWNERS)~~ [mdbooth] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
netlify[bot] commented 6 months ago

Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
Latest commit c4e5c2a99503874e160154e091cf712788213566
Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65f9bd5f12e23900085875bb
Deploy Preview https://deploy-preview-1957--kubernetes-sigs-cluster-api-openstack.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

jichenjc commented 6 months ago

/test pull-cluster-api-provider-openstack-e2e-test

huxcrux commented 6 months ago

This looks good, skipping /lgtm to allow the full e2e test to run.

mdbooth commented 6 months ago

This should have had

/hold

mdbooth commented 6 months ago

This is a bug in this patch:

  failureMessage: 'failed to reconcile security groups: unable to generate desired
    security group rules: security group k8s-cluster-e2e-bql68j-cluster-e2e-bql68j-secgroup-bastion
    not found'
mdbooth commented 6 months ago

D'oh! That's so dumb: when I was moving the code which generates observedSecGroups I didn't actually add the created groups to the map 🤦

We have test coverage of the various methods called by ReconcileSecurityGroups, but none of ReconcileSecurityGroups itself, so the local tests missed this. I'll see if I can add something which would have caught this.

mdbooth commented 6 months ago

I've fixed the bug and added some unit tests which would have triggered it.

mdbooth commented 6 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth commented 6 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth commented 6 months ago

/test pull-cluster-api-provider-openstack-e2e-full-test

huxcrux commented 6 months ago

/lgtm

mdbooth commented 6 months ago

/hold cancel