Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

dns: de-dup returned entries #433

Closed
mattklein123 opened this issue Feb 6, 2017 · 2 comments · Fixed by #452
Closed

dns: de-dup returned entries #433

mattklein123 opened this issue Feb 6, 2017 · 2 comments · Fixed by #452
Labels
bug
Milestone
1.2.0

Comments

@mattklein123
Copy link
Member

This was raised here: #431

If DNS returns the same entry multiple times, we will keep adding a host and effectively leak. Either the DNS impl itself should dedup, or the dymamic cluster add/remove code should do it.

cc @htuch
@mattklein123 mattklein123 added this to the 1.2.0 milestone Feb 6, 2017
@htuch
Copy link
Member

htuch commented Feb 6, 2017

To what extent does order and multiplicity matter? Could we just pass a set to ResolveCb instead of a list in DnsResolver? Might the upper level cluster code use multiplicity to influence weight or order to determine preference?

@mattklein123
Copy link
Member Author

Order doesn't currently matter, but the code in:
https://github.com/lyft/envoy/blob/master/source/common/upstream/upstream_impl.cc#L273

Does not correctly handle duplicates.

IMO, it would be better to fix the above code since that will cover not only DNS, but SDS, etc. also, and dealing with duplicates is really an envoy "problem" and not a DNS problem.

@timperrett timperrett mentioned this issue Feb 6, 2017
Closed
mattklein123 added a commit that referenced this issue Feb 9, 2017
@mattklein123
correctly de-dup DNS/SDS hosts based on address
507e799 
Previously we would incorrectly keep adding hosts and essentially
leak memory.

fixes #433
@mattklein123 mattklein123 mentioned this issue Feb 9, 2017
Merged
mattklein123 added a commit that referenced this issue Feb 9, 2017
@mattklein123
correctly de-dup DNS/SDS hosts based on address (#452)
c9b0621 
Previously we would incorrectly keep adding hosts and essentially
leak memory.

fixes #433
rshriram pushed a commit to rshriram/envoy that referenced this issue Oct 30, 2018
@lizan @rshriram
Use per request AsyncClient (envoyproxy#433)
933b1c3
rshriram pushed a commit to rshriram/envoy that referenced this issue Oct 30, 2018
@istio-testing
Update_Dependencies (envoyproxy#433)
a08d675 
Automatic merge from submit-queue.

[DO NOT MERGE] Auto PR to update dependencies of mixerclient

This PR will be merged automatically once checks are successful.
```release-note
none
```
jplevyak pushed a commit to jplevyak/envoy that referenced this issue Apr 13, 2020
@PiotrSikora
ci: increase timeout for a few //test/common/stats:... tests. (envoyp…
e176da6 
…roxy#433)

Those tests occasionally timeout on istio/envoy CI.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
wolfguoliang pushed a commit to wolfguoliang/envoy that referenced this issue Jan 23, 2021
@vgbhfive
zh-translation: docs/root/configuration/listeners/network_filters/thr… (
2f60025 
envoyproxy#433)

* zh-translation: docs/root/configuration/listeners/network_filters/thrift_proxy_filter.rst

* fix translate
jpsim pushed a commit that referenced this issue Nov 28, 2022
@rebello95 @jpsim
ios: support setting domain from typed config (#432)
c9fc0f6 
Adds support for setting the domain used in the base cluster of the Envoy config. This is necessary until `dynamic_forward_proxy` is working (see #433).

Android change will come in a follow-up PR.

Risk Level: Low
Testing: Locally on demos & unit tests

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this issue Nov 29, 2022
@rebello95 @jpsim
ios: support setting domain from typed config (#432)
01b4140 
Adds support for setting the domain used in the base cluster of the Envoy config. This is necessary until `dynamic_forward_proxy` is working (see #433).

Android change will come in a follow-up PR.

Risk Level: Low
Testing: Locally on demos & unit tests

Signed-off-by: Michael Rebello <me@michaelrebello.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
1.2.0
Development

Successfully merging a pull request may close this issue.

correctly de-dup DNS/SDS hosts based on address envoyproxy/envoy
2 participants
@mattklein123 @htuch