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

types - dns: add a new dns type with more specific fqdn definitions #1347

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

b4ldr
Copy link
Collaborator

@b4ldr b4ldr commented Apr 28, 2023

This PR introduces a new set of types for validating FQDN's. it creates a type for the more loose rfc definitions of domain names and a stricter and likely more useful iana type.

This allows us to create a new type that more does to what most users expect i.e. that a dns name is one that works on the internt, without breaking current uses for users that may be using the Stdlib::Fqdn to validate validate DNS names that don't work with the IANA roots.

The intention of this patch would be to deprecate the currnet Stdlib::Fqdn type and encourage users to move to the appropriate Stdlib::DNS::* type which for most users will likely be the stricter Stdlib::DNS::Fqdn type

Note: this PR is intentionally a bit rough to first garner thoughts as to if this is the correct direction

Fixes #1282 (not sure it fixes but want it tagged)

Copy link
Collaborator

@smortex smortex left a comment

Choose a reason for hiding this comment

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

This look very complex, and makes me think that choosing the correct type will be tricky. They would need an extended documentation to help choose the right one.

I am not sure that such a level of detail is important at the stdlib level, but understand it is required in some advanced cases. So I have nothing against this, but I do not feel comfortable with it neither 🤔 🤷 😁

types/dns/punycode.pp Outdated Show resolved Hide resolved
types/dns/rfc/fqdn.pp Outdated Show resolved Hide resolved
This PR introduces a new set of types for validating FQDN's.  it creates
a type for the more loose rfc definitions of domain names and a stricter
and likely more useful iana type.

This allows us to create a new type that more does to what most users
expect i.e. that a dns name is one that works on the internt, without
breaking current uses for users that may be using the Stdlib::Fqdn to
validate validate DNS names that don't work with the IANA roots.

The intention of this patch would be to deprecate the currnet Stdlib::Fqdn
type and encourage users to move to the appropriate Stdlib::DNS::* type
which for most users will likely be the stricter Stdlib::DNS::Fqdn type

Note: this PR is intentionally a bit rough to first garner thoughts as to
if this is the correct direction

Fixes puppetlabs#1282 (not sure it fixes but want it tagged)
@b4ldr
Copy link
Collaborator Author

b4ldr commented May 3, 2023

This look very complex, and makes me think that choosing the correct type will be tricky.

The intention is that Stdlib::DNS::Fqdn will point to the "correct" or at least most useful type and picky users can pick the more specific type if needed, this is sort of similar to the Stdlib::IP namespace

They would need an extended documentation to help choose the right one.

If people think this is the right direction happy to clean up the pr and add some better docs

I am not sure that such a level of detail is important at the stdlib level, but understand it is required in some advanced cases.

My intention here is to provide a migration path away from Stdlib::Fqdn which i suspect is not what most people want with something more strict which likely matches what they do want, whilst at the same time provide types for the edge case that may break if we make Stdlib::Fqdn stricter.

@smortex
Copy link
Collaborator

smortex commented May 3, 2023

Fine! This looks reasonable to me. Let's collect more people feedback!

@JonasVerhofste
Copy link

I just took 15 min trying to figure out why my logic was taking a certain codepath, until I figured out that $foo =~ Stdlib::Fqdn also matches on IPv4 addresses. So +1 from me for making it a bit more specific so it doesn't, or at least provide an alternative that doesn't.

Copy link

@JonasVerhofste JonasVerhofste left a comment

Choose a reason for hiding this comment

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

This probably also needs some unit tests in spec/type_aliases to detect breaking changes in the future. They will also help with clarifying the difference between Stdlib::DNS::Fqdn::IANA::ASCII and Stdlib::DNS::Fqdn::Rfc.

types/fqdn.pp Outdated Show resolved Hide resolved
Co-authored-by: Jonas Verhofsté <[email protected]>
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.

Stdlib::Fqdn type accepts ipv4 addresses, also allows truncated addresses
4 participants