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.

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

Support Security Group Name #410

Open
wants to merge 3 commits into
base: sg-name
Choose a base branch
from
Open

Support Security Group Name #410

wants to merge 3 commits into from

Conversation

GnatorX
Copy link
Contributor

@GnatorX GnatorX commented Apr 18, 2024

  • sg name field in crd

  • add implementation

  • Move to getsgforVPC, move SG names into SG, unique items validation

  • Add TTL cache (15 minutes) to Security group name to ID calls to reduce calls to GetSecurityGroupForVPC


Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

* sg name field in crd

* add implementation

* Move to getsgforVPC, move SG names into SG, unique items validation

---------

Co-authored-by: Garvin Pang <[email protected]>
@GnatorX GnatorX marked this pull request as ready for review April 18, 2024 16:06
@GnatorX GnatorX requested a review from a team as a code owner April 18, 2024 16:06
@sushrk
Copy link
Contributor

sushrk commented Apr 24, 2024

Hi @GnatorX, are you planning to add TTL cache changes in the same PR?

@GnatorX
Copy link
Contributor Author

GnatorX commented Apr 25, 2024

I have added TTL cache. Sorry for the delay.

@GnatorX
Copy link
Contributor Author

GnatorX commented Apr 29, 2024

@sushrk

Copy link
Contributor

@sushrk sushrk 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 for the PR!
I had a few minor comments, but overall looks good. After merge, I can continue testing on this.

apis/vpcresources/v1beta1/securitygrouppolicy_types.go Outdated Show resolved Hide resolved
return securityGroup.securityGroupName, nil
}

securityGroupNameToIdCache := cache.NewTTLStore(securityGroupCacheKeyFunc, 15*time.Minute)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: The TTL duration could be made configurable by the user based on their use-case.

I'll discuss this internally with the team.

Choose a reason for hiding this comment

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

@vgrigoruk did this get discussed internally?

pkg/aws/ec2/api/wrapper.go Outdated Show resolved Hide resolved
pkg/utils/helper.go Outdated Show resolved Hide resolved
pkg/utils/setup_test.go Outdated Show resolved Hide resolved
@vgrigoruk
Copy link

Hi @GnatorX. Do you have time to follow up on the feedback provided? I'm very excited to see this feature in one of the next releases of AWS VPC CNI, so let me know if I can help picking it up from here.

@garvinp-stripe
Copy link

garvinp-stripe commented Jul 2, 2024

Ya I can do that. Sorry this is the same person as @GnatorX, sometimes I forget to switch between my accounts

@GnatorX
Copy link
Contributor Author

GnatorX commented Jul 2, 2024

All comments addressed. Just need to know if we are interested in exposing the cache ttl

@GnatorX
Copy link
Contributor Author

GnatorX commented Jul 30, 2024

@haouc Hey, is there someone from AWS side that can pick this back up?

@taer
Copy link

taer commented Sep 10, 2024

@GnatorX can you maybe add @haouc to the reviewers? and re-request @sushrk

I'd love to make all envs for my deploys look the same instead of the random sg-ids in each different env.

@GnatorX
Copy link
Contributor Author

GnatorX commented Sep 10, 2024

i dont think i have those powers. I know that sushrk is no longer at AWS however we would need someone else to pick this up from their side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants