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

Reject config with unknown directives before committing to it #1897

Conversation

rousskov
Copy link
Contributor

Ideally, we want to reject configurations with unknown directives before
applying any configuration changes that correspond to known directives,
but current apply-as-you-parse architecture makes that impractical.
Pending smooth reconfiguration refactoring will make that possible, but
we can make a step towards that ideal future now.

Rejecting bad configurations before calling configDoConfigure() reduces
the set of configuration errors that Squid can detect in one execution
(because configDoConfigure() error-checking code is not reached), but
that small reduction is a lesser evil compared to running
configDoConfigure() with a clearly broken config, especially when we are
going to kill Squid anyway. While many legacy parse_foo() functions do
apply significant changes before configDoConfigure(), we cannot easily
prevent that (for now). We can easily prevent configDoConfigure().

Ideally, we want to reject configurations with unknown directives before
applying any configuration changes that correspond to known directives,
but current apply-as-you-parse architecture makes that impractical.
Pending smooth reconfiguration refactoring will make that possible, but
we can make a step towards that ideal future now.

Rejecting bad configurations before calling configDoConfigure() reduces
the set of configuration errors that Squid can detect in one execution
(because configDoConfigure() error-checking code is not reached), but
that small reduction is a lesser evil compared to running
configDoConfigure() with a clearly broken config, especially when we are
going to kill Squid anyway. While many legacy parse_foo() functions do
apply significant changes before configDoConfigure(), we cannot easily
prevent that (for now). We can easily prevent configDoConfigure().
Copy link
Contributor Author

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

This PR is a yet another tiny step towards smooth reconfiguration support, addressing an existing TODO. It is probably the last change I can carve out before #1840 is finally merged. Isolating and maintaining these pending changes wastes a lot of my time. Please move #1840 forward!

This review annotates this PR but does not request any changes.

@@ -625,22 +625,16 @@ Configuration::Parse()

defaults_postscriptum();

if (unrecognizedDirectives)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It may be tempting to throw even earlier, when unrecognizedDirectives constant is set above defaults_if_none() and defaults_postscriptum() calls. However, we should not do that. It is not obvious in the current code, but those two functions can also detect unrecognized directives and similar problems. Pending smooth reconfiguration changes will make this code look natural, removing the current "Why not throw earlier?" question.

@rousskov rousskov added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels S-could-use-an-approval An approval may speed this PR merger (but is not required) labels Aug 28, 2024
Copy link
Contributor

@kinkie kinkie left a comment

Choose a reason for hiding this comment

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

LGTM

squid-anubis pushed a commit that referenced this pull request Aug 31, 2024
Ideally, we want to reject configurations with unknown directives before
applying any configuration changes that correspond to known directives,
but current apply-as-you-parse architecture makes that impractical.
Pending smooth reconfiguration refactoring will make that possible, but
we can make a step towards that ideal future now.

Rejecting bad configurations before calling configDoConfigure() reduces
the set of configuration errors that Squid can detect in one execution
(because configDoConfigure() error-checking code is not reached), but
that small reduction is a lesser evil compared to running
configDoConfigure() with a clearly broken config, especially when we are
going to kill Squid anyway. While many legacy parse_foo() functions do
apply significant changes before configDoConfigure(), we cannot easily
prevent that (for now). We can easily prevent configDoConfigure().
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label Aug 31, 2024
@rousskov rousskov removed the S-could-use-an-approval An approval may speed this PR merger (but is not required) label Aug 31, 2024
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels Sep 1, 2024
kinkie pushed a commit to kinkie/squid that referenced this pull request Oct 12, 2024
…cache#1897)

Ideally, we want to reject configurations with unknown directives before
applying any configuration changes that correspond to known directives,
but current apply-as-you-parse architecture makes that impractical.
Pending smooth reconfiguration refactoring will make that possible, but
we can make a step towards that ideal future now.

Rejecting bad configurations before calling configDoConfigure() reduces
the set of configuration errors that Squid can detect in one execution
(because configDoConfigure() error-checking code is not reached), but
that small reduction is a lesser evil compared to running
configDoConfigure() with a clearly broken config, especially when we are
going to kill Squid anyway. While many legacy parse_foo() functions do
apply significant changes before configDoConfigure(), we cannot easily
prevent that (for now). We can easily prevent configDoConfigure().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants