From 7e059e201eee8da41387c5f181db86dccf1fe8c7 Mon Sep 17 00:00:00 2001 From: Gabriel Krenn Date: Fri, 25 Oct 2024 12:39:54 +0200 Subject: [PATCH] Refactored to match other reconcilers --- .../processmoduleconfigsecret/reconciler.go | 124 +++++++----------- .../reconciler_test.go | 18 ++- pkg/util/conditions/secret.go | 19 ++- pkg/util/conditions/suffix.go | 11 +- 4 files changed, 82 insertions(+), 90 deletions(-) diff --git a/pkg/controllers/dynakube/processmoduleconfigsecret/reconciler.go b/pkg/controllers/dynakube/processmoduleconfigsecret/reconciler.go index c3647a81c9..6557a50b7a 100644 --- a/pkg/controllers/dynakube/processmoduleconfigsecret/reconciler.go +++ b/pkg/controllers/dynakube/processmoduleconfigsecret/reconciler.go @@ -9,12 +9,12 @@ import ( "github.com/Dynatrace/dynatrace-operator/pkg/controllers/dynakube/activegate/capability" "github.com/Dynatrace/dynatrace-operator/pkg/controllers/dynakube/connectioninfo" "github.com/Dynatrace/dynatrace-operator/pkg/util/conditions" - secrets "github.com/Dynatrace/dynatrace-operator/pkg/util/kubeobjects/secret" + k8ssecret "github.com/Dynatrace/dynatrace-operator/pkg/util/kubeobjects/secret" "github.com/Dynatrace/dynatrace-operator/pkg/util/timeprovider" "github.com/pkg/errors" corev1 "k8s.io/api/core/v1" - k8serrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) @@ -30,6 +30,7 @@ type Reconciler struct { dtClient dtclient.Client dk *dynakube.DynaKube timeProvider *timeprovider.Provider + secretQuery k8ssecret.QueryObject } func NewReconciler(clt client.Client, @@ -37,113 +38,87 @@ func NewReconciler(clt client.Client, dtClient dtclient.Client, dk *dynakube.DynaKube, timeProvider *timeprovider.Provider) *Reconciler { - return &Reconciler{ + r := &Reconciler{ client: clt, apiReader: apiReader, dtClient: dtClient, dk: dk, timeProvider: timeProvider, } + r.secretQuery = k8ssecret.Query(clt, apiReader, log) + + return r } func (r *Reconciler) Reconcile(ctx context.Context) error { - if r.dk.CloudNativeFullstackMode() || r.dk.ApplicationMonitoringMode() { - err := r.reconcileSecret(ctx) - if err != nil { - log.Info("could not reconcile pull secret") - - return errors.WithStack(err) + if !(r.dk.CloudNativeFullstackMode() || r.dk.ApplicationMonitoringMode()) { + if meta.FindStatusCondition(*r.dk.Conditions(), pmcConditionType) == nil { + return nil } - } else { - _ = meta.RemoveStatusCondition(&r.dk.Status.Conditions, pmcConditionType) - // TODO: Add cleanup here - log.Info("skipping process module config secret reconciler") - } - return nil -} + defer meta.RemoveStatusCondition(r.dk.Conditions(), pmcConditionType) + + err := r.deleteSecret(ctx, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: extendWithSuffix(r.dk.Name), + Namespace: r.dk.Namespace, + }, + }) -func (r *Reconciler) reconcileSecret(ctx context.Context) error { - if r.isFirstRun() { - err := r.createSecret(ctx) if err != nil { - return errors.WithMessage(err, "could not get or create secret") + return errors.WithMessage(err, "failed to delete processModuleConfig secret") } + + return nil } - if err := r.ensureSecret(ctx); err != nil { - return errors.WithMessage(err, "could not update secret") + err := r.reconcileSecret(ctx) + if err != nil { + return errors.WithMessage(err, "failed to create processModuleConfig secret") } return nil } -func (r *Reconciler) createSecret(ctx context.Context) error { - log.Info("creating process module config secret") - - newSecret, err := r.prepareSecret(ctx) - if err != nil { - return err +func (r *Reconciler) reconcileSecret(ctx context.Context) error { + if !conditions.IsOutdated(r.timeProvider, r.dk, pmcConditionType) { + return nil } - if err = r.client.Create(ctx, newSecret); err != nil { - conditions.SetKubeApiError(r.dk.Conditions(), pmcConditionType, err) + log.Info("processModuleConfig is outdated, updating") + conditions.SetSecretOutdated(r.dk.Conditions(), pmcConditionType, "secret is outdated, update in progress") + secret, err := r.prepareSecret(ctx) + if err != nil { return err } - conditions.SetSecretCreated(r.dk.Conditions(), pmcConditionType, newSecret.Name) - - return nil + return r.createOrUpdateSecret(ctx, secret) } -func (r *Reconciler) ensureSecret(ctx context.Context) error { - oldSecret, err := getSecret(ctx, r.apiReader, r.dk.Name, r.dk.Namespace) - if k8serrors.IsNotFound(err) { - log.Info("secret was removed unexpectedly, ensuring process module config secret") - - return r.createSecret(ctx) - } else if err != nil { +func (r *Reconciler) createOrUpdateSecret(ctx context.Context, secret *corev1.Secret) error { + _, err := r.secretQuery.WithOwner(r.dk).CreateOrUpdate(ctx, secret) + if err != nil { conditions.SetKubeApiError(r.dk.Conditions(), pmcConditionType, err) - return err + return errors.Errorf("failed to create secret '%s': %v", secret.Name, err) } - if conditions.IsOutdated(r.timeProvider, r.dk, pmcConditionType) { - conditions.SetSecretOutdated(r.dk.Conditions(), pmcConditionType, oldSecret.Name+" is outdated, update in progress") // Necessary to update the LastTransitionTime, also it is a nice failsafe - - return r.updateSecret(ctx, oldSecret) - } + conditions.SetSecretCreatedOrUpdated(r.dk.Conditions(), pmcConditionType, secret.Name) return nil } -func (r *Reconciler) updateSecret(ctx context.Context, oldSecret *corev1.Secret) error { - log.Info("updating process module config secret") - - newSecret, err := r.prepareSecret(ctx) - if err != nil { - return err - } - - oldSecret.Data = newSecret.Data - if err = r.client.Update(ctx, oldSecret); err != nil { +func (r *Reconciler) deleteSecret(ctx context.Context, secret *corev1.Secret) error { + if err := r.secretQuery.Delete(ctx, secret); err != nil { conditions.SetKubeApiError(r.dk.Conditions(), pmcConditionType, err) return err } - conditions.SetSecretUpdated(r.dk.Conditions(), pmcConditionType, newSecret.Name) - return nil } -func (r *Reconciler) isFirstRun() bool { - condition := meta.FindStatusCondition(r.dk.Status.Conditions, pmcConditionType) - - return condition == nil -} - func (r *Reconciler) prepareSecret(ctx context.Context) (*corev1.Secret, error) { pmc, err := r.dtClient.GetProcessModuleConfig(ctx, 0) if err != nil { @@ -152,7 +127,7 @@ func (r *Reconciler) prepareSecret(ctx context.Context) (*corev1.Secret, error) return nil, err } - tenantToken, err := secrets.GetDataFromSecretName(ctx, r.apiReader, types.NamespacedName{ + tenantToken, err := k8ssecret.GetDataFromSecretName(ctx, r.apiReader, types.NamespacedName{ Name: r.dk.OneagentTenantSecret(), Namespace: r.dk.Namespace, }, connectioninfo.TenantTokenKey, log) @@ -195,11 +170,11 @@ func (r *Reconciler) prepareSecret(ctx context.Context) (*corev1.Secret, error) return nil, err } - newSecret, err := secrets.Build(r.dk, + newSecret, err := k8ssecret.Build(r.dk, extendWithSuffix(r.dk.Name), map[string][]byte{SecretKeyProcessModuleConfig: marshaled}) - secrets.SetType(corev1.SecretTypeOpaque) + k8ssecret.SetType(corev1.SecretTypeOpaque) if err != nil { conditions.SetKubeApiError(r.dk.Conditions(), pmcConditionType, err) @@ -211,7 +186,9 @@ func (r *Reconciler) prepareSecret(ctx context.Context) (*corev1.Secret, error) } func GetSecretData(ctx context.Context, apiReader client.Reader, dynakubeName string, dynakubeNamespace string) (*dtclient.ProcessModuleConfig, error) { - secret, err := getSecret(ctx, apiReader, dynakubeName, dynakubeNamespace) + typedName := types.NamespacedName{Namespace: dynakubeNamespace, Name: extendWithSuffix(dynakubeName)} + + secret, err := k8ssecret.Query(nil, apiReader, log).Get(ctx, typedName) if err != nil { return nil, err } @@ -224,17 +201,6 @@ func GetSecretData(ctx context.Context, apiReader client.Reader, dynakubeName st return processModuleConfig, nil } -func getSecret(ctx context.Context, apiReader client.Reader, dynakubeName string, dynakubeNamespace string) (*corev1.Secret, error) { - var config corev1.Secret - - err := apiReader.Get(ctx, client.ObjectKey{Name: extendWithSuffix(dynakubeName), Namespace: dynakubeNamespace}, &config) - if err != nil { - return nil, err - } - - return &config, nil -} - func unmarshal(secret *corev1.Secret) (*dtclient.ProcessModuleConfig, error) { var config *dtclient.ProcessModuleConfig diff --git a/pkg/controllers/dynakube/processmoduleconfigsecret/reconciler_test.go b/pkg/controllers/dynakube/processmoduleconfigsecret/reconciler_test.go index 7a29214d43..d74798ccc9 100644 --- a/pkg/controllers/dynakube/processmoduleconfigsecret/reconciler_test.go +++ b/pkg/controllers/dynakube/processmoduleconfigsecret/reconciler_test.go @@ -52,7 +52,7 @@ func TestReconcile(t *testing.T) { oldTransitionTime := condition.LastTransitionTime require.NotNil(t, condition) require.NotEmpty(t, oldTransitionTime) - assert.Equal(t, conditions.SecretCreatedReason, condition.Reason) + assert.Equal(t, conditions.SecretCreatedOrUpdatedReason, condition.Reason) assert.Equal(t, metav1.ConditionTrue, condition.Status) // update should be blocked by timeout @@ -76,19 +76,29 @@ func TestReconcile(t *testing.T) { condition = meta.FindStatusCondition(*dk.Conditions(), pmcConditionType) require.NotNil(t, condition) require.Greater(t, condition.LastTransitionTime.Time, oldTransitionTime.Time) - assert.Equal(t, conditions.SecretUpdatedReason, condition.Reason) + assert.Equal(t, conditions.SecretCreatedOrUpdatedReason, condition.Reason) assert.Equal(t, metav1.ConditionTrue, condition.Status) }) - t.Run("Only runs when required, and cleans up condition", func(t *testing.T) { + t.Run("Only runs when required, and cleans up condition and pmc secret", func(t *testing.T) { dk := createDynakube(dynakube.OneAgentSpec{ ClassicFullStack: &dynakube.HostInjectSpec{}}) conditions.SetSecretCreated(dk.Conditions(), pmcConditionType, "this is a test") - reconciler := NewReconciler(nil, nil, nil, dk, timeprovider.New()) + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: extendWithSuffix(testName), + Namespace: testNamespace, + }, + Data: map[string][]byte{SecretKeyProcessModuleConfig: []byte("test")}, + } + mockK8sClient := fake.NewClient(secret) + + reconciler := NewReconciler(mockK8sClient, mockK8sClient, nil, dk, timeprovider.New()) err := reconciler.Reconcile(context.Background()) require.NoError(t, err) assert.Empty(t, *dk.Conditions()) + assert.Error(t, mockK8sClient.Get(context.Background(), client.ObjectKey{Name: extendWithSuffix(testName), Namespace: testNamespace}, &corev1.Secret{})) }) t.Run("No proxy is set when proxy enabled and custom no proxy set", func(t *testing.T) { dk := createDynakube(dynakube.OneAgentSpec{ diff --git a/pkg/util/conditions/secret.go b/pkg/util/conditions/secret.go index aa62b3c4f2..e909742926 100644 --- a/pkg/util/conditions/secret.go +++ b/pkg/util/conditions/secret.go @@ -6,10 +6,11 @@ import ( ) const ( - SecretCreatedReason = "SecretCreated" - SecretUpdatedReason = "SecretUpdated" - SecretOutdatedReason = "SecretOutdated" - SecretGenerationFailed = "SecretGenerationFailed" + SecretCreatedReason = "SecretCreated" + SecretUpdatedReason = "SecretUpdated" + SecretCreatedOrUpdatedReason = "SecretCreatedOrUpdated" + SecretOutdatedReason = "SecretOutdated" + SecretGenerationFailed = "SecretGenerationFailed" ) func SetSecretCreated(conditions *[]metav1.Condition, conditionType, name string) { @@ -32,6 +33,16 @@ func SetSecretUpdated(conditions *[]metav1.Condition, conditionType, name string _ = meta.SetStatusCondition(conditions, condition) } +func SetSecretCreatedOrUpdated(conditions *[]metav1.Condition, conditionType, name string) { + condition := metav1.Condition{ + Type: conditionType, + Status: metav1.ConditionTrue, + Reason: SecretCreatedOrUpdatedReason, + Message: appendCreatedOrUpdatedSuffix(name), + } + _ = meta.SetStatusCondition(conditions, condition) +} + func SetSecretGenFailed(conditions *[]metav1.Condition, conditionType string, err error) { condition := metav1.Condition{ Type: conditionType, diff --git a/pkg/util/conditions/suffix.go b/pkg/util/conditions/suffix.go index 720bf0598c..40f3192c43 100644 --- a/pkg/util/conditions/suffix.go +++ b/pkg/util/conditions/suffix.go @@ -3,9 +3,10 @@ package conditions import "fmt" const ( - createdSuffix = "created" - updatedSuffix = "updated" - outdatedSuffix = "outdated" + createdSuffix = "created" + updatedSuffix = "updated" + createdOrUpdatedSuffix = "created/updated" + outdatedSuffix = "outdated" ) func appendCreatedSuffix(name string) string { @@ -16,6 +17,10 @@ func appendUpdatedSuffix(name string) string { return fmt.Sprintf("%s %s", name, updatedSuffix) } +func appendCreatedOrUpdatedSuffix(name string) string { + return fmt.Sprintf("%s %s", name, createdOrUpdatedSuffix) +} + func appendOutdatedSuffix(name string) string { return fmt.Sprintf("%s %s", name, outdatedSuffix) }