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

Validate absolute DNS name requirement on StaticDNS CNAME #4743

Closed
3 of 16 tasks
jhg03a opened this issue May 30, 2020 · 13 comments · Fixed by #4934
Closed
3 of 16 tasks

Validate absolute DNS name requirement on StaticDNS CNAME #4743

jhg03a opened this issue May 30, 2020 · 13 comments · Fixed by #4934
Labels
bug something isn't working as intended documentation related to documentation high impact impacts the basic function, deployment, or operation of a CDN improvement The functionality exists but it could be improved in some way. Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1

Comments

@jhg03a
Copy link
Contributor

jhg03a commented May 30, 2020

I'm submitting a ...

  • bug report
  • new feature / enhancement request
  • improvement request (usability, performance, tech debt, etc.)
  • other

Traffic Control components affected ...

  • CDN in a Box
  • Documentation
  • Grove
  • Traffic Control Client
  • Traffic Monitor
  • Traffic Ops
  • Traffic Ops ORT
  • Traffic Portal
  • Traffic Router
  • Traffic Stats
  • Traffic Vault
  • unknown

Current behavior:

When I was creating a Static DNS CNAME record, I added domain.foo.com to a new DS and snapshotted.
This causes the snapshot to fail to apply with no user feedback except to go dig around in TR logs.

Expected / new behavior:

Please remove the footgun of forgetting the trailing dot in the CNAME address and better safeties to prevent it from becoming part of a snapshot.

Minimal reproduction of the problem with instructions:

  1. Add a staticDNS CNAME record with address lacking the final period.
  2. Snapshot
  3. Observe the Snapshot fails to apply
  4. Add a trailing period to the address field of record from 1.
  5. Snapshot
  6. Observe things work after head banging

Anything else:

WARN  2020-05-30T01:57:54.921 [New I/O worker #8] com.comcast.cdn.traffic_control.traffic_router.core.util.PeriodicResourceUpdater - 'ccr.testds.cdn.domain.com' is not an absolute name
org.xbill.DNS.RelativeNameException: 'domain.foo.com' is not an absolute name
        at org.xbill.DNS.Record.checkName(Record.java:721)
        at org.xbill.DNS.SingleNameBase.<init>(SingleNameBase.java:33)
        at org.xbill.DNS.SingleCompressedNameBase.<init>(SingleCompressedNameBase.java:23)
        at org.xbill.DNS.CNAMERecord.<init>(CNAMERecord.java:28)
        at com.comcast.cdn.traffic_control.traffic_router.core.dns.ZoneManager.addStaticDnsEntries(ZoneManager.java:703)
@jhg03a jhg03a added documentation related to documentation improvement The functionality exists but it could be improved in some way. Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1 labels May 30, 2020
@ocket8888
Copy link
Contributor

So is the preferred solution to make it so that a CNAME can be added without a trailing . or so that such CNAMEs would be rejected by the API?

@jhg03a
Copy link
Contributor Author

jhg03a commented May 30, 2020

Yes. Implementer's choice.

@jhg03a
Copy link
Contributor Author

jhg03a commented May 30, 2020

Or at least update the documentation with a CNAME example showing it's a requirement

@jhg03a
Copy link
Contributor Author

jhg03a commented May 30, 2020

Triaging silent errors, reading stack traces, and digging through 3rd party library source code to find out what "Absolute" means is not something operators can afford to be doing during an outage.

@mitchell852
Copy link
Member

when you create a federation cname, you have to add the trailing dot. i say we keep it consistent and just require the trailing dot.

@mitchell852
Copy link
Member

mitchell852 commented Jun 3, 2020

this is the error you get if you leave out the trailing dot.

org.xbill.DNS.RelativeNameException: 'foo' is not an absolute name

@mitchell852 mitchell852 added the bug something isn't working as intended label Jun 3, 2020
@mitchell852
Copy link
Member

also, i added the bug label because missing validation feels like a bug to me.

@mitchell852 mitchell852 changed the title Remove absolute DNS name requirement on StaticDNS CNAME Validate absolute DNS name requirement on StaticDNS CNAME Jun 3, 2020
@mitchell852
Copy link
Member

I also changed the title @jhg03a - hope you don't mind.

@jhg03a
Copy link
Contributor Author

jhg03a commented Jun 5, 2020

Yes. Implementer's choice.

That's fine. It just needs a helpful message to go with and not just "denied"

@mitchell852 mitchell852 added the high impact impacts the basic function, deployment, or operation of a CDN label Jul 13, 2020
@mitchell852
Copy link
Member

I put the major label on this as it will block a snapshot.

@rimashah25
Copy link
Contributor

rimashah25 commented Jul 23, 2020

@jhg03a : Does the trailing period affect only DNS type cname_record or the other 3 as well?

@jhg03a
Copy link
Contributor Author

jhg03a commented Jul 23, 2020

CNAME is where I found it. In theory it would be anywhere we specify the Dns name of a record that uses that library to validate.

@mitchell852
Copy link
Member

fixed per #4934

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something isn't working as intended documentation related to documentation high impact impacts the basic function, deployment, or operation of a CDN improvement The functionality exists but it could be improved in some way. Traffic Ops related to Traffic Ops Traffic Portal v1 related to Traffic Portal version 1
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants