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

mysql/cloudsql: ensure package works on Go 1.11 App Engine #494

Closed
vangent opened this issue Sep 28, 2018 · 21 comments
Closed

mysql/cloudsql: ensure package works on Go 1.11 App Engine #494

vangent opened this issue Sep 28, 2018 · 21 comments
Assignees
Labels
enhancement New feature or request

Comments

@vangent
Copy link
Contributor

vangent commented Sep 28, 2018

In GAE Standard, you have a built-in socket to CloudSQL at /cloudsql.

To use it, set host = '/cloudsql/{db_connection_name}'

Here's a Python sample using it.

In case we need the Go Cloud code to behave differently on GAE Standard, look for the GAE_ENV env var.

Here's some more sample code in Go:

func GetDB() *sql.DB {
	user := os.Getenv("DB_USER")
	db := os.Getenv("DB_DATABASE")
	inst := os.Getenv("DB_INSTANCE")
	pw := os.Getenv("DB_PASSWORD")

	// format: user:pw@unix(/cloudsql/<project-id>:<region>:<inst>)/dbname
	// Note that DB_INSTANCE is already in the format of project-id:region:instance-name
	dbURI := fmt.Sprintf("%s:%s@unix(/cloudsql/%s)/%s", user, pw, inst, db)
	// go-sql-driver/mysql has a special cloudsql driver, which has the format
	// user:pw@cloudsql(project-id:region:instance-name)/dbname
	// Except, this opens a connection to cloudsql://..., which GAE TI doesn't support.
	// broken := fmt.Sprintf("%s:%s@cloudsql(%s)/%s", user, pw, inst, db),
	conn, err := sql.Open("mysql", dbURI)
        ...

We might need to set the Addr field differently here, but not sure.

@vangent
Copy link
Contributor Author

vangent commented Sep 28, 2018

@steren

@zombiezen
Copy link
Contributor

Discussed with @ijt a bit offline. I'm not entirely clear: why is it necessary to act differently on App Engine versus using the normal network path? I'd rather that the codepaths stay the same wherever possible.

@steren
Copy link

steren commented Oct 9, 2018

The App Engine Standard environment cannot access the "normal network path".
The only way to connect privately to a Cloud SQL instance is using the /cloudsql socket.

@zombiezen
Copy link
Contributor

Even using the new Second Generation runtimes? #210 details some of the blockers with current runtimes; Go Cloud can only target the Second Generation runtimes anyway.

@steren
Copy link

steren commented Oct 9, 2018

Yes, Go 1.11 on App Engine Standard, a second generation runtime, is shipping this week and I confirm that the only way to connect privately to a Cloud SQL instance is via /cloudsql.

@ijt
Copy link
Contributor

ijt commented Oct 9, 2018

Here's a quote from Jason on the GAE runtimes team, regarding why it's not a good idea to avoid using the /cloudsql socket from GAE:

Ya, you need to open your database firewall rules effectively wide open to the internet.
It's quite risky which is why the /cloudsql solution is better.

@ijt
Copy link
Contributor

ijt commented Oct 9, 2018

The use of the /cloudsql socket is something most users of Go Cloud likely won't notice. If we find that it becomes better not to use it in the future, it's probably easy to do that without breaking any apps.

@steren
Copy link

steren commented Oct 10, 2018

Yep, as the Product Manager for Go on App Engine, I can confirm Jason's recommendation, and that's actually why I reached your team :)

In addition, be aware that Google Cloud Functions is also exposing /cloudsql as the only way to access Cloud SQL privately from a Cloud Function. (CC @stewblr PM for Cloud Functions)

@zombiezen
Copy link
Contributor

zombiezen commented Oct 10, 2018

Even if the process is running the Cloud SQL proxy? My understanding was that Cloud SQL proxy somehow bridges the traffic over TLS and thus does not require messing with the firewall configuration.

Go Cloud's current behavior is that it links the Cloud SQL proxy in as a library (yay Go!) and runs traffic that way.

@squee1945
Copy link
Member

For GAE Standard and GCF, the /cloudsql unix domain socket technique is the preferred way to connect to Cloud SQL without requiring special network configurations. Our client libraries should support this technique.

@sbuss
Copy link

sbuss commented Oct 10, 2018

/cloudsql is the most performant and secure option in GAE standard, it must be supported in this library

@zombiezen
Copy link
Contributor

@squee1945 and I chatted a bit about this issue.

