search

deis / builder

Git server and application builder for Deis Workflow
https://deis.com
MIT License
40 stars 41 forks source link

chore(glide): bump golang.org/x/crypto to b822463 #464

Closed bacongobbler closed 7 years ago

bacongobbler commented 7 years ago

closes #462

deis-bot commented 7 years ago

@mboersma, @arschles and @Joshua-Anderson are potential reviewers of this pull request based on my analysis of git blame information. Thanks @bacongobbler!

mboersma commented 7 years ago

This looks very hopeful. I haven't reproduced the actual issue but I'm happy to manually test this if it would help.

Bregor commented 7 years ago

I can test too

codecov-io commented 7 years ago

Current coverage is 56.11% (diff: 100%)

Merging #464 into master will increase coverage by 0.24%

@@             master       #464   diff @@
==========================================
  Files            29         29          
  Lines          1219       1219          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            681        684     +3   
+ Misses          500        497     -3   
  Partials         38         38

Powered by Codecov. Last update f760b86...d7f5ff0

vdice commented 7 years ago

I've updated the deis-builder deployment with this patch in both my local cluster and our team's long-running cluster and have not hit https://github.com/deis/builder/issues/462 in 5 hours so far (in that time it would have been hit many times based on my experience.)

I'll keep them running overnight but this definitely looks like the fix!

Bregor commented 7 years ago

Afraid to appear stupid, but how to build image with patch?..

$ git remote add bacongobbler https://github.com/bacongobbler/builder.git
$ git fetch bacongobbler
$ git merge bacongobbler/462-crypto-tip
$ docker build -t ... -f rootfs/Dockerfile .

Resulting image is something else, not deis/builder. What I am doing wrong?

bacongobbler commented 7 years ago

the README explains the dev workflow. https://github.com/deis/builder/blob/master/README.md#docker-based-development-environment

vdice commented 7 years ago

@Bregor Not at all; my fault for taking for granted how to patch with a PR artifact. A couple options:

  1. As @bacongobbler mentioned, we use make in all of the workflow component repos for build/test/deploy, so after the git steps mentioned above, you could run make deploy which will build, push and 'deploy' the image into whatever k8s cluster context kubectl is using via a kubectl patch... command.

Appropriate values need to be exported for the image registry used, as our default is IMAGE := ${DEIS_REGISTRY}${IMAGE_PREFIX}/${SHORT_NAME}:${VERSION} where, in this case, the default values are DEIS_REGISTRY=quay.io/, IMAGE_PREFIX=deis, SHORT_NAME=builder and VERSION=git-<HEAD commit sha>. If using quay.io, simply:

export IMAGE_PREFIX=<quay username>
make deploy

(You may have to ensure the new quay repo is made public for the k8s cluster to be able to pull the image.)

  1. Otherwise, I went the 'easy' route and just used the artifact from this PR and ran a helm upgrade:
helm upgrade <current release> workflow/workflow --set builder.org=deisci,builder.docker_tag=git-452b360

(Each PR, once ci has run, will generate a corresponding Docker image artifact in our deisci staging registry based on the PR commit (--short version), so in this case, we know to look for quay.io/deisci/builder:git-452b360)

Bregor commented 7 years ago

@bacongobbler, @vdice, thank you! I already built new image by instruction from readme.

Start checking ;)

Bregor commented 7 years ago

Heh... With patch:

2017-01-19T15:21:12.308040465Z 2017/01/19 15:21:12 Starting health check server on port 8092
2017-01-19T15:21:12.308144344Z 2017/01/19 15:21:12 Starting deleted app cleaner
2017-01-19T15:21:12.308163843Z 2017/01/19 15:21:12 Starting SSH server on 0.0.0.0:2223
2017-01-19T15:21:12.311120223Z Listening on 0.0.0.0:2223
2017-01-19T15:21:12.311245998Z Accepting new connections.
2017-01-19T15:25:47.369598431Z Accepted connection.
2017-01-19T15:25:48.619838718Z ---> ---> ---> panic: runtime error: slice bounds out of range
2017-01-19T15:25:48.619931565Z
2017-01-19T15:25:48.619951268Z goroutine 466 [running]:
2017-01-19T15:25:48.619963942Z panic(0x11ac4a0, 0xc4200180c0)
2017-01-19T15:25:48.619976085Z  /usr/local/go/src/runtime/panic.go:500 +0x1a1
2017-01-19T15:25:48.619989888Z github.com/deis/builder/vendor/golang.org/x/crypto/ssh.(*dsaPrivateKey).Sign(0xc420172100, 0x1c22840, 0xc4200155c0, 0xc4201bd5c0, 0x14, 0x20, 0x0, 0x7c0, 0xc4201bd5c0)
2017-01-19T15:25:48.620003385Z  /go/src/github.com/deis/builder/vendor/golang.org/x/crypto/ssh/keys.go:457 +0x3bf
2017-01-19T15:25:48.620015732Z github.com/deis/builder/vendor/golang.org/x/crypto/ssh.signAndMarshal(0x1c2c000, 0xc420172100, 0x1c22840, 0xc4200155c0, 0xc4201bd5c0, 0x14, 0x20, 0x113d960, 0x1102e63, 0x11e09a0, ...)
2017-01-19T15:25:48.620028225Z  /go/src/github.com/deis/builder/vendor/golang.org/x/crypto/ssh/server.go:159 +0x6d
2017-01-19T15:25:48.620040302Z github.com/deis/builder/vendor/golang.org/x/crypto/ssh.(*dhGroup).Server(0xc4200d17c0, 0x7f0c6ccb8a00, 0xc420180000, 0x1c22840, 0xc4200155c0, 0xc4204cc900, 0x1c2c000, 0xc420172100, 0x40df34, 0x133b4ad, ...)
2017-01-19T15:25:48.620052572Z  /go/src/github.com/deis/builder/vendor/golang.org/x/crypto/ssh/kex.go:188 +0x511
2017-01-19T15:25:48.620064175Z github.com/deis/builder/vendor/golang.org/x/crypto/ssh.(*handshakeTransport).server(0xc420369040, 0x1c2bfc0, 0xc4200d17c0, 0xc420324600, 0xc4204cc900, 0x1, 0x0, 0x2)
2017-01-19T15:25:48.620076238Z  /go/src/github.com/deis/builder/vendor/golang.org/x/crypto/ssh/handshake.go:537 +0x1db
2017-01-19T15:25:48.620088708Z github.com/deis/builder/vendor/golang.org/x/crypto/ssh.(*handshakeTransport).enterKeyExchange(0xc420369040, 0xc420226460, 0xd9, 0xd9, 0x1, 0xc420456560)
2017-01-19T15:25:48.620100648Z  /go/src/github.com/deis/builder/vendor/golang.org/x/crypto/ssh/handshake.go:502 +0x37d
2017-01-19T15:25:48.620112662Z github.com/deis/builder/vendor/golang.org/x/crypto/ssh.(*handshakeTransport).kexLoop(0xc420369040)
2017-01-19T15:25:48.620124112Z  /go/src/github.com/deis/builder/vendor/golang.org/x/crypto/ssh/handshake.go:271 +0x1e9
2017-01-19T15:25:48.620136215Z created by github.com/deis/builder/vendor/golang.org/x/crypto/ssh.newServerTransport
2017-01-19T15:25:48.620147475Z  /go/src/github.com/deis/builder/vendor/golang.org/x/crypto/ssh/handshake.go:121 +0x227

Maybe I do something terrible wrong?..

Bregor commented 7 years ago

Patched with following commands:

git remote add bacongobbler https://github.com/bacongobbler/builder.git
git fetch bacongobbler
git merge bacongobbler/462-crypto-tip
make bootstrap
make build

cd rootfs
docker build -t quay.io/evilmartians/deis-builder:v2.6.1.1 .
docker push quay.io/evilmartians/deis-builder:v2.6.1.1
kubectl set image deployment deis-builder deis-builder=quay.io/evilmartians/deis-builder:v2.6.1.1 -n deis
bacongobbler commented 7 years ago

If you're using the branch directly, I'd just check out the branch and run the following to confirm you're using the right image:

git remote add bacongobbler https://github.com/bacongobbler/builder.git
git fetch bacongobbler 462-crypto-tip
git checkout bacongobbler/462-crypto-tip
make bootstrap docker-build
docker tag deis/builder:git-452b360 quay.io/evilmartians/deis-builder:git-452b360
docker push quay.io/evilmartians/deis-builder:git-452b360
kubectl set image deployment deis-builder deis-builder=quay.io/evilmartians/deis-builder:git-452b360 -n deis

Then I'd just double check that your pod manifests reference that image.

vdice commented 7 years ago

Update: both patched clusters did indeed still hit the panic overnight. Though perhaps not much consolation, they occurred seemingly far less frequently (2 restarts over 21h). It appears this patch doesn't fix the base issue -- although it doesn't appear to harm, either.

bacongobbler commented 7 years ago

This is the line upstream where it's failing: https://github.com/golang/crypto/blob/b8a2a83acfe6e6770b75de42d5ff4c67596675c0/ssh/keys.go#L457

Bregor commented 7 years ago

Restarts are more rare for now but still about once per hour:

$ kubectl get po -n deis
NAME                                     READY     STATUS    RESTARTS   AGE
deis-builder-2354050293-4wnx1            1/1       Running   111        5d
vdice commented 7 years ago

As confirmed by @Bregor , this unfortunately doesn't fix the original issue (https://github.com/deis/builder/issues/462) yet it is agreed the patch represents an improvement. With that in mind, LGTM.