Resolve consul plugin data race #6271

sam-heilbron · 2022-04-07T21:25:47Z

Description

Resolve a data race when resolving IP addresses

Context

Why fallback on errors?

In the consul plugin, we attempt to resolve IP addresses and then fallback to some previous state. This was introduced in #4563 and that PR/issue have the relevant context for why we want to support that.

What is the bug?

The data race occurs because we are accessing a map without synchronization

What is the solution?

We have a DnsResolver in the Consul plugin. It is responsible for taking an address and resolving it to return a list of IP Addresses or an error. Our decision to cache previous results can be wrapped into this resolver and let the plugin be unaware of its existence. Therefore, I introduced a new DnsResolver implementation, one that wraps the previous one but incorporates memoization. We inject the plugin with this new resolver so the plugin doesn't have to be aware of the change.

How did I test it?

We have an existing eds test which I updated to use our new dns resolver and confirmed the test still works as expected.

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

solo-changelog-bot · 2022-04-07T21:25:57Z

Issues linked to changelog:
#4782

sam-heilbron · 2022-04-07T21:27:27Z

projects/gloo/pkg/plugins/consul/eds.go

-		}
-		ipAddresses = addresses
-	} else {
-		previousResolutions[address] = ipAddresses


The data race occurred because we were writing to the map from multiple places. One solution would have been to pass around the map and its lock (or some synchronized map). I opted instead to pull this caching behavior out of the plugin altogether.

sam-heilbron · 2022-04-07T21:28:28Z

changelog/v1.12.0-beta2/consul-plugin-data-race.yaml

+changelog:
+  - type: FIX
+    issueLink: https://github.com/solo-io/gloo/issues/4782
+    resolvesIssue: false


leaving open for backports

github-actions · 2022-04-07T21:38:56Z

Visit the preview URL for this PR (updated for commit 5a212b5):

https://gloo-edge--pr6271-test-flakes-consul-p-5rbepwim.web.app

_{(expires Fri, 15 Apr 2022 03:57:09 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

projects/gloo/pkg/plugins/consul/dns_resolver.go

kdorosh

lgtm!

…m/solo-io/gloo into test-flakes/consul-plugin-data-race

nfuden

Nice fix! This one's been a pain for a while.

* use dependency injectio to handle fallback resolution * add changelog * fixup dns resolvers and test

* 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>

* use dependency injectio to handle fallback resolution * add changelog * fixup dns resolvers and test

* 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 and others added 3 commits April 7, 2022 17:17

use dependency injectio to handle fallback resolution

3c3b81c

add changelog

76a99fd

Merge branch 'master' into test-flakes/consul-plugin-data-race

987c8aa

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 commented Apr 7, 2022

View reviewed changes

nfuden reviewed Apr 7, 2022

View reviewed changes

projects/gloo/pkg/plugins/consul/dns_resolver.go Outdated Show resolved Hide resolved

projects/gloo/pkg/plugins/consul/dns_resolver.go Outdated Show resolved Hide resolved

kdorosh reviewed Apr 7, 2022

View reviewed changes

sam-heilbron added 2 commits April 7, 2022 23:43

fixup dns resolvers and test

465e5d9

Merge branch 'test-flakes/consul-plugin-data-race' of ssh://github.co…

5a212b5

…m/solo-io/gloo into test-flakes/consul-plugin-data-race

sam-heilbron changed the title ~~[WIP] Resolve consul plugin data race~~ Resolve consul plugin data race Apr 8, 2022

sam-heilbron marked this pull request as ready for review April 8, 2022 11:03

sam-heilbron requested review from kdorosh and nfuden April 8, 2022 11:07

nfuden approved these changes Apr 8, 2022

View reviewed changes

soloio-bulldozer bot merged commit 4f133dd into master Apr 8, 2022

soloio-bulldozer bot deleted the test-flakes/consul-plugin-data-race branch April 8, 2022 11:09

sam-heilbron added a commit that referenced this pull request Apr 11, 2022

Resolve consul plugin data race (#6271)

d97ee38

* use dependency injectio to handle fallback resolution * add changelog * fixup dns resolvers and test

sam-heilbron mentioned this pull request Apr 11, 2022

[1.10] Reduce test flakes #6297

Merged

8 tasks

sam-heilbron added a commit that referenced this pull request Apr 11, 2022

Resolve consul plugin data race (#6271)

8394f8d

* use dependency injectio to handle fallback resolution * add changelog * fixup dns resolvers and test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve consul plugin data race #6271

Resolve consul plugin data race #6271

sam-heilbron commented Apr 7, 2022 •

edited

Loading

solo-changelog-bot bot commented Apr 7, 2022

sam-heilbron Apr 7, 2022

sam-heilbron Apr 7, 2022

github-actions bot commented Apr 7, 2022 •

edited

Loading

kdorosh left a comment

nfuden left a comment

Resolve consul plugin data race #6271

Resolve consul plugin data race #6271

Conversation

sam-heilbron commented Apr 7, 2022 • edited Loading

Description

Context

Why fallback on errors?

What is the bug?

What is the solution?

How did I test it?

Checklist:

solo-changelog-bot bot commented Apr 7, 2022

sam-heilbron Apr 7, 2022

Choose a reason for hiding this comment

sam-heilbron Apr 7, 2022

Choose a reason for hiding this comment

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

kdorosh left a comment

Choose a reason for hiding this comment

nfuden left a comment

Choose a reason for hiding this comment

sam-heilbron commented Apr 7, 2022 •

edited

Loading

github-actions bot commented Apr 7, 2022 •

edited

Loading