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

OpenSearch Service CDK setup #14

Merged
merged 8 commits into from
Feb 15, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions opensearch-service-domain-cdk/.gitignore
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
6 changes: 6 additions & 0 deletions opensearch-service-domain-cdk/.npmignore
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
130 changes: 130 additions & 0 deletions opensearch-service-domain-cdk/README.md

Large diffs are not rendered by default.

14 changes: 14 additions & 0 deletions opensearch-service-domain-cdk/bin/app.ts
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}`,
Copy link

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.

Copy link
Owner Author

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

Copy link
Owner Author

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

description: "This stack contains resources to create/manage an OpenSearch Service domain"
});
44 changes: 44 additions & 0 deletions opensearch-service-domain-cdk/cdk.json
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
}
}
4 changes: 4 additions & 0 deletions opensearch-service-domain-cdk/default-values.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"engineVersion": "OS_1.3",
"domainName": "cdk-os-service-domain"
Copy link

Choose a reason for hiding this comment

The 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?

Copy link
Owner Author

Choose a reason for hiding this comment

The 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 default-values.json or certain options in thecdk.context.json so there aren't any surprises when deploying

Copy link
Owner Author

Choose a reason for hiding this comment

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

}
8 changes: 8 additions & 0 deletions opensearch-service-domain-cdk/jest.config.js
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
Copy link

Choose a reason for hiding this comment

The 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.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Be default from CDK, this isn't enabled

Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

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

Same comment - seems like something folks will want out of the box.

Copy link
Owner Author

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

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

What happens if the VPC is left undefined here? Does one get created?

Copy link
Owner Author

Choose a reason for hiding this comment

The 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

Copy link

Choose a reason for hiding this comment

The 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
});
}
}
147 changes: 147 additions & 0 deletions opensearch-service-domain-cdk/lib/stack-composer.ts
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')
Copy link

Choose a reason for hiding this comment

The 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)?

Copy link
Owner Author

Choose a reason for hiding this comment

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

In the getContextForType function the initial if statement will return undefined for any missing or empty string options. Undefined props get ignored when creating the Domain construct.

Copy link

Choose a reason for hiding this comment

The 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?

Copy link
Owner Author

Choose a reason for hiding this comment

The 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))

Choose a reason for hiding this comment

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

what happens if I pass in something undefined like OS_99.99?

Copy link
Owner Author

Choose a reason for hiding this comment

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

It will get passed to CFN, as it is valid format, however cdk deploy will fail with unsupported version error

}
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') {
Copy link

Choose a reason for hiding this comment

The 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 int.

Copy link
Owner Author

Choose a reason for hiding this comment

The 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 number as will numeric values place in cdk.context.json

Copy link

Choose a reason for hiding this comment

The 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
Copy link

Choose a reason for hiding this comment

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

A few questions/comments:

  • Why not parse the boolean as a boolean?
  • object is unclear as a type label. If we're parsing it as JSON, it should be labeled as JSON.
  • What all "objects" are you expecting for this code to have to deal with?

Copy link
Owner Author

Choose a reason for hiding this comment

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

  1. A limitation of JS, there doesn't seem to be a boolean method to parse a string and many sites recommend using JSON.parse() 🤢
  2. I believe this object is essentially a JSON object. I am not aware of say a JSON specific class name that we could attribute. Lmk if you have something, as this was my initial hunch but was unsuccessful in finding anything
  3. The formats I expect for objects here are JSON object format {"Version": "2012-10-17", ...} and a string array format ["subnet-123", "subnet-124"]. This will be clearer when I add sample types to the readme

Copy link

Choose a reason for hiding this comment

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

  1. Bleh, OK.
  2. As I read it, we're providing the "type labeling" here. It's probably just a nitpick, but I'd love to make it clear this is a JSON-ish thing. Maybe a good compromise is to change the label to json_object?
  3. I see. I wouldn't invest too heavily in this sort of thing, as users that want to customize their stacks to any degree other than the most basic will probably just do it via writing TypeScript CDK. And, frankly, I think that's what we should encourage them to do rather than providing a way to pass through complex object configuration via JSON.

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?

Copy link
Owner Author

Choose a reason for hiding this comment

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

object is a TS type that we have to adhere to unfortunately

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
}

}
}
Loading