Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨ AllNodes security groups API #1826

Merged
merged 1 commit into from
Feb 21, 2024
Merged

✨ AllNodes security groups API #1826

merged 1 commit into from
Feb 21, 2024

Conversation

EmilienM
Copy link
Contributor

@EmilienM EmilienM commented Jan 15, 2024 •
edited by mdbooth
Loading

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

Fixes: #1274 #1704
/hold

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 15, 2024
@netlify Netlify
Copy link

netlify bot commented Jan 15, 2024 •
edited
Loading

✅ 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...

QR Code

Use your smartphone camera to open QR code link.

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

@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jan 15, 2024
@EmilienM EmilienM marked this pull request as draft January 15, 2024 21:32
EmilienM
EmilienM commented Jan 16, 2024
View reviewed changes
api/v1alpha7/conversion.go Outdated Show resolved Hide resolved
@EmilienM EmilienM force-pushed the secGroup-v1beta1 branch 5 times, most recently from eeedb9d to ebb6cc6 Compare January 16, 2024 13:54
@k8s-ci-robot 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
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 EmilienM mentioned this pull request Jan 16, 2024
Closed
@EmilienM EmilienM force-pushed the secGroup-v1beta1 branch 4 times, most recently from d4a3641 to 59034d8 Compare January 18, 2024 20:11
@EmilienM EmilienM changed the title WIP - AllNodes security groups API AllNodes security groups API Jan 18, 2024
@EmilienM EmilienM marked this pull request as ready for review January 18, 2024 20:11
@k8s-ci-robot 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
EmilienM
EmilienM commented Jan 18, 2024
View reviewed changes
api/v1alpha5/conversion.go Outdated Show resolved Hide resolved
@k8s-ci-robot 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
dulek reviewed Feb 16, 2024
View reviewed changes
Copy link
Contributor

@dulek dulek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just one remark, otherwise this looks good!

pkg/cloud/services/networking/securitygroups.go Outdated Show resolved Hide resolved
@EmilienM EmilienM force-pushed the secGroup-v1beta1 branch 2 times, most recently from 195ac07 to c54e5f0 Compare February 19, 2024 13:53
@EmilienM
Copy link
Contributor Author

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

@EmilienM
Copy link
Contributor Author

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

@EmilienM
Copy link
Contributor Author

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

@EmilienM
Copy link
Contributor Author

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

1 similar comment
@EmilienM
Copy link
Contributor Author

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

@EmilienM EmilienM mentioned this pull request Feb 19, 2024
Merged
3 tasks
@EmilienM
Copy link
Contributor Author

/hold

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

@EmilienM
Copy link
Contributor Author

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

@k8s-ci-robot 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
@mdbooth
Copy link
Contributor

mdbooth commented Feb 20, 2024

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
Copy link
Contributor Author

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
bilbobrovall reviewed Feb 20, 2024
View reviewed changes
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{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@EmilienM
AllNodes security groups API
418ce3d 
Co-Authored-By: Emilien Macchi <emilien@redhat.com>
Co-Authored-By: Matthew Booth <mdbooth@redhat.com>
@EmilienM
Copy link
Contributor Author

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

@EmilienM
Copy link
Contributor Author 

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
lentzi90 reviewed Feb 21, 2024
View reviewed changes
Copy link
Contributor

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!
/lgtm

@k8s-ci-robot 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 k8s-ci-robot merged commit fc47622 into kubernetes-sigs:main Feb 21, 2024
10 checks passed
@mdbooth mdbooth deleted the secGroup-v1beta1 branch February 21, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dulek dulek dulek left review comments

@mdbooth mdbooth mdbooth approved these changes

@MaysaMacedo MaysaMacedo MaysaMacedo left review comments

@huxcrux huxcrux huxcrux left review comments

@lentzi90 lentzi90 lentzi90 left review comments

@JoelSpeed JoelSpeed JoelSpeed left review comments

@bilbobrovall bilbobrovall bilbobrovall left review comments

@jichenjc jichenjc Awaiting requested review from jichenjc

Assignees

@lentzi90 lentzi90

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
CAPO Roadmap
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support different CNI plugin
9 participants
@EmilienM @huxcrux @mdbooth @k8s-ci-robot @dulek @MaysaMacedo @lentzi90 @JoelSpeed @bilbobrovall