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

Postgres support for private_service_access #43

Merged
merged 3 commits into from
Aug 16, 2019
Merged

Postgres support for private_service_access #43

merged 3 commits into from
Aug 16, 2019

Conversation

dpetersen
Copy link
Contributor

I am trying to use the postgresql submodule with the private_service_access submodule that was created as part of #35 for MySQL (really appreciate that work!). There's nothing in private_service_access that's MySQL specific, but the peering_completed variable workaround to enforce dependency order was only added to safer_mysql.

I also added a private_address output to postgresql, trying to mirror the preexisting instance_address output. The README for the project says this works with a 1.12.x version of the google provider, but private_ip_address wasn't introduced until version version 2.1.0. Does the README cover submodules, too? What's your policy on requiring a newer version of the provider?

I tried to stick as closely as possible to naming and descriptions to what was already in the codebase. Let me know if you'd like anything changed. With these changes I was able to use private networking on our postgres instance, which is awesome. Thanks!

This is taken as-is from the safer_mysql module. It's a way for this
module to optionally depend upon the private_service_access module.
@aaron-lane aaron-lane added this to In progress in Cloud Foundation Toolkit Jul 4, 2019
@namusyaka namusyaka self-requested a review July 6, 2019 12:26
Cloud Foundation Toolkit automation moved this from In progress to Review in progress Jul 9, 2019
Copy link
Contributor

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

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

@dpetersen Thanks for the contribution. I've left a comment about this changes. Please take a look.

modules/postgresql/main.tf Outdated Show resolved Hide resolved
@aodj
Copy link

aodj commented Aug 7, 2019

How is this PR looking? It's exactly what I'm waiting for (along with 0.12 changes) but it seems to have stalled

On Postgres instances that are not passed the peering_completed ordering
variable, there's no reason to set a label. This only sets the label
(which is a necessary evil to ensure dependency ordering) in scenarios
where it's required.
@dpetersen
Copy link
Contributor Author

@aodj thank you for the reminder. I have been busy and let this slide. I just made the modifications that @namusyaka suggested and they seem to work as desired. Instances that aren't passed peering_completed no longer set the label, while instances with peering_completed successfully wait for the peering to be created and are labeled.

Thanks, everybody. Sorry for the delay.

@aodj
Copy link

aodj commented Aug 8, 2019

@dpetersen Amazing!

@morgante morgante requested a review from namusyaka August 8, 2019 09:13
@namusyaka
Copy link
Contributor

I'm sorry for the late response.
I'll review this change in a few days.
Thanks

@morgante morgante removed this from Review in progress in Cloud Foundation Toolkit Aug 13, 2019
Copy link
Contributor

@namusyaka namusyaka left a comment

Choose a reason for hiding this comment

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

The change looks good. But considering the consistency for module design, I'm not sure if this change should be adopted into the postgresql module. Extracting this change as new module (e.g. safer_postgresql) is better, right?

@dpetersen @morgante What do you think?

@morgante
Copy link
Contributor

I'm fine with directly adding it.

@dpetersen
Copy link
Contributor Author

I only really read the MySQL code enough to know what changes needed to be made to support private networking. I actually though the "safe" part was a reference to mysqld-safe! But now I see it's a hardened alternative to the main module.

While using private networking on CloudSQL Postgres is probably safer than public, there are other reasons to prefer private networking: no need to configure a CloudSQL Proxy and lower latency from your VPC. The CloudSQL proxy feature was our primary motivation to run private instances, since the proxy as a sidecar container doesn't play well with Kubernetes Jobs. I think you'll want this change in this module regardless of whether you create a hardened safer_postgresql module, too.

@namusyaka
Copy link
Contributor

Hmm OK. After migrating our modules into 0.12, we should be able to drop the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants