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: Support ALB authentication using AWS Cognito #102

Merged
merged 5 commits into from
May 27, 2020
Merged

feat: Support ALB authentication using AWS Cognito #102

merged 5 commits into from
May 27, 2020

Conversation

dynamike
Copy link
Member

@dynamike dynamike commented Feb 21, 2020

This PR will allow this module to enforce authentication at the ALB, which is helpful when running Atlantis on the public internet. AWS Cognito is able to connect to your favorite identity provider using SAML.

Copy link
Member

@antonbabenko antonbabenko 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.

I have a few notes for you to update if you can:

  1. ALB module has support for all actions including authenticate-cognito (see: https://github.com/terraform-aws-modules/terraform-aws-alb/blob/567bf7b2e2455651b4973164a6e8c80101c816e7/examples/complete-alb/main.tf#L137-L156 )
  2. Remove provider github block and make it as an external/conditional dependency, if really necessary.
  3. Add example in examples folder showing complete integration - call Atlantis module and create required AWS Cognito resources.
  4. Rebase this PR. The master branch has been updated since you created this.

@dynamike
Copy link
Member Author

I'll get these updated. Thanks for the review!

@antonbabenko
Copy link
Member

@dynamike Please take a look at #122

@dynamike dynamike closed this May 27, 2020
@dynamike dynamike reopened this May 27, 2020
@dynamike dynamike changed the title Support ALB authentication using AWS Cognito fear: Support ALB authentication using AWS Cognito May 27, 2020
@dynamike dynamike changed the title fear: Support ALB authentication using AWS Cognito feat: Support ALB authentication using AWS Cognito May 27, 2020
@dynamike
Copy link
Member Author

@antonbabenko I think I've updated everything based off #123 and #122 . Did you still want me to put and a usage example in examples?

Copy link
Member

@antonbabenko antonbabenko left a comment

Choose a reason for hiding this comment

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

Looks very good!

README.md Outdated Show resolved Hide resolved
README.md Outdated
```

Read more in [this post](https://medium.com/@alsmola/alb-authentication-with-g-suite-saml-using-cognito-858e35564dc8) and a helpful [SAML Cognito Terraform module](https://github.com/alloy-commons/alloy-open-source/tree/master/terraform-modules/gsuite-saml-cognito).
Copy link
Member

Choose a reason for hiding this comment

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

medium.com is a paid service and not everyone can read that post. It would be great if you include relevant information into README of your module so that everything is in one place. And remove the link from this module to the blog post (unless you can find it available on free resource).

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried cleaning up this section and pointed to the example usage for the gsuite-saml-cognito module. Let me know if you have other thoughts.

Copy link
Member

Choose a reason for hiding this comment

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

Have you pushed your changes? I don't see them.

Copy link
Member Author

Choose a reason for hiding this comment

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

sigh. Sorry about that. They should be pushed now.

@antonbabenko
Copy link
Member

No need to update examples. I left couple small README-related comments for your review, and after those are fixed I will merge.

@antonbabenko antonbabenko merged commit c06bd5c into terraform-aws-modules:master May 27, 2020
@antonbabenko
Copy link
Member

Great! Thank you very much.

v2.17.0 has been just released.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants