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

Add disable providers by default option #4166

Merged
merged 11 commits into from
Feb 5, 2024
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@
/deploy/kubernetes @elastic/obs-cloudnative-monitoring
/dev-tools/kubernetes @elastic/obs-cloudnative-monitoring
/internal/pkg/composable/providers/kubernetes @elastic/obs-cloudnative-monitoring
/internal/pkg/agent/application/configuration_embed_changed_test.go @elastic/cloudbeat
5 changes: 5 additions & 0 deletions _meta/config/providers.yml.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@
# and conditionals. Each provider's keys are automatically prefixed with the name
# of the provider.

# All registered providers are enabled by default.

# Disable all providers by default and only enable explicitly configured providers.
# providers_default_disable: true

#providers:

# Agent provides information about the running agent.
Expand Down
32 changes: 32 additions & 0 deletions changelog/fragments/1707070032-providers-default-disable.yaml
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
5 changes: 5 additions & 0 deletions elastic-agent.docker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,11 @@ agent.logging.to_stderr: true
# and conditionals. Each provider's keys are automatically prefixed with the name
# of the provider.

# All registered providers are enabled by default.

# Disable all providers by default and only enable explicitly configured providers.
# providers_default_disable: true

#providers:

# Agent provides information about the running agent.
Expand Down
5 changes: 5 additions & 0 deletions elastic-agent.reference.yml
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,11 @@ agent.logging.to_stderr: true
# and conditionals. Each provider's keys are automatically prefixed with the name
# of the provider.

# All registered providers are enabled by default.

# Disable all providers by default and only enable explicitly configured providers.
# providers_default_disable: true

#providers:

# Agent provides information about the running agent.
Expand Down
5 changes: 5 additions & 0 deletions elastic-agent.yml
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,11 @@ agent.logging.to_stderr: true
# and conditionals. Each provider's keys are automatically prefixed with the name
# of the provider.

# All registered providers are enabled by default.

# Disable all providers by default and only enable explicitly configured providers.
# providers_default_disable: true
Copy link
Contributor

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 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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.


#providers:

# Agent provides information about the running agent.
Expand Down
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)
}
4 changes: 4 additions & 0 deletions internal/pkg/agent/storage/replace_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,10 @@ func (r *ReplaceOnSuccessStore) Save(in io.Reader) error {
return nil
}

if shouldSkipReplace(target, r.replaceWith) {
return nil
}

// Windows is tricky with the characters permitted for the path and filename, so we have
// to remove any colon from the string. We are using nanosec precision here because of automated
// tools.
Expand Down
38 changes: 38 additions & 0 deletions internal/pkg/agent/storage/skip_replace.go
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
}
67 changes: 67 additions & 0 deletions internal/pkg/agent/storage/skip_replace_test.go
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))
})
}
}
34 changes: 32 additions & 2 deletions internal/pkg/agent/storage/storage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package storage

import (
"bytes"
"errors"
"fmt"
"io"
"os"
Expand All @@ -14,7 +15,6 @@ import (
"testing"
"time"

"github.com/pkg/errors"
"github.com/stretchr/testify/require"
)

Expand Down Expand Up @@ -101,6 +101,33 @@ func TestReplaceOrRollbackStore(t *testing.T) {

})

t.Run("when replace is skipped due to target already containing source content", func(t *testing.T) {
yamlTarget := []byte("fleet:\n enabled: true\nother: value\n")
yamlReplaceWith := []byte("#This comment is left out\nfleet:\n enabled: true\n")
target, err := genFile(yamlTarget)

require.NoError(t, err)
dir := filepath.Dir(target)
defer os.RemoveAll(dir)

requireFilesCount(t, dir, 1)

s := NewReplaceOnSuccessStore(
target,
yamlReplaceWith,
failure,
)

err = s.Save(in)
require.Error(t, err)

writtenContent, err := os.ReadFile(target)
require.NoError(t, err)

require.True(t, bytes.Equal(writtenContent, yamlTarget))
requireFilesCount(t, dir, 1)
})

t.Run("when target file do not exist", func(t *testing.T) {
s := NewReplaceOnSuccessStore(
fmt.Sprintf("%s/%d", os.TempDir(), time.Now().Unix()),
Expand Down Expand Up @@ -176,7 +203,10 @@ func genFile(b []byte) (string, error) {
if err != nil {
return "", err
}
f.Write(b)
_, err = f.Write(b)
if err != nil {
return "", err
}
name := f.Name()
if err := f.Close(); err != nil {
return "", err
Expand Down
3 changes: 2 additions & 1 deletion internal/pkg/composable/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,5 +8,6 @@ import "github.com/elastic/elastic-agent/internal/pkg/config"

// Config is config for multiple providers.
type Config struct {
Providers map[string]*config.Config `config:"providers"`
Providers map[string]*config.Config `config:"providers"`
ProvidersDefaultDisable bool `config:"providers_default_disable"`
}
4 changes: 2 additions & 2 deletions internal/pkg/composable/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case a provider is configured and the ProvidersDefaultDisable is set, should we continue or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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

Copy link

Choose a reason for hiding this comment

The 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
}
Expand All @@ -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
}
Expand Down
Loading
Loading