Change remap loading to be fatal during startup. #8802

SolidWallOfCode · 2022-04-20T16:55:28Z

This also enables a missing "remap.config" to be treated as an empty file (that is, it "parses" successfully and yields no remap rules).

Due to feedback, this now adds a configuration variable, "proxy.config.url_remap.min_rules_required" to control how many rules are need in "remap.config" to be considered a valid configuration. With this a deployment can easily tune whether empty/missing file is acceptable. The PR has been updated with documentation and tests for this. Note the value is dynamic and it is therefore possible to have different behavior on startup vs. runtime (e.g. allow empty on start but not on reload).

Closes #8785

bneradt · 2022-04-20T17:16:31Z

Thanks for working on this @SolidWallOfCode.

I think we'll want to mark this for the 9.2.x project as well since the patch that introduced the issue went into 9.2.x.

randall · 2022-04-20T17:18:08Z

proxy/http/remap/RemapConfig.cc


  std::error_code ec;
  std::string content{ts::file::load(ts::file::path{path}, ec)};
+  if (ec.value() == ENOENT) { // a missing file is ok - treat as empty, no rules.


Maybe I haven't had enough coffee today, but how is a missing file ok? I know we'd want that to be a failure for our systems.

As a general policy for ATS we want to not require the existence of configuration files. If an empty file is acceptable, it shouldn't be required to actually put one there.

We have a specific setting to not force having a remap.config -- proxy.config.url_remap.remap_required https://docs.trafficserver.apache.org/admin-guide/files/records.config.en.html#proxy-config-url-remap-remap-required, which has existed since the first commit. I'd expect a missing file to need that setting set to 0.

No, that setting means a remap rule is not required to forward the transaction. It has nothing to do with whether there is an actual "remap.config" file.

We have a specific setting to not force having a remap.config -- proxy.config.url_remap.remap_required https://docs.trafficserver.apache.org/admin-guide/files/records.config.en.html#proxy-config-url-remap-remap-required, which has existed since the first commit. I'd expect a missing file to need that setting set to 0.

This also seems correct to me. Do we at least want a warning in diags.log if remap.config is empty? Do we know of ATS users who don't do any remapping?

randall

Just want some clarification before this is merged.

ezelkow1 · 2022-04-20T18:08:52Z

Does this still allow a remap to be partially loaded when it's bad? IMHO that's still a very bad issue since it allows problems to crop up that are even worse than just not reloading the file, you could end up with all_services-1 not being able to serve

SolidWallOfCode · 2022-04-20T19:46:41Z

No. The partial loading still happens during startup, but ATS is then immediately shut down so it's irrelevant. Reloads continue to be atomic (all or nothing of the new configuration).

zwoop · 2022-04-20T21:07:24Z

Reading comments, I think we

Should not change behavior in 9.2.x to allow non-existing files. One can argue this is a bug, but the effects of someone deploying this, assuming this is how it works, could be catastrophic.
We can change 10.0.0 to allow no remap.config and allow startup as such.
In 10.0.x, I would urge though to not allow a config reload, if the previous config had a remap.config file, and the reload has lost it. I.e. don't allow for a "rm remap.config" + reload to cause a complete failure.

As such, my strong preference is that we back out this train of PRs from 9.2.x, and roll forward with 10.0.x

bneradt · 2022-04-20T22:06:56Z

As such, my strong preference is that we back out this train of PRs from 9.2.x, and roll forward with 10.0.x

In that case, we should revert #7782 from 9.2.x and not cherry-pick this PR to 9.2.x. I'll create a PR to revert #7782 from 9.2.x.

Revert "Short circuit remap reload when a valid remap file is not specified (apache#7782)" This reverts commit d7847f2. This is being reverted from 9.2.x because changing remap.config loading behavior was deemed too big a change to make within the 9.x releases. See the discussion in apache#8802. These remap.config changes will be pursued in the 10.x train.

zwoop · 2022-04-20T22:57:51Z

I think @SolidWallOfCode has a strong preference to not revert.

SolidWallOfCode · 2022-04-21T12:07:35Z

I went back and checked. Prior to #7782 a missing "remap.config" was allowed. This is a reversion to previous behavior.

I have checked this in four cases to verify the operation is what is requested.

Valid, empty, missing "remap.config" at start up - successful start.
"remap.config" containing an error at start up - emergency shutdown (no restart).
Valid, empty, missing "remap.config" for reload - remap rules are updated.
"remap.config" containing an error for reload - remap rules are not updated.

This makes "remap.config" loading atomic - all of the file, or none of it.

bryancall · 2022-04-21T18:18:40Z

Since ATS prior to #7782 has the features you want, can't we just revert #7782? If not what is the difference in behavior now (after this PR) vs pre-#7782?

SolidWallOfCode · 2022-04-22T14:33:27Z

This change unifies start up and reload handling of "remap.config". It also changes startup failure to Emergency from Fatal to avoid a restart loop. I've also tested this version and verified it behaves as requested.

However, due to some pushback, I plan to add a configuration variable to enable requiring a minimum number of rules in the "remap.config" to be valid. The default, at least for 9.2, will be 1. This means an empty or missing file won't replace a valid file. That can be changed by adjusting the configuration value. LinkedIn and Apple. I pondered a configuration for requiring a file but decided that was too indirect - a file with just comments is really an empty file. Valid rules are the correct metric for determining the "emptiness" of a file.

bryancall · 2022-04-25T23:23:25Z

@bneradt is going to review this

doc/admin-guide/files/records.config.en.rst

bneradt

Looks good. Just a few minor suggestions.

bneradt · 2022-04-27T14:04:25Z

doc/admin-guide/files/records.config.en.rst

   Set this variable to ``1`` if you want to retain the client host
   header in a request during remapping.

+.. ts:cv:: CONFIG proxy.config.url_remap.min_rules_required INT 1


I believe we want to use 0 here since that's the default.

doc/admin-guide/files/records.config.en.rst

tests/gold_tests/remap/remap_reload.test.py

zwoop

Thanks Alan, appreciate you humoring the senile old geyser.

randall

Thanks ! This is great!

Revert "Short circuit remap reload when a valid remap file is not specified (#7782)" This reverts commit d7847f2. This is being reverted from 9.2.x because changing remap.config loading behavior was deemed too big a change to make within the 9.x releases. See the discussion in #8802. These remap.config changes will be pursued in the 10.x train. Co-authored-by: bneradt <bneradt@yahooinc.com>

SolidWallOfCode added the Remap label Apr 20, 2022

SolidWallOfCode added this to the 10.0.0 milestone Apr 20, 2022

SolidWallOfCode requested review from sudheerv and zwoop April 20, 2022 16:55

SolidWallOfCode requested a review from bryancall as a code owner April 20, 2022 16:55

SolidWallOfCode self-assigned this Apr 20, 2022

bneradt previously approved these changes Apr 20, 2022

View reviewed changes

randall reviewed Apr 20, 2022

View reviewed changes

randall requested changes Apr 20, 2022

View reviewed changes

bneradt mentioned this pull request Apr 20, 2022

9.2.x: Revert remap.config loading changes #8803

Merged

SolidWallOfCode dismissed bneradt’s stale review via 4962232 April 25, 2022 15:34

bneradt requested changes Apr 26, 2022

View reviewed changes

doc/admin-guide/files/records.config.en.rst Outdated Show resolved Hide resolved

bneradt requested changes Apr 27, 2022

View reviewed changes

zwoop previously approved these changes Apr 27, 2022

View reviewed changes

randall previously approved these changes Apr 28, 2022

View reviewed changes

Add configuration to require minimum number of rules in remap.config.

c16dbb5

SolidWallOfCode dismissed stale reviews from randall and zwoop via c16dbb5 April 29, 2022 23:13

SolidWallOfCode force-pushed the i-8785 branch from dae5087 to c16dbb5 Compare April 29, 2022 23:13

bneradt approved these changes May 2, 2022

View reviewed changes

SolidWallOfCode merged commit 692223c into apache:master May 6, 2022

Change remap loading to be fatal during startup. #8802

Change remap loading to be fatal during startup. #8802

Uh oh!

Conversation

SolidWallOfCode commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bneradt commented Apr 20, 2022

Uh oh!

randall Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode Apr 20, 2022

Choose a reason for hiding this comment

Uh oh!

mlibbey Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

SolidWallOfCode Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

ywkaras Apr 21, 2022

Choose a reason for hiding this comment

Uh oh!

randall left a comment

Choose a reason for hiding this comment

Uh oh!

ezelkow1 commented Apr 20, 2022

Uh oh!

SolidWallOfCode commented Apr 20, 2022

Uh oh!

zwoop commented Apr 20, 2022

Uh oh!

bneradt commented Apr 20, 2022

Uh oh!

zwoop commented Apr 20, 2022

Uh oh!

SolidWallOfCode commented Apr 21, 2022

Uh oh!

bryancall commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SolidWallOfCode commented Apr 22, 2022

Uh oh!

bryancall commented Apr 25, 2022

Uh oh!

Uh oh!

bneradt left a comment

Choose a reason for hiding this comment

Uh oh!

bneradt Apr 27, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zwoop left a comment

Choose a reason for hiding this comment

Uh oh!

randall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

SolidWallOfCode commented Apr 20, 2022 •

edited

Loading

bryancall commented Apr 21, 2022 •

edited

Loading