Skip to content

feat(vpc-lattice-alpha): initial ServiceNetwork and Service L2 constructs #35055

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vishaalmehrishi
Copy link
Contributor

@vishaalmehrishi vishaalmehrishi commented Jul 24, 2025

This is a WIP change - sharing to get early feedback

Issue # (if applicable)

Closes #.

Reason for this change

Initial implementation of the vpc-lattice-alpha L2 constructs.

RFC: https://github.com/aws/aws-cdk-rfcs/blob/main/text/0502_aws-vpclattice.md

The implementation details may differ from the RFC in some ways - I am using this exercise as a way to evaluate the L2 creation process and find potential improvements.

Description of changes

So far, just the main constructs: ServiceNetwork and Service. These are not complete either - I am continuing to work on them.

Describe any new or updated permissions being added

N/A

Description of how you validated changes

Wrote a couple of basic unit tests. I will add comprehensive testing once the constructs are in a near-final state.

Checklist


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

@aws-cdk-automation aws-cdk-automation requested a review from a team July 24, 2025 13:01
@github-actions github-actions bot added the p2 label Jul 24, 2025
@vishaalmehrishi vishaalmehrishi requested a review from mrgrain July 24, 2025 13:02
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jul 24, 2025
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter fails with the following errors:

❌ Features must contain a change to an integration test file and the resulting snapshot.

If you believe this pull request should receive an exemption, please comment and provide a justification. A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed, add Clarification Request to a comment.

this.serviceNetworkArn = resource.attrArn;
this.serviceNetworkId = resource.attrId;

// TODO: enforce at least one service? A service network without services is just...a network
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mrgrain thoughts on this? IMO enforcing this is good, because a service network should have at least one service. However, this is not enforced by CFN or the AWS Console UX when creating a service network.

https://docs.aws.amazon.com/vpc-lattice/latest/ug/create-service-network.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems overly prescriptive to me and would prevent common coding patterns like

const sn = new ServiceNetwork(scope, 'ServiceNetwork', { ... });

// do stuff

// somewhere completely different do
sn. associateServices(...);

/**
* Base class for VPC Lattice service networks
*/
export abstract class ServiceNetworkBase extends Resource implements IServiceNetwork {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Guidelines say not to export the base class, but there's no way around it if the base class is in a different file (which is not necessary, of course).

Is it a hard requirement not to expose the base class?

Copy link
Contributor

Choose a reason for hiding this comment

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

The important part will be that it's not exported from index.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that - without the index.ts export, the import in service-network.ts does not work (or I am doing something wrong).

DDB Tablev2 also exports the base class FYI:

export * from './table-v2-base';
(not saying this is correct, just pointing it out).

@vishaalmehrishi vishaalmehrishi added the pr/do-not-merge This PR should not be merged at this time. label Jul 24, 2025
Comment on lines +45 to +50
/**
* Tags for the service network
*
* @default - No tags
*/
readonly tags?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* Tags for the service network
*
* @default - No tags
*/
readonly tags?: string;
/**
* Tags for the service network
*
* @default - No tags
*/
readonly tags?: { [key: string]: string };

Required as per https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md#tags
But I don't think we actually do this in a lot of places. @rix0rrr might have a better idea what the actual latest recommendation is here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah thanks, I haven't really touched tags yet - forgot that I left that in :D. I will make the change.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 231075d
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository


// TODO: validation?

// TODO: tags
Copy link
Contributor

Choose a reason for hiding this comment

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

constructor(scope: Construct, id: string, props: ServiceNetworkProps) {
super(scope, id);

// TODO: validation?
Copy link
Contributor

Choose a reason for hiding this comment

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

Use validateAllProps helper. Extend mini-framework if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note, this is relatively new. We should add this to the design guide.

authType: props.authType,
name: props.serviceNetworkName,
sharingConfig: {
enabled: props.enableSharing ?? false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Side note: we probably should have a centralized way to apply defaults to input props.

*
* @default - No hosted zone ID
*/
readonly hostedZoneId?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not IHostedZone?

Should the whole of DnsEntry just be a hosted zone?

/**
* DNS entry information for a VPC Lattice service
*/
export interface DnsEntry {
Copy link
Contributor

@mrgrain mrgrain Jul 24, 2025

Choose a reason for hiding this comment

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

Side note: an interface with all optional props always raises questions for me. What happens if an empty object is provided? Is this a mutex?

* The VPC lattice service ID
*/
// TODO: restore ID? Constructing the ARN from the ID will need accountId, region, etc
// readonly serviceId: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

see other comment

* @returns An object representing the imported VPC Lattice service.
*/
public static fromServiceAttributes(scope: Construct, id: string, attrs: ServiceAttributes): IService {
class UnownedServiceReference extends ServiceBase {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! Unowned might be to technical, certainly love the Reference part. Whatever we land on, we should update the guide with this.

Comment on lines +201 to +203
if (props.authType === ServiceAuthType.AWS_IAM && props.authPolicy === undefined) {
throw new ValidationError('An auth policy must be provided when the authType is `AWS_IAM`', this);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

this tells me that this should be a single property with a more complex object:

  /**
   * The authentication type for the service
   *
   * @default ServiceAuth.NONE
   */
  readonly auth: ServiceAuth;
class ServiceAuth {
  public static NONE = new ServiceAuth('NONE');
  public static iam(policy: PolicyDocument): ServiceAuth {
    return new ServiceAuth('IAM', {
      authPolicy: policy,
    );
  }

  private constructor() { ... }
}

There are plenty of examples around this in the code base. But needs a better example in the design guidelines
https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md#enums

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr/do-not-merge This PR should not be merged at this time.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants