Skip to content

Fix certificates chain generation regression introduced with 12.4.0-rc1

Tomasz Maczukin requested to merge fix-certificates-chain-generation into master

What does this MR do?

Fixes a regression introduced with !1581 (merged).

Why was this MR needed?

Usage of github.com/zakjan/cert-chain-resolver/certUtil added in !1581 (merged) allowed us to finally upgrade Runner from Go 1.8. But the implementation contains a bug, that in some cases may end with a panic that crashes the running Runner process:

panic: runtime error: index out of range
goroutine 87 [running]:
gitlab.com/gitlab-org/gitlab-runner/vendor/github.com/zakjan/cert-chain-resolver/certUtil.AddRootCA(0x0, 0x0, 0x0, 0x0, 0x181daa0, 0xc43a667aa0, 0x2, 0x4)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/vendor/github.com/zakjan/cert-chain-resolver/certUtil/chain.go:57 +0x2f8
gitlab.com/gitlab-org/gitlab-runner/network.(*client).getCAChain(0xc42009c480, 0xc439c61e40, 0x181ef80, 0x2254160)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/network/client.go:191 +0x33e
gitlab.com/gitlab-org/gitlab-runner/network.(*client).doJSON(0xc42009c480, 0xc4223e6280, 0xe, 0x16cb258, 0x3, 0xc8, 0x13e42c0, 0xc42b23f970, 0x0, 0x0, ...)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/network/client.go:320 +0x251
gitlab.com/gitlab-org/gitlab-runner/network.(*GitLabClient).doJSON(0xc4201cda80, 0x1841e20, 0xc428373d08, 0x16cb258, 0x3, 0xc4223e6280, 0xe, 0xc8, 0x13e42c0, 0xc42b23f970, ...)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/network/gitlab.go:168 +0x193
gitlab.com/gitlab-org/gitlab-runner/network.(*GitLabClient).UpdateJob(0xc4201cda80, 0xc4203d3760, 0x12, 0xc8, 0x0, 0xd, 0xc4203d37c0, 0x13, 0xc42042dd70, 0x1e, ...)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/network/gitlab.go:308 +0x2ae
gitlab.com/gitlab-org/gitlab-runner/network.(*clientJobTrace).sendUpdate(0xc42dd40b40, 0x0)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/network/trace.go:217 +0x191
gitlab.com/gitlab-org/gitlab-runner/network.(*clientJobTrace).finalStatusUpdate(0xc42dd40b40)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/network/trace.go:114 +0x2f
gitlab.com/gitlab-org/gitlab-runner/network.(*clientJobTrace).finish(0xc42dd40b40)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/network/trace.go:133 +0x6f
gitlab.com/gitlab-org/gitlab-runner/network.(*clientJobTrace).Fail(0xc42dd40b40, 0x0, 0x0, 0x0, 0x0)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/network/trace.go:56 +0xdc
gitlab.com/gitlab-org/gitlab-runner/network.(*clientJobTrace).Success(0xc42dd40b40)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/network/trace.go:38 +0x38
gitlab.com/gitlab-org/gitlab-runner/common.(*Build).setTraceStatus(0xc437420e00, 0x1849120, 0xc42dd40b40, 0x0, 0x0)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/common/build.go:485 +0x47c
gitlab.com/gitlab-org/gitlab-runner/common.(*Build).Run.func1(0xc437420e00, 0x1849120, 0xc42dd40b40, 0xc42b1aaa00, 0xc42b1aaa20)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/common/build.go:529 +0x55
gitlab.com/gitlab-org/gitlab-runner/common.(*Build).Run(0xc437420e00, 0xc420132790, 0x1849120, 0xc42dd40b40, 0xc4268a9ac0, 0x8)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/common/build.go:578 +0x713
gitlab.com/gitlab-org/gitlab-runner/commands.(*RunCommand).processRunner(0xc4200e6780, 0x18, 0xc42056a160, 0xc420087aa0, 0x0, 0x0)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/commands/multi.go:228 +0x5c7
gitlab.com/gitlab-org/gitlab-runner/commands.(*RunCommand).processRunners(0xc4200e6780, 0x18, 0xc42041a3c0, 0xc420087aa0)
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/commands/multi.go:261 +0x24f
created by gitlab.com/gitlab-org/gitlab-runner/commands.(*RunCommand).startWorkers
        /builds/gitlab-org/gitlab-runner/.gopath/src/gitlab.com/gitlab-org/gitlab-runner/commands/multi.go:284 +0x84

This MR fixes this behavior. Additionally - because we needed to start checking errors from the former getCAChain() method, it adds a little refactoring which have a positive side effect - certificates fetching is done only when it's needed - on RequestJob() calls.

The MR contains also tests added for certificates chain building, that cover the regression failure.

Are there points in the code the reviewer needs to double check?

Does this MR meet the acceptance criteria?

  • Documentation created/updated
  • Added tests for this feature/bug
  • In case of conflicts with master - branch was rebased

What are the relevant issue numbers?

Edited by Tomasz Maczukin

Merge request reports