Skip to content
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

AWS PCA Issuer #359

Merged
merged 3 commits into from
Apr 28, 2022
Merged

AWS PCA Issuer #359

merged 3 commits into from
Apr 28, 2022

Conversation

anshrma
Copy link
Contributor

@anshrma anshrma commented Mar 18, 2022

What does this PR do?

This PR adds a new module for helm installation of AWS PCA issuer, which is an extension of cert-manager for certificate management using AWS ACM.

Motivation

Working with a large Partner in API Gateway space (another PR coming next), which requires this

More

  • [ Y] Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • [ NA] Yes, I have added a new example under examples to support my PR
  • [NA] Yes, I have created another PR for add-ons under add-ons repo (if applicable)
  • [ Y] Yes, I have updated the docs for this feature
  • Yes, I ran pre-commit run -a with this PR

Note: Not all the PRs required examples and docs except a new pattern or add-on added.

For Moderators

  • E2E Test successfully complete before merge?

Additional Notes

Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @anshrma for the PR. This will be a great add-on. Would you mind creating a pattern to show with example on how to use this Add-on?

Update docs page as well here https://github.com/aws-samples/aws-eks-accelerator-for-terraform/tree/main/docs/add-ons

modules/kubernetes-addons/aws-pca-issuer/main.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/aws-pca-issuer/variables.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/main.tf Outdated Show resolved Hide resolved
Copy link
Contributor

@askulkarni2 askulkarni2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anshrma thanks so much for the PR. Its looking good, some minor changes are needed as per our convention. Also please address @vara-bonthu's comments as well.

docs/add-ons/aws-pca-issuer.md Outdated Show resolved Hide resolved
modules/kubernetes-addons/aws-pca-issuer/README.md Outdated Show resolved Hide resolved
modules/kubernetes-addons/aws-pca-issuer/locals.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/aws-pca-issuer/main.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/aws-pca-issuer/locals.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/main.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/main.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/main.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/variables.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/variables.tf Outdated Show resolved Hide resolved
@anshrma
Copy link
Contributor Author

anshrma commented Mar 22, 2022

@vara-bonthu / @askulkarni2 - Thanks for review. Addressed all the feedback with updated code. Pls let me know, if anything else. Once this gets merged, next i will send another PR for a partner using these add-ons

Copy link
Contributor

@askulkarni2 askulkarni2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anshrma some nitpicky comments, but PR is looking really good.

One question on whether we should remove all IRSA for cert-manager. I don't have enough knowledge there, will wait on a few confirmations there from @vara-bonthu, @kcaws.

docs/add-ons/aws-privateca-issuer.md Outdated Show resolved Hide resolved
docs/add-ons/aws-privateca-issuer.md Show resolved Hide resolved
docs/add-ons/aws-privateca-issuer.md Outdated Show resolved Hide resolved
examples/tls-with-aws-pca-issuer/main.tf Show resolved Hide resolved
examples/tls-with-aws-pca-issuer/main.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/main.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/variables.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/main.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/variables.tf Outdated Show resolved Hide resolved
@anshrma anshrma requested a review from askulkarni2 March 25, 2022 23:09
@anshrma
Copy link
Contributor Author

anshrma commented Mar 25, 2022

@askulkarni2 / @vara-bonthu - Addressed all the review feedback.

Copy link
Contributor

@askulkarni2 askulkarni2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@anshrma thanks for the updates. LGTM!

Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work on this PR @anshrma.

Please see comments and it requires few changes.

examples/tls-with-aws-pca-issuer/README.md Outdated Show resolved Hide resolved
examples/tls-with-aws-pca-issuer/README.md Outdated Show resolved Hide resolved
examples/tls-with-aws-pca-issuer/main.tf Outdated Show resolved Hide resolved
examples/tls-with-aws-pca-issuer/main.tf Outdated Show resolved Hide resolved
examples/tls-with-aws-pca-issuer/main.tf Show resolved Hide resolved
modules/kubernetes-addons/cert-manager/locals.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/cert-manager/locals.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/cert-manager/locals.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/main.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/variables.tf Outdated Show resolved Hide resolved
@vara-bonthu
Copy link
Contributor

Yes, I have provided the IRSA policies above

@askulkarni2
Copy link
Contributor

@anshrma have you had a chance to review @vara-bonthu's feedback?

@anshrma
Copy link
Contributor Author

anshrma commented Apr 14, 2022

@anshrma have you had a chance to review @vara-bonthu's feedback?

Hi @askulkarni2 - Will pick this up tomorrow. Back today after PTO. Thanks

@anshrma
Copy link
Contributor Author

anshrma commented Apr 15, 2022

Thanks for all the feedback @vara-bonthu . Done with all the changes suggested.

@anshrma anshrma requested a review from vara-bonthu April 15, 2022 04:04
@anshrma
Copy link
Contributor Author

anshrma commented Apr 25, 2022

hi @vara-bonthu / @askulkarni2 - Pls review this when you get a chance. All the feedback comments have been addressed.

Copy link
Contributor

@vara-bonthu vara-bonthu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍🏼 Needs to run E2E

@askulkarni2
Copy link
Contributor

@anshrma I am running our e2e tests and I see some terraform fmt errors. See https://github.com/aws-ia/terraform-aws-eks-blueprints/runs/6201070488?check_suite_focus=true. Can you please fix them?

FYI.. I am also running terratest here https://github.com/aws-ia/terraform-aws-eks-blueprints/runs/6201005974?check_suite_focus=true.

@anshrma
Copy link
Contributor Author

anshrma commented Apr 27, 2022

@anshrma I am running our e2e tests and I see some terraform fmt errors. See https://github.com/aws-ia/terraform-aws-eks-blueprints/runs/6201070488?check_suite_focus=true. Can you please fix them?

FYI.. I am also running terratest here https://github.com/aws-ia/terraform-aws-eks-blueprints/runs/6201005974?check_suite_focus=true.

hi @askulkarni2 - fixed the terraform fmt errors

@askulkarni2 askulkarni2 merged commit acd5ffa into aws-ia:main Apr 28, 2022
alidonmez pushed a commit to alidonmez/terraform-aws-eks-blueprints-1 that referenced this pull request Mar 25, 2023
…n/test/util/types/node-18.14.6

Bump @types/node from 18.11.18 to 18.14.6 in /test/util
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants