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

NewClient panics if http.client is nil and LINODE_CA is set #635

Merged
merged 5 commits into from
Dec 20, 2024

Conversation

kokes
Copy link
Contributor

@kokes kokes commented Dec 2, 2024

I tried a new linodego with a custom CA and a nil HTTP client and encountered a panic. Here's a minimal reproducer (that I now include in the test suite):

func TestClient_CustomRootCAWithoutCustomRoundTripper(t *testing.T) {
	caFile, err := os.CreateTemp(t.TempDir(), "linodego_test_ca_*")
	if err != nil {
		t.Fatalf("Failed to create temp ca file: %s", err)
	}
	defer os.Remove(caFile.Name())

	t.Setenv(APIHostCert, caFile.Name())

        NewClient(nil)
}

Once I run this test, I get:

$ go test -run=TestClient_CustomRootCAWithoutCustomRoundTripper ./...
?   	github.com/linode/linodego/cmd	[no test files]
?   	github.com/linode/linodego/internal/parseabletime	[no test files]
?   	github.com/linode/linodego/internal/testutil	[no test files]
--- FAIL: TestClient_CustomRootCAWithoutCustomRoundTripper (0.00s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x0 pc=0x102dbb3d8]

goroutine 22 [running]:
testing.tRunner.func1.2({0x102fb5dc0, 0x1034115e0})
	/opt/homebrew/Cellar/go/1.23.3/libexec/src/testing/testing.go:1632 +0x1bc
testing.tRunner.func1()
	/opt/homebrew/Cellar/go/1.23.3/libexec/src/testing/testing.go:1635 +0x334
panic({0x102fb5dc0?, 0x1034115e0?})
	/opt/homebrew/Cellar/go/1.23.3/libexec/src/runtime/panic.go:785 +0x124
github.com/linode/linodego.NewClient(0x0)
	/Users/okokes/git/linode/public/linodego/client.go:741 +0x208
github.com/linode/linodego.TestClient_CustomRootCAWithoutCustomRoundTripper(0x140000c6b60)
	/Users/okokes/git/linode/public/linodego/client_test.go:589 +0xdc
testing.tRunner(0x140000c6b60, 0x10306a230)
	/opt/homebrew/Cellar/go/1.23.3/libexec/src/testing/testing.go:1690 +0xe4
created by testing.(*T).Run in goroutine 1
	/opt/homebrew/Cellar/go/1.23.3/libexec/src/testing/testing.go:1743 +0x314
FAIL	github.com/linode/linodego	1.116s
ok  	github.com/linode/linodego/internal/duration	0.713s [no tests to run]
FAIL

This is a followup to #593 - in this PR this LINODE_CA support got fixed, but not in the case of hc == nil, the constructor would panic upon hc.Transport.

@kokes kokes marked this pull request as ready for review December 2, 2024 09:36
@kokes kokes requested a review from a team as a code owner December 2, 2024 09:36
@kokes kokes requested review from jriddle-linode and yec-akamai and removed request for a team December 2, 2024 09:36
@jcallahan-akamai jcallahan-akamai requested review from zliang-akamai and removed request for yec-akamai December 20, 2024 14:21
Copy link
Member

@zliang-akamai zliang-akamai left a comment

Choose a reason for hiding this comment

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

LGTM!

client.go Outdated Show resolved Hide resolved
Co-authored-by: Zhiwei Liang <121905282+zliang-akamai@users.noreply.github.com>
Copy link
Collaborator

@jriddle-linode jriddle-linode left a comment

Choose a reason for hiding this comment

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

LGTM! Agree with Zhiwei's recommendation though.

@jriddle-linode
Copy link
Collaborator

@kokes looks like it needs a make format

@kokes
Copy link
Contributor Author

kokes commented Dec 20, 2024

I committed from my phone and didn't realise your inline suggestion only fixed the function definition, not the call itself, updated that in a subsequent commit.

Copy link
Collaborator

@jriddle-linode jriddle-linode left a comment

Choose a reason for hiding this comment

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

LGTM

@jriddle-linode jriddle-linode merged commit aee0bb2 into linode:main Dec 20, 2024
10 checks passed
@zliang-akamai zliang-akamai added the bugfix for any bug fixes in the changelog. label Dec 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix for any bug fixes in the changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants