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 redis TLS support #60

Merged
merged 1 commit into from
Jul 16, 2021

Conversation

jmymy
Copy link
Contributor

@jmymy jmymy commented Jul 13, 2021

No description provided.

@comment-bot-dev
Copy link

comment-bot-dev commented Jul 13, 2021

Thanks for the PR! 🚀
✅ Lint checks have passed.

@jmymy jmymy marked this pull request as ready for review July 13, 2021 17:42
Copy link
Member

@bharathkkb bharathkkb left a comment

Choose a reason for hiding this comment

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

LGTM thanks for adding the test!

variables.tf Outdated
variable "transit_encryption_mode" {
description = "The TLS mode of the Redis instance, If not provided, TLS is disabled for the instance."
type = string
default = "DISABLED"
Copy link
Member

Choose a reason for hiding this comment

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

This seems like something we should enable by default with a breaking release although there are some performance caveats.
/cc @morgante

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, we should cut a breaking change for this. It looks more secure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enable redis_auth as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not. For auth, I think IAM is better.

Choose a reason for hiding this comment

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

This feature requires the Certificate Authorities (CA) that are used to verify the identity of the server, see: https://cloud.google.com/memorystore/docs/redis/in-transit-encryption

I think the output should include the cert so I can say, save it in Secret Manager for use by a VM or Container.

I might be able to fork and implement the CA feature at some future time, but not for a few weeks at least. Should I open a new issue or PR to change the default to "DISABLED" for now?

Thank you for your work on the modules by the way, they have been very helpful!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open an issue please.. i might be able to get to it today

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#99

Choose a reason for hiding this comment

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

Wow, blown away by the help. Let me know if I can contribute anything to follow up (docs, testing). Cheers!

@jmymy jmymy changed the title feat: add redis TLS support feat!: add redis TLS support Jul 14, 2021
@jmymy
Copy link
Contributor Author

jmymy commented Jul 16, 2021

@bharathkkb @morgante good to go on this? Would like to close out this week so I can use it next week.

@morgante morgante merged commit 75c7033 into terraform-google-modules:master Jul 16, 2021
@jmymy jmymy deleted the redis-tls branch July 23, 2021 13:07
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.

5 participants