Integration test coverage for path filtering

[hackebrot]

Description

This pull request is a follow-up for #311 and uses that branch as a base. I will rebase this once #311 is merged.

Please see the test scenario descriptions in integration-tests/volumes/client/scenarios.yml and let me know if you have any questions about the scenarios.

Note that I've observed unexpected behavior when working this, which I described in #311 (comment). 🚧

Testing

Verify that the following 2 new integration tests run and pass on CI:

• advertiser_url_path_filter_prefix
• advertiser_url_path_filter_exact

Issue(s)

See #311

[hackebrot]

I added a new integration test that uses a adm_settings.json file with a prefix path filter with a value without a trailing / which I expect to cause Contile to panic. My understanding of how panics are handled in Contile or its underlying web framework may be inaccurate.

Do you expect tiles from the advertiser hosts with invalid path filters to be ignored? I'm seeing a 204 No Content.

[hackebrot]

It seems that the panic! in Contile when loading the settings JSON does not cause the entire actix-web server to terminate, but only the worker that encounters that panic. So invalid settings will not cause the server to terminate and clients can still send requests to the server.

[jrconlin]

Ah! Good catch!

The panic would get recorded by Sentry, and the code enforces a terminating slash.
@ncloudioj, I'm tempted to have settings record it as either a new ConfigurationError, or record it as a warning and have the settings module correct the path when it encounters it, instead of panicking.

[ncloudioj]

Ah! Good catch!

The panic would get recorded by Sentry, and the code enforces a terminating slash.

That's quite surprising to me. I'd guess all the Actix servers (i.e. workers) would load the same settings, shouldn't they all panic? How did it serve the request and return 200 🤔

@ncloudioj, I'm tempted to have settings record it as either a new ConfigurationError, or record it as a warning and have the settings module correct the path when it encounters it, instead of panicking.

Yep, I think we can do that although it'll be preferred to fail soon and loud.

[hackebrot]

That's quite surprising to me. I'd guess all the Actix servers (i.e. workers) would load the same settings, shouldn't they all panic? How did it serve the request and return 200 🤔

Based on the results in the integration tests, I am wondering if only workers that happen to process tiles from partner API for an advertiser for which there is an invalid path filter in the settings panic and all other workers are not impacted. Could that be the case? Does settings loading happen lazily?

[pjenvey]

I added a new integration test that uses a adm_settings.json file with a prefix path filter with a value without a trailing / which I expect to cause Contile to panic. My understanding of how panics are handled in Contile or its underlying web framework may be inaccurate.

I'm not seeing any panic running the more-path-filtering branch w/ CONTILE_ADM_SETTINGS=adm_settings_panic.json -- seems like it's not triggering it.

Indeed rust's panic will only affect the current thread (potentially only one actix-web worker while the others continue). However contile's initialization of settings occur within server::Server::with_settings outside of actix-web's workers. An error there should panic the main (and only thread at that time) thread, aborting the entire program.

[ncloudioj]

FWIW, #314 has removed this panic, instead, it simply returns an error when detecting malformed path filters (e.g. prefix values without the trailing "/" or exact values without the leading "/").

[pjenvey]

FWIW, #314 has removed this panic, instead, it simply returns an error when detecting malformed path filters (e.g. prefix values without the trailing "/" or exact values without the leading "/").

It does still "panic" though -- for the sake of this PR the behavior is the same (it was changed to return Errors but such initialization errors are propagated out to contile's main function, ultimately causing it to abort w/ a non zero return code before the server is launched).

[hackebrot]

I rebased this branch to reorder, fixup and reword some of the commits. I also renamed what we originally called panic integration tests to init_error to make it clear that we're testing for an unrecoverable error during Contile's initialization as @pjenvey correctly pointed out. In this case the init error is because of invalid contents in the settings file.

[ncloudioj]

LGTM, thanks!

[jrconlin]

Not needed now, but you can use timeout --kill-after=DURATION --signal=SIGNAL ... to automatically kill a command with SIGNAL after DURATION seconds.

[hackebrot]

Okay. Here with --abort-on-container-exit docker compose will shut down all services as soon as the contile service exits or if that doesn't happen after the client exits because the test ran.

[ashrivastava-qa]

LGTM 👍