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-ec2): VpcEndpointService allowedPrincipals has type of ArnPrincipal[], but should also support the ServicePrincipal type #29478

Closed
7e11 opened this issue Mar 14, 2024 · 6 comments · Fixed by #29512
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/medium Medium work item – several days of effort p2

Comments

@7e11
Copy link
Contributor

7e11 commented Mar 14, 2024

Describe the bug

VpcEndpointService has the member allowedPrincipals which is of type ArnPrincipal[]. However, if you use the AWS console, allowlisting a service principal is supported as well. Users are not able to use the type ServicePrincipal in allowedPrincipals in CDK. This is a feature gap.

packages/aws-cdk-lib/aws-ec2/lib/vpc-endpoint-service.ts

Expected Behavior

VpcEndpointService allowedPrincipals should support ArnPrincipal as well as ServicePrincipal.

Current Behavior

VpcEndpointService allowedPrincipals only supports ArnPrincipal.

Reproduction Steps

const privateLinkVPCEService = new VpcEndpointService(this, 'PrivateLinkVPCEService', {
  vpcEndpointServiceLoadBalancers: [privateLinkNlb],
  acceptanceRequired: false,
  allowedPrincipals: [new ServicePrincipal('someservice.aws.internal'),  // <-- type error. Only ArnPrincipal[] allowed.
});

Possible Solution

Modify the VpcEndpointService construct to support ServicePrincipal as well as ArnPrincipal for the member allowedPrincipals

Additional Information/Context

For context, this is an AWS internal service which is onboarding to privatelink, and only wants to allowlist specific service principals. This is possible in the console, but not in CDK.


There is a workaround I've been using, but it's a little janky. It's possible to pass in a service principal to the allowedPrincipals by wrapping it in the ArnPrincipal type. For example:

new ArnPrincipal('someservice.aws.internal');

You could also use L1 constructs, but that gets painful if you're using VpcEndpointServiceDomainName, because there is no corresponding L1 construct and it's nontrivial.

CDK CLI Version

2.130.0 (build bd6e5ee)

Framework Version

2.130.0

Node.js Version

v16.7.0

OS

macOS Ventura 13.6.4

Language

TypeScript

Language Version

No response

Other information

No response

@7e11 7e11 added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Mar 14, 2024
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Mar 14, 2024
@7e11 7e11 changed the title (aws-ec2): VpcEndpointService allowedPrincipals has type of ArnPrincipal[], but should also support ServicePrincipal (aws-ec2): VpcEndpointService allowedPrincipals has type of ArnPrincipal[], but should also support the ServicePrincipal type Mar 14, 2024
@7e11
Copy link
Contributor Author

7e11 commented Mar 14, 2024

This is what the VPCE principal allowlisting page looks like in the console. It also claims to only accept ARNs, but service principals also work.
VPCE Allow Principals

@7e11
Copy link
Contributor Author

7e11 commented Mar 14, 2024

Here's a redacted service principle in the "Allow Principles" list after adding it in the above UI. The type is listed as Service, instead of Arn.
VPCE Allow principals list

@pahud
Copy link
Contributor

pahud commented Mar 15, 2024

The principals passed in will generate AllowedPrincipals in VPCEndpointServicePermissions, which literally accept any format of principals.

new CfnVPCEndpointServicePermissions(this, 'Permissions', {
serviceId: this.endpointService.ref,
allowedPrincipals: this.allowedPrincipals.map(x => x.arn),
});

We can't make it either ArnPrincipal type or ServicePrincipal type in CDK. I guess we need to document what we should do when we are assigning a service principal to it. And it could be the workaround you provided.

I'll request the team for more input on this. And let me know if you have any further thoughts.

@pahud pahud added p2 effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Mar 15, 2024
@GavinZZ
Copy link
Contributor

GavinZZ commented Mar 15, 2024

Hi @7e11, after investigation, I think you're right that although the documentation specifically mentions arn principal, the console doesn't stop you from using service principals. I agree with Pahud's suggestion on adding the workaround to README documentation.

Since this is an issue with potential workaround, I think marking this issue as p2 is reasonable here, and unfortunately we won't be able to prioritize working on a p2 issue. Meanwhile we welcome community contribution and it will be appreciated if you'll be able to make the documentation update.

@7e11
Copy link
Contributor Author

7e11 commented Mar 15, 2024

Gotcha -- so because using SPs is a bit of an undocumented feature, we can't alter the construct to accept other principal types. I'll update the readme soon.

@mergify mergify bot closed this as completed in #29512 Mar 21, 2024
mergify bot pushed a commit that referenced this issue Mar 21, 2024
…service principal in VPCEService `allowedPrincipals` (#29512)

`VpcEndpointService` has the member `allowedPrincipals` which is of type `ArnPrincipal[]`. However, `ServicePrincipal` is also valid and works in the AWS console. This documentation update includes a workaround for including service principals in the `allowedPrincipals`.

### Issue #29478

Closes #29478

### Reason for this change

`VpcEndpointService` has the member `allowedPrincipals` which is of type `ArnPrincipal[]`. However, if you use the AWS console, allowlisting a service principal is supported as well. Users are not able to use the type `ServicePrincipal` in `allowedPrincipals` in CDK. This is a feature gap.

I brought this up in #29478, and was told that the type couldn't be changed, but the workaround I was using could be added to the documentation.

### Description of changes

Documentation update for the `aws-ec2` module which includes a workaround for including service principals in the `allowedPrincipals`.

### Description of how you validated changes

N/A - minor documentation changes only

### Checklist
- [X] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
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.

jun1-t pushed a commit to jun1-t/aws-cdk that referenced this issue Mar 23, 2024
…service principal in VPCEService `allowedPrincipals` (aws#29512)

`VpcEndpointService` has the member `allowedPrincipals` which is of type `ArnPrincipal[]`. However, `ServicePrincipal` is also valid and works in the AWS console. This documentation update includes a workaround for including service principals in the `allowedPrincipals`.

### Issue aws#29478

Closes aws#29478

### Reason for this change

`VpcEndpointService` has the member `allowedPrincipals` which is of type `ArnPrincipal[]`. However, if you use the AWS console, allowlisting a service principal is supported as well. Users are not able to use the type `ServicePrincipal` in `allowedPrincipals` in CDK. This is a feature gap.

I brought this up in aws#29478, and was told that the type couldn't be changed, but the workaround I was using could be added to the documentation.

### Description of changes

Documentation update for the `aws-ec2` module which includes a workaround for including service principals in the `allowedPrincipals`.

### Description of how you validated changes

N/A - minor documentation changes only

### Checklist
- [X] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
ahammond pushed a commit to ahammond/aws-cdk that referenced this issue Mar 26, 2024
…service principal in VPCEService `allowedPrincipals` (aws#29512)

`VpcEndpointService` has the member `allowedPrincipals` which is of type `ArnPrincipal[]`. However, `ServicePrincipal` is also valid and works in the AWS console. This documentation update includes a workaround for including service principals in the `allowedPrincipals`.

### Issue aws#29478

Closes aws#29478

### Reason for this change

`VpcEndpointService` has the member `allowedPrincipals` which is of type `ArnPrincipal[]`. However, if you use the AWS console, allowlisting a service principal is supported as well. Users are not able to use the type `ServicePrincipal` in `allowedPrincipals` in CDK. This is a feature gap.

I brought this up in aws#29478, and was told that the type couldn't be changed, but the workaround I was using could be added to the documentation.

### Description of changes

Documentation update for the `aws-ec2` module which includes a workaround for including service principals in the `allowedPrincipals`.

### Description of how you validated changes

N/A - minor documentation changes only

### Checklist
- [X] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*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 bug This issue is a bug. effort/medium Medium work item – several days of effort p2
Projects
None yet
3 participants