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.

Sign up for GitHub

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

Jump to bottom

Remove protocol restriction for SRV records in cloudflare_dns module. Fixes #36708 #36709

Merged
merged 1 commit into from Mar 9, 2018
Merged

Remove protocol restriction for SRV records in cloudflare_dns module. Fixes #36708 #36709

merged 1 commit into from Mar 9, 2018

Conversation

DennisGlindhart
Copy link
Contributor

@DennisGlindhart DennisGlindhart commented Feb 26, 2018 •
edited

SUMMARY

Remove protocol restriction for SRV records in cloudflare_dns module. Fixes #36708

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

cloudflare_dns

ANSIBLE VERSION
ansible 2.4.3.0
  python version = 2.7.14 (default, Feb 17 2018, 10:42:17) [GCC 7.3.1 20180130 (Red Hat 7.3.1-2)]

@ansibot
Copy link
Contributor

ansibot commented Feb 26, 2018

cc @andreaso @mgruener
click here for bot help

@ansibot ansibot added bugfix_pull_request community_review In order to be merged, this PR must follow the community review workflow. module This issue/PR relates to a module. needs_triage Needs a first human triage before being processed. net_tools Net-tools category new_contributor This PR is the first contribution by a new community member. support:community This issue/PR relates to code supported by the Ansible community. labels Feb 26, 2018
@Akasurde Akasurde mentioned this pull request Feb 26, 2018
Closed
@Akasurde Akasurde removed the needs_triage Needs a first human triage before being processed. label Feb 26, 2018
Akasurde
Akasurde reviewed Feb 26, 2018
View reviewed changes
lib/ansible/modules/net_tools/cloudflare_dns.py Outdated
@@ -44,7 +44,7 @@
proto:
description: Service protocol. Required for C(type=SRV)
required: false
choices: [ 'tcp', 'udp' ]
choices: [ 'tcp', 'udp', 'tls' ]
Copy link
Member

Choose a reason for hiding this comment

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

Could you please mention like this in description

option 'tls' is added version 2.6

@andreaso
Copy link
Contributor

andreaso commented Feb 26, 2018 •
edited

Ok, up until now I had simply assumed that tcp and udp were the only two allowed values for the SRV record protocol field. Yet, taking a closer look at RFC 2782 those two protocols are simply listed as "at present the most useful values for this field".

Likewise CloudFlare appear to accept all kinds of values in the protocol position, not simply tcp, udp, and tls.

@DennisGlindhart Is there any specific and/or common use case for naming the tls protocol in SRV records? That is, does it make the most sense to explicit add tls as an allowed parameter value, or does it make more sense to fully remove the present restrictions?

@DennisGlindhart
Copy link
Contributor Author

@andreaso According to RFC 2782 all kinds of values are allowed, yes. Although, using the Cloudflare Admin-UI in the browser, it seems that only tcp, udp and tls are possible to create in CF. It might be worth investigating whether or not CF allows for other types when using the API instead.

If CF really does support arbitrary values, I think it would make sense to remove the restriction completely.

I'll try to investigate further and adjust PR accordingly.

@andreaso
Copy link
Contributor

Using the API (or more specifically, by using a locally modified cloudflare_dns module) I managed to create a SRV record with the made-up zebra protocol.

On the other hand, there is a benefit in keeping a restrictive parameter list, as it makes it easier to use the module properly. Especially since it's to my knowledge is very unusual for people to need SRV records for arbitrary protocols.

Hence back to my original question, what's the use case for the tls protocol in this context?

@DennisGlindhart
Copy link
Contributor Author

