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: synthesize option #22599

Merged
merged 1 commit into from Aug 26, 2022

Conversation

jacekmigacz
Copy link
Contributor

Provides a configuration switch for RR synthesis in resolved.

@poettering
Copy link
Member

uh, this seems overly broad. What is the usecase here? we synthesize a variety of RRs, and I doubt most of those are anything anyone would ever want to disable.

So what's the rationale here? please eboarate.

@poettering poettering added needs-reporter-feedback ❓ There's an unanswered question, the reporter needs to answer resolve labels Feb 22, 2022
@jacekmigacz
Copy link
Contributor Author

There is at least this case: https://lists.freedesktop.org/archives/systemd-devel/2021-September/046856.html
Proposed solution is good, but if it's not a resolvectl that makes a query, then you are out of luck.

@poettering
Copy link
Member

There's now an env var too: SYSTEMD_NSS_RESOLVE_SYNTHESIZE, see nss-resolve man page. Wouldn't that suffice? See 8ef114c

@poettering
Copy link
Member

So I'd be with a change like this to disable synthesis in resolved server-side, but I think it should be more focussed: only on the local hostname itself (leave the synthesis of localhost and such in place). And it should be controlled via env var rather than get a first class config setting. (i.e. this is a political thing: it should have a hackish feeling to turn on this option)

i.e. if people add Environment=SYSTEMD_RESOLVED_SYNTHESIZE_HOSTNAME=0 to systemd-resolved.service we would turn off the logic. Would need a simple patch to src/resolve/resolved-synthesize.c

@poettering
Copy link
Member

ping?

@@ -444,6 +445,11 @@ int dns_synthesize_answer(
} else if (dns_name_address(name, &af, &address) > 0) {
int v, w;

if (getenv_bool("SYSTEMD_RESOLVED_SYNTHESIZE_HOSTNAME") == 0) {
nxdomain = true;
Copy link
Member

Choose a reason for hiding this comment

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

hm, nxdomain seems a bit drastic? i.e. if you return nxdomain then this ends the lookup logic. But it appears to me, if the env var is set we simply shouldn't respond at a ll, i.e. just do continue here...

also, shouldn't the manager_is_own_hostname() branch above get the same treatment? to disable both directions of local hostname resolution? from address to name and from name to address?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the nxdomain, you are right. That is too drastic. I will just continue iteration as suggested.

I guess both directions makes sense. I will do it for normal and reverse lookups (and change ENVIRONMENT.md).

@jacekmigacz jacekmigacz force-pushed the resolved-synthesize-option branch 2 times, most recently from 1064f8f to 69146fa Compare August 22, 2022 16:27
`systemd-resolved`:

* `$SYSTEMD_RESOLVED_SYNTHESIZE_HOSTNAME=1` — if set, `systemd-resolved` will
synthesize system hostname on both regular and reverse lookups.
Copy link
Member

Choose a reason for hiding this comment

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

the text is wrong. It will synthesize this by default. But if you set the variable to zero then it won't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I wrongly deducted previous ENVs. Fixed now.

@poettering poettering added good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed and removed needs-reporter-feedback ❓ There's an unanswered question, the reporter needs to answer labels Aug 25, 2022
@yuwata yuwata merged commit d896260 into systemd:main Aug 26, 2022
41 of 45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation good-to-merge/waiting-for-ci 👍 PR is good to merge, but CI hasn't passed at time of review. Please merge if you see CI has passed resolve
Development

Successfully merging this pull request may close these issues.

None yet

3 participants