First, stating explicitly because at least @squee1945 was tripped up this: this repository (Go Cloud) is not the Go GCP client libraries, despite the very similar naming.

Second, with Go Cloud, there is no additional setup for connecting to Cloud SQL other the parameters below:

type Params struct {
ProjectID string
Region string
Instance string
User string
Password string // may be empty, see https://cloud.google.com/sql/docs/sql-proxy#user
Database string
}

These parameters mirror the ones necessary for the /cloudsql mount. Because Go Cloud uses the Cloud SQL proxy as a library, it does not require modifying the firewall settings. The usual instructions for installing the proxy do not apply, because the logic is statically linked.

The final concern that @squee1945 raised was around performance. For that, I don't have any benchmarks or information at this time. I can see the cloudsql.Open function in this repository using the /cloudsql mount when available to provide the same functionality, although there could be some slight confusion as the source of credentials could be different (but not by default).

However, I am not sure how to reconcile all this information with @vangent's original report. @vangent, have you explicitly confirmed that dialing via Cloud SQL proxy is failing on the new runtime? And if so, can you elaborate on what the precise failure is?

@steren
Copy link

steren commented Oct 10, 2018

We are aware that this is not the Go Google Cloud client library. However, I expect Go Cloud to adapt to all cloud providers, this includes App Engine Standard on GCP.

Today Go Cloud mysql does not work on the Go 1.11 runtime for app engine. This is why this bug was opened.

@zombiezen
Copy link
Contributor

Acknowledged, but I haven't seen the due diligence on why it doesn't work. The explanations given do not make sense to me.

@vangent
Copy link
Contributor Author

vangent commented Oct 10, 2018

I have no evidence that it does not work other than the assertions in this issue and related email threads (e.g., The App Engine Standard environment cannot access the "normal network path".).

@zombiezen zombiezen added the in progress This is being actively worked on label Oct 12, 2018
@zombiezen zombiezen modified the milestones: Sprint 17, Sprint 18 Oct 12, 2018
@steren
Copy link

steren commented Oct 12, 2018

Just to follow up: the requirement is for the Go Cloud MySQL package to work on the Go 1.11 App Engine standard environment.

Whatever mechanism you want to use is left to your discretion.

  • Be aware that a /cloudsql socket is available
  • if the Cloud SQL proxy works, then it's also a fine solution

@steren
Copy link

steren commented Oct 12, 2018

I recommend renaming that issue with the exact requirement, without specifying a solution.

@squee1945
Copy link
Member

if the Cloud SQL proxy works, then it's also a fine solution

Modulo memory requirement; GAE Std F1's only have 128MB for user code and data.

@zombiezen zombiezen changed the title mysql/cloudsql: make cloudsql work on AppEngine with local /cloudsql socket mysql/cloudsql: ensure package works on Go 1.11 App Engine Oct 12, 2018
@zombiezen
Copy link
Contributor

@steren Good suggestion. I retitled the issue.

@ijt ijt removed their assignment Oct 17, 2018
@ijt
Copy link
Contributor

ijt commented Oct 17, 2018

I see no reason not to give Go Cloud users the option to use the /cloudsql socket. Then they can use Go Cloud with MySQL on App Engine without having to bring in this proxy that they normally wouldn't have to use on App Engine.

@zombiezen
Copy link
Contributor

zombiezen commented Oct 17, 2018

@ijt and I discussed during sprint planning: it's more a matter of ensuring that we are testing that this works properly. If we are supporting multiple code paths to connect to Cloud SQL, then we have to make sure we have testing in place to address both of them. The difficulty with this new codepath is that it must be tested in a specific hosting environment: it can't run on Travis, for instance.

@shantuo shantuo modified the milestones: Sprint 18, Sprint 19 Nov 1, 2018
@vangent vangent modified the milestones: Sprint 19, Sprint 20 Nov 20, 2018
@vangent vangent modified the milestones: Sprint 20, Sprint 21 Dec 14, 2018
@vangent vangent modified the milestones: Sprint 21, Sprint 22 Jan 22, 2019
@vangent vangent modified the milestones: Sprint 22, Sprint 23 Feb 11, 2019
@vangent vangent removed this from the Sprint 23 milestone Mar 1, 2019
@vangent vangent closed this as completed Aug 10, 2019
@go-cloud-bot go-cloud-bot bot removed the in progress This is being actively worked on label Aug 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants