Fix DatadogAgent secret backend usage (#454)

* secret backend fixes and test updates

apis/datadoghq/v1alpha1/datadogagent_types.go

@@ -118,7 +118,6 @@ type AgentCredentials struct {
 
 	// UseSecretBackend use the Agent secret backend feature for retreiving all credentials needed by
 	// the different components: Agent, Cluster, Cluster-Checks.
-	// If `useSecretBackend: true`, other credential parameters will be ignored.
 	// default value is false.
 	UseSecretBackend *bool `json:"useSecretBackend,omitempty"`
 }

config/crd/bases/v1/datadoghq.com_datadogagents.yaml

@@ -11897,9 +11897,7 @@ spec:
                   useSecretBackend:
                     description: 'UseSecretBackend use the Agent secret backend feature
                       for retreiving all credentials needed by the different components:
-                      Agent, Cluster, Cluster-Checks. If `useSecretBackend: true`,
-                      other credential parameters will be ignored. default value is
-                      false.'
+                      Agent, Cluster, Cluster-Checks. default value is false.'
                     type: boolean
                 type: object
               features:

config/crd/bases/v1beta1/datadoghq.com_datadogagents.yaml

@@ -11484,8 +11484,7 @@ spec:
                 useSecretBackend:
                   description: 'UseSecretBackend use the Agent secret backend feature
                     for retreiving all credentials needed by the different components:
-                    Agent, Cluster, Cluster-Checks. If `useSecretBackend: true`, other
-                    credential parameters will be ignored. default value is false.'
+                    Agent, Cluster, Cluster-Checks. default value is false.'
                   type: boolean
               type: object
             features:

controllers/datadogagent/agent.go

@@ -35,7 +35,7 @@ import (
 )
 
 func (r *Reconciler) reconcileAgent(logger logr.Logger, dda *datadoghqv1alpha1.DatadogAgent, newStatus *datadoghqv1alpha1.DatadogAgentStatus) (reconcile.Result, error) {
-	result, err := r.manageAgentDependencies(logger, dda, newStatus)
+	result, err := r.manageAgentDependencies(logger, dda)
 	if utils.ShouldReturn(result, err) {
 		return result, err
 	}
@@ -270,8 +270,8 @@ func (r *Reconciler) updateDaemonSet(logger logr.Logger, dda *datadoghqv1alpha1.
 	return reconcile.Result{RequeueAfter: 5 * time.Second}, nil
 }
 
-func (r *Reconciler) manageAgentDependencies(logger logr.Logger, dda *datadoghqv1alpha1.DatadogAgent, newStatus *datadoghqv1alpha1.DatadogAgentStatus) (reconcile.Result, error) {
-	result, err := r.manageAgentSecret(logger, dda, newStatus)
+func (r *Reconciler) manageAgentDependencies(logger logr.Logger, dda *datadoghqv1alpha1.DatadogAgent) (reconcile.Result, error) {
+	result, err := r.manageAgentSecret(logger, dda)
 	if utils.ShouldReturn(result, err) {
 		return result, err
 	}

controllers/datadogagent/clusteragent.go

@@ -35,7 +35,7 @@ import (
 )
 
 func (r *Reconciler) reconcileClusterAgent(logger logr.Logger, dda *datadoghqv1alpha1.DatadogAgent, newStatus *datadoghqv1alpha1.DatadogAgentStatus) (reconcile.Result, error) {
-	result, err := r.manageClusterAgentDependencies(logger, dda, newStatus)
+	result, err := r.manageClusterAgentDependencies(logger, dda)
 	if utils.ShouldReturn(result, err) {
 		return result, err
 	}
@@ -190,13 +190,13 @@ func newClusterAgentDeploymentFromInstance(logger logr.Logger, dda *datadoghqv1a
 	return dca, hash, err
 }
 
-func (r *Reconciler) manageClusterAgentDependencies(logger logr.Logger, dda *datadoghqv1alpha1.DatadogAgent, newStatus *datadoghqv1alpha1.DatadogAgentStatus) (reconcile.Result, error) {
-	result, err := r.manageAgentSecret(logger, dda, newStatus)
+func (r *Reconciler) manageClusterAgentDependencies(logger logr.Logger, dda *datadoghqv1alpha1.DatadogAgent) (reconcile.Result, error) {
+	result, err := r.manageAgentSecret(logger, dda)
 	if utils.ShouldReturn(result, err) {
 		return result, err
 	}
 
-	result, err = r.manageExternalMetricsSecret(logger, dda, newStatus)
+	result, err = r.manageExternalMetricsSecret(logger, dda)
 	if utils.ShouldReturn(result, err) {
 		return result, err
 	}
@@ -605,10 +605,6 @@ func getEnvVarsForClusterAgent(logger logr.Logger, dda *datadoghqv1alpha1.Datado
 			Name:  datadoghqv1alpha1.DDClusterAgentKubeServiceName,
 			Value: getClusterAgentServiceName(dda),
 		},
-		{
-			Name:      datadoghqv1alpha1.DDClusterAgentAuthToken,
-			ValueFrom: getClusterAgentAuthToken(dda),
-		},
 		{
 			Name:  datadoghqv1alpha1.DDLeaderElection,
 			Value: "true",
@@ -627,6 +623,11 @@ func getEnvVarsForClusterAgent(logger logr.Logger, dda *datadoghqv1alpha1.Datado
 		},
 	}
 
+	envVars = append(envVars, corev1.EnvVar{
+		Name:      datadoghqv1alpha1.DDClusterAgentAuthToken,
+		ValueFrom: getClusterAgentAuthToken(dda),
+	})
+
 	if spec.ClusterName != "" {
 		envVars = append(envVars, corev1.EnvVar{
 			Name:  datadoghqv1alpha1.DDClusterName,

controllers/datadogagent/clusterchecksrunner.go

@@ -34,7 +34,7 @@ import (
 )
 
 func (r *Reconciler) reconcileClusterChecksRunner(logger logr.Logger, dda *datadoghqv1alpha1.DatadogAgent, newStatus *datadoghqv1alpha1.DatadogAgentStatus) (reconcile.Result, error) {
-	result, err := r.manageClusterChecksRunnerDependencies(logger, dda, newStatus)
+	result, err := r.manageClusterChecksRunnerDependencies(logger, dda)
 	if utils.ShouldReturn(result, err) {
 		return result, err
 	}
@@ -197,8 +197,8 @@ func newClusterChecksRunnerDeploymentFromInstance(
 	return dca, hash, err
 }
 
-func (r *Reconciler) manageClusterChecksRunnerDependencies(logger logr.Logger, dda *datadoghqv1alpha1.DatadogAgent, newStatus *datadoghqv1alpha1.DatadogAgentStatus) (reconcile.Result, error) {
-	result, err := r.manageAgentSecret(logger, dda, newStatus)
+func (r *Reconciler) manageClusterChecksRunnerDependencies(logger logr.Logger, dda *datadoghqv1alpha1.DatadogAgent) (reconcile.Result, error) {
+	result, err := r.manageAgentSecret(logger, dda)
 	if utils.ShouldReturn(result, err) {
 		return result, err
 	}
@@ -340,10 +340,6 @@ func buildClusterChecksRunnerConfigurationConfigMap(dda *datadoghqv1alpha1.Datad
 func getEnvVarsForClusterChecksRunner(dda *datadoghqv1alpha1.DatadogAgent) []corev1.EnvVar {
 	spec := &dda.Spec
 	envVars := []corev1.EnvVar{
-		{
-			Name:      datadoghqv1alpha1.DDAPIKey,
-			ValueFrom: getAPIKeyFromSecret(dda),
-		},
 		{
 			Name:  datadoghqv1alpha1.DDClusterChecksEnabled,
 			Value: "true",
@@ -356,10 +352,6 @@ func getEnvVarsForClusterChecksRunner(dda *datadoghqv1alpha1.DatadogAgent) []cor
 			Name:  datadoghqv1alpha1.DDClusterAgentKubeServiceName,
 			Value: getClusterAgentServiceName(dda),
 		},
-		{
-			Name:      datadoghqv1alpha1.DDClusterAgentAuthToken,
-			ValueFrom: getClusterAgentAuthToken(dda),
-		},
 		{
 			Name:  datadoghqv1alpha1.DDExtraConfigProviders,
 			Value: datadoghqv1alpha1.ClusterChecksConfigProvider,
@@ -418,6 +410,16 @@ func getEnvVarsForClusterChecksRunner(dda *datadoghqv1alpha1.DatadogAgent) []cor
 		},
 	}
 
+	envVars = append(envVars, corev1.EnvVar{
+		Name:      datadoghqv1alpha1.DDAPIKey,
+		ValueFrom: getAPIKeyFromSecret(dda),
+	})
+
+	envVars = append(envVars, corev1.EnvVar{
+		Name:      datadoghqv1alpha1.DDClusterAgentAuthToken,
+		ValueFrom: getClusterAgentAuthToken(dda),
+	})
+
 	if spec.ClusterName != "" {
 		envVars = append(envVars, corev1.EnvVar{
 			Name:  datadoghqv1alpha1.DDClusterName,

controllers/datadogagent/secret.go

@@ -7,8 +7,6 @@ package datadogagent
 
 import (
 	"context"
-	"fmt"
-	"time"
 
 	"github.com/go-logr/logr"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -17,37 +15,30 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 
 	datadoghqv1alpha1 "github.com/DataDog/datadog-operator/apis/datadoghq/v1alpha1"
-	"github.com/DataDog/datadog-operator/pkg/controller/utils/condition"
 	"github.com/DataDog/datadog-operator/pkg/controller/utils/datadog"
 	"github.com/DataDog/datadog-operator/pkg/kubernetes"
 )
 
 type managedSecret struct {
 	name        string
 	requireFunc func(dda *datadoghqv1alpha1.DatadogAgent) bool
-	createFunc  func(name string, dda *datadoghqv1alpha1.DatadogAgent) (*corev1.Secret, error)
+	createFunc  func(name string, dda *datadoghqv1alpha1.DatadogAgent) *corev1.Secret
 }
 
-func (r *Reconciler) manageSecret(logger logr.Logger, secret managedSecret, dda *datadoghqv1alpha1.DatadogAgent, newStatus *datadoghqv1alpha1.DatadogAgentStatus) (reconcile.Result, error) {
+func (r *Reconciler) manageSecret(logger logr.Logger, secret managedSecret, dda *datadoghqv1alpha1.DatadogAgent) (reconcile.Result, error) {
 	if !secret.requireFunc(dda) {
 		result, err := r.cleanupSecret(dda.Namespace, secret.name, dda)
 		return result, err
 	}
 
-	now := metav1.NewTime(time.Now())
 	secretObj := &corev1.Secret{}
 	err := r.client.Get(context.TODO(), types.NamespacedName{Namespace: dda.Namespace, Name: secret.name}, secretObj)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
-			s, errCreate := secret.createFunc(secret.name, dda)
-			if errCreate != nil {
-				condition.UpdateDatadogAgentStatusConditions(newStatus, now, datadoghqv1alpha1.DatadogAgentConditionTypeSecretError, corev1.ConditionTrue, fmt.Sprintf("%v", err), false)
-				return reconcile.Result{}, fmt.Errorf("cannot create secret %s, err: %w", secret.name, errCreate)
-			}
+			s := secret.createFunc(secret.name, dda)
 
 			return r.createSecret(logger, s, dda)
 		}
@@ -77,10 +68,7 @@ func (r *Reconciler) updateIfNeededSecret(secret managedSecret, dda *datadoghqv1
 		return reconcile.Result{}, nil
 	}
 
-	newSecret, err := secret.createFunc(secret.name, dda)
-	if err != nil {
-		return reconcile.Result{}, err
-	}
+	newSecret := secret.createFunc(secret.name, dda)
 
 	result := reconcile.Result{}
 	if !(apiequality.Semantic.DeepEqual(newSecret.Data, currentSecret.Data) &&
@@ -124,16 +112,3 @@ func (r *Reconciler) cleanupSecret(namespace, name string, dda *datadoghqv1alpha
 
 	return reconcile.Result{}, err
 }
-
-func dataFromCredentials(credentials *datadoghqv1alpha1.DatadogCredentials) map[string][]byte {
-	data := make(map[string][]byte)
-	// Create secret using DatadogAgent credentials if it exists
-	if credentials.APIKey != "" {
-		data[datadoghqv1alpha1.DefaultAPIKeyKey] = []byte(credentials.APIKey)
-	}
-	if credentials.AppKey != "" {
-		data[datadoghqv1alpha1.DefaultAPPKeyKey] = []byte(credentials.AppKey)
-	}
-
-	return data
-}

controllers/datadogagent/secret_agent.go

@@ -6,36 +6,27 @@
 package datadogagent
 
 import (
-	"fmt"
-	"os"
-
+	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	datadoghqv1alpha1 "github.com/DataDog/datadog-operator/apis/datadoghq/v1alpha1"
-	apiutils "github.com/DataDog/datadog-operator/apis/utils"
 	"github.com/DataDog/datadog-operator/pkg/config"
 	"github.com/DataDog/datadog-operator/pkg/controller/utils"
-	"github.com/go-logr/logr"
 )
 
-func (r *Reconciler) manageAgentSecret(logger logr.Logger, dda *datadoghqv1alpha1.DatadogAgent, newStatus *datadoghqv1alpha1.DatadogAgentStatus) (reconcile.Result, error) {
-	return r.manageSecret(logger, managedSecret{name: utils.GetDefaultCredentialsSecretName(dda), requireFunc: needAgentSecret, createFunc: newAgentSecret}, dda, newStatus)
+func (r *Reconciler) manageAgentSecret(logger logr.Logger, dda *datadoghqv1alpha1.DatadogAgent) (reconcile.Result, error) {
+	return r.manageSecret(logger, managedSecret{name: utils.GetDefaultCredentialsSecretName(dda), requireFunc: needAgentSecret, createFunc: newAgentSecret}, dda)
 }
 
-func newAgentSecret(name string, dda *datadoghqv1alpha1.DatadogAgent) (*corev1.Secret, error) {
-	if err := checkCredentials(dda); err != nil {
-		return nil, err
-	}
-
+func newAgentSecret(name string, dda *datadoghqv1alpha1.DatadogAgent) *corev1.Secret {
 	labels := getDefaultLabels(dda, datadoghqv1alpha1.DefaultClusterAgentResourceSuffix, getClusterAgentVersion(dda))
 	annotations := getDefaultAnnotations(dda)
 
 	creds := dda.Spec.Credentials
-	data := dataFromCredentials(&creds.DatadogCredentials)
+	data := getKeysFromCredentials(&creds.DatadogCredentials)
 
-	// Agent credentials has two more fields
 	if creds.Token != "" {
 		data[datadoghqv1alpha1.DefaultTokenKey] = []byte(creds.Token)
 	} else if isClusterAgentEnabled(dda.Spec.ClusterAgent) {
@@ -55,44 +46,22 @@ func newAgentSecret(name string, dda *datadoghqv1alpha1.DatadogAgent) (*corev1.S
 		Type: corev1.SecretTypeOpaque,
 		Data: data,
 	}
-	return secret, nil
+	return secret
 }
 
-// needAgentSecret checks if a secret should be used or created due to the cluster agent being defined, or if any api or app key
-// is configured, AND the secret backend is not used
+// needAgentSecret checks if a secret should be used or created.
 func needAgentSecret(dda *datadoghqv1alpha1.DatadogAgent) bool {
+	// If credentials is nil, there is nothing to create a secret from.
 	if dda.Spec.Credentials == nil {
-		// If Credentials are not specified we fail downstream to have the error surfaced in the status of the DatadogAgent
 		return false
 	}
-	return (isClusterAgentEnabled(dda.Spec.ClusterAgent) || (dda.Spec.Credentials.APIKey != "" || os.Getenv(config.DDAPIKeyEnvVar) != "") || (dda.Spec.Credentials.AppKey != "" || os.Getenv(config.DDAppKeyEnvVar) != "")) &&
-		!apiutils.BoolValue(dda.Spec.Credentials.UseSecretBackend)
-}
-
-func checkCredentials(dda *datadoghqv1alpha1.DatadogAgent) error {
-	if dda.Spec.Credentials == nil {
-		return fmt.Errorf("unable to create agent secret: missing .spec.Credentials")
-	}
 
-	creds := dda.Spec.Credentials
-
-	if !checkKeyAndSecret(creds.APIKey, creds.APIKeyExistingSecret, creds.APISecret) {
-		if os.Getenv(config.DDAPIKeyEnvVar) == "" {
-			return fmt.Errorf("unable to create agent credential secret: missing Api-Key information")
-		}
-	}
-
-	if !checkKeyAndSecret(creds.AppKey, creds.AppKeyExistingSecret, creds.APPSecret) {
-		if os.Getenv(config.DDAppKeyEnvVar) == "" {
-			return fmt.Errorf("unable to create agent credential secret: missing App-Key information")
-		}
+	// If API key, app key _and_ token don't need a new secret, then don't create one.
+	if checkAPIKeySufficiency(&dda.Spec.Credentials.DatadogCredentials, config.DDAPIKeyEnvVar) &&
+		checkAppKeySufficiency(&dda.Spec.Credentials.DatadogCredentials, config.DDAppKeyEnvVar) &&
+		!isClusterAgentEnabled(dda.Spec.ClusterAgent) {
+		return false
 	}
-	return nil
-}
 
-func checkKeyAndSecret(value, secretName string, secret *datadoghqv1alpha1.Secret) bool {
-	if value != "" || secretName != "" || (secret != nil && secret.SecretName != "") {
-		return true
-	}
-	return false
+	return true
 }