fix(dump) address free mode workaround regression #335

rainest · 2021-04-21T18:02:31Z

I goofed checking 8254879 and only confirmed the negative case 🤦

We don't want this to happen if the mTLS auth credentials endpoint is available:

$ /tmp/main-deck dump -o main.yaml
panic: interface conversion: error is nil, not *kong.APIError

goroutine 36 [running]:
github.com/kong/deck/dump.GetAllMTLSAuths(0xc80cb8, 0xc000139240, 0xc000172400, 0xf592b0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
	/home/traines/src/deck/dump/dump.go:671 +0x5b5
github.com/kong/deck/dump.getConsumerConfiguration.func8(0x0, 0x0)
	/home/traines/src/deck/dump/dump.go:128 +0xef
golang.org/x/sync/errgroup.(*Group).Go.func1(0xc0001dd2c0, 0xc000142f50)
	/home/traines/go/pkg/mod/golang.org/x/sync@v0.0.0-20201020160332-67f06af15bc9/errgroup/errgroup.go:57 +0x85
created by golang.org/x/sync/errgroup.(*Group).Go
	/home/traines/go/pkg/mod/golang.org/x/sync@v0.0.0-20201020160332-67f06af15bc9/errgroup/errgroup.go:54 +0x6b
10:16:08-0700 yagody $ /tmp/329-deck dump -o 329.yaml
panic: interface conversion: error is nil, not *kong.APIError

8254879 did not properly handle _successful_ mTLS responses, as the nil error would not coerce to a Kong API error. Move the workaround into the error handling block to ensure that there is in fact an error response before checking its status.

codecov-commenter · 2021-04-21T18:03:58Z

Codecov Report

Merging #335 (5045caf) into main (6ede1a8) will not change coverage.
The diff coverage is 0.00%.

❗ Current head 5045caf differs from pull request most recent head d4b7649. Consider uploading reports for the commit d4b7649 to get more accurate results

@@           Coverage Diff           @@
##             main     #335   +/-   ##
=======================================
  Coverage   52.58%   52.58%           
=======================================
  Files          60       60           
  Lines        4874     4874           
=======================================
  Hits         2563     2563           
  Misses       2013     2013           
  Partials      298      298

Impacted Files	Coverage Δ
cmd/konnect_dump.go	`27.08% <0.00%> (ø)`
dump/dump.go	`1.56% <0.00%> (ø)`
file/types.go	`43.43% <ø> (ø)`

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 6ede1a8...d4b7649. Read the comment docs.

dump/dump.go

mflendrich

+1 to Harry's comment, otherwise lgtm

Yet another reason why we need integration tests.

xref #63 - to provide context about regressions we've encountered

573de55 did not properly handle _successful_ mTLS responses, as the nil error would not coerce to a Kong API error. Move the workaround into the error handling block to ensure that there is in fact an error response before checking its status.

rainest requested a review from a team as a code owner April 21, 2021 18:02

hbagdi reviewed Apr 21, 2021

View reviewed changes

dump/dump.go Outdated Show resolved Hide resolved

mflendrich reviewed Apr 22, 2021

View reviewed changes

pr: tighten error type check

58199c9

rainest requested review from hbagdi and mflendrich April 22, 2021 16:54

mflendrich approved these changes Apr 22, 2021

View reviewed changes

rainest merged commit b58856a into main Apr 22, 2021

rainest deleted the fix/mtls-noerror branch April 22, 2021 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dump) address free mode workaround regression #335

fix(dump) address free mode workaround regression #335

rainest commented Apr 21, 2021

codecov-commenter commented Apr 21, 2021

mflendrich left a comment

fix(dump) address free mode workaround regression #335

fix(dump) address free mode workaround regression #335

Conversation

rainest commented Apr 21, 2021

codecov-commenter commented Apr 21, 2021

Codecov Report

mflendrich left a comment

Choose a reason for hiding this comment