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.

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

feat(dns): Rewrite dns persistence to allow virtual-outbound to be added #2484

Merged
merged 5 commits into from
Aug 11, 2021

Conversation

lahabana
Copy link
Contributor

@lahabana lahabana commented Aug 3, 2021

Currently the persisted state on the vips was very restraining.
With the addition of virtual-outbounds we'll need something more flexible
This migrates the state to a new format that will be extensible.

It also avoids reusing vips straight away which might avoid some issues when services are added and removed at once.

@lahabana lahabana requested a review from lobkovilya August 3, 2021 12:41
@lahabana lahabana requested a review from a team as a code owner August 3, 2021 12:41
Currently the persisted state on the vips was very restraining.
With the addition of virtual-outbounds we'll need something more flexible
This migrates the state to a new format that will be extensible.

Signed-off-by: Charly Molter <charly.molter@konghq.com>
@lahabana lahabana force-pushed the feat/virtual-outbound_dnspersistence branch from 1188d51 to 7d7785c Compare August 5, 2021 10:08
This avoids computing the same thing twice. Now that allocator
persists the correct configuration we can just reuse this

Signed-off-by: Charly Molter <charly.molter@konghq.com>
@lahabana lahabana force-pushed the feat/virtual-outbound_dnspersistence branch from 1f3f318 to 952b21e Compare August 9, 2021 13:20
…bound_dnspersistence

Signed-off-by: Charly Molter <charly.molter@konghq.com>
@codecov-commenter
Copy link

Codecov Report

Merging #2484 (6529843) into master (5721243) will increase coverage by 0.00%.
The diff coverage is 79.67%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2484   +/-   ##
=======================================
  Coverage   52.28%   52.28%           
=======================================
  Files         865      866    +1     
  Lines       48098    48130   +32     
=======================================
+ Hits        25147    25165   +18     
- Misses      20919    20926    +7     
- Partials     2032     2039    +7     
Impacted Files Coverage Δ
pkg/xds/sync/dataplane_proxy_builder.go 0.00% <0.00%> (ø)
pkg/dns/resolver/resolver.go 66.66% <64.28%> (+1.66%) ⬆️
pkg/dns/vips/interfaces.go 54.76% <66.66%> (-19.53%) ⬇️
pkg/dns/vips/virtual_outbound.go 79.10% <79.10%> (ø)
pkg/dns/vips_allocator.go 74.78% <79.59%> (-1.03%) ⬇️
pkg/dns/vips/persistence.go 65.38% <80.00%> (+0.91%) ⬆️
pkg/dns/vips/global_view.go 86.20% <86.20%> (ø)
pkg/dns/vips_synchronizer.go 82.85% <100.00%> (+2.85%) ⬆️
pkg/xds/context/context.go 64.70% <100.00%> (-1.97%) ⬇️
pkg/xds/server/components.go 55.55% <100.00%> (ø)
... and 11 more

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 5721243...6529843. Read the comment docs.

Copy link
Contributor

@lobkovilya lobkovilya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks great! I guess it's a lot of hard work to refactor/improve this part of the codebase. Thanks! I have few comments, mostly about naming, but overall it totally makes sense to me

pkg/dns/vips/global_view.go Outdated Show resolved Hide resolved
pkg/dns/vips/virtual_outbound.go Outdated Show resolved Hide resolved
pkg/dns/vips_allocator.go Outdated Show resolved Hide resolved
pkg/dns/vips_allocator.go Outdated Show resolved Hide resolved
pkg/dns/vips_allocator.go Outdated Show resolved Hide resolved
oldVob, ok := vo.byHostname[entry]
if ok {
if !oldVob.Equal(vob) {
changes = append(changes, Change{Type: Modify, Entry: entry})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How Modify can happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When we add a new outbound (for example same host but different port).

pkg/dns/vips/virtual_outbound.go Outdated Show resolved Hide resolved
@lahabana lahabana force-pushed the feat/virtual-outbound_dnspersistence branch from b2a03a7 to 386edb8 Compare August 10, 2021 12:39
Signed-off-by: Charly Molter <charly.molter@konghq.com>
…bound_dnspersistence

Signed-off-by: Charly Molter <charly.molter@konghq.com>
@lahabana lahabana merged commit e96348b into kumahq:master Aug 11, 2021
@lahabana lahabana deleted the feat/virtual-outbound_dnspersistence branch March 29, 2024 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants