dig lookup: Allow to pass port for DNS lookup #8966
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ansibullbot
added
feature
felixfontein
added
check-before-release
felixfontein
reviewed
Oct 2, 2024
| @@ -0,0 +1,4 @@ | |||
| --- | |||
| minor_changes: | |||
| - dig lookup module - add ``port`` option to specify DNS server port (https://github.com/ansible-collections/community.general/pull/8966). | |||
There was a problem hiding this comment.
Suggested change
| - dig lookup module - add ``port`` option to specify DNS server port (https://github.com/ansible-collections/community.general/pull/8966). | |
| - dig lookup plugin - add ``port`` option to specify DNS server port (https://github.com/ansible-collections/community.general/pull/8966). |
plugins/lookup/dig.py
Outdated
| @@ -358,7 +363,6 @@ def run(self, terms, variables=None, **kwargs): | |||
| nameservers.append(nsaddr) | |||
| except Exception as e: | |||
| raise AnsibleError("dns lookup NS: %s" % to_native(e)) | |||
| myres.nameservers = nameservers | |||
There was a problem hiding this comment.
The old code only keeps the nameservers from the last entry in @, while with your change all nameservers provided by the user are used.
I would say this is a bugfix. (Or is myres.nameservers = ... doing some magic so that if you do it twice, the list of nameservers is extended?)
Do you mind moving the nameserver modification to a separate PR? Bugfixes are backported to more stable branches than new features (which adding port is).
There was a problem hiding this comment.
I didn't even notice there was a bug and my change fixed it. Thanks for pointing that out. I'll open another PR for just that bugfix.
plugins/lookup/dig.py
Outdated
Comment on lines
390
to
391
| elif opt == 'port': | ||
| port = int(arg) |
There was a problem hiding this comment.
This processing is only for legacy argument parsing.
Suggested change
| elif opt == 'port': | |
| port = int(arg) |
|
Thanks for your review! I've applied the suggested changes |
|
Created separate PR with just the multi-nameservers-bugfix as #8970 |
|
This will likely need a rebase to resolve the conflict once #8970 has been merged. |
|
This now needs a rebase. |
ansibullbot
added
needs_rebase
dnspython accepts a port as part of the nameserver. Currently, the nameservers are passed as strings which leads dnspython to create Nameserver objects out of them using the port that is currently set in the Resolver instance. That creation of Nameserver objects is done right when the `nameservers` property is set. If a port is to be set by us, the `port` attribute of the Resolver needs to be set before the nameservers are passed to the Resolver so when the nameservers are passed, that new port is used to create the Nameserver objects. Therefore, the assignment of the `nameservers` property of the Resolver is moved after the argument processing so the `port` attribute is (if it's given in the lookup-call) definitely set before the `nameservers` property.
JaegerMaKn
force-pushed
the
feature-dig-port
branch
from
fe59150 to
a830110
Compare
|
Rebased |
ansibullbot
removed
needs_rebase
felixfontein
approved these changes
Oct 7, 2024
Backport to stable-9: 💚 backport PR created✅ Backport PR branch: Backported as #9004 🤖 @patchback |
felixfontein
removed
the
check-before-release
patchback bot
pushed a commit
that referenced
this pull request
Oct 7, 2024
dnspython accepts a port as part of the nameserver. Currently, the nameservers are passed as strings which leads dnspython to create Nameserver objects out of them using the port that is currently set in the Resolver instance. That creation of Nameserver objects is done right when the `nameservers` property is set. If a port is to be set by us, the `port` attribute of the Resolver needs to be set before the nameservers are passed to the Resolver so when the nameservers are passed, that new port is used to create the Nameserver objects. Therefore, the assignment of the `nameservers` property of the Resolver is moved after the argument processing so the `port` attribute is (if it's given in the lookup-call) definitely set before the `nameservers` property. (cherry picked from commit 5e6b8e5)
|
@JaegerMaKn thanks for your contribution! |
felixfontein
pushed a commit
that referenced
this pull request
Oct 7, 2024
… for DNS lookup (#9004) dig lookup: Allow to pass port for DNS lookup (#8966) dnspython accepts a port as part of the nameserver. Currently, the nameservers are passed as strings which leads dnspython to create Nameserver objects out of them using the port that is currently set in the Resolver instance. That creation of Nameserver objects is done right when the `nameservers` property is set. If a port is to be set by us, the `port` attribute of the Resolver needs to be set before the nameservers are passed to the Resolver so when the nameservers are passed, that new port is used to create the Nameserver objects. Therefore, the assignment of the `nameservers` property of the Resolver is moved after the argument processing so the `port` attribute is (if it's given in the lookup-call) definitely set before the `nameservers` property. (cherry picked from commit 5e6b8e5) Co-authored-by: JaegerMaKn <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Add new option
portto thediglookup module to specify which port is to be used for DNS query.This is necessary for setups in which a (custom/internal) DNS server listens on a non-default port.
ISSUE TYPE
COMPONENT NAME
digADDITIONAL INFORMATION
dnspython accepts a port as part of the nameserver and not, like the other optioins that are currently present in the
digmodule, to be passed to thequeryfunction.Currently, the nameservers are passed as strings which leads dnspython to create Nameserver objects out of them using the port that is currently set in the Resolver instance. That creation of Nameserver objects is done right when the
nameserversproperty is set.If a port is to be set by us, the
portattribute of the Resolver needs to be set before the nameservers are passed to the Resolver so when the nameservers are passed, that new port is used to create the Nameserver objects.Therefore, the assignment of the
nameserversproperty of the Resolver is moved after the argument processing so theportattribute is (if it's given) definitely set before thenameserversproperty.