implement postgres support for TLS #3488

nadilas · 2020-08-29T09:46:47Z

TLS support implemented for postgres plugin

The changes were tested by deploying cadence and cadence_visibility to a clean postgres database using TLS connection via the cadence-sql-tool

Potential risks
Backwards compatibility for password authentication to be tested by test-suite on a dev database instance

CLAassistant · 2020-08-29T09:46:52Z

All committers have signed the CLA.

longquanzheng · 2020-09-03T03:27:27Z

common/persistence/sql/sqlplugin/postgres/plugin.go

+	if attrs != "" {
+		dsn += "?" + attrs
+	}
+	return dsn
 }

 // TODO: implement postgres specific support for TLS


Should we remove the TODO? Or remove the method if it's not being used.

We could. The only reason I kept it in, because I thought it to be an interface implementation.
Although I must say, I did a pass at temporal as well and the source files were slightly different, I might do another commit before going forward to synchronize the two directly, if that’s okay for you?

Not quite follow exactly. Another commit for this? I thought this commit is working already. Or do you mean for other purpose.

Sorry about the confusion, it does work, however I want to synchronize with https://github.com/temporalio/temporal/blob/master/common/persistence/sql/sqlplugin/postgres/plugin.go so I will do an additional commit tomorrow, before we go ahead with this one. I hope that makes sense.

longquanzheng · 2020-09-03T03:30:58Z

Thank you for making this @nadilas for us!

longquanzheng · 2020-09-10T22:31:42Z

common/persistence/sql/sqlplugin/postgres/plugin.go

-	// Maps struct names in CamelCase to snake without need for db struct tags.
-	db.MapperFunc(strcase.ToSnake)
-	return db, nil
+	dsn := fmt.Sprintf(dsnFmt, "postgresql", cfg.User, host, port, dbName)


Tests are failling. Seems like this is not compatible with pq server that without TSL enabled:

FAIL: TestSQLHistoryV2PersistenceSuite (0.00s) | 1s -- | -- | panic: pq: SSL is not enabled on the server [recovered] | panic: pq: SSL is not enabled on the server | | goroutine 10 [running]: | testing.tRunner.func1(0xc000392300) | /usr/local/go/src/testing/testing.go:874 +0x69f | panic(0x2030fe0, 0xc0000b8880) | /usr/local/go/src/runtime/panic.go:679 +0x1b2 | github.com/uber/cadence/common/persistence/sql.(*TestCluster).CreateDatabase(0xc0005a4840) | /cadence/common/persistence/sql/sqlPersistenceTest.go:113 +0x2df | github.com/uber/cadence/common/persistence/sql.(*TestCluster).SetupTestDatabase(0xc0005a4840) | /cadence/common/persistence/sql/sqlPersistenceTest.go:74 +0x51 | github.com/uber/cadence/common/persistence/persistence-tests.(*TestBase).Setup(0xc0003c2658) | /cadence/common/persistence/persistence-tests/persistenceTestBase.go:178 +0xdf | github.com/uber/cadence/common/persistence/sql/sqlplugin/postgres.TestSQLHistoryV2PersistenceSuite(0xc000392300) | /cadence/common/persistence/sql/sqlplugin/postgres/postgres_server_test.go:68 +0xed | testing.tRunner(0xc000392300, 0x22e8f98) | /usr/local/go/src/testing/testing.go:909 +0x19a | created by testing.(*T).Run | /usr/local/go/src/testing/testing.go:960 +0x652 | FAIL github.com/uber/cadence/common/persistence/sql/sqlplugin/postgres 0.041s | FAIL | make: *** [Makefile:196: cover_profile] Error 1 | ⚠️ Failed to run command, exited with 2

I just made the necessary adjustments, can you please test again?

Still not passing:

panic: pq: password authentication failed for user "postgres" [recovered] -- | panic: pq: password authentication failed for user "postgres"

You can see it if you click the details in
buildkite/cadence-server/pr/golang-unit-test — Failed (exit status 2)

Are you able to run the test locally? That will be easier to debug and fix the test.

yeah e2e tests

Sure since we have it in this repo. The reason we didn't do that is that we ran into timing issues with buildkite. But it should be fine since we are running tests in parallel now.

Synchronize with https://github.com/temporalio/temporal/blob/master/common/persistence/sql/sqlplugin/postgres/plugin.go

meiliang86 · 2020-09-11T16:06:34Z

Unrelated to this, we might want to add the pg tests to buildkite, similar to what we do for MySQL. @longquanzheng what do you think?

longquanzheng · 2020-09-11T17:17:25Z

Unrelated to this, we might want to add the pg tests to buildkite, similar to what we do for MySQL. @longquanzheng what do you think?

https://github.com/uber/cadence/blob/master/docker/buildkite/docker-compose.yml
I thought we already have pg for running "unit" test, but not e2e integ test. Do you mean that?
We could let it run too.

meiliang86 · 2020-09-14T21:55:39Z

yeah e2e tests

meiliang86 · 2020-09-14T21:56:47Z

@nadilas unit tests are still failing. Can you check?

FAIL: TestSQLHistoryV2PersistenceSuite (0.00s) | 1s
-- | --
  | panic: pq: password authentication failed for user "postgres" [recovered]
  | panic: pq: password authentication failed for user "postgres"
  |  
  | goroutine 13 [running]:
  | testing.tRunner.func1(0xc000300200)
  | /usr/local/go/src/testing/testing.go:874 +0x69f
  | panic(0x207e420, 0xc0003b4000)
  | /usr/local/go/src/runtime/panic.go:679 +0x1b2
  | github.com/uber/cadence/common/persistence/sql.(*TestCluster).CreateDatabase(0xc0005ac6e0)
  | /cadence/common/persistence/sql/sqlPersistenceTest.go:113 +0x2df
  | github.com/uber/cadence/common/persistence/sql.(*TestCluster).SetupTestDatabase(0xc0005ac6e0)
  | /cadence/common/persistence/sql/sqlPersistenceTest.go:74 +0x51
  | github.com/uber/cadence/common/persistence/persistence-tests.(*TestBase).Setup(0xc00003e798)
  | /cadence/common/persistence/persistence-tests/persistenceTestBase.go:178 +0xdf
  | github.com/uber/cadence/common/persistence/sql/sqlplugin/postgres.TestSQLHistoryV2PersistenceSuite(0xc000300200)
  | /cadence/common/persistence/sql/sqlplugin/postgres/postgres_server_test.go:68 +0xed
  | testing.tRunner(0xc000300200, 0x22e90f8)
  | /usr/local/go/src/testing/testing.go:909 +0x19a
  | created by testing.(*T).Run
  | /usr/local/go/src/testing/testing.go:960 +0x652
  | FAIL	github.com/uber/cadence/common/persistence/sql/sqlplugin/postgres	0.043s

longquanzheng · 2020-09-24T05:15:33Z

common/persistence/sql/sqlplugin/postgres/plugin.go

-	dataSourceNamePostgres         = "user=%v host=%v port=%v dbname=%v sslmode=disable %v "
-	dataSourceNamePostgresPassword = "password=%v"
+	PluginName = "postgres"
+	dsnFmt = "postgres://%s@%s:%s/%s"


Is the test failure because this is not compatible with basic authentication?

longquanzheng · 2020-09-28T23:00:47Z

Thank you very much for making this PR.

@nadilas Hey I added more document to https://github.com/uber/cadence/blob/master/CONTRIBUTING.md
Hope it will help contributors from external like you!

Or I can take this over, if it's too complex for you :P

nadilas · 2020-09-29T21:29:09Z

I’m sorry @longquanzheng Life happened, I got sidetracked by my day job 😅
I’ll clean this up this weekend.

But if you have it urgent feel free to step in.

longquanzheng · 2020-09-29T22:06:16Z

I’m sorry @longquanzheng Life happened, I got sidetracked by my day job 😅
I’ll clean this up this weekend.

But if you have it urgent feel free to step in.

Oh thanks for the reply. It's not urgent at all. Feel free to slack me when you got stuck. It's not really easy thing to start development for Cadence project. We really love to have your contribution.

longquanzheng · 2020-10-12T00:50:52Z

@meiliang86 I just rebased the PR, can you help kick off the test?

longquanzheng · 2020-10-12T20:52:46Z

@nadilas looks like my rebase at Github UI doens't work, we need to do rebase manually so re-sync the go.mod. I don't have permission to push into your branch, could you rebase it?

nadilas · 2020-10-13T21:28:44Z

@longquanzheng I updated the branch, let's try this again :)

longquanzheng · 2020-10-13T22:15:01Z

@longquanzheng I updated the branch, let's try this again :)

