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

Fix panic when k3sURLEnvIndex is -1 #1252

Closed
wants to merge 0 commits into from

Conversation

moio
Copy link

@moio moio commented Mar 28, 2023

What

Fixes a panic condition - first found in terraform-provider-k3d acceptance tests.

Why

Symptom is:

panic: runtime error: index out of range [-1]

goroutine 1713 [running]:
github.com/k3d-io/k3d/v5/pkg/client.NodeAddToCluster(...)
	/home/runner/go/pkg/mod/github.com/k3d-io/k3d/v5@v5.4.9/pkg/client/node.go:200 +0x1ed0

I believe there is a logic mistake in the code pointed by the error above, and this PR attempts to address it.

Specifically, k3sURLEnvIndex can be -1 when node.Env[k3sURLEnvIndex] is evaluated - specifically when k3sURLEnvFound is false. So introduce a condition to guard the lookup on the slice in that case.

Additionally, since k3sURLEnvFound == false implies checkK3SURLIsActive(...) == false (there can't be an active URL if none was found in the first place), remove dead code in the first branch of the if statement and rework the condition to work equivalently.

Implications

Should only fix this particular panic and not cause any adverse side effects.

@all-contributors please add @moio for code

@moio
Copy link
Author

moio commented Mar 28, 2023

cc @4p00rv as code here was introduced in #1190 - I'd appreciate a look to double check I did not accidentally alter the intended original meaning.

@4p00rv
Copy link
Contributor

4p00rv commented Apr 5, 2023

@moio That code was unnecessary. Thanks for simplifying. Looks good from my end :)

aescaler-raft pushed a commit to aescaler-raft/k3d that referenced this pull request Apr 8, 2023
Signed-off-by: Silvio Moioli <silvio@moioli.net>
@moio
Copy link
Author

moio commented Apr 13, 2023

Is there anything more I could do to have this merged?

@iwilltry42
Copy link
Member

Thank you very much for this change @moio and thanks @4p00rv for checking :)

@iwilltry42
Copy link
Member

OK, I'm a little lost and don't know what happened here. With a base branch update this was closed 🤔

@moio
Copy link
Author

moio commented May 2, 2023

Well, I see the commit in the main branch, so I am happy 😉

Thanks!

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.

3 participants