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

Test Deployed Configurations #524

Open
brunobat opened this issue Feb 27, 2020 · 15 comments
Open

Test Deployed Configurations #524

brunobat opened this issue Feb 27, 2020 · 15 comments
Labels
use case 💡 An issue which illustrates a desired use case for the specification

Comments

@brunobat
Copy link

brunobat commented Feb 27, 2020

Once the service is deployed on an environment, most properties are overridden.
These override values might be wrong and even empty, causing runtime errors that are hard to detect upfront.
We had quite a few mistakes on our environment configs because property values are manually set and there is not a standard way to validate them systematically.

The use case:
As a devops engineer, I'd like to have a way to test my configurations at deployment time and fail the deployment if needed.

This will require validation per configuration or set of configurations. Without diving into a possible solution, integrating this with a Readiness health check would be neat.

@radcortez
Copy link
Contributor

Hum.... basically what you want is a report of each config property effective value and the origin ConfigSource for debug purposes I guess?

Deployment already fails if you miss a configuration, but how should we determine that the deployment should fail if you put a wrong value in it?

@brunobat
Copy link
Author

brunobat commented Feb 27, 2020

There are multiple checks we could do:

  1. Report properties without override and add a config to fail if that happens;
  2. Fail if some property is set to empty;
  3. Add an actual readiness healthcheck that would be a runtime test for a config or set of configs;
  4. Some kind of bean validation approach to values;
  5. Something else.

The point is that this is a real problem asking for a solution :)

@radcortez
Copy link
Contributor

radcortez commented Feb 27, 2020

There are multiple checks we could do:

  1. Report properties without override and add a config to fail if that happens;

You can actually do this now, by not using default values.

  1. Fail if some property is set to empty;

This can also be done, by adding your own converter.

  1. Add an actual readiness healthcheck that would be a runtime test for a config or set of configs;

I guess a config endpoint may be exposed, but you could implement your own health check and inject the configs you want to check.

  1. Something else.

The point is that this is a real problem asking for a solution :)

Not saying that we shouldn't look into this and figure out if there is something that we can facilitate.

@kenfinnigan
Copy link
Contributor

If this were to be proposed, I would recommend an implementation experiment with how it can be implemented before defining an API in the spec

@brunobat
Copy link
Author

I also prefer that approach.

@radcortez
Copy link
Contributor

There are multiple ideas here. I think we need to handle them separately and figure out what makes sense or not. I'm happy to try some of these in SmallRye.

Also, there are some other issues that might be related with this is someway or another:

Report properties without override and add a config to fail if that happens;

#312 - Enable users to determine 'winning' source for a value - where is it coming from?
Determine the winning source can also tell you if the value was overridden or not. And in here I assume that it is related with the default value?

Fail is not covered here. How will you determine that a fail has to occur? It seems strange, because you already have that behaviour by not setting a default value and not injecting an Optional. On App start the deployment will fail because the config is not present.

Fail if some property is set to empty;

Tricky again, because you may have cases where it is acceptable to have empty values, I guess we are referring to an empty String or an empty Collection. And of course, you may want to make sure that these configuration actually hold something. Even if we validate them, it doesn't mean that the configuration is semantically correct.

I guess this should be evaluated with #365 - Null must be injectable

Add an actual readiness healthcheck that would be a runtime test for a config or set of configs;

Thinking a little bit better about this, I believe that it shouldn't be Config responsibility to do this, but could be a feature of Health Check. Config should stay free of dependencies to other MP specs.

Some kind of bean validation approach to values;

I've proposed a few times already to include Bval in MP. It will definitely be interesting if we could leverage Bval. On the other hand, I have some concerns about adding Bval as a dependency to Config. It may complicate the standalone behaviour of Config.

@dmlloyd
Copy link
Contributor

dmlloyd commented Feb 28, 2020

Yeah I don't think bean validation will work for Config. It could work on just the CDI layer (since that's where annotations can be applied), but the core API doesn't use CDI, so then we'd once again be in a situation where the CDI functionality is substantially different from the programmatic API.

Within Quarkus we implement validation using wrapping converters which perform a validation, either on the input string (before delegating to the wrapped converter) or on the output object (after delegating to the wrapped converter). This system works pretty well and allows validation to be done at the core API level. However it also relies on other features such as being able to access converters from the config API and being able to get values specifying a converter rather than a Class, two API extensions that SmallRye config provides and which has been discussed inconclusively in other issues.

@radcortez
Copy link
Contributor

You could do it if on the consumer side you reimplement all built in converters to apply the required validations. It would be a lot of work for the consumer.

The clean way imo is to support the ConfigAccessor and provide the necessary methods to perform the required actions.

@dmlloyd
Copy link
Contributor

dmlloyd commented Feb 28, 2020

You could do it if on the consumer side you reimplement all built in converters to apply the required validations. It would be a lot of work for the consumer.

For the API consumer who wants to validate a given property with specific validations, they have to somehow specify the validations they want to perform, and they then have to execute the property get in the context of those validations. That work is unavoidable and will look similar no matter what approach is taken.

The clean way imo is to support the ConfigAccessor and provide the necessary methods to perform the required actions.

I'm not sure that's substantially cleaner TBH; the user still has to specify all the validations to perform. If you have methods that define the possible validations, it's not much of an improvement IMO, especially because then there will be methods for known/predefined validations and some other (object-based) method(s) for unknown/user-defined validations. Then for the latter case you have to consider: does the validation apply pre-conversion to the input string, or post-conversion to the converted object? Now there are a lot of ways to do the same thing.

Accessors are likely to be short-lived, based on the past and proposed implementations, so are we really getting benefit from this approach?

@radcortez
Copy link
Contributor

When I said cleaner, it was in the sense to provide an API that could support these kind of cases. Yes, the user would still have to specify the validations, but they could do it in per config / property basis, instead of the converter which will get applied to all types. You may have different validations for same types depending on your semantics.

Maybe we can think about adding lifecycle methods (beforeLookup, afterLoad). These might help people to customised their need like this (validation) and it may also help us to implement other stuff (like property replacement).

@dmlloyd
Copy link
Contributor

dmlloyd commented Feb 28, 2020

When I said cleaner, it was in the sense to provide an API that could support these kind of cases. Yes, the user would still have to specify the validations, but they could do it in per config / property basis, instead of the converter which will get applied to all types. You may have different validations for same types depending on your semantics.

Ah I see the confusion. I'm assuming that there's a means to specify the converter for a getValue operation, so it could be something along the lines of what we do for validations in SmallRye.

    Blah value = config.getValue("prop.name", new MinimumConverter(config.getConverter(Blah.class), Blah.MEDIUM_SIZED_BLAH));

or something like that.

Maybe we can think about adding lifecycle methods (beforeLookup, afterLoad). These might help people to customised their need like this (validation) and it may also help us to implement other stuff (like property replacement).

This idea seems to walk in the direction of ahead-of-time declaration of configuration behaviors rather than at API use time. Is this a direction we want to push towards?

@dmlloyd
Copy link
Contributor

dmlloyd commented Feb 28, 2020

Another issue that moves in this direction is #475.

@radcortez
Copy link
Contributor

Not sure, just trying to think is some ideas so we can discuss what is the best approach :)

The issue here is the consistency between Standalone vs CDI Injection usage. We would also need to support setting Converters in the Config injection point. And then I start to question myself if we really want / need to open all sorts of annotation configuration.

I like the current annotation config. It's concise and simple. Just don't want to explode it with all sorts of other configurations :)

@Emily-Jiang
Copy link
Member

As for the value validation, I think the implementation does the value validation check can post a better message. For MicroProfile Config, the main focus is to get the config value. I think validating the config value is left for microservices. Having said that, maybe we can build on #312 to list the other config sources as well.

@dmlloyd
Copy link
Contributor

dmlloyd commented Mar 2, 2020

As for the value validation, I think the implementation does the value validation check can post a better message. For MicroProfile Config, the main focus is to get the config value. I think validating the config value is left for microservices. Having said that, maybe we can build on #312 to list the other config sources as well.

I'd like to point out that "required/optional" is a kind of validation that MP Config already supports today. Also I'd like to remind you that we should not dismiss use cases categorically. Unless we can point to a better specific solution for a config-related problem (validation in this case), we shouldn't say "it's better solved in xxxxx". It's clearly a valid use case, and SmallRye shows us that there is at least one means to accomplish it that is cleanly aligned with the existing design of MP Config. I'd also contend that existing validation technologies cannot help us in terms of the programmatic API. So, pointing users to other places to solve this problem is not really appropriate.

But, the question still stands: do we want to be moving more towards ahead-of-time declaration of config behaviors? In Quarkus we've moved in this direction and have seen many benefits. I think we'll have to incorporate #475 if we want to support #405, both of which point towards ahead-of-time declaration.

This was referenced Mar 2, 2020
@dmlloyd dmlloyd added the use case 💡 An issue which illustrates a desired use case for the specification label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
use case 💡 An issue which illustrates a desired use case for the specification
Projects
None yet
Development

No branches or pull requests

5 participants