-
Notifications
You must be signed in to change notification settings - Fork 148
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
Add disable providers by default option #4166
Changes from 6 commits
9777a0c
f4bfd9d
fda9be7
8847241
3bebb4f
350f4e1
ae43d01
e4f0ae6
1045e2a
6fcae25
91795ed
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
# Kind can be one of: | ||
# - breaking-change: a change to previously-documented behavior | ||
# - deprecation: functionality that is being removed in a later release | ||
# - bug-fix: fixes a problem in a previous version | ||
# - enhancement: extends functionality but does not break or fix existing behavior | ||
# - feature: new functionality | ||
# - known-issue: problems that we are aware of in a given version | ||
# - security: impacts on the security of a product or a user’s deployment. | ||
# - upgrade: important information for someone upgrading from a prior version | ||
# - other: does not fit into any of the other categories | ||
kind: enhancement | ||
|
||
# Change summary; a 80ish characters long description of the change. | ||
summary: Add providers_default_disable configuration flag to disable providers by default | ||
|
||
# Long description; in case the summary is not enough to describe the change | ||
# this field accommodate a description without length limits. | ||
# NOTE: This field will be rendered only for breaking-change and known-issue kinds at the moment. | ||
#description: | ||
|
||
# Affected component; usually one of "elastic-agent", "fleet-server", "filebeat", "metricbeat", "auditbeat", "all", etc. | ||
component: elastic-agent | ||
|
||
# PR URL; optional; the PR number that added the changeset. | ||
# If not present is automatically filled by the tooling finding the PR where this changelog fragment has been added. | ||
# NOTE: the tooling supports backports, so it's able to fill the original PR number instead of the backport PR number. | ||
# Please provide it if you are adding a fragment for a different PR. | ||
pr: https://github.com/elastic/elastic-agent/pull/4166 | ||
|
||
# Issue URL; optional; the GitHub issue related to this changeset (either closes or is part of). | ||
# If not present is automatically filled by the tooling with the issue linked to the PR number. | ||
issue: https://github.com/elastic/elastic-agent/issues/4145 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
// or more contributor license agreements. Licensed under the Elastic License; | ||
// you may not use this file except in compliance with the Elastic License. | ||
|
||
package application | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
"gopkg.in/yaml.v2" | ||
) | ||
|
||
// This test exists to notify the cloudbeat team in case the default agent fleet config is changed. | ||
func TestDefaultAgentFleetConfig(t *testing.T) { | ||
cfg := map[string]interface{}{} | ||
|
||
err := yaml.Unmarshal(DefaultAgentFleetConfig, &cfg) | ||
assert.NoError(t, err) | ||
|
||
assert.Equal(t, map[string]interface{}{"fleet": map[interface{}]interface{}{"enabled": true}}, cfg) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
// or more contributor license agreements. Licensed under the Elastic License; | ||
// you may not use this file except in compliance with the Elastic License. | ||
|
||
package storage | ||
|
||
import ( | ||
"strings" | ||
|
||
"github.com/google/go-cmp/cmp" | ||
"gopkg.in/yaml.v3" | ||
) | ||
|
||
// This is a temporary solution to avoid replacing the target file if the content of the replacement is contained in the target file. | ||
// It only works for YAML files, since the only use case is for the default agent fleet config. | ||
// Returns true only if the replacement configuration is already contained in the original. | ||
func shouldSkipReplace(original []byte, replacement []byte) bool { | ||
replacementYaml := map[string]interface{}{} | ||
originalYaml := map[string]interface{}{} | ||
|
||
err := yaml.Unmarshal(replacement, &replacementYaml) | ||
if err != nil { | ||
return false | ||
} | ||
|
||
err = yaml.Unmarshal(original, &originalYaml) | ||
if err != nil { | ||
return false | ||
} | ||
|
||
diff := cmp.Diff(replacementYaml, originalYaml) | ||
if strings.HasPrefix(diff, "-") || strings.Contains(diff, "\n-") { | ||
// Lines starting with - represent values in the replacement that are not present in the original | ||
return false | ||
} | ||
|
||
return true | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
// or more contributor license agreements. Licensed under the Elastic License; | ||
// you may not use this file except in compliance with the Elastic License. | ||
|
||
package storage | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestShouldSkipReplace(t *testing.T) { | ||
tests := []struct { | ||
name string | ||
original []byte | ||
replacement []byte | ||
expected bool | ||
}{ | ||
{ | ||
name: "original and replacement are the same", | ||
original: []byte("fleet:\n enabled: true\n"), | ||
replacement: []byte("fleet:\n enabled: true\n"), | ||
expected: true, | ||
}, | ||
{ | ||
name: "original and replacement are different", | ||
original: []byte("fleet:\n enabled: true\n"), | ||
replacement: []byte("fleet:\n enabled: false\n"), | ||
expected: false, | ||
}, | ||
{ | ||
name: "original is not a valid yaml", | ||
original: []byte("fleet: enabled: true\n"), | ||
expected: false, | ||
}, | ||
{ | ||
name: "replacement is not a valid yaml", | ||
replacement: []byte("fleet: enabled: true\n"), | ||
expected: false, | ||
}, | ||
{ | ||
name: "original and replacement are different just in comments and spaces", | ||
original: []byte("#bla bla bla\nfleet:\n enabled: true\n"), | ||
replacement: []byte("fleet: \n enabled: true\n#oh right bla bla bla\n"), | ||
expected: true, | ||
}, | ||
{ | ||
name: "original contains replacement and more", | ||
original: []byte("#bla bla bla\nfleet:\n enabled: true\nanother: value\nmore:\n stuff: true\n"), | ||
replacement: []byte("fleet:\n enabled: true\n"), | ||
expected: true, | ||
}, | ||
{ | ||
name: "original contains replacement and more, but in different order", | ||
original: []byte("fleet:\n a_key_that_ruins: sad\n enabled: true\n"), | ||
replacement: []byte("fleet:\n enabled: true\n"), | ||
expected: true, | ||
}, | ||
} | ||
|
||
for _, tt := range tests { | ||
t.Run(tt.name, func(t *testing.T) { | ||
assert.Equal(t, tt.expected, shouldSkipReplace(tt.original, tt.replacement)) | ||
}) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In case a provider is configured and the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, providers can be explicitly configured. The flag just changes the default behavior There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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",
},
},
},
}, |
||
// explicitly disabled; skipping | ||
continue | ||
} | ||
|
@@ -84,7 +84,7 @@ func New(log *logger.Logger, c *config.Config, managed bool) (Controller, error) | |
dynamicProviders := map[string]*dynamicProviderState{} | ||
for name, builder := range Providers.dynamicProviders { | ||
pCfg, ok := providersCfg.Providers[name] | ||
if ok && !pCfg.Enabled() { | ||
if (ok && !pCfg.Enabled()) || (!ok && providersCfg.ProvidersDefaultDisable) { | ||
// explicitly disabled; skipping | ||
continue | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 underagent.*
.What about:
agent.providers.initial_default: false
? Theinitial_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.