Fix parent_select optional scheme #8831
Conversation
| scheme = PL_NH_SCHEME_NONE; | ||
| PL_NH_Note("Invalid 'scheme' value, '%s', for the strategy named '%s', setting to PL_NH_SCHEME_NONE", scheme_val.c_str(), | ||
| strategy_name.c_str()); | ||
| PL_NH_Note("Invalid scheme '%s' for strategy '%s', setting to NONE", scheme_val.c_str(), strategy_name.c_str()); |
There was a problem hiding this comment.
Since this isn't a constructor, seem slightly fragile to assume data member contain values set at construction. Hopefully no one ever assumes this function can re-init as well as init.
There was a problem hiding this comment.
I didn't write the func, it's inherited from core. But it has to exist because the object is constructed by the YAML library.
I think Init is effectively like a constructor, because we can't construct, since we can't prevent the YAML library from doing the construction.
But, I think Init does make that assumption. There are probably other data it doesn't reset either. I don't object to making Init safe in case someone re-init's an object, and resetting all members to their defaults, but that's probably a larger change and might be better as its own PR
| nh.scheme = PL_NH_SCHEME_HTTPS; | ||
| } else { | ||
| throw YAML::ParserException(node["scheme"].Mark(), "no valid scheme defined, valid schemes are http or https"); | ||
| PL_NH_Note("Invalid scheme '%s' for protocol, setting to NONE", scheme_val.c_str()); |
(cherry picked from commit 695f9f6)
|
Cherry-picked to v9.2.x |
* asf/9.2.x: Updated ChangeLog Fix parent_select optional scheme (apache#8831) Add `#pragma once` for PendingAction.h (apache#8846) Promote class PendingAction from HttpSM.h for use in other classes. (apache#8423) Fixes leak in SNIAction name globbing (apache#8827) Handle opentelemetry-cpp v1.3.0 upgrade for otel_tracer plugin (apache#8834) Fix overflow conditions in prefetch plugin (apache#8660) Remove incorrect comment from base64 functions (apache#8835) Add autest to cover updates to cache with alternates (apache#8779)
Fixes core strategies and parent_select plugin scheme to be optional, and adds a test.
Backstory: some time ago I noticed the strategies.yaml scheme was optional, and a strategy without a scheme would match a host without a scheme. This behavior seemed to be incidental, but it made it much easier for me to generate config, it would have been difficult to get the scheme in the parent generation, all that logic lived in a separate area of our code for remap generation. And the "scheme" isn't actually used by the strategy for anything except matching hosts; the actual scheme used for upstream requests is always the remap target scheme.
So I documented the behavior:
https://docs.trafficserver.apache.org/en/latest/admin-guide/files/strategies.yaml.en.html
This was broken recently, and changed to reject strategies.yaml files without schemes (as part of a larger validation PR). Which broke all our generated strategies.yaml files.
This PR fixes it to match the docs, and adds an autest to ensure it doesn't break again, and also comments in the code to make the intent clear.