OpenSearch cluster cdk changes #3
Conversation
README.md
Outdated
| | securityGroupId (Optional) | boolean | Re-use existing security group, provide security group id | | ||
| | cidr (Optional) | string | User provided CIDR block for new Vpc, default is `10.0.0.0/16` | | ||
| | managerNodeCount (Optional) | integer | Number of cluster manager nodes, default is 3 | | ||
| | dateNodeCount (Optional) | integer | Number of data nodes, default is 2 | |
There was a problem hiding this comment.
| | dateNodeCount (Optional) | integer | Number of data nodes, default is 2 | | |
| | dataNodeCount (Optional) | integer | Number of data nodes, default is 2 | |
There was a problem hiding this comment.
Thanks for pointing this out, fixed!
README.md
Outdated
| | managerNodeCount (Optional) | integer | Number of cluster manager nodes, default is 3 | | ||
| | dateNodeCount (Optional) | integer | Number of data nodes, default is 2 | |
There was a problem hiding this comment.
should have an option for ML node count also? as we have now MLCommons plugin also.
| @@ -0,0 +1,17 @@ | |||
| #!/usr/bin/env node | |||
| force_flush_interval: 5, | ||
| }, | ||
| }), | ||
| InitCommand.shellCommand('set -ex;/opt/aws/amazon-cloudwatch-agent/bin/amazon-cloudwatch-agent-ctl -a stop'), |
There was a problem hiding this comment.
I believe all shell commands have set -xe set by default. Can you check and remove from everywhere if not required?
There was a problem hiding this comment.
Already tested, needs to be present for each statement for it to be reflected in the init log, setting it on top did not log all the commands.
lib/networking/vpc-stack.ts
Outdated
|
|
||
| /* The security group allows all ip access by default to all the ports. | ||
| Please update below if you want to restrict access to certain ips and ports */ | ||
| this.osSecurityGroup.addIngressRule(Peer.anyIpv4(), Port.allTcp()); |
There was a problem hiding this comment.
See the comment about public access above.
rishabh6788
requested review from
gaiksaya
and removed request for
navneet1v
README.md
Outdated
| | jvmSysProps (Optional) | string | A comma-separated list of key=value pairs that will be added to `jvm.options` as JVM system properties. | | ||
|
|
||
| #### Restricting Server Access | ||
| You need to restrict access to your jenkins endpoint (load balancer). |
There was a problem hiding this comment.
This should be opensearch cluster acess :)
|
|
||
| Be sure to: | ||
| ## Getting Started |
There was a problem hiding this comment.
Create a bullet list with all headers? Similar to https://github.com/opensearch-project/opensearch-ci/blob/main/README.md?plain=1#L3-L26 so that users can have easy access?
README.md
Outdated
| #### Please note the load-balancer url is internet facing and can be accessed by anyone. | ||
| To restrict access please refer [Client IP Preservation](https://docs.aws.amazon.com/elasticloadbalancing/latest/network/load-balancer-target-groups.html#client-ip-preservation) to restrict access on internet-facing network load balancer. | ||
| You need to add the ip/prefix-list rule in the security group created in network stack. |
There was a problem hiding this comment.
Can you replace this section with restricting server access one? I believe you posted that somewhere else?
lib/infra/infra-stack.ts
Outdated
| }); | ||
|
|
||
| const instanceRole = new Role(this, 'instanceRole', { | ||
| roleName: 'OS-Instance-Role', |
There was a problem hiding this comment.
remove role name as this will block users from deploying multiple clusters in same account. The cluster creation will fail saying resource already exists.
lib/infra/infra-stack.ts
Outdated
| } | ||
|
|
||
| const alb = new NetworkLoadBalancer(this, 'publicNlb', { | ||
| loadBalancerName: 'opensearch-nlb', |
There was a problem hiding this comment.
Avoid resource naming. This avoids user to deploy multiple clusters
lib/networking/vpc-stack.ts
Outdated
| if (props.vpcId === undefined) { | ||
| console.log('No VPC Provided, creating new'); | ||
| this.vpc = new Vpc(this, 'opensearchClusterVpc', { | ||
| vpcName: 'opensearch-cluster-vpc', |
| maxAzs: props.maxAzs, | ||
| subnetConfiguration: [ | ||
| { | ||
| name: 'public-subnet', |
There was a problem hiding this comment.
Same as above avoid resource naming
There was a problem hiding this comment.
name here is a mandatory attribute and since it is tied to the VPC, having duplicates will not cause any failure as it is more of a tag than an actual resource name. @gaiksaya
| cidrMask: 24, | ||
| }, | ||
| { | ||
| name: 'private-subnet', |
There was a problem hiding this comment.
Same as above avoid resource naming
There was a problem hiding this comment.
name here is a mandatory attribute and since it is tied to the VPC, having duplicates will not cause any failure as it is more of a tag than an actual resource name. @gaiksaya
Signed-off-by: Rishabh Singh <sngri@amazon.com>
Signed-off-by: Rishabh Singh sngri@amazon.com
Description
This PR adds capability to spin up single-node and multi-node OpenSearch cluster using aws-cdk.
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.