-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
AddOn - AWS Secrets Manager and Config Provider for Secret Store CSI Driver #471
Conversation
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 work @anshrma ! LGTM 👍🏼
Few minor changes requested. We just need to run some E2E tests before merging the PR
examples/secrets-management-with-csi-secrets-driver-aws/variables.tf
Outdated
Show resolved
Hide resolved
modules/kubernetes-addons/csi-secrets-store-provider-aws/locals.tf
Outdated
Show resolved
Hide resolved
modules/kubernetes-addons/csi-secrets-store-provider-aws/locals.tf
Outdated
Show resolved
Hide resolved
Thanks @vara-bonthu - Addressed your feedbacks. |
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!
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.
Thanks for the PR! Please update your branch with the latest on main
as there have been a number of changes to the examples/
format
@@ -0,0 +1,131 @@ | |||
# EKS Cluster Deployment with new VPC |
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.
Instead of creating a whole new example just for these two addons, instead can we just add their implementation into the https://github.com/aws-ia/terraform-aws-eks-blueprints/tree/main/examples/complete-kubernetes-addons
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.
@askulkarni2 / @kcoleman731 what are your thoughts on this as we continue to expand the number of addons?
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 agree with @bryantbiggs . We should start moving the individual Add-on examples to [complete-kubernetes-addons](https://github.com/aws-ia/terraform-aws-eks-blueprints/tree/main/examples/complete-kubernetes-addons)
. This will help us to manage limited exmaples
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 can add some more details to docs
.md
files as well. New users should be able to goto docs to findout the usage of this Add-on.
@@ -0,0 +1,11 @@ | |||
data "aws_iam_policy_document" "secrets_management_policy" { |
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.
when you move this over to the complete-kubernetes-addons
example, no need to put this in a separate file. Instead, locate it next to where it is used. Here that would be either before or after https://github.com/aws-ia/terraform-aws-eks-blueprints/pull/471/files#diff-6355537d0664dc024a0086c9581f3a707332dd67c563f98cbe5ee18d4fd0e44bR164
examples/secrets-management-with-csi-secrets-driver-aws/main.tf
Outdated
Show resolved
Hide resolved
examples/secrets-management-with-csi-secrets-driver-aws/main.tf
Outdated
Show resolved
Hide resolved
examples/secrets-management-with-csi-secrets-driver-aws/secretconfig.yaml
Outdated
Show resolved
Hide resolved
examples/secrets-management-with-csi-secrets-driver-aws/README.md
Outdated
Show resolved
Hide resolved
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.
First of all thanks so much for this PR! The PR is looking good. There is one main outstanding design item. We probably want to keep the secrets provisioning design similar to how we do in CDK (if possible). Refer to https://github.com/aws-quickstart/cdk-eks-blueprints/blob/main/docs/extensibility.md#passing-secrets-to-add-ons.
examples/secrets-management-with-csi-secrets-driver-aws/main.tf
Outdated
Show resolved
Hide resolved
examples/secrets-management-with-csi-secrets-driver-aws/main.tf
Outdated
Show resolved
Hide resolved
examples/secrets-management-with-csi-secrets-driver-aws/main.tf
Outdated
Show resolved
Hide resolved
# Creating IAM Role for Service Account | ||
#--------------------------------------------------------------- | ||
|
||
module "iam_role_service_account" { |
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.
So, the problem here is that the addon itself has no clue what secrets are being retrieved. The way it works, secrets are retrieved by the AWS provider when a pod needing those secrets is started and removed afterwords. The service-account associated with the respective pods needs to have the needed IRSA config. These policies cannot be added to the addon. In CDK, we do this when the Team
or AddOn
secrets are defined. The same thing needs to happen here. That is to say, in order to consume secrets, there are two paths:
- You need to define your secrets via
Teams
configuration (similar to how we do labels and quotas today and we will have to update the Teams module for this). We need to add the required policies to the IRSA config we define for the Team. - For add-ons, this will be a parameter for the add-on in which case we will add the required policies to the addons IRSA config.
CC: @kcaws @shapirov103 I am assuming we want to keep the design for secrets somewhat similar between the two projects.
modules/kubernetes-addons/csi-secrets-store-provider-aws/main.tf
Outdated
Show resolved
Hide resolved
…ueprints into secrets-manager
modules/kubernetes-addons/csi-secrets-store-provider-aws/locals.tf
Outdated
Show resolved
Hide resolved
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.
@anshrma looking good, just one final change for using namespace resource instead of irsa module as suggested by Vara. Also please address the CI failures. Running pre-commit run -a
should fix most of them.
examples/secrets-management-with-csi-secrets-driver-aws/main.tf
Outdated
Show resolved
Hide resolved
modules/kubernetes-addons/csi-secrets-store-provider-aws/locals.tf
Outdated
Show resolved
Hide resolved
modules/kubernetes-addons/csi-secrets-store-provider-aws/locals.tf
Outdated
Show resolved
Hide resolved
…ueprints into secrets-manager
Thanks @askulkarni2 . Done with the changes requested and addressed the CI failures. Please let me know, if any other changes required. |
Hey @anshrma, @askulkarni2 and @bryantbiggs, Whaat is missing in this PR to go forward? |
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.
Updated, tested and approved!
…Driver (aws-ia#471) Co-authored-by: Bryant Biggs <bryantbiggs@gmail.com> Co-authored-by: Apoorva Kulkarni <kuapoorv@amazon.com>
What does this PR do?
This PR adds two Kubernetes add-on , (1) AWS Secrets Manager and Config Provider for Secret Store CSI Driver (2) Secrets Store CSI Driver
Motivation
AWS partner needing this add on to complete their development work.
Closes #435
More
pre-commit run -a
with this PRNote: Not all the PRs required examples and docs except a new pattern or add-on added.
For Moderators
Additional Notes