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

check for null before dereferencing #786

Merged

Conversation

kingdonb
Copy link
Contributor

Fixes #785, I think! Not sure if it is needed to check the inverse, if
dev.PersistentVolumeStorageClass is unset I think it will be fine
(because it's not being dereferenced here.)

Proposed changes

  • Check for null value before dereferencing pvc.Spec.StorageClassName
  • If null is found there, display the same error that would show as if the field is blank rather than missing.

Resulting output:

First the error, from okteto cli at version 1.8.1, then ~bin/okteto is the updated version at okteto version a199e14

kbarret8@kbarret8-mbp:~/projects/ruby-getting-started (master * u=)$ okteto up
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x22a2c5e]

goroutine 35 [running]:
github.com/okteto/okteto/pkg/k8s/volumes.checkPVCValues(0xc00020afc0, 0xc0008d2000, 0x12, 0x0)
	github.com/okteto/okteto/pkg/k8s/volumes/crud.go:68 +0x21e
github.com/okteto/okteto/pkg/k8s/volumes.Create(0x2fb2d60, 0xc0004d59c0, 0xc0008d2000, 0xc0014fe000, 0x4454f00, 0x1094300)
	github.com/okteto/okteto/pkg/k8s/volumes/crud.go:45 +0x301
github.com/okteto/okteto/cmd.(*UpContext).devMode(0xc0008dc090, 0xc0002d6900, 0x0, 0x0, 0x0)
	github.com/okteto/okteto/cmd/up.go:386 +0x169
github.com/okteto/okteto/cmd.(*UpContext).Activate(0xc0008dc090, 0x0)
	github.com/okteto/okteto/cmd/up.go:250 +0x353
created by github.com/okteto/okteto/cmd.RunUp
	github.com/okteto/okteto/cmd/up.go:169 +0x1a6
kbarret8@kbarret8-mbp:~/projects/ruby-getting-started (master * u=)$ ~/bin/okteto up
Installing dependencies...
syncthing-macos-amd64-v1.4.0.tar.gz 9.16 MiB / 9.16 MiB [-----------------------------------------------------------------------------------------------------] 100.00% 3.42 MiB p/s
 ✓  Dependencies successfully installed
 x  couldn't activate your development environment: current okteto volume storageclass is '' instead of 'local-path'. Run 'okteto down -v' and try again
kbarret8@kbarret8-mbp:~/projects/ruby-getting-started (master * u=)$ okteto down -v
 ✓  Development environment deactivated
 ✓  Persistent volume removed

kbarret8@kbarret8-mbp:~/projects/ruby-getting-started (master * u=)$ ~/bin/okteto up
 ✓  Development environment activated
 ✓  Files synchronized
    Namespace: kingdonb
    Name:      hello-world
    Forward:   1234 -> 1234
               8080 -> 8080

@derek
Copy link

derek bot commented Mar 27, 2020

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

@derek derek bot added the no-dco label Mar 27, 2020
@codecov
Copy link

codecov bot commented Mar 27, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@2e3cfdb). Click here to learn what that means.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #786   +/-   ##
=========================================
  Coverage          ?   32.34%           
=========================================
  Files             ?       63           
  Lines             ?     4956           
  Branches          ?        0           
=========================================
  Hits              ?     1603           
  Misses            ?     3219           
  Partials          ?      134
Impacted Files Coverage Δ
pkg/k8s/volumes/crud.go 26.15% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e3cfdb...3fe300f. Read the comment docs.

@kingdonb
Copy link
Contributor Author

I just checked what happens if you remove storageClass setting from okteto.yml, it doesn't do anything. Maybe it should complain in a reverse fashion (current okteto volume storageclass is '%s' instead of '')

I'll overwrite the commit that I provided with a signed-off copy and someone can review this

Fixes okteto#785.

Not sure if it is valuable or needed to check the inverse, if
dev.PersistentVolumeStorageClass is unset it will not crash
(because it's not being dereferenced here.)

Tested that it doesn't crash as I encountered in okteto#785 report anymore!
:+1:

Signed-off-by: Kingdon Barrett <kingdon.b@nd.edu>
@derek derek bot added the no-dco label Mar 27, 2020
@derek
Copy link

derek bot commented Mar 27, 2020

Thank you for your contribution. I've just checked and your commit doesn't appear to be signed-off. That's something we need before your Pull Request can be merged. Please see our contributing guide.
Tip: if you only have one commit so far then run: git commit --amend --signoff and then git push --force.

@rberrelleza
Copy link
Member

Thanks for the contribution @kingdonb!

@rberrelleza
Copy link
Member

failure is unrelated to this PR, I'll merge and fix the core issue on a separate branch.

@rberrelleza rberrelleza merged commit f967e44 into okteto:master Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing storageClass in okteto.yml after "okteto up" raises SIGSEGV
2 participants