It failed at :

make: *** [Makefile:171: fmt] Error 1

Can you run

make fmt

to re-format the code?
(I just made a PR to update our contribute guide to include that instruction: https://github.com/uber/cadence/pull/3642/files)

common/persistence/sql/sqlplugin/postgres/postgres_server_test.go

…/plugin.go removes duplicate getTestClusterSetup() from postgres_server_test.go ran make fmt

nadilas · 2020-10-14T16:51:38Z

@longquanzheng ready to rumble.

coveralls · 2020-10-14T19:37:00Z

Coverage increased (+0.02%) to 65.141% when pulling 0439167 on nadilas:patch-1 into ba0eacb on uber:master.

meiliang86 · 2020-10-14T22:01:03Z

Thanks @nadilas and @longquanzheng !

JosefWN · 2020-11-20T14:25:17Z

common/persistence/sql/sqlplugin/postgres/plugin.go

+	sslParams = url.Values{}
+	if cfg.TLS != nil && cfg.TLS.Enabled {
+		sslParams.Set("ssl", "true")
+		sslParams.Set("sslmode", "require")


I know I'm late to the party, but is it wise to hard code this? How would I connect using the more secure modes verify-ca or verify-full for example?

Could as well let the user choose?

sslParams.Set("sslmode", cfg.TLS.SSLMode)

Just looking at the config it's hard to know what cfg.TLS.Enabled means; is it allow, prefer, require, verify-ca or verify-full? All of those modes are enabling TLS support to various degrees.

Hey @JosefWN you are not late! Apparently I don't have enough expertise/experience about security so I didn't think too much about that.
Is value of sslmode the only thing needed to change to support all other modes like you mention? If so then I could make a PR out for this.

Or it would be nice if you can make a PR for this. Because I am not sure how to test it.

I think making the sslmode configurable is sufficient (which would also make cfg.TLS.Enabled redundant).

Perhaps something like this:

sslParams.Set("sslmode", cfg.SSLMode) if cfg.SSLMode != "disable" && cfg.TLS != nil { sslParams.Set("sslrootcert", cfg.TLS.CaFile) sslParams.Set("sslkey", cfg.TLS.KeyFile) sslParams.Set("sslcert", cfg.TLS.CertFile) }

The default is prefer, so it should be backwards compatible even if SSLMode is not set (but would need to test that it actually works).

Thanks a lot! PR out here: https://github.com/uber/cadence/pull/3787/files
But I think we have to make default as require now...since people started to use it. I believe it's okay, lmk if I am wrong.

implement postgres support for TLS

4f517f4

anish531213 and others added 2 commits August 31, 2020 14:34

Merge branch 'master' into patch-1

94c8cef

Merge branch 'master' into patch-1

482f3d8

longquanzheng reviewed Sep 3, 2020

View reviewed changes

Merge branch 'master' into patch-1

6484cbf

longquanzheng reviewed Sep 10, 2020

View reviewed changes

nadilas and others added 2 commits September 11, 2020 11:49

Syncrhonize with Temporal postgres plugin

55e5da9

Synchronize with https://github.com/temporalio/temporal/blob/master/common/persistence/sql/sqlplugin/postgres/plugin.go

Merge branch 'master' into patch-1

de0fde0

Merge branch 'master' into patch-1

717e016

longquanzheng mentioned this pull request Sep 21, 2020

Add e2e integration tests for Postgres and remove 2dc runs #3532

Merged

longquanzheng reviewed Sep 24, 2020

View reviewed changes

nadilas and others added 10 commits October 3, 2020 17:32

implement postgres support for TLS

41b52b6

Enforce re-replication context timeout for standby tasks (#3473)

674eb00

Enable processing queue split policy by domainID (#3486)

995493f

Allow DLQ cli use a range of shard ids (#3481)

a43df67

Integrate current execution check with replication resender (#3487)

ed904c2

Do not extend activity expiration time (#3489)

c65e4c9

Start queue processor before failover callback registration (#3494)

f383e1e

Clean up kafka replicagtion in worker (#3493)

c73d42d

[SQL]Fix upsert SQL template for Postgres plugin (#3498)

936ba28

Ignore reapplication if the domain is pending active (#3502)

3d778ed

Merge branch 'master' into patch-1

3d18b6b

Merge branch 'master' into patch-1

dbc483f

Merge branch 'master' into patch-1

275ff1b

longquanzheng approved these changes Oct 13, 2020

View reviewed changes

longquanzheng reviewed Oct 13, 2020

View reviewed changes

common/persistence/sql/sqlplugin/postgres/postgres_server_test.go Outdated Show resolved Hide resolved

longquanzheng mentioned this pull request Oct 13, 2020

Update CONTRIBUTING.md to include make fmt command #3642

Merged

fixes testPassword issue blocking postgres_server_test.go in postgres…

f03a71b

…/plugin.go removes duplicate getTestClusterSetup() from postgres_server_test.go ran make fmt

Merge branch 'master' into patch-1

36da622

longquanzheng added automerge and removed automerge labels Oct 14, 2020

János Veres and others added 4 commits October 14, 2020 19:44

fixes go.sum updates needed issue

c55e87a

Merge branch 'master' into patch-1

2aaa1d8

tidy go.sum

019b1b1

Merge branch 'master' into patch-1

0439167

longquanzheng added automerge and removed automerge labels Oct 14, 2020

longquanzheng merged commit 0ea68b8 into cadence-workflow:master Oct 14, 2020

longquanzheng mentioned this pull request Oct 22, 2020

Connecting to Postgresql with SSL #3344

Closed

JosefWN reviewed Nov 20, 2020

View reviewed changes

longquanzheng mentioned this pull request Nov 22, 2020

Support different SSL modes #3787

Merged

github-actions bot pushed a commit to vytautas-karpavicius/cadence that referenced this pull request Feb 4, 2021

implement postgres support for TLS (cadence-workflow#3488)

60e4f38

yux0 pushed a commit to yux0/cadence that referenced this pull request May 4, 2021

implement postgres support for TLS (cadence-workflow#3488)

dc00759

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement postgres support for TLS #3488

implement postgres support for TLS #3488

nadilas commented Aug 29, 2020

CLAassistant commented Aug 29, 2020 •

edited

Loading

longquanzheng Sep 3, 2020 •

edited

Loading

nadilas Sep 3, 2020

longquanzheng Sep 3, 2020

nadilas Sep 7, 2020

longquanzheng commented Sep 3, 2020

longquanzheng Sep 10, 2020

nadilas Sep 11, 2020

longquanzheng Sep 11, 2020

longquanzheng Sep 11, 2020

longquanzheng Sep 18, 2020

meiliang86 commented Sep 11, 2020

longquanzheng commented Sep 11, 2020

meiliang86 commented Sep 14, 2020

meiliang86 commented Sep 14, 2020

longquanzheng Sep 24, 2020

longquanzheng commented Sep 28, 2020

nadilas commented Sep 29, 2020

longquanzheng commented Sep 29, 2020

longquanzheng commented Oct 12, 2020

longquanzheng commented Oct 12, 2020

nadilas commented Oct 13, 2020

longquanzheng commented Oct 13, 2020 •

edited

Loading

nadilas commented Oct 14, 2020

coveralls commented Oct 14, 2020

meiliang86 commented Oct 14, 2020

JosefWN Nov 20, 2020 •

edited

Loading

longquanzheng Nov 21, 2020

JosefWN Nov 21, 2020

longquanzheng Nov 22, 2020

implement postgres support for TLS #3488

implement postgres support for TLS #3488

Conversation

nadilas commented Aug 29, 2020

CLAassistant commented Aug 29, 2020 • edited Loading

longquanzheng Sep 3, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

longquanzheng commented Sep 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meiliang86 commented Sep 11, 2020

longquanzheng commented Sep 11, 2020

meiliang86 commented Sep 14, 2020

meiliang86 commented Sep 14, 2020

Choose a reason for hiding this comment

longquanzheng commented Sep 28, 2020

nadilas commented Sep 29, 2020

longquanzheng commented Sep 29, 2020

longquanzheng commented Oct 12, 2020

longquanzheng commented Oct 12, 2020

nadilas commented Oct 13, 2020

longquanzheng commented Oct 13, 2020 • edited Loading

nadilas commented Oct 14, 2020

coveralls commented Oct 14, 2020

meiliang86 commented Oct 14, 2020

JosefWN Nov 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CLAassistant commented Aug 29, 2020 •

edited

Loading

longquanzheng Sep 3, 2020 •

edited

Loading

longquanzheng commented Oct 13, 2020 •

edited

Loading

JosefWN Nov 20, 2020 •

edited

Loading