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

resolved: support for dnssec requests in the stub & a lot of other stuff #17535

Closed
wants to merge 69 commits into from

Conversation

poettering
Copy link
Member

@poettering poettering commented Nov 6, 2020

@poettering
Copy link
Member Author

This is material for v248. I split some commits out of it, with smaller less risky stuff, as #17534 which should be good for v247

@lgtm-com
Copy link

lgtm-com bot commented Nov 6, 2020

This pull request introduces 1 alert when merging 6de96aa into ea394d4 - view on LGTM.com

new alerts:

  • 1 for Use of potentially dangerous function

@lgtm-com
Copy link

lgtm-com bot commented Nov 6, 2020

This pull request introduces 1 alert when merging c8022f3 into 2386d1c - view on LGTM.com

new alerts:

  • 1 for Use of potentially dangerous function

@lgtm-com
Copy link

lgtm-com bot commented Nov 9, 2020

This pull request introduces 1 alert when merging ae10b79 into a1b24ee - view on LGTM.com

new alerts:

  • 1 for Use of potentially dangerous function

Copy link
Member

@keszybz keszybz left a comment

Choose a reason for hiding this comment

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

I didn't do a full review of the bigger commits, but the shorter ones all look nice.

src/resolve/resolved-dns-answer.c Outdated Show resolved Hide resolved
src/resolve/resolved-dns-transaction.c Show resolved Hide resolved
dns_scope_next_dns_server(t->scope);

if (dns_scope_get_dns_server(t->scope) == t->server) {
log_debug_errno(r, "Still pointing to extra listener after switching DNS servers, refusing operation.");
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be quite a late point to figure this out. I'd instead expect that our own servers are dropped when parsing /etc/resolv.conf (and also maybe when parsing configuration and/or data received over dns). Why would we ever want to keep such servers on our list?

Copy link
Member Author

Choose a reason for hiding this comment

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

so i pondered about that, and i figured it's better to keep them around but not use them than to not even keep them around. And that's because resolved is multiple things: it's both a DNS client and a manager for /etc/resolv.conf. And the additional listeners are probably something that should show up in /etc/resolv.conf if we are told so (if you don't want them to be used, why have them?), and given that we likely write them out it hence made sense to allow pushing them into resolved, even if we don't want to use them ourselves.

Hope that makes sense?

man/resolvectl.xml Outdated Show resolved Hide resolved
man/resolvectl.xml Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Nov 11, 2020

This pull request introduces 1 alert when merging ac499b9 into 23dce98 - view on LGTM.com

new alerts:

  • 1 for Use of potentially dangerous function

@mrc0mmand
Copy link
Member

mrc0mmand commented Nov 11, 2020

The CentOS CI results won't appear here properly until the next force-push as I made a slight mistake during OCP debugging, apologies.

@lgtm-com
Copy link

lgtm-com bot commented Nov 11, 2020

This pull request introduces 1 alert when merging cc8f745 into 23dce98 - view on LGTM.com

new alerts:

  • 1 for Use of potentially dangerous function

@poettering
Copy link
Member Author

I guess we can close this now. Everything got split out and merged now, except for two final PRs: #18686 + #17800. I guess we don't need to keep this PR open anymore, since there's nothing here that wasn't in either of those branches.

Thanks everyone for the reviews, much appreciated, in particular @keszybz and @bluca!

@poettering poettering closed this Feb 18, 2021
@bluca
Copy link
Member

bluca commented Feb 18, 2021

thanks for splitting it up in logical pieces - it made reviewing much much easier

keszybz added a commit to keszybz/systemd that referenced this pull request Mar 26, 2021
As noted in systemd#17535 (comment),
"raw" is misleading in this context. Let's use a more descriptive term.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment