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

Fix t3c to parent.config gen for IP origins #7021

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

rob05c
Copy link
Member

@rob05c rob05c commented Aug 12, 2022

Currently, the cache config doesn't do any insight into the Delivery Service OrgServerFQDN field, and simply injects it as if it were a URI with an FQDN.

This causes issues as a parent.config dest_domain=x with an IP address is silently ignored by ATS, causing the corresponding remap.config line to either fall through to the dest_domain=. default rule, or to not use parent.config at all and go direct if no default rule exists in parent.config. ATC cache config should always create a default rule, with default parents. But these parents won't be the parents specifically assigned to that service, and won't contain any custom rules like retry codes and number of retries.

This fixes the cache config to generate dest_ip=x instead of dest_domain=x if the DS OrgServerFQDN is an IP address.

  • I tested, and this is not an issue with strategies.yaml
  • I tested both IPv4 an IPv6 addresses, both work correctly with this change
  • This is not an issue for MSO, as the issue is with the dest_ parent.config directive, and MSO uses the OrgServerFQDN as the remap target and parent dest, with real origins being inserted in the parent= directive where IPs are fine.
  • Note this will not send any proper hostname in the Host header of the request. If the origin requires a particular host be requested, it must be in the DS Origin. ATC simply lacks the data otherwise, and moreover ATS itself will always use the remap target as the host header, ATC doesn't have the ability to change that.

Which Traffic Control components are affected by this PR?

  • Traffic Control Cache Config (t3c, formerly ORT)

What is the best way to verify this PR?

Run tests. Set an IP as an origin on a Delivery Service that uses cache parents, generate config, verify parent.config dest_ip is set, verify a request uses the parents specified on the line and not the parents specified in the default rule and doesn't go direct.

  • note the DS OrgServerFQDN must be a URI, not an FQDN, contrary to the field name. I.e. http://192.0.2.1 not simply 192.0.2.1.

If this is a bugfix, which Traffic Control versions contained the bug?

All prior versions.

PR submission checklist

@rob05c rob05c added bug something isn't working as intended cache-config Cache config generation labels Aug 12, 2022
@rawlinp
Copy link
Contributor

rawlinp commented Aug 12, 2022

Does this fix the following issue? #890

@rob05c
Copy link
Member Author

rob05c commented Aug 12, 2022

Does this fix the following issue? #890

Yes, I added a link.

it seems to work.

It doesn't work, that statement is wrong. I think we just didn't realize it before, because it falls back to the default, and we didn't used to have complex custom parents until recently.

I'd also note, that issue doesn't capture the complete issue with the bug. #890 just says we should do it, it doesn't say why. The reason and bug is that dest_domain=ip is silently ignored by ATS as if the entire line doesn't exist.

@rawlinp
Copy link
Contributor

rawlinp commented Aug 13, 2022

#890 mentions cache.config too, do we already use dest_ip in that file?

@rob05c
Copy link
Member Author

rob05c commented Aug 15, 2022

#890 mentions cache.config too, do we already use dest_ip in that file?

No, that's a separate bug. I removed the Fixes directive.

Copy link
Contributor

@jpappa200 jpappa200 left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me!

@ocket8888 ocket8888 added the medium impact impacts a significant portion of a CDN, or has the potential to do so label Aug 18, 2022
@ocket8888 ocket8888 merged commit 9457e34 into apache:master Aug 18, 2022
zrhoffman pushed a commit to zrhoffman/trafficcontrol that referenced this pull request Oct 2, 2022
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 cache-config Cache config generation medium impact impacts a significant portion of a CDN, or has the potential to do so
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants