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

feat: Add support for AppMesh controller addon with AppMesh mTLS example #539

Merged
merged 11 commits into from
Oct 28, 2022

Conversation

svelango
Copy link
Contributor

@svelango svelango commented May 12, 2022

What does this PR do?

Adds AppMesh Addon

Motivation

Closes tickets as much as possible

More

  • [ X] Yes, I have tested the PR using my local account setup (Provide any test evidence report under Additional Notes)
  • Yes, I have added a new example under examples to support my PR
  • Yes, I have created another PR for add-ons under add-ons repo (if applicable)
  • [X ] 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

@kcoleman731 kcoleman731 left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you for the PR! Minor comments for you.

README.md Outdated Show resolved Hide resolved
modules/kubernetes-addons/aws-app-mesh/locals.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/aws-app-mesh/locals.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/aws-app-mesh/locals.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/aws-app-mesh/locals.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/aws-app-mesh/locals.tf Outdated Show resolved Hide resolved
modules/kubernetes-addons/aws-app-mesh/variables.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.

@svelango thanks for the PR. We will need some additional things:

  1. For gitops mode, please open another PR here. See other addons for example for what's needed.
  2. Add documentation under docs
  3. Add a detailed example under examples to demonstrate a non-trivial usage of the add-on.
  4. Provide some information in the PR comments on how this was tested.

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 for the PR @svelango Few minor comments needs incorporating and the comments from @kcoleman731

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/aws-app-mesh/locals.tf Outdated Show resolved Hide resolved
@github-actions
Copy link
Contributor

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added stale and removed stale labels Jun 28, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Aug 7, 2022

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Aug 7, 2022
@Zvikan
Copy link
Contributor

Zvikan commented Aug 10, 2022

@svelango any updates on this?

@github-actions github-actions bot removed the stale label Aug 11, 2022
@bryantbiggs bryantbiggs self-assigned this Aug 11, 2022
@svelango
Copy link
Contributor Author

The examples directory is there. Need to see why build failed. Checking.

@svelango
Copy link
Contributor Author

@svelango is ACM PCA required for AppMesh mTLS or can folks use cert-manager as the CA? If we can demonstrate this with cert-manager I think that would make the example more applicable to a broader audience and also easier for folks to get up and running with.

Ref: aws/aws-app-mesh-roadmap#34

@bryantbiggs Picked ACM as we are adding ACM addon into EKS so as a example added a sample for ACM.

@bryantbiggs
Copy link
Contributor

@svelango is ACM PCA required for AppMesh mTLS or can folks use cert-manager as the CA? If we can demonstrate this with cert-manager I think that would make the example more applicable to a broader audience and also easier for folks to get up and running with.
Ref: aws/aws-app-mesh-roadmap#34

@bryantbiggs Picked ACM as we are adding ACM addon into EKS so as a example added a sample for ACM.

Lets use cert-manager here please

@svelango
Copy link
Contributor Author

svelango commented Sep 2, 2022

@svelango is ACM PCA required for AppMesh mTLS or can folks use cert-manager as the CA? If we can demonstrate this with cert-manager I think that would make the example more applicable to a broader audience and also easier for folks to get up and running with.
Ref: aws/aws-app-mesh-roadmap#34

@bryantbiggs Picked ACM as we are adding ACM addon into EKS so as a example added a sample for ACM.

Lets use cert-manager here please

hi @bryantbiggs If I understand we wanted to use Cert-Manager and ACM PCA as Issuer and not directly work with ACM to get certificate?

@github-actions
Copy link
Contributor

github-actions bot commented Oct 3, 2022

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Oct 3, 2022
@github-actions
Copy link
Contributor

Pull request closed due to inactivity.

Copy link
Contributor

@allamand allamand left a comment

Choose a reason for hiding this comment

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

There is not output from terraform, so how can we connect to the cluster ?

Also It missing some Readme with guidance on how to uses this AppMesh add-on with a simple example to follow

README.md Outdated
# Deploy Kubernetes Add-ons with sub module
#--------------------------------------------
module "eks-blueprints-kubernetes-addons" {
source = "github.com/aws-ia/terraform-aws-eks-blueprints//modules/kubernetes-addons"
Copy link
Contributor

Choose a reason for hiding this comment

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

we should specify version here

README.md Outdated
enable_ingress_nginx = true
enable_appmesh_controller = true

depends_on = [module.eks-blueprints.managed_node_groups]
Copy link
Contributor

Choose a reason for hiding this comment

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

depend_on is not recommended with modules

@bryantbiggs bryantbiggs temporarily deployed to EKS Blueprints Test October 28, 2022 18:40 Inactive
@bryantbiggs bryantbiggs changed the title Adding AppMesh Addon feat: Add support for AppMesh controller addon with AppMesh mTLS example Oct 28, 2022
@bryantbiggs bryantbiggs merged commit 3a16188 into aws-ia:main Oct 28, 2022
vara-bonthu pushed a commit that referenced this pull request Nov 11, 2022
…ple (#539)

Co-authored-by: Elango Sundararajan <[email protected]>
Co-authored-by: Bryant Biggs <[email protected]>
allamand pushed a commit to allamand/terraform-aws-eks-blueprints that referenced this pull request Dec 15, 2022
allamand pushed a commit to allamand/terraform-aws-eks-blueprints that referenced this pull request Jan 10, 2023
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.

7 participants