Add disable providers by default option #4166
Conversation
|
This pull request does not have a backport label. Could you fix it @eyalkraft? 🙏
NOTE: |
|
Pinging @elastic/elastic-agent (Team:Elastic-Agent) |
| @@ -65,7 +65,7 @@ func New(log *logger.Logger, c *config.Config, managed bool) (Controller, error) | |||
| contextProviders := map[string]*contextProviderState{} | |||
| for name, builder := range Providers.contextProviders { | |||
| pCfg, ok := providersCfg.Providers[name] | |||
| if ok && !pCfg.Enabled() { | |||
| if (ok && !pCfg.Enabled()) || (!ok && providersCfg.ProvidersDefaultDisable) { | |||
There was a problem hiding this comment.
In case a provider is configured and the ProvidersDefaultDisable is set, should we continue or not?
There was a problem hiding this comment.
Yes, providers can be explicitly configured. The flag just changes the default behavior
There was a problem hiding this comment.
So in that test-case, what is the expected output?
{
cfg: map[string]interface{}{
"agent.providers.initial_default": "true",
"providers": map[string]interface{}{
"env": map[string]interface{}{
"enabled": "false",
},
},
},
},
elastic-agent.yml
Outdated
| # All registered providers are enabled by default. | ||
|
|
||
| # Disable all providers by default and only enable explicitly configured providers. | ||
| # providers_default_disable: true |
There was a problem hiding this comment.
I don't really like this as top-level field. The agent has a couple sections in its configuration outputs, inputs, management, agent. This as a top-level option is kinda floating out in the wrong area. Being that you are changing the default option of the Elastic Agent for all provides I would think this should be under agent.*.
What about: agent.providers.initial_default: false? The initial_default can be a *bool to allow the default to be true when unset, true (explicity set), or false.
There was a problem hiding this comment.
Since the config struct is only used in a single place I think it could be cleaner to keep it as a boolean, setting its value to true before calling Unpack.
We wouldn't know if it was explicitly configured to true or just left out but I don't think it matters.
wdyt?
There was a problem hiding this comment.
That does work but I always worry about issues with defaults actually being initialized and not accidentally cleared to false before the unpack. I prefer to be safer with the *bool.
|
Looks like one integration test fails during setup. elastic-agent/testing/integration/container_cmd_test.go Lines 59 to 63 in 6f7a116 Could the test setup be flaky? Should I re trigger CI somehow (empty commit?) |
|
|
|
Manually tested latest changes - Works as expected 👍 |
|
buildkite test it |
|
95.0% Coverage on New Code
0.0% Duplication on New Code
What does this PR do?
Two main things (let me know if you prefer two separate PRs)
providers_default_disableconfiguration flag, to allow changing the default providers behavior.Why is it important?
Checklist
./changelog/fragmentsusing the changelog toolAuthor's Checklist
Post-merge:
How to test this PR locally
Related issues
Use cases
Screenshots
Tested on my serverless dev cluster.
Old agent image, and New image with old configmap: docker provider tries starting.
New image with new configmap: no provider is starting
New image with docker provider explicitly enabled: provider is starting
Logs
Questions to ask yourself