Skip to content

Commit

Permalink
fix(opensearch): Don't throw a Az count mismatch for imported VPCs (#…
Browse files Browse the repository at this point in the history
…22654)

Solving issue #22651 

Creating a domain fails for imported vpc/subnets when zone awareness is enabled and the cdk context is cleared.
When the CDK context is already retrieved and the VPC is in the context, the deployment works.

This is due to the fact that when there is no context yet, the subnet count is always 0. That's why I decided to disable it. If it's not correct, it will fail when applying the CloudFormation template.

----

### All Submissions:

* [X] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
stijnbrouwers committed Dec 12, 2022
1 parent 813c2f1 commit 6a28b7f
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 4 deletions.
29 changes: 26 additions & 3 deletions packages/@aws-cdk/aws-opensearchservice/lib/domain.ts
Expand Up @@ -1235,8 +1235,11 @@ export class Domain extends DomainBase implements IDomain, ec2.IConnectable {
let securityGroups: ec2.ISecurityGroup[] | undefined;
let subnets: ec2.ISubnet[] | undefined;

let skipZoneAwarenessCheck: boolean = false;
if (props.vpc) {
subnets = selectSubnets(props.vpc, props.vpcSubnets ?? [{ subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS }]);
const subnetSelections = props.vpcSubnets ?? [{ subnetType: ec2.SubnetType.PRIVATE_WITH_EGRESS }];
subnets = selectSubnets(props.vpc, subnetSelections);
skipZoneAwarenessCheck = zoneAwarenessCheckShouldBeSkipped(props.vpc, subnetSelections);
securityGroups = props.securityGroups ?? [new ec2.SecurityGroup(this, 'SecurityGroup', {
vpc: props.vpc,
description: `Security group for domain ${this.node.id}`,
Expand All @@ -1248,8 +1251,12 @@ export class Domain extends DomainBase implements IDomain, ec2.IConnectable {
}
}

// If VPC options are supplied ensure that the number of subnets matches the number AZ
if (subnets && zoneAwarenessEnabled && new Set(subnets.map((subnet) => subnet.availabilityZone)).size < availabilityZoneCount) {
// If VPC options are supplied ensure that the number of subnets matches the number AZ (only if the vpc is not imported from another stack)
if (subnets &&
zoneAwarenessEnabled &&
!skipZoneAwarenessCheck &&
new Set(subnets.map((subnet) => subnet.availabilityZone)).size < availabilityZoneCount
) {
throw new Error('When providing vpc options you need to provide a subnet for each AZ you are using');
}

Expand Down Expand Up @@ -1776,6 +1783,22 @@ function selectSubnets(vpc: ec2.IVpc, vpcSubnets: ec2.SubnetSelection[]): ec2.IS
return selected;
}

/**
* Check if any of the subnets are pending lookups. If so, the zone awareness check should be skipped, otherwise it will always throw an error
*
* @param vpc The vpc to which the subnets apply
* @param vpcSubnets The vpc subnets that should be checked
* @returns true if there are pending lookups for the subnets
*/
function zoneAwarenessCheckShouldBeSkipped(vpc: ec2.IVpc, vpcSubnets: ec2.SubnetSelection[]): boolean {
for (const selection of vpcSubnets) {
if (vpc.selectSubnets(selection).isPendingLookup) {
return true;
};
}
return false;
}

/**
* Initializes an instance type.
*
Expand Down
23 changes: 22 additions & 1 deletion packages/@aws-cdk/aws-opensearchservice/test/domain.test.ts
Expand Up @@ -2,7 +2,7 @@
import { Match, Template } from '@aws-cdk/assertions';
import * as acm from '@aws-cdk/aws-certificatemanager';
import { Metric, Statistic } from '@aws-cdk/aws-cloudwatch';
import { Vpc, EbsDeviceVolumeType, Port, SecurityGroup } from '@aws-cdk/aws-ec2';
import { Vpc, EbsDeviceVolumeType, Port, SecurityGroup, SubnetType } from '@aws-cdk/aws-ec2';
import * as iam from '@aws-cdk/aws-iam';
import * as kms from '@aws-cdk/aws-kms';
import * as logs from '@aws-cdk/aws-logs';
Expand Down Expand Up @@ -1406,6 +1406,27 @@ describe('custom error responses', () => {
})).toThrow(/you need to provide a subnet for each AZ you are using/);
});

test('Imported VPC with subnets that are still pending lookup won\'t throw Az count mismatch', () => {
const vpc = Vpc.fromLookup(stack, 'ExampleVpc', {
vpcId: 'vpc-123',
});
let subnets = vpc.selectSubnets({
subnetType: SubnetType.PRIVATE_WITH_EGRESS,
});

new Domain(stack, 'Domain1', {
version: defaultVersion,
vpc,
vpcSubnets: [subnets],
zoneAwareness: {
enabled: true,
availabilityZoneCount: 3,
},
});

Template.fromStack(stack).resourceCountIs('AWS::OpenSearchService::Domain', 1);
});

test('error when master, data or Ultra Warm instance types do not end with .search', () => {
const error = /instance types must end with ".search"/;
expect(() => new Domain(stack, 'Domain1', {
Expand Down

0 comments on commit 6a28b7f

Please sign in to comment.