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

Add Security Group lookup by name #4241

Closed
2 tasks
thesurlydev opened this issue Sep 25, 2019 · 30 comments · Fixed by #17246
Closed
2 tasks

Add Security Group lookup by name #4241

thesurlydev opened this issue Sep 25, 2019 · 30 comments · Fixed by #17246
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1

Comments

@thesurlydev
Copy link
Contributor

thesurlydev commented Sep 25, 2019

Similar to SecurityGroup.fromSecurityGroupId(), please add a SecurityGroup.fromSecurityGroupName.

Use Case

There are cases where existing SecurityGroups need to be referenced and have well-known names but not the ids. It would be nice to be able to look up SGs by name. Otherwise, a custom resource and/or SDK is needed to do the lookup.

Proposed Solution

Add a SecurityGroup.fromSecurityGroupName method.

Other

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@thesurlydev thesurlydev added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Sep 25, 2019
@SomayaB SomayaB added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Sep 25, 2019
@rix0rrr
Copy link
Contributor

rix0rrr commented Sep 26, 2019

We will not be able to make a security group from a name. We might be able to look one up, given CLI support.

@rix0rrr rix0rrr removed the needs-triage This issue or PR still needs to be triaged. label Sep 26, 2019
@thesurlydev
Copy link
Contributor Author

I just want to be able to look up an existing SG by name.

@tmo-trustpilot
Copy link

I have the same issue, I am trying to create an AWS Batch compute environment in an with existing subnets and security groups.

I am able to find the private subnets by looking up the VPC by name, but I have no way of finding the ids of the existing security groups.

const vpc = ec2.Vpc.fromLookup(this, "vpc", {
  vpcName: `${vpcName}`
});

const subnetIds = vpc.privateSubnets.map(sn => sn.subnetId);

const computeResourcesProperties: batch.CfnComputeEnvironment.ComputeResourcesProperty = {
    type: "EC2",
    ...snip...
    subnets: subnetIds,
    securityGroupIds: ???
};

Attempting to use a security group name seems to provision fine until the compute environment tries to add instances to the cluster: CLIENT_ERROR - Invalid value '<SNIP>' for groupName. You may not reference Amazon VPC security groups by name. Please use the corresponding id for this operation.

@rix0rrr rix0rrr added the effort/medium Medium work item – several days of effort label Jan 23, 2020
@rix0rrr rix0rrr added the p2 label Aug 12, 2020
@kulesha
Copy link

kulesha commented Sep 1, 2020

is this feature available yet ?
it would be super useful to us too ..
in our case we have one cdk script that deploys 'infrastructure' ( vpc, subnets, security groups etc .. )
and another one is creating spot fleets to run applications in this infrastructure ..
at the moment we have to hard code the security group id in the application script ..

@michal-ziarnik
Copy link

It would be useful to us too. We're trying to create Redshift cluster and having an option to do a lookup for existing Security Groups by name would be a grate help.

@levarne
Copy link

levarne commented Oct 26, 2020

nothing? Need to reference Security group per environment like I did with my existing VPC

cont vpc = Vpc.fromLookup(this, 'VPC', {
    vpcName: `dynamo-${environment}-app`,
})

Would be nice to do:

const groupWebAccess = SecurityGroup.fromLookup(this, 'Security-group-WebAccess', {
     groupName: `dynamo-${environment}-app-webAccess`
})
const groupProxyServer= SecurityGroup.fromLookup(this, 'Security-group-WebAccess', {
     groupName: `dynamo-${environment}-app-proxyServer`
})

new WebpackFunction(this, 'Metric', {
  vpc,
  securityGroups: [groupWebAccess, groupProxyServer]
})

But now I have to write each environments ID into a env file or configuration,
the above will allow me to change the security group without needing to redeploy code.

@calebpalmer
Copy link

I would also like this too. I'm trying to deploy a Simple AD and some workspaces, I need to update the ingress rules on the worker sg created by the simple ad and the only way to know if (I think) is by the name it creates for it, directoryId_workspaceMembers.

@felipepulpo
Copy link

+1

@daminisinha
Copy link

+1, need this feature please

@suryakiran7
Copy link

+1

6 similar comments
@suhas-hr
Copy link

suhas-hr commented Feb 4, 2021

+1

@laurabaste
Copy link

+1

@Prashanna313
Copy link

+1

@Amchien
Copy link

Amchien commented Mar 17, 2021

+1

@erikdebruin
Copy link

+1

@Nicoowr
Copy link

Nicoowr commented Mar 31, 2021

+1

@argenstijn
Copy link

+10

@argenstijn
Copy link

any process?

@rix0rrr rix0rrr removed their assignment Jun 3, 2021
@yukihato
Copy link

yukihato commented Jun 9, 2021

+1

1 similar comment
@ammonsabrinaPFG
Copy link

+1

@argenstijn
Copy link

@NetaNir
What is the status of this request?

@mmislam101
Copy link

mmislam101 commented Jul 28, 2021

+12

@james-bw
Copy link

+1

@njlynch njlynch added p1 and removed p2 labels Aug 24, 2021
@njlynch
Copy link
Contributor

njlynch commented Aug 24, 2021

Given the attention this issue has received, I'm correctly labeling it as a p1, which means it should be on our near-term roadmap.

We welcome community contributions! If you are able, we encourage you to contribute. If you decide to contribute, please start an engineering discussion in this issue to ensure there is a commonly understood design before submitting code. This will minimize the number of review cycles and get your code merged faster. Security group lookup by id was added in #11089, and would be a good inspiration for this work.

@jumic
Copy link
Contributor

jumic commented Sep 4, 2021

I would suggest to extend the existing logic from PR #11089 and introduce the additional properties securityGroupName and vpc.
From my point of view, the main topic which should be discussed is the interface in class SecurityGroup.

ec2 SecurityGroup
Method fromLookup() already exists.

public static fromLookup(scope: Construct, id: string, securityGroupId: string) {

Create a new method fromLookupAttributes() which can be used to lookup a SecurityGroup by name or id. (I'm not sure if this is a good name, actually I would prefere fromLookup() which is already in use.)

public static fromLookupAttributes(scope: Construct, id: string, options: SecurityGroupLookupOptions)

Move code from fromLookup() to this new method and use the new method in fromLookup().

The interface will allow a security group name or id (if both or non of them are specified, it will throw an error).

export interface SecurityGroupLookupOptions {
  readonly securityGroupName?: string;
  readonly securityGroupId?: string;
  readonly vpc?: IVpc;
}

Using this interface it is possible to extend the attributes later (e.g. lookup by tag).

(Alternative solution: implement method fromLookupName() with string-parameter securityGroupName similar to fromLookup() which has a string-parameter, too.) (Not recommended anymore after adding attribute vpc).

SecurityGroupContextQuery
In SecurityGroupContextQuery change variable securityGroupId to optional and add optional variables securityGroupName and vpcId.

SecurityGroupContextProviderPlugin
In SecurityGroupContextProviderPlugin add additional parameters GroupNames and vpc-id filter to describeSecurityGroups(). It is possible to query the security group by name using this SDK method. Furthermore, check if both id and name or neither id nor name will be passed and throw an error in this case.

@njlynch I would be happy to have your feedback.

Enhancement to original version:
I added parameter vpc (vpcId). The same security group name can be used in multiple VPCs. For example, each VPC has a default security group named default. In this case, parameter vpc has to be specified. Otherwise, it is not possible to identify the correct security group.

@Arthus15
Copy link

Arthus15 commented Oct 4, 2021

Any update on this topic?

@ranma2913
Copy link

Requesting this be looked at for my group too!

@ranma2913
Copy link

I would suggest to extend the existing logic from PR #11089 and introduce the additional property securityGroupName. From my point of view, the main topic which should be discussed is the interface in class SecurityGroup.

ec2 SecurityGroup Method fromLookup() already exists.

public static fromLookup(scope: Construct, id: string, securityGroupId: string) {

Create a new method fromLookupAttributes() which can be used to lookup a SecurityGroup by name or id. (I'm not sure if this is a good name, actually I would prefere fromLookup() which is already in use.)

public static fromLookupAttributes(scope: Construct, id: string, options: SecurityGroupLookupOptions)

Move code from fromLookup() to this new method and use the new method in fromLookup().

The interface will allow a security group name or id (if both or non of them are specified, it will throw an error).

export interface SecurityGroupLookupOptions {
  readonly securityGroupName?: string;
  readonly securityGroupId?: string;
}

Using this interface it is possible to extend the attributes later (e.g. lookup by tag).

(Alternative solution: implement method fromLookupName() with string-parameter securityGroupName similar to fromLookup() which has a string-parameter, too.)

SecurityGroupContextQuery In SecurityGroupContextQuery change variable securityGroupId to optional and add optional variable securityGroupName.

SecurityGroupContextProviderPlugin In SecurityGroupContextProviderPlugin add additional parameter GroupNames to describeSecurityGroups(). It is possible to query the security group by name using this SDK method. Furthermore, check if both id and name or neither id nor name will be passed and throw an error in this case.

@njlynch I would be happy to have your feedback.

I'll use this right away! any idea when it might go in?

@njlynch
Copy link
Contributor

njlynch commented Oct 25, 2021

@jumic - Thanks for the proposed solution, and apologies for the delay in commenting.

Everything you've described sounds good.

In SecurityGroupContextProviderPlugin add additional parameters GroupNames and vpc-id filter to describeSecurityGroups(). It is possible to query the security group by name using this SDK method. Furthermore, check if both id and name or neither id nor name will be passed and throw an error in this case.

Note that per the documentation, it appears we'll need to use the filters property to filter on group name and/or vpc; using the groupNames property directly will only work with the default VPC.

I look forward to seeing the PR!

@mergify mergify bot closed this as completed in #17246 Nov 16, 2021
mergify bot pushed a commit that referenced this issue Nov 16, 2021
Support looking up a security group by name.

Currently, looking up a security group is only possible by ID. This PR enhances the existing implementation to support lookup by security group name.

`securityGroupName` or `securityGroupId` can be passed to the new method `SecurityGroup.fromLookupAttributes`. In addition, property `vpc` provides the option to restrict the lookup method to a specific VPC.

If no or more than one security group is found, an error is thrown.

Closes #4241.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

mpvosseller pushed a commit to mpvosseller/aws-cdk that referenced this issue Nov 16, 2021
Support looking up a security group by name.

Currently, looking up a security group is only possible by ID. This PR enhances the existing implementation to support lookup by security group name.

`securityGroupName` or `securityGroupId` can be passed to the new method `SecurityGroup.fromLookupAttributes`. In addition, property `vpc` provides the option to restrict the lookup method to a specific VPC.

If no or more than one security group is found, an error is thrown.

Closes aws#4241.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this issue Feb 21, 2022
Support looking up a security group by name.

Currently, looking up a security group is only possible by ID. This PR enhances the existing implementation to support lookup by security group name.

`securityGroupName` or `securityGroupId` can be passed to the new method `SecurityGroup.fromLookupAttributes`. In addition, property `vpc` provides the option to restrict the lookup method to a specific VPC.

If no or more than one security group is found, an error is thrown.

Closes aws#4241.

----

*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
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p1
Projects
None yet