@andreaso Yep - I was also able to create some "random" protocol via the API.
My particular case for tls was setting up some Office 365/Lync
SRV _sip._tls.example.com > sipdir.online.lync.com
(Example here: https://support.microsoft.com/en-us/help/2566790/troubleshooting-skype-for-business-online-dns-configuration-issues-in)

So TL;DR: Cloudflare API allows us to follow the RFC and use arbitrary protocol. Although tcp, udp and tls are probably enough for 99% of users, there are valid scenarios for other protocols as well. (Maybe we will soon see more protocols used (i.e SCTP or DCCP) when NAT is more or less going away with IPv6 currently preventing use of other protocols than TCP and UDP)

I don't think removing the restriction makes the module that much harder to use, and following the standard is more important when we have the possibility. We can write the "normal" values (tcp & udp) in the description. I think people using this module will mostly be copying the values from some reference (like the link above) - or alternatively - know what they are doing.

I'm thinking of just remove the restrictions then - Agree?

@andreaso
Copy link
Contributor

Thanks for helping me sort out what is what here! At this point I can see the merit with both approaches, and I'll be willing to approve whichever one you choose to move forward with in this pull request.

@DennisGlindhart DennisGlindhart changed the title Add tls to list of allowed protocols for SRV records in cloudflare_dns module. Fixes #36708 Remove protocol restriction for SRV records in cloudflare_dns module. Fixes #36708 Feb 28, 2018
@DennisGlindhart
Copy link
Contributor Author

I have now removed the restriction altogether so we follow the RFC.

I've also "rebased" / removed my previous commits (adding tls) so this is one clean commit and changed the title of the PR to be more correct. I think this should be ready for merge now.

Thanks for all your help!

cc @andreaso

Akasurde
Akasurde requested changes Feb 28, 2018
View reviewed changes
lib/ansible/modules/net_tools/cloudflare_dns.py Outdated
@@ -42,9 +42,8 @@
required: false
default: "1"
proto:
description: Service protocol. Required for C(type=SRV)
description: Service protocol. Required for C(type=SRV). Common values are tcp and udp. (Before Ansible 2.6 only tcp and udp were available)
Copy link
Member

Choose a reason for hiding this comment

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

Missing . at end.

lib/ansible/modules/net_tools/cloudflare_dns.py Outdated
@@ -42,9 +42,8 @@
required: false
default: "1"
proto:
description: Service protocol. Required for C(type=SRV)
description: Service protocol. Required for C(type=SRV). Common values are tcp and udp. (Before Ansible 2.6 only tcp and udp were available)
Copy link
Member

Choose a reason for hiding this comment

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

Also, add this note is CHANGELOG.md telling end-user about this.

@ansibot ansibot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. and removed community_review In order to be merged, this PR must follow the community review workflow. labels Feb 28, 2018
@ansibot ansibot added the support:core This issue/PR relates to code supported by the Ansible Engineering Team. label Feb 28, 2018
@andreaso
Copy link
Contributor

shipit

@andreaso
Copy link
Contributor

@Akasurde: Happy as well?

@ansibot ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 2, 2018
@DennisGlindhart
Copy link
Contributor Author

@Akasurde I adjusted according to your review/change requests and pushed a new commit replacing the existing one with the intention to to keep history simple, but the automated bot/checks does not seem to have picked up the change (maybe because it wasn't a new commit on top of the existing one).
TL;DR Your requested changes have been implemented, so this should AFAIK be ready for merge :)

@ansibot ansibot added stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Mar 9, 2018
@ansibot ansibot added core_review In order to be merged, this PR must follow the core review workflow. and removed needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR. stale_review Updates were made after the last review and the last review is more than 7 days old. labels Mar 9, 2018
gundalow
gundalow approved these changes Mar 9, 2018
View reviewed changes
Copy link
Contributor

@gundalow gundalow left a comment

Choose a reason for hiding this comment

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

Assuming udp and UdP work the same. Otherwise use tolower

@andreaso
Copy link
Contributor

andreaso commented Mar 9, 2018 •
edited

As a general rule DNS names are compared without considering case.

When it comes to SRV records RFC 2782 explicitly states that "The Proto is case insensitive."

@gundalow
Copy link
Contributor

gundalow commented Mar 9, 2018

Thanks for confirming

@ansibot ansibot removed the stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. label Mar 9, 2018
@Akasurde
Copy link
Member

Akasurde commented Mar 9, 2018

rebuild_merge

@ansibot ansibot merged commit 5506229 into ansible:devel Mar 9, 2018
@Akasurde
Copy link
Member

Akasurde commented Mar 9, 2018

@DennisGlindhart @andreaso @gundalow Thanks for contribution.

@DennisGlindhart DennisGlindhart deleted the cloudflare-srvtls branch March 9, 2018 14:38
@ansible ansible locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Akasurde Akasurde Akasurde approved these changes

@gundalow gundalow gundalow approved these changes

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug. core_review In order to be merged, this PR must follow the core review workflow. module This issue/PR relates to a module. net_tools Net-tools category new_contributor This PR is the first contribution by a new community member. support:community This issue/PR relates to code supported by the Ansible community. support:core This issue/PR relates to code supported by the Ansible Engineering Team.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cloudflare DNS does not allow for tls protocol type
5 participants
@DennisGlindhart @ansibot @andreaso @gundalow @Akasurde