Resolve panic in gateway validation client #6260

sam-heilbron · 2022-04-07T16:21:46Z

Description

Ensure that the gateway validation client is never set to nil

Context

We encountered this bug in tests that would occasionally panic, however the bug itself could impact customers

Verification

I wrote a test first, and add that as the first commit. If you checkout just that test (no other changes) it will panic in the way that we noticed in some e2e tests. If you then checkout my fix and re-run the same test, it will pass.

General Thoughts

I believe this issue arises because we have a local variable for clientReinstantiateError and a class variable for constructorClient. This means that we can ensure that on a clientReinstantiateError we do not retry, but we do set the client to nil which could be picked up on the subsequent request.

Checklist:

I included a concise, user-facing changelog (for details, see https://github.com/solo-io/go-utils/tree/master/changelogutils) which references the issue that is resolved.
If I updated APIs (our protos) or helm values, I ran make -B install-go-tools generated-code to ensure there will be no code diff
I followed guidelines laid out in the Gloo Edge contribution guide
I opened a draft PR or added the work in progress label if my PR is not ready for review
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have added tests that prove my fix is effective or that my feature works

BOT NOTES:
resolves #6237

solo-changelog-bot · 2022-04-07T16:21:55Z

Issues linked to changelog:
#6237

changelog/v1.12.0-beta2/robust-client-panic.yaml

kdorosh

seems reasonable to me

Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>

github-actions · 2022-04-07T16:35:39Z

Visit the preview URL for this PR (updated for commit 4db0b08):

https://gloo-edge--pr6260-test-flakes-6237-ug3745ow.web.app

_{(expires Thu, 14 Apr 2022 17:19:55 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

gunnar-solo

Nice fix! The null guard makes sense to me, per the original issue hitting a nil on robust_client.go:66.

The patched line is the only place (I can see) where c.validationClient is set

* add test * add fix for panicing test * unfocus test * add changelog * goimports -w * Update changelog/v1.12.0-beta2/robust-client-panic.yaml Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io> Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io> Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

* Reduce test flakes (#6211) * First pass of cleanup * more * Fixup gateway e2e tests * add changelog * fix route options tests? * cleanup comments in assertions * Just after each to match just before * re-add config Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> * Resolve panic in gateway validation client (#6260) * add test * add fix for panicing test * unfocus test * add changelog * goimports -w * Update changelog/v1.12.0-beta2/robust-client-panic.yaml Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io> Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io> Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> * Resolve consul plugin data race (#6271) * use dependency injectio to handle fallback resolution * add changelog * fixup dns resolvers and test * Resolve data race in setup syncer test (#6282) * add changelogs * delete duplicate changelogs Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>

* add test * add fix for panicing test * unfocus test * add changelog * goimports -w * Update changelog/v1.12.0-beta2/robust-client-panic.yaml Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io> Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io> Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

* Reduce test flakes (#6211) * First pass of cleanup * more * Fixup gateway e2e tests * add changelog * fix route options tests? * cleanup comments in assertions * Just after each to match just before * re-add config Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> * Resolve panic in gateway validation client (#6260) * add test * add fix for panicing test * unfocus test * add changelog * goimports -w * Update changelog/v1.12.0-beta2/robust-client-panic.yaml Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io> Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io> Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> * Resolve consul plugin data race (#6271) * use dependency injectio to handle fallback resolution * add changelog * fixup dns resolvers and test * Resolve data race in setup syncer test (#6282) * add changelogs * delete duplicate changelogs Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io> Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>

* Reduce test flakes (#6211) * First pass of cleanup * more * Fixup gateway e2e tests * add changelog * fix route options tests? * cleanup comments in assertions * Just after each to match just before * re-add config Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> * Resolve panic in gateway validation client (#6260) * add test * add fix for panicing test * unfocus test * add changelog * goimports -w * Update changelog/v1.12.0-beta2/robust-client-panic.yaml Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io> Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io> Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> * Resolve consul plugin data race (#6271) * use dependency injectio to handle fallback resolution * add changelog * fixup dns resolvers and test * Resolve data race in setup syncer test (#6282) * move changelog * fixup registry * de duplicate changelogs * delete dynamic forward proxy test * delete hybrid test * undo kube2e changes, panic occurring with gomega * update changelog * undo delete Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com> Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>

sam-heilbron added 6 commits April 7, 2022 12:16

add test

00f03a0

add fix for panicing test

9a18483

unfocus test

4cbe3a9

Merge branch 'master' into test-flakes/6237

4389d15

add changelog

8b19089

goimports -w

a994870

github-actions bot added the keep pr updated signals bulldozer to keep pr up to date with base branch label Apr 7, 2022

sam-heilbron requested review from kdorosh and gunnar-solo April 7, 2022 16:25

sam-heilbron marked this pull request as ready for review April 7, 2022 16:25

kdorosh reviewed Apr 7, 2022

View reviewed changes

changelog/v1.12.0-beta2/robust-client-panic.yaml Outdated Show resolved Hide resolved

kdorosh reviewed Apr 7, 2022

View reviewed changes

Update changelog/v1.12.0-beta2/robust-client-panic.yaml

c9aa193

Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>

gunnar-solo approved these changes Apr 7, 2022

View reviewed changes

Merge refs/heads/master into test-flakes/6237

4db0b08

soloio-bulldozer bot merged commit 1d72050 into master Apr 7, 2022

soloio-bulldozer bot deleted the test-flakes/6237 branch April 7, 2022 17:42

sam-heilbron mentioned this pull request Apr 11, 2022

[1.10] Reduce test flakes #6297

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve panic in gateway validation client #6260

Resolve panic in gateway validation client #6260

sam-heilbron commented Apr 7, 2022 •

edited by solo-changelog-bot bot

Loading

solo-changelog-bot bot commented Apr 7, 2022

kdorosh left a comment

github-actions bot commented Apr 7, 2022 •

edited

Loading

gunnar-solo left a comment

Resolve panic in gateway validation client #6260

Resolve panic in gateway validation client #6260

Conversation

sam-heilbron commented Apr 7, 2022 • edited by solo-changelog-bot bot Loading

Description

Context

Verification

General Thoughts

Checklist:

solo-changelog-bot bot commented Apr 7, 2022

kdorosh left a comment

Choose a reason for hiding this comment

github-actions bot commented Apr 7, 2022 • edited Loading

gunnar-solo left a comment

Choose a reason for hiding this comment

sam-heilbron commented Apr 7, 2022 •

edited by solo-changelog-bot bot

Loading

github-actions bot commented Apr 7, 2022 •

edited

Loading