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

feat(securityGroups): Improve robustness of security group reconcilers #373

Closed
wants to merge 1 commit into from
Closed

feat(securityGroups): Improve robustness of security group reconcilers #373

wants to merge 1 commit into from

Conversation

gvdhart
Copy link
Contributor

@gvdhart gvdhart commented Sep 6, 2024 •
edited
Loading

Rework of the security group reconcilers so that they are more robust and stable.

Add option in security group spec to remove default outbound rule that allows all outbound traffic

fixes #344
fixes #326
fixes #322

@gvdhart gvdhart force-pushed the feat/security-groups/fix-security-group-reconciliation branch from 236a1d3 to 8659d02 Compare September 6, 2024 10:01
@gvdhart gvdhart changed the title feat(securityGroups): Improve robustness of security group reconcilers Draft: feat(securityGroups): Improve robustness of security group reconcilers Sep 6, 2024
@gvdhart gvdhart marked this pull request as draft September 6, 2024 10:32
@gvdhart gvdhart changed the title Draft: feat(securityGroups): Improve robustness of security group reconcilers feat(securityGroups): Improve robustness of security group reconcilers Sep 6, 2024
@gvdhart gvdhart force-pushed the feat/security-groups/fix-security-group-reconciliation branch 4 times, most recently from d79ec97 to b3f3ebb Compare September 9, 2024 12:45
@gvdhart gvdhart mentioned this pull request Sep 9, 2024
Closed
@gvdhart gvdhart force-pushed the feat/security-groups/fix-security-group-reconciliation branch 4 times, most recently from 7eb0552 to bbcd302 Compare September 10, 2024 10:02
@gvdhart
Copy link
Contributor Author

gvdhart commented Sep 10, 2024 •
edited
Loading

Tested with a large cluster (10+ sg's and 30+ sg rules) and it works well, reconciliation time of sg is greatly improved and more robust.
Adding security groups after the OscCluster has been reconciled works.
Adding security group rules after the OscCluster has been reconciled works.
Deleting the OscCluster will correctly delete all sg rules and sg's.
Deleting a single sg after the OscCluster has been reconciled works.

TODO:

  • Deleting a single sg rule from the OscCluster resource does not work as we require the sgRule spec to delete it, however the spec is already deleted when running the reconcile loop.
  • Write tests.

@gvdhart gvdhart mentioned this pull request Sep 10, 2024
Closed
@gvdhart gvdhart force-pushed the feat/security-groups/fix-security-group-reconciliation branch 3 times, most recently from 5b13a4d to 3b16deb Compare September 11, 2024 14:18
@gvdhart gvdhart force-pushed the feat/security-groups/fix-security-group-reconciliation branch from 3b16deb to 240af5b Compare September 18, 2024 14:27
@gvdhart gvdhart force-pushed the feat/security-groups/fix-security-group-reconciliation branch from 240af5b to cdd42da Compare September 24, 2024 13:41
@gvdhart gvdhart marked this pull request as ready for review September 24, 2024 13:41
outscale-hmi
outscale-hmi approved these changes Sep 29, 2024
View reviewed changes
controllers/osccluster_securitygroup_controller_unit_test.go
assert.Nil(t, sgrtc.expReconcileSecurityGroupRuleErr)
}
t.Logf("find reconcileSecurityGroupRule %v\n", reconcileSecurityGroupRule)
// TO FIX
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please fix the test first

controllers/osccluster_securitygroup_controller_unit_test.go
assert.Nil(t, sgrtc.expReconcileSecurityGroupRuleErr)
}
t.Logf("find reconcileSecurityGroupRule %v\n", reconcileSecurityGroupRule)
// TO FIX
Copy link
Contributor

Choose a reason for hiding this comment

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

same here,

@outscale-hmi
Copy link
Contributor

Hello,

I reviewed this PR and I’m okay with the overall logic. However, some unit tests are failing because of missing calls in certain test cases, additionally, there's a segmentation fault occurring, which likely needs addressing. Could you please update the unit tests to include these missing calls to ensure full coverage of the new logic?

@outscale-hmi
Copy link
Contributor

see #385

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@outscale-hmi outscale-hmi outscale-hmi approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
2 participants
@gvdhart @outscale-hmi