✨ AllNodes security groups API #1826

EmilienM · 2024-01-15T21:31:38Z

What this PR does / why we need it:

New API for all nodes managed security group API.

TODOs:

squashed commits
if necessary:
- includes documentation
- adds unit tests
- adds e2e tests

netlify · 2024-01-15T21:31:44Z

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

Name	Link
🔨 Latest commit	`418ce3d`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65d4bfe789a999000873ff67
😎 Deploy Preview	https://deploy-preview-1826--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

api/v1alpha7/conversion.go

pkg/cloud/services/networking/securitygroups.go

api/v1alpha7/conversion.go

api/v1alpha5/conversion.go

dulek

Just one remark, otherwise this looks good!

pkg/cloud/services/networking/securitygroups.go

EmilienM · 2024-02-19T13:57:57Z

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

EmilienM · 2024-02-19T15:03:51Z

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

EmilienM · 2024-02-19T16:09:25Z

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

EmilienM · 2024-02-19T16:52:07Z

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

EmilienM · 2024-02-19T18:43:13Z

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

EmilienM · 2024-02-19T19:21:02Z

/hold

I've spotted an issue where Security Groups never stop reconciling :(

EmilienM · 2024-02-19T19:26:36Z

found it and fixed it 👍
/hold cancel
/test pull-cluster-api-provider-openstack-e2e-full-test

mdbooth · 2024-02-20T11:08:22Z

found it and fixed it 👍

Comparing pointers instead of their values! It's obvious once you've seen it, but I could probably stare at it for an hour and not spot it.

Thanks for the extra testing diligence. Do you think there's an automated test we could have written which would have caught this?

EmilienM · 2024-02-20T14:53:20Z

found it and fixed it 👍

Comparing pointers instead of their values! It's obvious once you've seen it, but I could probably stare at it for an hour and not spot it.

Thanks for the extra testing diligence. Do you think there's an automated test we could have written which would have caught this?

done

bilbobrovall · 2024-02-20T14:56:03Z

api/v1alpha5/conversion.go

