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

Core: Add dump-config option to print read configuration on startup #9131

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

Conversation

drbugfinder-work
Copy link
Contributor

This PR will add "-a" "--dump-config" argument option which will use the flb_cf_dump method to print the internal representation of the loaded configuration during startup. This can be very helpful for debugging purposes.


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • [N/A] Example configuration file for the change
  • Debug log output from testing the change
  • [N/A] Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

  • [N/A] Run local packaging test showing all targets (including any new ones) build.
  • [N/A] Set ok-package-test label to test for all targets (requires maintainer to do).

Documentation

  • Documentation required for this feature

Backporting

  • [N/A] Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@edsiper
Copy link
Member

edsiper commented Sep 6, 2024

from a security standpoint, should we make it optional at build time so production images don't ship this option ? (risk of exposing secrets ?), while -debug images can have it ?

cc: @patrick-stephens

@drbugfinder-work
Copy link
Contributor Author

drbugfinder-work commented Sep 6, 2024

from a security standpoint, should we make it optional at build time so production images don't ship this option ? (risk of exposing secrets ?), while -debug images can have it ?

cc: @patrick-stephens

That's a good idea. It is definitely an option, which is only used for development or debugging purposes. This should then be documented, that it is only available in dev/debug builds.

@edsiper The current state of this PR far from perfect. As I said in the community meeting, I want to start a discussion first.
I have two open points:

  • Is this also applicable for static configs (I've never used them before)?
  • I noticed that in case a config file contains errors (which would normally print a config file error), this now leads to a segfault. I'm guessing this is because the flb_cf is either not initialized or already destroyed at the point, when the dump is called. So there are more checks needed.

EDIT: Added checks

@patrick-stephens
Copy link
Contributor

If we can set it via environment variable then that would be the best way - not custom builds as debug and prod images use the same binaries.

@drbugfinder-work
Copy link
Contributor Author

drbugfinder-work commented Sep 6, 2024

If we can set it via environment variable then that would be the best way - not custom builds as debug and prod images use the same binaries.

@patrick-stephens That's what we actually do already. However, for configurations with hard-coded credentials there could be a credential-leak potentially. Especially if one forwards Fluent Bits own logs...

@patrick-stephens
Copy link
Contributor

patrick-stephens commented Sep 6, 2024

If we're doing separate builds for debug and production then the current image does not support that - plus I'd be dubious of it, the point of the debug image is to debug the production binary/config so if it differs then is that delta going to cause problems?

We could add this as an option enabled by default using an env var. I'm not sure of the issue with requiring it to be compiled out but potentially we could disable its usage if the env var is present/missing at runtime. It's a separate parameter currently so if you can enable it then you can likely also dump those secrets too (if you can change config then just get stdout to print the value).

An alternative is to ensure it does not print out the values of env vars or similar - people can dump those if they need separately anyway.

I'm not really sure and am not a security expert though. It just feels like a load of complexity for security-by-obscurity reasons so I'd just say keep it simple.

@drbugfinder-work
Copy link
Contributor Author

If we're doing separate builds for debug and production then the current image does not support that - plus I'd be dubious of it, the point of the debug image is to debug the production binary/config so if it differs then is that delta going to cause problems?

We could add this as an option enabled by default using an env var. I'm not sure of the issue with requiring it to be compiled out but potentially we could disable its usage if the env var is present/missing at runtime. It's a separate parameter currently so if you can enable it then you can likely also dump those secrets too (if you can change config then just get stdout to print the value).

An alternative is to ensure it does not print out the values of env vars or similar - people can dump those if they need separately anyway.

I'm not really sure and am not a security expert though. It just feels like a load of complexity for security-by-obscurity reasons so I'd just say keep it simple.

@patrick-stephens The environment variables are not printed with as values, they are just printed unresolved as "${MYENV}", so this shouldn't be an issue. The only case where credentials would be visible is when they are hard-coded in the configuration. However, when a user intentionally makes use of this dump config option, it should be obvious, that it will do exactly what it says.
@edsiper @patrick-stephens: Please also see fluentd as a reference, where they did it the other way round https://docs.fluentd.org/deployment/command-line-option There is the parameter --suppress-config-dump which will prevent fluentd from printing the configuration on startup. So they print the configuration as default during startup.

@patrick-stephens
Copy link
Contributor

If we're doing separate builds for debug and production then the current image does not support that - plus I'd be dubious of it, the point of the debug image is to debug the production binary/config so if it differs then is that delta going to cause problems?
We could add this as an option enabled by default using an env var. I'm not sure of the issue with requiring it to be compiled out but potentially we could disable its usage if the env var is present/missing at runtime. It's a separate parameter currently so if you can enable it then you can likely also dump those secrets too (if you can change config then just get stdout to print the value).
An alternative is to ensure it does not print out the values of env vars or similar - people can dump those if they need separately anyway.
I'm not really sure and am not a security expert though. It just feels like a load of complexity for security-by-obscurity reasons so I'd just say keep it simple.

@patrick-stephens The environment variables are not printed with as values, they are just printed unresolved as "${MYENV}", so this shouldn't be an issue. The only case where credentials would be visible is when they are hard-coded in the configuration. However, when a user intentionally makes use of this dump config option, it should be obvious, that it will do exactly what it says. @edsiper @patrick-stephens: Please also see fluentd as a reference, where they did it the other way round https://docs.fluentd.org/deployment/command-line-option There is the parameter --suppress-config-dump which will prevent fluentd from printing the configuration on startup. So they print the configuration as default during startup.

Wow :)

I definitely think it should be something you turn on and then if you've made that choice it seems fine with what we have. It should default to off and then no effect on existing deployments. We could potentially default enable it as well for the debug image via the args it uses by default.

This can be useful for debugging purposes when e.g. a lot of includes are used.

Signed-off-by: Richard Treu <richard.treu@sap.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants