-
Notifications
You must be signed in to change notification settings - Fork 0
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
OpenSearch Service CDK setup #14
Changes from 6 commits
2451ab7
be3be3b
535f4ed
97dd8e4
1cac51d
55d140f
525118b
b887998
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
*.js | ||
!jest.config.js | ||
*.d.ts | ||
node_modules | ||
|
||
# CDK asset staging directory | ||
.cdk.staging | ||
cdk.out |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
*.ts | ||
!*.d.ts | ||
|
||
# CDK asset staging directory | ||
.cdk.staging | ||
cdk.out |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
#!/usr/bin/env node | ||
import 'source-map-support/register'; | ||
import {App} from 'aws-cdk-lib'; | ||
import {StackComposer} from "../lib/stack-composer"; | ||
|
||
const app = new App(); | ||
const stage = process.env.CDK_DEPLOYMENT_STAGE | ||
const account = process.env.CDK_DEFAULT_ACCOUNT | ||
const region = process.env.CDK_DEFAULT_REGION | ||
new StackComposer(app, { | ||
env: { account: account, region: region }, | ||
stackName: `OpenSearchServiceDomainCDKStack-${stage}-${region}`, | ||
description: "This stack contains resources to create/manage an OpenSearch Service domain" | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
{ | ||
"app": "npx ts-node --prefer-ts-exts bin/app.ts", | ||
"watch": { | ||
"include": [ | ||
"**" | ||
], | ||
"exclude": [ | ||
"README.md", | ||
"cdk*.json", | ||
"**/*.d.ts", | ||
"**/*.js", | ||
"tsconfig.json", | ||
"package*.json", | ||
"yarn.lock", | ||
"node_modules", | ||
"test" | ||
] | ||
}, | ||
"context": { | ||
"@aws-cdk/aws-lambda:recognizeLayerVersion": true, | ||
"@aws-cdk/core:checkSecretUsage": true, | ||
"@aws-cdk/core:target-partitions": [ | ||
"aws", | ||
"aws-cn" | ||
], | ||
"@aws-cdk-containers/ecs-service-extensions:enableDefaultLogDriver": true, | ||
"@aws-cdk/aws-ec2:uniqueImdsv2TemplateName": true, | ||
"@aws-cdk/aws-ecs:arnFormatIncludesClusterName": true, | ||
"@aws-cdk/aws-iam:minimizePolicies": true, | ||
"@aws-cdk/core:validateSnapshotRemovalPolicy": true, | ||
"@aws-cdk/aws-codepipeline:crossAccountKeyAliasStackSafeResourceName": true, | ||
"@aws-cdk/aws-s3:createDefaultLoggingPolicy": true, | ||
"@aws-cdk/aws-sns-subscriptions:restrictSqsDescryption": true, | ||
"@aws-cdk/aws-apigateway:disableCloudWatchRole": true, | ||
"@aws-cdk/core:enablePartitionLiterals": true, | ||
"@aws-cdk/aws-events:eventsTargetQueueSameAccount": true, | ||
"@aws-cdk/aws-iam:standardizedServicePrincipals": true, | ||
"@aws-cdk/aws-ecs:disableExplicitDeploymentControllerForCircuitBreaker": true, | ||
"@aws-cdk/aws-iam:importedRoleStackSafeDefaultPolicyName": true, | ||
"@aws-cdk/aws-s3:serverAccessLogsUseBucketPolicy": true, | ||
"@aws-cdk/aws-route53-patters:useCertificate": true, | ||
"@aws-cdk/customresources:installLatestAwsSdkDefault": false | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
{ | ||
"engineVersion": "OS_1.3", | ||
"domainName": "cdk-os-service-domain" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the domain name is region- or account-unique, then we probably should embed the region/stage name into it. Also, we probably don't want to embed "CDK" into the name but open to it if you see value in that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely agree "cdk" not needed here and we should have a way to give region/stage here. Do you mind if I put this in a later task? I want to make it so this can be for any resource and still be embedded in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
module.exports = { | ||
testEnvironment: 'node', | ||
roots: ['<rootDir>/test'], | ||
testMatch: ['**/*.test.ts'], | ||
transform: { | ||
'^.+\\.tsx?$': 'ts-jest' | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,100 @@ | ||
import { Construct } from 'constructs'; | ||
import {EbsDeviceVolumeType, IVpc, Vpc} from "aws-cdk-lib/aws-ec2"; | ||
import {Domain, EngineVersion, TLSSecurityPolicy} from "aws-cdk-lib/aws-opensearchservice"; | ||
import {RemovalPolicy, SecretValue, Stack, StackProps} from "aws-cdk-lib"; | ||
import {IKey, Key} from "aws-cdk-lib/aws-kms"; | ||
import {PolicyStatement} from "aws-cdk-lib/aws-iam"; | ||
import {ILogGroup, LogGroup} from "aws-cdk-lib/aws-logs"; | ||
import {Secret} from "aws-cdk-lib/aws-secretsmanager"; | ||
|
||
|
||
export interface opensearchServiceDomainCdkProps extends StackProps{ | ||
readonly version: EngineVersion, | ||
readonly domainName: string, | ||
readonly dataNodeInstanceType?: string, | ||
readonly dataNodes?: number, | ||
readonly dedicatedManagerNodeType?: string, | ||
readonly dedicatedManagerNodeCount?: number, | ||
readonly warmInstanceType?: string, | ||
readonly warmNodes?: number | ||
readonly accessPolicies?: PolicyStatement[], | ||
readonly useUnsignedBasicAuth?: boolean, | ||
readonly fineGrainedManagerUserARN?: string, | ||
readonly fineGrainedManagerUserName?: string, | ||
readonly fineGrainedManagerUserSecretManagerKeyARN?: string, | ||
readonly enforceHTTPS?: boolean, | ||
readonly tlsSecurityPolicy?: TLSSecurityPolicy, | ||
readonly ebsEnabled?: boolean, | ||
readonly ebsIops?: number, | ||
readonly ebsVolumeSize?: number, | ||
readonly ebsVolumeType?: EbsDeviceVolumeType, | ||
readonly encryptionAtRestEnabled?: boolean, | ||
readonly encryptionAtRestKmsKeyARN?: string, | ||
readonly appLogEnabled?: boolean, | ||
readonly appLogGroup?: string, | ||
readonly nodeToNodeEncryptionEnabled?: boolean, | ||
readonly vpcId?: string, | ||
readonly domainRemovalPolicy?: RemovalPolicy | ||
} | ||
|
||
|
||
export class OpensearchServiceDomainCdkStack extends Stack { | ||
constructor(scope: Construct, id: string, props: opensearchServiceDomainCdkProps) { | ||
super(scope, id, props); | ||
|
||
// The code that defines your stack goes here | ||
|
||
// Retrieve existing account resources if defined | ||
const earKmsKey: IKey|undefined = props.encryptionAtRestKmsKeyARN && props.encryptionAtRestEnabled ? | ||
Key.fromKeyArn(this, "earKey", props.encryptionAtRestKmsKeyARN) : undefined | ||
Comment on lines
+48
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we creating this if it's undefined? This seems like something folks will want out of the box. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Be default from CDK, this isn't enabled There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per our discussion, we ended up agreeing to go with the resource API defaults for the time being until we develop our own, evidence-based opinions on a "best practice" implementation. |
||
|
||
const managerUserSecret: SecretValue|undefined = props.fineGrainedManagerUserSecretManagerKeyARN ? | ||
Secret.fromSecretCompleteArn(this, "managerSecret", props.fineGrainedManagerUserSecretManagerKeyARN).secretValue : undefined | ||
|
||
const appLG: ILogGroup|undefined = props.appLogGroup && props.appLogEnabled ? | ||
LogGroup.fromLogGroupArn(this, "appLogGroup", props.appLogGroup) : undefined | ||
Comment on lines
+54
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment - seems like something folks will want out of the box. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Be default from CDK, this isn't enabled |
||
|
||
const vpc: IVpc|undefined = props.vpcId ? | ||
Vpc.fromLookup(this, "domainVPC", {vpcId: props.vpcId}) : undefined | ||
Comment on lines
+57
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if the VPC is left There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Be default no VPC is created, but an existing VPC can be specified in the context There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Per discussion - if left undefined, the deployment of the CFN resources will still go through without error. What will happen is the Domain will be created in an OpenSearch Service managed account/VPC instead of one owned by the user. This is the default experience provided by the resource API. |
||
|
||
|
||
const domain = new Domain(this, 'Domain', { | ||
version: props.version, | ||
domainName: props.domainName, | ||
accessPolicies: props.accessPolicies, | ||
useUnsignedBasicAuth: props.useUnsignedBasicAuth, | ||
capacity: { | ||
dataNodeInstanceType: props.dataNodeInstanceType, | ||
dataNodes: props.dataNodes, | ||
masterNodeInstanceType: props.dedicatedManagerNodeType, | ||
masterNodes: props.dedicatedManagerNodeCount, | ||
warmInstanceType: props.warmInstanceType, | ||
warmNodes: props.warmNodes | ||
}, | ||
fineGrainedAccessControl: { | ||
masterUserArn: props.fineGrainedManagerUserARN, | ||
masterUserName: props.fineGrainedManagerUserName, | ||
masterUserPassword: managerUserSecret | ||
}, | ||
nodeToNodeEncryption: props.nodeToNodeEncryptionEnabled, | ||
encryptionAtRest: { | ||
enabled: props.encryptionAtRestEnabled, | ||
kmsKey: earKmsKey | ||
}, | ||
enforceHttps: props.enforceHTTPS, | ||
tlsSecurityPolicy: props.tlsSecurityPolicy, | ||
ebs: { | ||
enabled: props.ebsEnabled, | ||
iops: props.ebsIops, | ||
volumeSize: props.ebsVolumeSize, | ||
volumeType: props.ebsVolumeType | ||
}, | ||
logging: { | ||
appLogEnabled: props.appLogEnabled, | ||
appLogGroup: appLG | ||
}, | ||
vpc: vpc, | ||
removalPolicy: props.domainRemovalPolicy | ||
}); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,147 @@ | ||
import {Construct} from "constructs"; | ||
import {RemovalPolicy, Stack, StackProps} from "aws-cdk-lib"; | ||
import {OpensearchServiceDomainCdkStack} from "./opensearch-service-domain-cdk-stack"; | ||
import {EngineVersion, TLSSecurityPolicy} from "aws-cdk-lib/aws-opensearchservice"; | ||
import {EbsDeviceVolumeType} from "aws-cdk-lib/aws-ec2"; | ||
import {PolicyStatement} from "aws-cdk-lib/aws-iam"; | ||
import * as defaultValuesJson from "../default-values.json" | ||
|
||
export class StackComposer { | ||
public stacks: Stack[] = []; | ||
|
||
constructor(scope: Construct, props: StackProps) { | ||
|
||
let version: EngineVersion | ||
|
||
const defaultValues: { [x: string]: (string); } = defaultValuesJson | ||
const domainName = getContextForType('domainName', 'string') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if the field isn't defined (e.g. the line isn't in the context file or command-line args)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add a comment explaining that, or better yet make it clear in-code? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should clear up in next revision |
||
const dataNodeType = getContextForType('dataNodeType', 'string') | ||
const dataNodeCount = getContextForType('dataNodeCount', 'number') | ||
const dedicatedManagerNodeType = getContextForType('dedicatedManagerNodeType', 'string') | ||
const dedicatedManagerNodeCount = getContextForType('dedicatedManagerNodeCount', 'number') | ||
const warmNodeType = getContextForType('warmNodeType', 'string') | ||
const warmNodeCount = getContextForType('warmNodeCount', 'number') | ||
const useUnsignedBasicAuth = getContextForType('useUnsignedBasicAuth', 'boolean') | ||
const fineGrainedManagerUserARN = getContextForType('fineGrainedManagerUserARN', 'string') | ||
const fineGrainedManagerUserName = getContextForType('fineGrainedManagerUserName', 'string') | ||
const fineGrainedManagerUserSecretManagerKeyARN = getContextForType('fineGrainedManagerUserSecretManagerKeyARN', 'string') | ||
const enforceHTTPS = getContextForType('enforceHTTPS', 'boolean') | ||
const ebsEnabled = getContextForType('ebsEnabled', 'boolean') | ||
const ebsIops = getContextForType('ebsIops', 'number') | ||
const ebsVolumeSize = getContextForType('ebsVolumeSize', 'number') | ||
const encryptionAtRestEnabled = getContextForType('encryptionAtRestEnabled', 'boolean') | ||
const encryptionAtRestKmsKeyARN = getContextForType("encryptionAtRestKmsKeyARN", 'string') | ||
const loggingAppLogEnabled = getContextForType('loggingAppLogEnabled', 'boolean') | ||
const loggingAppLogGroupARN = getContextForType('loggingAppLogGroupARN', 'string') | ||
const noneToNodeEncryptionEnabled = getContextForType('nodeToNodeEncryptionEnabled', 'boolean') | ||
const vpcId = getContextForType('vpcId', 'string') | ||
|
||
if (!domainName) { | ||
throw new Error("Domain name is not present and is a required field") | ||
} | ||
|
||
const engineVersion = getContextForType('engineVersion', 'string') | ||
if (engineVersion && engineVersion.startsWith("OS_")) { | ||
// Will accept a period delimited version string (i.e. 1.3) and return a proper EngineVersion | ||
version = EngineVersion.openSearch(engineVersion.substring(3)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what happens if I pass in something undefined like OS_99.99? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It will get passed to CFN, as it is valid format, however |
||
} | ||
else if (engineVersion && engineVersion.startsWith("ES_")) { | ||
version = EngineVersion.elasticsearch(engineVersion.substring(3)) | ||
} | ||
else { | ||
throw new Error("Engine version is not present or does not match the expected format, i.e. OS_1.3 or ES_7.9") | ||
} | ||
|
||
const accessPolicyJson = getContextForType('accessPolicies', 'object') | ||
const accessPolicies = accessPolicyJson ? parseAccessPolicies(accessPolicyJson) : undefined | ||
|
||
const tlsSecurityPolicyName = getContextForType('tlsSecurityPolicy', 'string') | ||
const tlsSecurityPolicy: TLSSecurityPolicy|undefined = tlsSecurityPolicyName ? TLSSecurityPolicy[tlsSecurityPolicyName as keyof typeof TLSSecurityPolicy] : undefined | ||
if (tlsSecurityPolicyName && !tlsSecurityPolicy) { | ||
throw new Error("Provided tlsSecurityPolicy does not match a selectable option, for reference https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_opensearchservice.TLSSecurityPolicy.html") | ||
} | ||
|
||
const ebsVolumeTypeName = getContextForType('ebsVolumeType', 'string') | ||
const ebsVolumeType: EbsDeviceVolumeType|undefined = ebsVolumeTypeName ? EbsDeviceVolumeType[ebsVolumeTypeName as keyof typeof EbsDeviceVolumeType] : undefined | ||
if (ebsVolumeTypeName && !ebsVolumeType) { | ||
throw new Error("Provided ebsVolumeType does not match a selectable option, for reference https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_ec2.EbsDeviceVolumeType.html") | ||
} | ||
|
||
const domainRemovalPolicyName = getContextForType('domainRemovalPolicy', 'string') | ||
const domainRemovalPolicy = domainRemovalPolicyName ? RemovalPolicy[domainRemovalPolicyName as keyof typeof RemovalPolicy] : undefined | ||
if (domainRemovalPolicyName && !domainRemovalPolicy) { | ||
throw new Error("Provided domainRemovalPolicy does not match a selectable option, for reference https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.RemovalPolicy.html") | ||
} | ||
|
||
const opensearchStack = new OpensearchServiceDomainCdkStack(scope, 'opensearchDomainStack', { | ||
version: version, | ||
domainName: domainName, | ||
dataNodeInstanceType: dataNodeType, | ||
dataNodes: dataNodeCount, | ||
dedicatedManagerNodeType: dedicatedManagerNodeType, | ||
dedicatedManagerNodeCount: dedicatedManagerNodeCount, | ||
warmInstanceType: warmNodeType, | ||
warmNodes: warmNodeCount, | ||
accessPolicies: accessPolicies, | ||
useUnsignedBasicAuth: useUnsignedBasicAuth, | ||
fineGrainedManagerUserARN: fineGrainedManagerUserARN, | ||
fineGrainedManagerUserName: fineGrainedManagerUserName, | ||
fineGrainedManagerUserSecretManagerKeyARN: fineGrainedManagerUserSecretManagerKeyARN, | ||
enforceHTTPS: enforceHTTPS, | ||
tlsSecurityPolicy: tlsSecurityPolicy, | ||
ebsEnabled: ebsEnabled, | ||
ebsIops: ebsIops, | ||
ebsVolumeSize: ebsVolumeSize, | ||
ebsVolumeType: ebsVolumeType, | ||
encryptionAtRestEnabled: encryptionAtRestEnabled, | ||
encryptionAtRestKmsKeyARN: encryptionAtRestKmsKeyARN, | ||
appLogEnabled: loggingAppLogEnabled, | ||
appLogGroup: loggingAppLogGroupARN, | ||
nodeToNodeEncryptionEnabled: noneToNodeEncryptionEnabled, | ||
vpcId: vpcId, | ||
domainRemovalPolicy: domainRemovalPolicy, | ||
...props, | ||
}); | ||
|
||
this.stacks.push(opensearchStack) | ||
|
||
function getContextForType(optionName: string, expectedType: string): any { | ||
const option = scope.node.tryGetContext(optionName) | ||
|
||
// If no context is provided and a default value exists, use it | ||
if (option === undefined && defaultValues[optionName]) { | ||
return defaultValues[optionName] | ||
} | ||
|
||
// Filter out invalid or missing options by setting undefined (empty strings, null, undefined, NaN) | ||
if (option !== false && option !== 0 && !option) { | ||
return undefined | ||
} | ||
// Values provided by the CLI will always be represented as a string and need to be parsed | ||
if (typeof option === 'string') { | ||
if (expectedType === 'number') { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're parsing it as an int, the type label should probably be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't believe there is integer type in TS/JS (lmk if wrong here), parseInt will return a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sigh, Google says you're right. Gross. Ok, disregard the comment. |
||
return parseInt(option) | ||
} | ||
if (expectedType === 'boolean' || expectedType === 'object') { | ||
return JSON.parse(option) | ||
} | ||
Comment on lines
+125
to
+127
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A few questions/comments:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Action Item - can you call out in the README that users are encouraged to customize the deployment by changing the CDK TypeScript, and that the configuration-by-context option is primarily provided for testing/development purposes? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sounds good, will make this callout in the README |
||
} | ||
// Values provided by the cdk.context.json should be of the desired type | ||
if (typeof option !== expectedType) { | ||
throw new Error(`Type provided by cdk.context.json for ${optionName} was ${typeof option} but expected ${expectedType}`) | ||
} | ||
return option | ||
} | ||
|
||
function parseAccessPolicies(jsonObject: { [x: string]: any; }): PolicyStatement[] { | ||
let accessPolicies: PolicyStatement[] = [] | ||
const statements = jsonObject['Statement'] | ||
for (let i = 0; i < statements.length; i++) { | ||
const statement = PolicyStatement.fromJson(statements[i]) | ||
accessPolicies.push(statement) | ||
} | ||
return accessPolicies | ||
} | ||
|
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick - Stack and resource names have a maximum length, and the
CDKStack
portion of this one probably doesn't provide value. Consider removing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually think the CDK portion is rather helpful to quickly identify in the Console that this was deployed with CDK which helps with identifying if a CDKToolkit stack is missing or why the CFN template looks the way it does
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will try to shorten this though