+	out.Name = in.Name
+	out.Rules = make([]SecurityGroupRule, len(in.Rules))
+	for i, rule := range in.Rules {
+		out.Rules[i] = SecurityGroupRule{


Couldn't this be written as ?

Suggested change

out.Rules[i] = SecurityGroupRule{

out.Rules[i] = SecurityGroupRule{

ID: rule.ID,

Direction: rule.Direction,

Description: pointer.StringDeref(rule.Description, ""),

....

On a related out of scope note, we should probably upgrade k8s.io/utils and start using k8s.io/utils/ptr that uses generics instead

yeah, I started doing it and it's a big change. Here is an issue to track it: #1896

api/v1alpha8/openstackcluster_webhook.go

Co-Authored-By: Emilien Macchi <[email protected]> Co-Authored-By: Matthew Booth <[email protected]>

EmilienM · 2024-02-20T15:07:16Z

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

EmilienM · 2024-02-20T16:07:20Z

E0220 15:43:45.873177       1 controller.go:329] "Reconciler error" err="failed to reconcile load balancer: load balancer \"k8s-clusterapi-cluster-self-hosted-021hq8-self-hosted-95qimh-kubeapi\" with id 24695b59-be36-446c-8348-611cbe8d090e is not active after timeout: timed out waiting for the condition" controller="openstackcluster" controllerGroup="infrastructure.cluster.x-k8s.io" controllerKind="OpenStackCluster" OpenStackCluster="self-hosted-021hq8/self-hosted-95qimh" namespace="self-hosted-021hq8" name="self-hosted-95qimh" reconcileID="91718218-6629-47d3-b426-28a4249b30cf"

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

lentzi90

Thanks for this!
/lgtm

k8s-ci-robot requested review from dulek and lentzi90 January 15, 2024 21:31

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 15, 2024

EmilienM marked this pull request as draft January 15, 2024 21:32

EmilienM commented Jan 16, 2024

View reviewed changes

api/v1alpha7/conversion.go Outdated Show resolved Hide resolved

EmilienM commented Jan 16, 2024

View reviewed changes

pkg/cloud/services/networking/securitygroups.go Outdated Show resolved Hide resolved

EmilienM commented Jan 16, 2024

View reviewed changes

pkg/cloud/services/networking/securitygroups.go Outdated Show resolved Hide resolved

EmilienM force-pushed the secGroup-v1beta1 branch 5 times, most recently from eeedb9d to ebb6cc6 Compare January 16, 2024 13:54

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 16, 2024

mdbooth reviewed Jan 16, 2024

View reviewed changes

api/v1alpha7/conversion.go Outdated Show resolved Hide resolved

api/v1alpha7/conversion.go Outdated Show resolved Hide resolved

EmilienM force-pushed the secGroup-v1beta1 branch from ebb6cc6 to ce9b441 Compare January 16, 2024 16:45

EmilienM mentioned this pull request Jan 16, 2024

✨ proposal: new API for managed Security Groups #1756

Closed

EmilienM force-pushed the secGroup-v1beta1 branch 4 times, most recently from d4a3641 to 59034d8 Compare January 18, 2024 20:11

EmilienM changed the title ~~WIP - AllNodes security groups API~~ AllNodes security groups API Jan 18, 2024

EmilienM marked this pull request as ready for review January 18, 2024 20:11

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 18, 2024

k8s-ci-robot requested a review from jichenjc January 18, 2024 20:12

EmilienM commented Jan 18, 2024

View reviewed changes

api/v1alpha5/conversion.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 16, 2024

dulek reviewed Feb 16, 2024

View reviewed changes

pkg/cloud/services/networking/securitygroups.go Outdated Show resolved Hide resolved

EmilienM force-pushed the secGroup-v1beta1 branch 2 times, most recently from 195ac07 to c54e5f0 Compare February 19, 2024 13:53

EmilienM force-pushed the secGroup-v1beta1 branch from c54e5f0 to dd1f1c1 Compare February 19, 2024 18:45

EmilienM mentioned this pull request Feb 19, 2024

✨ Move spec.allowAllInClusterTraffic to spec.ManagedSecurityGroups #1892

Merged

3 tasks

EmilienM force-pushed the secGroup-v1beta1 branch from dd1f1c1 to e2f1d46 Compare February 19, 2024 19:25

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2024

EmilienM requested a review from lentzi90 February 20, 2024 14:08

EmilienM force-pushed the secGroup-v1beta1 branch from e2f1d46 to 0ff6779 Compare February 20, 2024 14:52

bilbobrovall reviewed Feb 20, 2024

View reviewed changes

api/v1alpha8/openstackcluster_webhook.go Outdated Show resolved Hide resolved

AllNodes security groups API

418ce3d

Co-Authored-By: Emilien Macchi <[email protected]> Co-Authored-By: Matthew Booth <[email protected]>

EmilienM force-pushed the secGroup-v1beta1 branch from 0ff6779 to 418ce3d Compare February 20, 2024 15:06

lentzi90 reviewed Feb 21, 2024

View reviewed changes

k8s-ci-robot assigned lentzi90 Feb 21, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 21, 2024

k8s-ci-robot merged commit fc47622 into kubernetes-sigs:main Feb 21, 2024

mdbooth deleted the secGroup-v1beta1 branch February 21, 2024 12:58

-		out.Rules[i] = SecurityGroupRule{
+		out.Rules[i] = SecurityGroupRule{
+			ID:                      rule.ID,
+			Direction:          rule.Direction,
+			Description:      pointer.StringDeref(rule.Description, ""),
+			....

✨ AllNodes security groups API #1826

✨ AllNodes security groups API #1826

Uh oh!

Conversation

EmilienM commented Jan 15, 2024 • edited by mdbooth Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netlify bot commented Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dulek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EmilienM commented Feb 19, 2024

Uh oh!

EmilienM commented Feb 19, 2024

Uh oh!

EmilienM commented Feb 19, 2024

Uh oh!

EmilienM commented Feb 19, 2024

Uh oh!

EmilienM commented Feb 19, 2024

Uh oh!

EmilienM commented Feb 19, 2024

Uh oh!

EmilienM commented Feb 19, 2024

Uh oh!

mdbooth commented Feb 20, 2024

Uh oh!

EmilienM commented Feb 20, 2024

Uh oh!

bilbobrovall Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

EmilienM Feb 20, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

EmilienM commented Feb 20, 2024

Uh oh!

EmilienM commented Feb 20, 2024

Uh oh!

lentzi90 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

EmilienM commented Jan 15, 2024 •

edited by mdbooth

Loading

netlify bot commented Jan 15, 2024 •

edited

Loading