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

temporal-sql-tool: add missing err returns #2250

Merged

Conversation

danielhochman
Copy link
Contributor

What changed?
There were two instances I found in temporal-sql-tool where an error was logged but not returned, causing execution to continue erroneously and ultimately panic.

I think using Fatal instead of Error would be better than having to ensure these returns are everywhere, but I wanted to keep the change set small.

Why?
In my case this resulted in a harmless panic when running the update-schema command of temporal-sql-tool with a bad password.

Before

Error is logged and execution continues.

panic: runtime error: invalid memory address or nil pointer dereference ...

2021-12-02T14:42:21.990-0600 ERROR Unable to connect to SQL database. {"error": "pq: password authentication failed for user "temporal_admin"", "logging-call-at": "handler.go:73"}
2021-12-02T14:42:21.990-0600 INFO UpdateSchemeTask started {"config": {"DBName":"","TargetVersion":"","SchemaDir":"/tmp/temporal-1.13.1/schema/postgresql/v96/temporal/versioned","IsDryRun":false}, "logging-call-at": "updatetask.go:98"}
panic: runtime error: invalid memory address or nil pointer dereference
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x10 pc=0xec4114]

goroutine 1 [running]:
go.temporal.io/server/tools/sql.(*Connection).Close(0xc000050000)
/tmp/temporal-1.13.1/tools/sql/conn.go:123 +0x14
panic({0xf7ea00, 0x1a85d60})
/usr/local/go/src/runtime/panic.go:1038 +0x215
go.temporal.io/server/tools/sql.(*Connection).ReadSchemaVersion(0x12c01c0)
/tmp/temporal-1.13.1/tools/sql/conn.go:68 +0x14
go.temporal.io/server/tools/common/schema.(*UpdateTask).Run(0xc00069f5e8)
/tmp/temporal-1.13.1/tools/common/schema/updatetask.go:107 +0x29b
go.temporal.io/server/tools/common/schema.Update(0x12c01c0, {0x12f44a0, 0x0}, {0x12f0178, 0xc00040de00})
/tmp/temporal-1.13.1/tools/common/schema/handler.go:48 +0x98
go.temporal.io/server/tools/sql.updateSchema(0x7f2c01ad8628, {0x12f0178, 0xc00040de00})
/tmp/temporal-1.13.1/tools/sql/handler.go:77 +0x586
go.temporal.io/server/tools/sql.cliHandler(0x2, 0x112c970, {0x12f0178, 0xc00040de00})
/tmp/temporal-1.13.1/tools/sql/main.go:48 +0x56
go.temporal.io/server/tools/sql.BuildCLIOptions.func2(0xc00017f600)
/tmp/temporal-1.13.1/tools/sql/main.go:185 +0x2b
github.com/urfave/cli.HandleAction({0xf370c0, 0xc00040de20}, 0xd)
/home/dhochman/go/pkg/mod/github.com/urfave/cli@v1.22.5/app.go:526 +0x50
github.com/urfave/cli.Command.Run({{0x10b5fc1, 0xd}, {0x0, 0x0}, {0xc00040de60, 0x1, 0x1}, {0x10e3812, 0x27}, {0x0, ...}, ...}, ...)
/home/dhochman/go/pkg/mod/github.com/urfave/cli@v1.22.5/command.go:173 +0x652
github.com/urfave/cli.(*App).Run(0xc0001728c0, {0xc000032100, 0x10, 0x10})
/home/dhochman/go/pkg/mod/github.com/urfave/cli@v1.22.5/app.go:277 +0x705
go.temporal.io/server/tools/sql.RunTool({0xc000032100, 0x10, 0x10})
/tmp/temporal-1.13.1/tools/sql/main.go:42 +0x3c
main.main()
/tmp/temporal-1.13.1/cmd/tools/sql/main.go:36 +0x2e

After

Error is logged and execution ends.

2021-12-02T14:43:05.462-0600    ERROR   Unable to connect to SQL database.      {"error": "pq: password authentication failed for user \"temporal_admin\"", "logging-call-at": "handler.go:73"}

How did you test it?
Manually

Potential risks
None

Is hotfix candidate?
No

@danielhochman danielhochman requested a review from a team December 2, 2021 20:57
@CLAassistant
Copy link

CLAassistant commented Dec 2, 2021

CLA assistant check
All committers have signed the CLA.

@wxing1292 wxing1292 enabled auto-merge (squash) December 3, 2021 07:08
@yiminc
Copy link
Member

yiminc commented Dec 3, 2021

@danielhochman could you please sign the CLA or we won't be able to merge this PR.

@danielhochman
Copy link
Contributor Author

@yiminc I'm aware and working on it, waiting on some internal review and approval here.

@yiminc
Copy link
Member

yiminc commented Dec 17, 2021

@danielhochman, any update on the CLA? We would like to get this merged. If we could not have this merged by 12/31, we would abandon this PR and produce our own fix for this.

@wxing1292 wxing1292 merged commit dc0c75a into temporalio:master Jan 4, 2022
@danielhochman danielhochman deleted the fix-panic-if-auth-failed-sql-tool branch January 4, 2022 17:54
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.

4 participants