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

AWS add vpc-endpoint filter for vpc #5934

Merged

Conversation

anovis
Copy link
Contributor

@anovis anovis commented Jul 8, 2020

Adds vpc-endpoint filter for a vpc.

for example

            policies:
              - name: s3-vpc-endpoint-enabled
                resource: vpc
                filters:
                  - type: vpc-endpoint
                    key: ServiceName
                    value: com.amazonaws.us-east-1.s3

closes #5933

c7n/resources/vpc.py Outdated Show resolved Hide resolved
JohnHillegass
JohnHillegass previously approved these changes Jul 8, 2020
Copy link
Collaborator

@JohnHillegass JohnHillegass left a comment

Choose a reason for hiding this comment

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

lgtm!

@PratMis
Copy link
Collaborator

PratMis commented Jul 8, 2020

Looks like we can choose the subnets in a vpc to associate the vpc-endpoint to. For ex - If we have two subnets in a vpc, we can choose to grant one subnet access to the endpoint while blocking the other.
Feel like this filter should be at the subnet level than having it at the vpc level, since a vpc can have multiple subnets.

@JohnHillegass
Copy link
Collaborator

JohnHillegass commented Jul 8, 2020

Looks like we can choose the subnets in a vpc to associate the vpc-endpoint to. For ex - If we have two subnets in a vpc, we can choose to grant one subnet access to the endpoint while blocking the other.
Feel like this filter should be at the subnet level than having it at the vpc level, since a vpc can have multiple subnets.

@PratMis Fair point! I had an impression the original intention was to simply check if a VPC endpoint is connected to a VPC. I guess it depends on the implementation need. @anovis do you need to also look at the subnet level? I guess there could be a subtle distinction between all vpcs must have a s3 vpc endpoint attached and all subnets in a vpc must have a s3 vpc endpoint attached

@anovis
Copy link
Contributor Author

anovis commented Jul 8, 2020

yeah makes sense. i can make the same filter for the subnet resource as well.

c7n/resources/vpc.py Outdated Show resolved Hide resolved
@kapilt kapilt dismissed JohnHillegass’s stale review July 9, 2020 00:47

correctness issue

'vpc-endpoint',
rinherit=ValueFilter.schema)

def get_related_ids(self, resources):
Copy link
Collaborator

Choose a reason for hiding this comment

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

get related ids is generally intended to be a lightweight implementation, it gets called frequently, ie once per resource, doing an api call / or cache lookup isn't ideal for an implementation.

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 good call out. i had noticed that while writing the subnet tests.

Instead I modified the get_related and process_resource, but not sure if that is the best way.

@anovis anovis requested a review from kapilt July 14, 2020 14:22
@kapilt
Copy link
Collaborator

kapilt commented Aug 4, 2020

so rather than duplicating the implementation for vpc and subnet, it would be better to have a related base class that addresses on the commonality. its distinct from the extant related base since its filtering the related resource to determine the related ids instead of using the policy resource to determine the ids. that feels like it has some reuse potential.

Copy link
Collaborator

@kapilt kapilt left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

@kapilt kapilt merged commit f96502a into cloud-custodian:master Oct 6, 2020
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.

AWS - vpc-endpoint filter for vpc resource
4 participants