-
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
Conversation
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #14 +/- ##
=======================================
Coverage 92.44% 92.44%
=======================================
Files 24 24
Lines 1086 1086
Branches 103 103
=======================================
Hits 1004 1004
Misses 72 72
Partials 10 10 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
{ | ||
"domainName": "cdk-os-service-domain", | ||
"engineVersion": "OS_1.3", | ||
"dataNodeType": "r6g.large.search", | ||
"dataNodeCount": 1, | ||
"dedicatedMasterNodeType": "r6g.large.search", | ||
"dedicatedMasterNodeCount": 3, | ||
"warmNodeType": "ultrawarm1.medium.search", | ||
"warmNodeCount": 2, | ||
"coldStorageEnabled": false, | ||
"zoneAwarenessAvailabilityZoneCount": 1 | ||
} |
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.
Things like instance-type availability tend to region specific, so we may need to move some or all of these default into code later so we can do something more clever. Not a problem for now as long as we're explicit in our documentation and expectations that this is tied to a specific region. Please make it clear which region this is for.
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 do 👍
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 opensearchStack = new OpensearchServiceDomainCdkStack(scope, 'OpenSearchServiceDomainCDKStack', { |
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.
It's generally best to embed things like stage (alpha/beta/gamma/prod) and region into top-level stack names (like 'OpenSearchServiceDomainCDKStack'
here) as it helps w/ naming collisions when people deploy multiple into the same account. Things like IAM Roles are global to the account and the CDK usually creates their names using the stack name and/or higher-level resource name as a suffix. This means if you deploy the same stack to two regions, and the CDK Stack has a region-agnostic name, you can end up with collisions on global resources even though the stacks themselves are deployed to separate regions.
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.
Definitely a valid point, I will try to shape these resource names to incorporate this
|
||
let version: EngineVersion | ||
|
||
const domainName = getContextForType('domainName', 'string') |
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.
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 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.
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Should clear up in next revision
if (tlsSecurityPolicyName && !tlsSecurityPolicy) { | ||
throw new Error("Provided tlsSecurityPolicy does not match a selectable option, i.e. 'TLS_1_2'") | ||
} |
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.
We have a lot of error handling here for bad input combinations. I'm concerned we're re-implementing the error handling in the OpenSearch Service API. What's the plan for keeping this up to date as API/CFN features and behaviors change, etc?
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.
Good callout. I'm not that tied to lines 55-65 for error handling, my main intention here was to highlight that these features have required options and not to leave a user hanging if they provide a subset and then the feature doesn't get enabled (Another option here may be to provide a potentially undefined value to the Domain construct and let it handle this, which I kind of like to remove our need to keep up with any change in options here)
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.
Lines 81-97, I feel some inkling to keep as we don't necessarily need to know the different options (and the error should probably be updated to just link the page to find the options) but it ensures a string that looks like it works but doesn't actually map to the object is called out. As far as updates, any major shift away from using these objects would require an update, but seems unlikely. Let me know if you have other opinions here
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.
+1
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.
As described above, I think we're better off inserting ourselves between the user and the AWS Resource APIs as little as possible and letting CDK/CFN surface invalid configurations.
I agree that the type conversions (lines 81+) are safer and I'm inclined to keep them as well.
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.
Similar response in other comment
} | ||
// 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 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
.
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 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
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.
Sigh, Google says you're right. Gross. Ok, disregard the comment.
if (expectedType === 'boolean' || expectedType === 'object') { | ||
return JSON.parse(option) | ||
} |
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.
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?
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.
- A limitation of JS, there doesn't seem to be a boolean method to parse a string and many sites recommend using JSON.parse() 🤢
- 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 - 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
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.
- Bleh, OK.
- 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
? - 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?
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.
object
is a TS type that we have to adhere to unfortunately
Sounds good, will make this callout in the README
// Falsy check modified to not include values we want to retain | ||
if (option !== false && option !== 0 && !option) { | ||
return undefined | ||
} |
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 didn't understand what this code is supposed to do, in context. Can you explain?
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.
This portion is essentially filtering out missing or empty values with a falsy check: https://developer.mozilla.org/en-US/docs/Glossary/Falsy
} | ||
// Values provided by the cdk.context.json should be of the desired type | ||
if (typeof option !== expectedType) { | ||
console.warn(`Type provided by cdk.context.json for ${optionName} was ${typeof option} but expected ${expectedType}`) |
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.
Err, shouldn't this result in an exception being thrown?
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 was on the fence about throwing an error or warning and seeing if CDK could handle. I probably prefer the exception approach to remove some ambiguity here, will update
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.
Addressed in next revision
return option | ||
} | ||
|
||
function parseAccessPolicies(jsonObject: any): PolicyStatement[] { |
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.
Why use a type hint here for jsonObject
if we're just indicating it can be any type? Can we be more specific?
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.
Agree we can at least make this object
@@ -0,0 +1,47 @@ | |||
{ |
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.
Which of these must a user fill in, and what is the plan to communicate defaults for the ones they don't fill in?
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.
My plan was to require domainName(this could also be left empty) and engineVersion. Everything else could be removed or left empty here. The README would have each of these options a description and an example value that could be used
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.
The README mentions that some are required. If the required ones are not present, what happens?
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.
General TS/CDK UX comment - I'm not crazy about having a configuration file that the user fills out alongside other code in the repo. It's lacking a separation between library code and customization. Even if that's how CDK typically stages stuff, it still smells.
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.
AFAIK, typically this sort of file wouldn't be distributed in the repo itself but left for the user to supply themselves or via command-line. I think it makes sense to have some way of supplying "default" values, but the more I think about it the more I agree that it's not by bundling this file into the repo.
How about we embed the default values in-code, but letting CDK Context override if supplied via command line or a config file? E.g. just make a default-values.ts
file that contains these values?
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 probably agree with this. Do we want to make any assumptions about default values? I'm okay with say having this default-values.ts
and maybe having a domain name and version in it but don't know if I would want to put anything else in it
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'm concerned that the list of option parameters that OpenSearch consumes are effectively repeated multiple times throughout this stack. As an exercise for later, are there any ways that we can tighten some of these mappings together - either pass them through or to guarantee that the identity bindings are preserved.
@@ -0,0 +1,14 @@ | |||
#!/usr/bin/env node | |||
import 'source-map-support/register'; | |||
import * as cdk from 'aws-cdk-lib'; |
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.
Is this considered acceptable form for TS/CDK?
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.
Ah, I missed this. AFAIK, * imports are evil in TypeScript just like other languages. Can you please import the specific types you need?
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 address in next revision
import {StackComposer} from "../lib/stack-composer"; | ||
|
||
const app = new cdk.App(); | ||
const stage = "dev" |
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.
this should be an environment variable too
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 address in next revision
``` | ||
cdk deploy --c domainName='cdk-os-service-domain' --c engineVersion="OS_1_3_6" --c dataNodeType="r6g.large.search" --c dataNodeCount=1 | ||
``` | ||
* Note that these context parameters can also be passed to `cdk synth` and `cdk bootstrap` commands |
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 can pass my datatype nodes, versions, etc to ANY of these 3 commands? What happens if I pass different ones and alternate the order? Do they override the cdk.context.json file?
Could you please provide a bit more explanation for a reader that may have no experience w/ CDK to understand what the appropriate workflows should be. The notes are also missing how to tear a stack down.
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.
Sure will add some details on using these options as mentioned and a stack tear down section
@@ -0,0 +1,47 @@ | |||
{ |
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.
The README mentions that some are required. If the required ones are not present, what happens?
@@ -0,0 +1,47 @@ | |||
{ |
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.
General TS/CDK UX comment - I'm not crazy about having a configuration file that the user fills out alongside other code in the repo. It's lacking a separation between library code and customization. Even if that's how CDK typically stages stuff, it still smells.
const enableVersionUpgrade = getContextForType('enableVersionUpgrade', 'boolean') | ||
|
||
// Check for partially configured features | ||
if ((cognitoIdentityPoolId || cognitoRoleARN || cognitoUserPoolId) && |
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.
How do you know that those 3 values are required for Cognito auth? How do you know that you've got all of the possible required groupings? (maybe you don't, and it's worth calling that out)
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.
Yeah, I have concerns about this too. I think the right approach is to provide a thin wrapper around the AWS APIs and let the CDK/CFN surface errors to the user if present. Don't think we want to insert ourselves into that process as it won't be sustainable.
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.
This is based off requirements that the Domain CDK construct has on required fields for certain objects/features. I don't have much attachment here and am okay to remove these checks. From the construct side, some objects have required fields that should only be created if we have the required fields otherwise CDK will fail synthesis and produce some very ambiguous error messages such as Cannot read property 'length' of undefined
for missing domain endpoint name field. To accommodate this, I feel we have to take on the required field assumption and check for this, but open to other work arounds
const engineVersion = getContextForType('engineVersion', 'string') | ||
if (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 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?
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.
It will get passed to CFN, as it is valid format, however cdk deploy
will fail with unsupported version error
if (tlsSecurityPolicyName && !tlsSecurityPolicy) { | ||
throw new Error("Provided tlsSecurityPolicy does not match a selectable option, i.e. 'TLS_1_2'") | ||
} |
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.
+1
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
|
||
Depending on your use-case, you may choose to provide options from both the `cdk.context.json` and the CDK CLI, in which case it is important | ||
to know the precedence level for context values. The below order shows these levels with values placed in the `cdk.context.json` having the most importance | ||
1. Created `cdk.context.json` in base directory |
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 - what is the base directory
you mention? I think users will find that ambiguous.
|
||
Also ensure you have configured the desired [AWS credentials](https://docs.aws.amazon.com/cdk/v2/guide/getting_started.html#getting_started_prerequisites), as these will dictate the region and account used for deployment | ||
|
||
A `CDK_DEPLOYMENT_STAGE` environment variable should also be set to assist in naming resources and preventing collisions |
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.
Can you please provide some example values this variable could take (e.g. gamma
, dev
, PROD
) and explain how they will be incorporated into the created resources.
Some configuration options available in other solutions (listed below) which enable/disable specific features do not exist in the current native CDK Domain construct. These options are inferred based on the presence or absence of related fields (i.e. if dedicatedMasterNodeCount is set to 1 it is | ||
inferred that dedicated master nodes should be enabled). These options are normally disabled by default, allowing for this inference. |
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.
Curious what's up with these line breaks? Seems like this should either be split earlier if it's at 120 characters, or not at all.
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.
Fixed, just my intelij not wanting to word wrap was cause
const region = process.env.CDK_DEFAULT_REGION | ||
new StackComposer(app, { | ||
env: { account: account, region: region }, | ||
stackName: `OpenSearchServiceDomainCDKStack-${stage}-${region}`, |
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
@@ -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 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?
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.
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
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.
const vpc: IVpc|undefined = props.vpcId ? | ||
Vpc.fromLookup(this, "domainVPC", {vpcId: props.vpcId}) : undefined |
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.
What happens if the VPC is left undefined
here? Does one get created?
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.
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 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 earKmsKey: IKey|undefined = props.encryptionAtRestKmsKeyARN && props.encryptionAtRestEnabled ? | ||
Key.fromKeyArn(this, "earKey", props.encryptionAtRestKmsKeyARN) : undefined |
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.
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 comment
The 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 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 appLG: ILogGroup|undefined = props.appLogGroup && props.appLogEnabled ? | ||
LogGroup.fromLogGroupArn(this, "appLogGroup", props.appLogGroup) : undefined |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Be default from CDK, this isn't enabled
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
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.
Ship it!
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.
LGTM - this is a WIP & feedback will help make us calibrate
MIGRATIONS-876: Provide an initial IaC CDK Managed OpenSearch Service solution for users Signed-off-by: Tanner Lewis <lewijacn@amazon.com>
Description
Initial implementation of an IaC solution for deploying Managed OpenSearch Service. Some initial options provided by the CDK Domain construct are configurable through the CDK context to customize domain deployment.
The major items in this task were:
Items for improvement noticed:
Issues Resolved
https://opensearch.atlassian.net/browse/MIGRATIONS-876
Testing
Unit test cases to be added, deployments have been performed to us-east-1 region
Base values provided in cdk.context.json generate following CFN template shown in AWS Console
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.