Add disable providers by default option (#4166)

* add disable providers by default option

* don't replace agent config upon enrollment if unnecessary

Author: eyalkraft

.github/CODEOWNERS

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

_meta/config/providers.yml.tmpl

@@ -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.
+# agent.providers.initial_default: false
+
 #providers:
 
 # Agent provides information about the running agent.

changelog/fragments/1707070032-providers-default-disable.yaml

@@ -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 agent.providers.initial_default 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

elastic-agent.docker.yml

@@ -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.
+# agent.providers.initial_default: false
+
 #providers:
 
 # Agent provides information about the running agent.

elastic-agent.reference.yml

@@ -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.
+# agent.providers.initial_default: false
+
 #providers:
 
 # Agent provides information about the running agent.

elastic-agent.yml

@@ -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.
+# agent.providers.initial_default: false
+
 #providers:
 
 # Agent provides information about the running agent.

internal/pkg/agent/application/configuration_embed_changed_test.go

@@ -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)
+}

internal/pkg/agent/storage/replace_store.go

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

internal/pkg/agent/storage/skip_replace.go

@@ -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
+}

internal/pkg/agent/storage/skip_replace_test.go

@@ -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))
+		})
+	}
+}

internal/pkg/agent/storage/storage_test.go

@@ -6,6 +6,7 @@ package storage
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -14,7 +15,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/pkg/errors"
 	"github.com/stretchr/testify/require"
 )
 
@@ -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()),
@@ -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

internal/pkg/composable/config.go

@@ -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"`
+	ProvidersInitialDefault *bool                     `config:"agent.providers.initial_default"`
 }

internal/pkg/composable/controller.go

@@ -61,11 +61,17 @@ func New(log *logger.Logger, c *config.Config, managed bool) (Controller, error)
 		}
 	}
 
+	//  Unless explicitly configured otherwise, All registered providers are enabled by default
+	providersInitialDefault := true
+	if providersCfg.ProvidersInitialDefault != nil {
+		providersInitialDefault = *providersCfg.ProvidersInitialDefault
+	}
+
 	// build all the context providers
 	contextProviders := map[string]*contextProviderState{}
 	for name, builder := range Providers.contextProviders {
 		pCfg, ok := providersCfg.Providers[name]
-		if ok && !pCfg.Enabled() {
+		if (ok && !pCfg.Enabled()) || (!ok && !providersInitialDefault) {
 			// explicitly disabled; skipping
 			continue
 		}
@@ -84,7 +90,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 && !providersInitialDefault) {
 			// explicitly disabled; skipping
 			continue
 		}