Skip to content

dnsdist: Add local ComboAddress parameter for SBind() at TeeAction() #11889

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

Merged
merged 5 commits into from
Sep 5, 2022

Conversation

FredericDT
Copy link
Contributor

@FredericDT FredericDT commented Aug 31, 2022

Short description

In my prod env, I need to Tee queries from specific ip address, since there are multiple ip addresses on a single interface and the receiver has source address based ACLs and Views.

So I proposed an optional parameter in TeeAction to set the socket bind address.

Usage:
addAction(AllRule(), TeeAction("192.0.2.54", false, "192.0.2.53"))

In which case, "192.0.2.54" is the ComboAddress of receiver, "192.0.2.53" is the ComboAddress of sender.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

Uasge:
    `addAction(AllRule(), TeeAction("192.0.2.54", false, "192.0.2.53"))`

    In which case, "192.0.2.54" is the ComboAddress of receiver, "192.0.2.53"
    is the ComboAddress of sender.

Signed-off-by: FredericDT <frederic.dt.twh@gmail.com>
    keywords

Signed-off-by: FredericDT <frederic.dt.twh@gmail.com>
@omoerbeek
Copy link
Member

I think there is need to have an extra boolean flag, a boost::optional<X> already has that flag in itself.

@rgacogne rgacogne added this to the dnsdist-1.8.0 milestone Aug 31, 2022
@FredericDT
Copy link
Contributor Author

FredericDT commented Aug 31, 2022

I think there is need to have an extra boolean flag, a boost::optional<X> already has that flag in itself.

Sounds good,

I think there are two options:

  1. Pass that boost::optional<std:string> to TeeAction class, build ComboAddress in the constructor.
  2. Pass boost::optional<ComboAddress> as parameter local.

I prefer the second way.

Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

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

Thanks for that pull request! I have made a few suggestions but nothing big, the code looks good.

boost::optional<ComboAddress> instead.

According to PowerDNS#11889

> An extra boolean flag is no needed for boost::optional<x>

Thanks Moerbeek and Gacogne

Signed-off-by: FredericDT <frederic.dt.twh@gmail.com>
The optional parameter `local` shall be added in version 1.8.0

Signed-off-by: FredericDT <frederic.dt.twh@gmail.com>
@FredericDT FredericDT requested a review from rgacogne August 31, 2022 08:11
Accoding to PowerDNS#11889
> rgacogne
> Since we only use the local address in the constructor, I don't think we need to keep it around?
>
> FredericDT
> Possibly using that d_local in statistical function?
>
> rgacogne
> I would prefer not keeping it for now, we can always add it back later when we actually decide to do something with it :)

Signed-off-by: FredericDT <frederic.dt.twh@gmail.com>
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@rgacogne rgacogne merged commit 79dca92 into PowerDNS:master Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants