Elasticsearch integration #114
Conversation
* updated tenant-onboarding.yaml to work with Elasticsearch * updated saas-boost-svc-onboarding.yaml to fit Elasticsearch permissions
|
Due to the necessity of having an Elasticsearch cluster associated to my application running on SaaS boost, I have created a basic onboarding template to generate such a stack. It's not ready for production nor to be joined to master, but after a talked with @4patelr and also since there is a lot to do I think it's a good starting point for the community to start contributing and make this feature ready for production. |
There was a problem hiding this comment.
Hello @AffiTheCreator , thank you for your contribution and opening a pull request! I've just completed an initial review of the work you've done so far with some high level comments of things that can be improved.
Are you also working on integrating this Elasticsearch feature into the installer and admin UI? Unless I misunderstand the use case, it may make sense for different tenants to be able to be provisioned with different instance types, Elasticsearch cluster sizes, etc.
Lastly, I would suggest both that you make sure your fork is up to date with main so we confirm that the only changes in this PR are changes you intend to go into your single commit and that you run a fresh SaaS Boost Install with this Elasticsearch integration to help us understand the end-to-end user experience with this feature.
Thanks for your contribution and your attention! We look forward to working with you to get this contribution in.
| # # Elasticsearch policy | ||
| # - Effect: Allow | ||
| # Action: | ||
| # - es:DescribeElasticsearchDomain | ||
| # Resource: | ||
| # - !Sub arn:aws:es:${AWS::Region}:${AWS::AccountId}:domain/*/* | ||
|
|
There was a problem hiding this comment.
Do these commented out lines need to be here?
| API_TRUST_ROLE: !Sub arn:aws:iam::${AWS::AccountId}:role/sb-private-api-trust-role-${Environment}-${AWS::Region} | ||
| API_GATEWAY_HOST: !Sub ${SaaSBoostPrivateApi}.execute-api.${AWS::Region}.amazonaws.com | ||
| API_GATEWAY_STAGE: !Ref PrivateApiStage |
There was a problem hiding this comment.
These variables are used by the onboarding service to properly function. Have you tested a fresh SaaS Boost install with these changes?
| are a-z (lowercase only), 0-9, and - (hyphen). | ||
| AllowedPattern: '[a-z][a-z0-9\\-]+' | ||
| ElasticsearchVersion: | ||
| Default: '6.2' |
There was a problem hiding this comment.
Why not choose the latest version?
| Type: Number | ||
| Default: '10' | ||
| Conditions: | ||
| EnforceHTTPS_false: !Equals [ !Ref "EnforceHTTPS", "false" ] |
There was a problem hiding this comment.
You could simplify this condition by just naming it ShouldEnforceHTTPS and checking that it's equal to true. Your results for checks against the condition just flip.
| # case specific - debug purpose | ||
| # - es:ESHttpPut | ||
| ############## |
There was a problem hiding this comment.
Can you describe in what cases this would be useful?
| Properties: | ||
| GroupId: !Ref ESSecurityGroup | ||
| IpProtocol: tcp | ||
| # To review !! |
There was a problem hiding this comment.
Can you clarify what you mean by this comment?
| EnforceHTTPS: !Ref EnforceHTTPS | ||
| EBSOptions: | ||
| EBSEnabled: !Ref EBSEnabled | ||
| Iops: 0 |
There was a problem hiding this comment.
Iops under EBSOptions means "The number of I/O operations per second (IOPS) that the volume supports. This property applies only to the Provisioned IOPS (SSD) EBS volume type. reference" It's also not required: why do we explicitly set it to 0 here?
| - Effect: Allow | ||
| Principal: | ||
| AWS: '*' | ||
| Action: "es:*" | ||
| Resource: !Sub "arn:aws:es:${AWS::Region}:${AWS::AccountId}:domain/${ElasticsearchName}/*" |
There was a problem hiding this comment.
This gives every AWS logged in user permissions to enact any Elasticsearch operation on this Elasticsearch domain. This must be restricted down to at the very least the account running this SaaS Boost installation by default. Developers using Elasticsearch as part of their SaaS Boost installation can change this to accept a wider set of incoming traffic if needed, but we shouldn't endorse that by default.
| @@ -18,13 +18,16 @@ Parameters: | |||
| Environment: | |||
| Description: Environment (test, uat, prod, etc.) | |||
| Type: String | |||
| Default: oct12 | |||
There was a problem hiding this comment.
These changes were removed by d0d0478. Is your fork up to date with main?
| @@ -586,7 +742,13 @@ Resources: | |||
| - !Sub arn:aws:ssm:${AWS::Region}:${AWS::AccountId}:parameter/saas-boost/${Environment}/DB_PORT | |||
| - !Sub arn:aws:ssm:${AWS::Region}:${AWS::AccountId}:parameter/saas-boost/${Environment}/DB_HOST | |||
| - !Sub arn:aws:ssm:${AWS::Region}:${AWS::AccountId}:parameter/saas-boost/${Environment}/DB_NAME | |||
| - !Sub arn:aws:ssm:${AWS::Region}:${AWS::AccountId}:parameter/saas-boost/${Environment}/METRICS_STREAM | |||
| - !Sub arn:aws:ssm:${AWS::Region}:${AWS::AccountId}:parameter/saas-boost/${Environment}METRICS_STREAM | |||
There was a problem hiding this comment.
You've removed the last / before METRICS_STREAM here.
closes #32
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license