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.

Sign up for GitHub

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

Show configuration file in reports #1790

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Show configuration file in reports #1790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/cf_gen.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -668,7 +668,8 @@ gen_dump(const EntryList &head, std::ostream &fout)
"static void" << std::endl <<
"dump_config(StoreEntry *entry)" << std::endl <<
"{" << std::endl <<
" debugs(5, 4, MYNAME);" << std::endl;
" debugs(5, 4, MYNAME);" << std::endl <<
" storeAppendPrintf(entry, \"# File: %s\\n\", ConfigFile);" << std::endl;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not violently against adding comments to the processed configuration file dump even though I am worried that this may open a Pandora box of arguing what comments are appropriate. There are a lot of things we could report here!

Please adjust this comment text to clarify that this is the top-level configuration filename rather than the only configuration file.

Reporting all configuration files used is better than reporting just the top-level one. I do not insist on this enhancement in this PR, but please keep it (i.e. future wording/context changes) in mind when adjusting this text in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggestions welcome, I am not set on specifics here beyond that it does have to be a #comment. IMO the fact of being top-level should be obvious to the admin reading it - usually by the fact it is /some/path/to/squid.conf.

The other files loaded would be nice, but are not yet recorded anywhere to be included in this output. I am leaving them until the ConfigParser refactoring is done.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the fact of being top-level should be obvious to the admin reading it

I agree, but that is not the concern detailed in this change request. That concern is that the admin may think that the named file is "the only configuration file".

Suggestions welcome ... [Reporting] The other files loaded would be nice, but ... I am leaving them until the ConfigParser refactoring is done.

If this PR does not improve code to (at least) detect use/presence of other configuration files, then
I suggest

  • "# Top-level configuration file: ..."
  • "# Based on top-level configuration file: ..."
  • "# Derived from top-level configuration file: ..."

... or something like that. Future PRs may add a list of various secondary configuration files used.


for (const auto &e : head) {

Expand Down
9 changes: 5 additions & 4 deletions src/main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,7 +650,9 @@ mainHandleCommandLineOption(const int optId, const char *optValue)
printf("For legal restrictions on distribution see https://www.openssl.org/source/license.html\n\n");
#endif
#endif
printf( "configure options: %s\n", SQUID_CONFIGURE_OPTIONS);

printf("Running Configuration in: %s\n", ConfigFile);
Copy link
Contributor

Choose a reason for hiding this comment

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

AFAICT from sample output reports (thank you for providing those!), this line will appear in "squid -v" output. I do not think it should appear there (because it will often not match the reality of a running Squid instance), especially when "squid -v" command line does not include "-f".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line appearing in -v output is the point. Such that admin can verify that their expected config file and/or command-line options used alongside -k parse are correct.

Consider an admin running a locally built Squid installed over a system package. Running Squid would default to /usr/local/squid/etc/squid.conf and squid -k parse report no errors. But admin editing the system /etc/squid/squid.conf. In such a case starting Squid manually would be fine loading the /usr/local config and not doing what the admin actually wanted. But service squid start could crash loading a broken /etc/squid/squid.conf.

Copy link
Contributor

@rousskov rousskov Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Alex: AFAICT ... this line will appear in "squid -v" output. I do not think it should appear there (because it will often not match the reality of a running Squid instance), especially when "squid -v" command line does not include "-f".

Amos: This line appearing in -v output is the point. Such that admin can verify that their expected config file and/or command-line options used alongside -k parse are correct.

The above response does not seem to be related to (and, hence, cannot address) my concern.

Consider an admin running a locally built Squid installed over a system package. Running Squid would default to /usr/local/squid/etc/squid.conf and squid -k parse report no errors. But admin editing the system /etc/squid/squid.conf. In such a case starting Squid manually would be fine loading the /usr/local config and not doing what the admin actually wanted. But service squid start could crash loading a broken /etc/squid/squid.conf.

$ squid -k parse
2024/04/30 18:14:27| Processing Configuration File: <filename> (depth 0)

$ sudo squid
$ head -n 1 cache.log
2024/04/30 18:14:50| Processing Configuration File: <filename> (depth 0)

I assume the "Consider an admin..." example above is implying that this specific admin will run squid -v and realize that squid -k parse and squid are using a different configuration file than the file the admin is editing; this particular admin does not notice a different configuration filename in squid -k parse output and in cache.log produced by squid (see the samples above for both), but does notice it in squid -v output.

To address my concern while still supporting the above use case, we can adjust squid -v (without -f) output to report "Default configuration filename:" instead of the proposed "Running Configuration in:".

printf("Build configure options: %s\n", SQUID_CONFIGURE_OPTIONS);

#if USE_WIN32_SERVICE

Expand Down Expand Up @@ -1484,6 +1486,8 @@ RegisterModules()
int
SquidMain(int argc, char **argv)
{
ConfigFile = xstrdup(DEFAULT_CONFIG_FILE);

Comment on lines +1489 to +1490
Copy link
Contributor

Choose a reason for hiding this comment

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

PR description: Ensure the ConfigFile macro is always set, even if it only points at the default squid.conf location.

I see no good reason for this code change and it feels like a step backwards: Why set a global variable to an incorrect (in some cases) value? Until command line arguments are parsed, we do not know the configuration file name. It may or may not be the default name.

Suggested change
ConfigFile = xstrdup(DEFAULT_CONFIG_FILE);

The suggestion above restores official code but may require other PR adjustments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change is what produces squid -v output:

Squid Cache: Version 7.0.0-VCS
Service Name: squid
Running Configuration in: /usr/local/squid/etc/squid.conf
Build configure options: ....

instead of:

Squid Cache: Version 7.0.0-VCS
Service Name: squid
Running Configuration in: (null)
Build configure options: ....

Copy link
Contributor

@rousskov rousskov May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This change is what produces squid -v output: ...

Yes, I am aware. This fact does not address the concern detailed in this change request. Clearly, the same output1 can be produced without this problematic code change (e.g., by adjusting PR-added code that produces that output).

Footnotes

  1. The problems with that output are discussed elsewhere. This change request is not about those problems. ↩

// We must register all modules before the first RunRegisteredHere() call.
// We do it ASAP/here so that we do not need to move this code when we add
// earlier hooks to the RegisteredRunner API.
Expand Down Expand Up @@ -1570,9 +1574,6 @@ SquidMain(int argc, char **argv)
{
int parse_err;

if (!ConfigFile)
ConfigFile = xstrdup(DEFAULT_CONFIG_FILE);

assert(!configured_once);

Mem::Init();
Expand Down
Loading