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

fix(opensearch): Don't throw a Az count mismatch for imported VPCs #22654

Merged
merged 17 commits into from Dec 12, 2022
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
5ca1bdc
bugfix(opensearch): Don't throw a Az count mismatch when using import…
stijnbrouwers Oct 26, 2022
caaeed3
Merge branch 'main' into bugfix/opensearch-domain-imported-vpc
stijnbrouwers Oct 26, 2022
059269c
fix(opensearch): Add ingegration test for imported vpc
stijnbrouwers Oct 26, 2022
3fd2d28
bugfix(opensearch): Fix integration test for imported vpc
stijnbrouwers Oct 26, 2022
64a9ea5
Merge branch 'main' into bugfix/opensearch-domain-imported-vpc
stijnbrouwers Nov 14, 2022
f67bc43
bugfix(opensearch): Fix integration test for imported vpc
stijnbrouwers Nov 16, 2022
497c2d5
Merge branch 'main' into bugfix/opensearch-domain-imported-vpc
stijnbrouwers Nov 16, 2022
8e95142
Merge branch 'main' into bugfix/opensearch-domain-imported-vpc
stijnbrouwers Nov 16, 2022
fce9d31
Merge branch 'main' into bugfix/opensearch-domain-imported-vpc
stijnbrouwers Nov 17, 2022
9cba3ba
Merge branch 'master' into bugfix/opensearch-domain-imported-vpc
stijnbrouwers Dec 10, 2022
b1fec5b
bugfix(opensearch-imported-vpc): Apply small (semantic) changes
stijnbrouwers Dec 10, 2022
274cbdf
Apply suggestions from code review
mrgrain Dec 12, 2022
6955389
Merge branch 'main' into bugfix/opensearch-domain-imported-vpc
mergify[bot] Dec 12, 2022
eaffe25
bugfix(opensearchservice): Fix build warning
stijnbrouwers Dec 12, 2022
7fc6894
bugfix(opensearchservice): Add integ test dependency
stijnbrouwers Dec 12, 2022
4fd65e4
bugfix(opensearchservice): Remove failing integ test due to no suppor…
stijnbrouwers Dec 12, 2022
dafd14d
Merge branch 'main' into bugfix/opensearch-domain-imported-vpc
mergify[bot] Dec 12, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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;
stijnbrouwers marked this conversation as resolved.
Show resolved Hide resolved
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,0 +1,20 @@
{
"version": "21.0.0",
"files": {
"b723f0bed9ea9a4e8d237408e0debfa443e0a646b76eeeab26215935a74297bf": {
"source": {
"path": "StackWithVpc.template.json",
"packaging": "file"
},
"destinations": {
"12345678-test-region": {
"bucketName": "cdk-hnb659fds-assets-12345678-test-region",
"objectKey": "b723f0bed9ea9a4e8d237408e0debfa443e0a646b76eeeab26215935a74297bf.json",
"region": "test-region",
"assumeRoleArn": "arn:${AWS::Partition}:iam::12345678:role/cdk-hnb659fds-file-publishing-role-12345678-test-region"
}
}
}
},
"dockerImages": {}
}