Skip to content

Add support for Security Group Adoption #165

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
Merged
merged 5 commits into from
Jan 15, 2024
Merged

Add support for Security Group Adoption #165

merged 5 commits into from
Jan 15, 2024

Conversation

pcolazurdo
Copy link
Contributor

@pcolazurdo pcolazurdo commented Dec 13, 2023 •
edited by a-hilaly
Loading

Issue #, if available: aws-controllers-k8s/community#1946

Description of changes:

The existing logic doesn't allow for adoption of existing SecurityGroups because spec.Name is a required property for creation but the aws SDK returns GroupName. The deepCopy in sdkFind then is not enough because of the different names.
By copying SecurityGroup GroupName into spec.Name as part of the sdk_read_many_post_set_output we solve this issue.

This fix allows adoption of existing security groups. Tests have been successful in at least two test scenarios: adopting an existing SG and then modifying its properties, trying to adopt an invalid SG.
I've also ran a few additional tests with the modified controller to be sure that existing functionality didn't break:

  • Create a new SG
  • Modify this SG
  • Delete this SG

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ack-prow ack-prow bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 13, 2023
@ack-prow ACK Prow
Copy link

ack-prow bot commented Dec 13, 2023

Hi @pcolazurdo. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ack-prow ack-prow bot requested review from a-hilaly and vijtrip2 December 13, 2023 12:53
@pcolazurdo pcolazurdo marked this pull request as ready for review December 13, 2023 18:06
@ack-prow ack-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2023
@a-hilaly
Copy link
Member

/ok-to-test

@ack-prow ack-prow bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 13, 2023
nnbu
nnbu reviewed Dec 14, 2023
View reviewed changes
templates/hooks/security_group/sdk_read_many_post_set_output.go.tpl
Comment on lines +2 to +5

// Needed because SecurityGroups Name are held in GroupName property of the AWS resource
ko.Spec.Name = resp.SecurityGroups[0].GroupName

Copy link
Contributor

Choose a reason for hiding this comment

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

You also need to add generated file in this PR.

This seems to be a good approach to handle adoptedResource

Copy link
Contributor

Choose a reason for hiding this comment

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

@pcolazurdo

Please let me know if you plan to make this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I was sick, new version with generated files should be in

@pcolazurdo pcolazurdo closed this Dec 20, 2023
@pcolazurdo
Copy link
Contributor Author

/retest

@pcolazurdo pcolazurdo reopened this Dec 20, 2023
@pcolazurdo
Copy link
Contributor Author

@nnbu - I rebased against the latest release. Changes seem to be ready for merge

a-hilaly
a-hilaly reviewed Jan 10, 2024
View reviewed changes
a-hilaly
a-hilaly approved these changes Jan 15, 2024
View reviewed changes
Copy link
Member

@a-hilaly a-hilaly left a comment

Choose a reason for hiding this comment

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

Thank you @pcolazurdo
/lgtm

@ack-prow ack-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2024
@ack-prow ACK Prow
Copy link

ack-prow bot commented Jan 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: a-hilaly, nnbu, pcolazurdo

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-prow ack-prow bot added the approved label Jan 15, 2024
@ack-prow ack-prow bot merged commit 1d88d6e into aws-controllers-k8s:main Jan 15, 2024
@pcolazurdo pcolazurdo deleted the fix-sg-adopt branch January 16, 2024 14:26
vflaux added a commit to vflaux/ack-ec2-controller that referenced this pull request Jan 18, 2024
@vflaux
chore(v1.2.0): create new release
8dac089 
    Merged PRs since last release:
    - aws-controllers-k8s#165
    - aws-controllers-k8s#174
vflaux added a commit to vflaux/ack-ec2-controller that referenced this pull request Jan 18, 2024
@vflaux
chore(v1.2.1): create new release
7cb3ab1 
    Merged PRs since last release:
    - aws-controllers-k8s#165
    - aws-controllers-k8s#174
vflaux added a commit to vflaux/ack-ec2-controller that referenced this pull request Jan 18, 2024
@vflaux
chore(v1.2.1): create new release
c72715e 
Merged PRs since last release:
- aws-controllers-k8s#165
- aws-controllers-k8s#174
@vflaux vflaux mentioned this pull request Jan 18, 2024
Merged
ack-prow bot pushed a commit that referenced this pull request Jan 18, 2024
@vflaux
chore(v1.2.1): create new release (#175)
1122aa5 
Merged PRs since last release:
- #165
- #174

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
@a-hilaly a-hilaly mentioned this pull request Jun 4, 2024
Closed
nnbu pushed a commit to nnbu/ack-ec2-controller that referenced this pull request Sep 18, 2024
@pcolazurdo
Add support for Security Group Adoption (aws-controllers-k8s#165)
0c04f83 
Issue #, if available: aws-controllers-k8s/community#1946

Description of changes:

The existing logic doesn't allow for adoption of existing SecurityGroups because spec.Name is a required property for creation but the aws SDK returns GroupName. The deepCopy in sdkFind then is not enough because of the different names. 
By copying SecurityGroup GroupName into spec.Name as part of the sdk_read_many_post_set_output we solve this issue.

This fix allows adoption of existing security groups. Tests have been successful in at least two test scenarios: adopting an existing SG and then modifying its properties, trying to adopt an invalid SG.
I've also ran a few additional tests with the modified controller to be sure that existing functionality didn't break:
* Create a new SG
* Modify this SG
* Delete this SG


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
nnbu pushed a commit to nnbu/ack-ec2-controller that referenced this pull request Sep 18, 2024
@vflaux
chore(v1.2.1): create new release (aws-controllers-k8s#175)
5b92b7c 
Merged PRs since last release:
- aws-controllers-k8s#165
- aws-controllers-k8s#174

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nnbu nnbu nnbu approved these changes

@a-hilaly a-hilaly a-hilaly approved these changes

@vijtrip2 vijtrip2 Awaiting requested review from vijtrip2

@jljaco jljaco Awaiting requested review from jljaco

@LikithaVemulapalli LikithaVemulapalli Awaiting requested review from LikithaVemulapalli

Assignees

@a-hilaly a-hilaly

Labels
approved lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@pcolazurdo @a-hilaly @nnbu