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

Jump to bottom

Validating configuration #325

Open
DonCallisto opened this issue Oct 3, 2019 · 9 comments · May be fixed by #329
Open

Validating configuration #325

DonCallisto opened this issue Oct 3, 2019 · 9 comments · May be fixed by #329

Comments

@DonCallisto
Copy link

We should try to enforce user's correct configuration as commented here.

Why? Because I've found a tricky misconfiguration that leaded to wrong parsing of my logs and in loosing them.

That was the (wrong) configuration stripped off the not congruous parts in order to avoid noise, please don't focus on meaning but keep attention on technical matters

monolog:
    channels: ['foo_channel']
    handlers:
        foo_channel_handler:
            channels: [foo_channel]
            type: stream # <--- THE "SILENT ERROR"
            level: error
            handler: grouped
        grouped:
            type:    group
            members: [streamed]
        streamed:
            type:  stream
            path:  "%kernel.logs_dir%/%kernel.environment%.log"
            level: debug
            formatter: monolog.formatter.session_request

monolog.formatter.session_request:
  class: Monolog\Formatter\LineFormatter
  arguments:
    - "[%%datetime%%] [%%extra.token%%] %%channel%%.%%level_name%%: %%message%% \n"

This configuration is clearly wrong (type: stream tries to use filter wrapper handler for a configuration error) but Symfony didn't raise an exception.
Why this could be a problem?
In this particular case I had two default values explicitated:

path --> https://github.com/symfony/monolog-bundle/blob/master/DependencyInjection/Configuration.php#L377
formatter (I mean, the Class) --> Monolog\Formatter\LineFormatter (AFAIK is the default one)

So in my tests the log was created at the right location using the desired formatter, except that I dind't noticed that [%%extra.token%%] was never included. This is due to the fact that, correctly, stream type never "pass" control to grouped and so to streamed.
That was nasty to debug and lost several hours.

We have two chances, both seems to introduce a BC break:

  1. Handlers type as configuration keys

Specifying the type of the handler as key and checking that at least one of the the types is specified under handler's name we can better state what to expect and what should not be nested there. One of the things that worry me the most with this approach is we should carry every acceptable value under each type key in the configuraton three.
Moreover this would introduce a new way of declaring (and so checking) custom handlers, something like "force" somehow the user to give a definition for its type.

  1. Performing semantic checks in MonologExtension

This seems the best way from my standpoint. We can easily check here that all expected attributes (and no more that those) are carried by the configuration, raising and error otherwise.

I think that both methods introduce a BC break as in the first case everyone needs to change configuration files whereas in the latter case we were accepting wrong configuration that could possibly not working anymore.

If this makes sense to you I'm willing to work in order to reach what I've shown above.
I'm also open to other ideas.

Let me know what do you think.
@Seldaek
Copy link
Member

Seldaek commented Oct 6, 2019

I think the second option is better, as it's BC from a config standpoint. It might break for invalid configs but that's probably a good thing..

Could you post the correct configuration though? Because I am still not sure what the error was in your config. I see a type:stream which makes no sense to me as you have a handler: grouped as well which can not be present on stream handlers as they don't accept nested handlers.

@DonCallisto
Copy link
Author

Because I am still not sure what the error was in your config. I see a type:stream which makes no sense to me as you have a handler: grouped as well which can not be present on stream handlers as they don't accept nested handlers.

Yeah, that's true, it was a bad configuration (no sense one indeed) so the right one is

foo_channel_handler:
  channels: [foo_channel]
  type: filter
  min_level: error
  handler: grouped

If you agree with the second option and no other ideas arise, I'll work on this on the next days.

@DonCallisto
Copy link
Author

I'm working on this issue, trying to validate the configuration directly in Configuration file.
One thing that, at the moment, is preventing me to complete the work is the default values of some nodes as, in validation phase, I get them from the configuration but I need to throw an exception only if user explictly defined them (in case that those values are simply "discharged" by the handler type).
Any ideas?

@DonCallisto
Copy link
Author

DonCallisto commented Oct 19, 2019 •
edited
Loading

Maybe there’s the chance to assign a default value only under certain circumstances (ie based on handler type?). I need to check.

@DonCallisto
Copy link
Author

This is a WIP but I've found, maybe, how to accomplish the combination of not having a default value (when that value is not intended to be used from handler type), letting the user use that attribute and throwing and exception if the attribute should not be used in the handler type.
Check it out https://github.com/symfony/monolog-bundle/compare/master...DonCallisto:config_validation?expand=1#diff-850942b3ba24ab03a40aaa81b6152852

WDYT?

@DonCallisto
Copy link
Author

Umh, diggin' my toes deeper I've found that beforeNormalization probably is not the right place to add the default values (in legit cases indeed the default value is not added).
I'll try to look deeper in how the Configuration works, in the meantime if someone has any ideas is welcome to share.
Thanks.

@DonCallisto
Copy link
Author

Ok I finally found how to proceed. I should use beforeNormalization on the whole ArrayNode in order to have the values defined by the user.
I'll work on this in the next days and I should be able to complete the task within the next week, hopefully.

@DonCallisto
Copy link
Author

I'm almost there.
Just wanted to share what I've done so far https://github.com/symfony/monolog-bundle/compare/master...DonCallisto:config_validation?expand=1

I'll finish in the next days.

WDYT? Is it good?

@DonCallisto DonCallisto linked a pull request Oct 31, 2019 that will close this issue
Open
@uncaught
Copy link

uncaught commented Oct 30, 2022 •
edited
Loading

You shouldn't validate config values at compile time. This breaks working with environment variables at runtime. Nobody should have to rebuild the container everytime the environment changes. That's often not even possible in a dockerized world.

This bundle already blocks several parameters from being used as real runtime environment variables, please don't make it more.

Even changing a handler type could make sense as environment variable (production container on AWS using a different handler than a production container on a simple webserver, etc.). But I understand that would lead to different built services with the current architecture.

If you need validation, you could make it a command, which can be run anytime you want and then evaluate the values at that time correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[WIP] Configuration Validation DonCallisto/monolog-bundle
3 participants
@Seldaek @DonCallisto @uncaught