Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
AG-1591, AG-1592: don't deploy unused containers, create DocumentDB cluster, allow connection from API container #17
base: dev
Are you sure you want to change the base?
AG-1591, AG-1592: don't deploy unused containers, create DocumentDB cluster, allow connection from API container #17
Changes from 6 commits
344fb27
45b5ed8
9456101
fb5b2fe
6b898bb
1f1bb23
4e98df5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 may need these to be https considering that the app will be served over https
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.
Sounds good, I can change that in the next commit
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.
isn't this a dup of
APP_VERSION
?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.
They're very similar, but not quite the same (e.g.
4.0.0
forAPP_VERSION
vsagora/v4.0.0.0
forTAG_NAME
). @sagely1 would know better, but I believe separate variables were created because the GitHub tag format was in flux. However, with that decision made and the format so similar, they should probably be collapsed to just one environment variable, but that will require work in the sage-monorepo first!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 settled on semantic versioning after some discussion. TAG_NAME will be a string of a format like so:
agora/vX.X.X with a potential suffix such as -rc.X so agora/vX.X.X-rc.X. I think we can just change this to get a tag_name variable instead. https://semver.org/
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.
Great! I've opened a ticket to track the work, since we'll need to update the sage-monorepo code first.
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 good practice is to occasionally rotate the secret, https://docs.aws.amazon.com/cdk/api/v2/python/aws_cdk.aws_secretsmanager/RotationSchedule.html
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.
Makes sense! Will the password environment variable in the API container be automatically updated when the secret is rotated?
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.
yes, i believe that's the best feature of secrets manager. here's a blog on how to setup https://aws.amazon.com/blogs/security/how-to-rotate-amazon-documentdb-and-amazon-redshift-credentials-in-aws-secrets-manager/
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 article you linked, the application retrieves the password directly from Secrets Manager at runtime. However, the Agora API container currently retrieves the password from an environment variable, which is set by the ServiceStack during deployment. If the secret is retrieved at deployment time, then would the API container environment variable only be updated when the stack is redeployed (and not automatically be updated when the secret is rotated)?
If that's right, then to allow us to use automatic password rotation, I think we'd need to update the API to retrieve the password directly from Secrets Manager (and refetch the credentials without restarting the container, as described here) at runtime, rather than getting the password from an environment variable. What do you think?
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.
ahh, yes you are correct. getting the secret from an env var will not work with secrets rotation. refactoring the API to get secrets directly from the secrets manager would be a better approach because is more secure and works with secrets rotation. I guess we can leave out secrets rotation at this time. I'll let you guys decide whether you want to refactor Agora to support this use case. I would highly recommend it though.
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 convention used in other files for
construct_id
is camel case. Also i don't think you don't need to get fancy here, you can just set to something like "DocDbMasterPassword"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.
27017
is used multiple times in this file. would it make sense to create a variable for this?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.
duplicate parameter?
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 remove in the next commit
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.
name parameter does not seem like it's required? if a name parameter is not required I would leave it out and let AWS name the resource. You can always dynamically get a resource name from its Ref.
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.
hmm, it looks like we add the
security_group
defined here to every container so how about we move thesecurity_group
to theServiceProps
class? It can just be added to thecontainer_security_groups
object as a default SG?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 create the
SecurityGroup
inServiceProps
, then I believeServiceProps
would need to inherit fromConstruct
, which would result in duplicate code between the props and the stack class. What do you think about continuing to createsecurity_group
inServiceStack
, so that the resource creation only happens in one class?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.
Not sure exactly what you mean but if you mean define a list of service groups in
app.py
then pass it toServiceProps
object then I think that will work as well.