Prevent update of dynakube image status incase of error during regist…

…ry call (#1791)

* Return error incase of http error during getting image manifest

* Fix linting

src/api/v1beta1/dynakube_status.go

@@ -124,11 +124,3 @@ func (dk *DynaKubeStatus) SetPhase(phase DynaKubePhaseType) bool {
 	dk.Phase = phase
 	return upd
 }
-
-// SetPhaseOnError fills the phase with the Error value in case of any error
-func (dk *DynaKubeStatus) SetPhaseOnError(err error) bool {
-	if err != nil {
-		return dk.SetPhase(Error)
-	}
-	return false
-}

src/controllers/dynakube/activegate/internal/authtoken/reconciler.go

@@ -46,7 +46,7 @@ func NewReconciler(clt client.Client, apiReader client.Reader, scheme *runtime.S
 func (r *Reconciler) Reconcile() error {
 	err := r.reconcileAuthTokenSecret()
 	if err != nil {
-		return errors.Errorf("failed to create activeGateAuthToken secret: %v", err)
+		return errors.WithMessage(err, "failed to create activeGateAuthToken secret")
 	}
 	return nil
 }
@@ -77,7 +77,7 @@ func (r *Reconciler) reconcileAuthTokenSecret() error {
 func (r *Reconciler) ensureAuthTokenSecret() error {
 	agSecretData, err := r.getActiveGateAuthToken()
 	if err != nil {
-		return errors.Errorf("failed to create secret '%s': %v", r.dynakube.ActiveGateAuthTokenSecret(), err)
+		return errors.WithMessagef(err, "failed to create secret '%s'", r.dynakube.ActiveGateAuthTokenSecret())
 	}
 	return r.createSecret(agSecretData)
 }

src/controllers/dynakube/dynakube_controller.go

@@ -124,9 +124,9 @@ func (controller *Controller) Reconcile(ctx context.Context, request reconcile.R
 
 		var serverErr dtclient.ServerError
 		isServerError := errors.As(err, &serverErr)
-		if isServerError && serverErr.Code == http.StatusTooManyRequests {
+		if isServerError && (serverErr.Code == http.StatusTooManyRequests || serverErr.Code == http.StatusServiceUnavailable) {
 			// should we set the phase to error ?
-			log.Info("request limit for Dynatrace API reached! Next reconcile in one minute")
+			log.Info("server is unavailable or request limit reached! trying again in one minute")
 			return reconcile.Result{RequeueAfter: requeueAfter}, nil
 		}
 		dynakube.Status.SetPhase(dynatracev1beta1.Error)

src/controllers/dynakube/dynakube_controller_test.go

@@ -3,6 +3,7 @@ package dynakube
 import (
 	"context"
 	"fmt"
+	"net/http"
 	"testing"
 
 	dynatracev1beta1 "github.com/Dynatrace/dynatrace-operator/src/api/v1beta1"
@@ -990,6 +991,49 @@ func TestTokenConditions(t *testing.T) {
 	})
 }
 
+func TestAPIError(t *testing.T) {
+	mockClient := createDTMockClient(dtclient.TokenScopes{dtclient.TokenScopeInstallerDownload},
+		dtclient.TokenScopes{dtclient.TokenScopeDataExport, dtclient.TokenScopeActiveGateTokenCreate},
+	)
+	instance := &dynatracev1beta1.DynaKube{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      testName,
+			Namespace: testNamespace,
+		},
+		Spec: dynatracev1beta1.DynaKubeSpec{
+			APIURL:   testApiUrl,
+			OneAgent: dynatracev1beta1.OneAgentSpec{CloudNativeFullStack: &dynatracev1beta1.CloudNativeFullStackSpec{HostInjectSpec: dynatracev1beta1.HostInjectSpec{}}},
+			ActiveGate: dynatracev1beta1.ActiveGateSpec{
+				Capabilities: []dynatracev1beta1.CapabilityDisplayName{
+					dynatracev1beta1.KubeMonCapability.DisplayName,
+				},
+			},
+		},
+	}
+	t.Run("should return error result on 503", func(t *testing.T) {
+		mockClient.On("GetActiveGateAuthToken", testName).Return(&dtclient.ActiveGateAuthTokenInfo{}, dtclient.ServerError{Code: http.StatusServiceUnavailable, Message: "Service unavailable"})
+		controller := createFakeClientAndReconciler(mockClient, instance, testPaasToken, testAPIToken)
+
+		result, err := controller.Reconcile(context.TODO(), reconcile.Request{
+			NamespacedName: types.NamespacedName{Namespace: testNamespace, Name: testName},
+		})
+
+		assert.NoError(t, err)
+		assert.Equal(t, errorUpdateInterval, result.RequeueAfter)
+	})
+	t.Run("should return error result on 429", func(t *testing.T) {
+		mockClient.On("GetActiveGateAuthToken", testName).Return(&dtclient.ActiveGateAuthTokenInfo{}, dtclient.ServerError{Code: http.StatusTooManyRequests, Message: "Too many requests"})
+		controller := createFakeClientAndReconciler(mockClient, instance, testPaasToken, testAPIToken)
+
+		result, err := controller.Reconcile(context.TODO(), reconcile.Request{
+			NamespacedName: types.NamespacedName{Namespace: testNamespace, Name: testName},
+		})
+
+		assert.NoError(t, err)
+		assert.Equal(t, errorUpdateInterval, result.RequeueAfter)
+	})
+}
+
 func assertCondition(t *testing.T, dk *dynatracev1beta1.DynaKube, expectedConditionType string, expectedConditionStatus metav1.ConditionStatus, expectedReason string, expectedMessage string) { //nolint:revive // argument-limit
 	t.Helper()
 

src/controllers/dynakube/oneagent/oneagent_reconciler_test.go

@@ -12,7 +12,6 @@ import (
 	"github.com/Dynatrace/dynatrace-operator/src/scheme"
 	"github.com/Dynatrace/dynatrace-operator/src/scheme/fake"
 	"github.com/Dynatrace/dynatrace-operator/src/version"
-	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
@@ -121,21 +120,6 @@ func TestReconcile_PhaseSetCorrectly(t *testing.T) {
 		Reason:  dynatracev1beta1.ReasonTokenReady,
 		Message: "Ready",
 	})
-
-	t.Run("SetPhaseOnError called with different values, object and return value correctly modified", func(t *testing.T) {
-		dk := base.DeepCopy()
-
-		res := dk.Status.SetPhaseOnError(nil)
-		assert.False(t, res)
-		assert.Equal(t, dk.Status.Phase, dynatracev1beta1.DynaKubePhaseType(""))
-
-		res = dk.Status.SetPhaseOnError(errors.New("dummy error"))
-		assert.True(t, res)
-
-		if assert.NotNil(t, dk.Status.Phase) {
-			assert.Equal(t, dynatracev1beta1.Error, dk.Status.Phase)
-		}
-	})
 }
 
 func TestReconcile_InstancesSet(t *testing.T) {

src/controllers/dynakube/version/activegate.go

@@ -64,5 +64,5 @@ func (updater *activeGateUpdater) CheckForDowngrade(latestVersion string) (bool,
 
 func (updater *activeGateUpdater) UseTenantRegistry(ctx context.Context, dockerCfg *dockerconfig.DockerConfig) error {
 	defaultImage := updater.dynakube.DefaultActiveGateImage()
-	return updateVersionStatusForTenantRegistry(ctx, updater.Target(), defaultImage, updater.versionFunc, dockerCfg)
+	return updateVersionStatusForTenantRegistry(ctx, updater.Target(), defaultImage, updater.versionFunc, dockerCfg, updater.dynakube)
 }

src/controllers/dynakube/version/oneagent.go

@@ -74,7 +74,7 @@ func (updater *oneAgentUpdater) UseTenantRegistry(ctx context.Context, dockerCfg
 	}
 
 	defaultImage := updater.dynakube.DefaultOneAgentImage()
-	return updateVersionStatusForTenantRegistry(ctx, updater.Target(), defaultImage, updater.versionFunc, dockerCfg)
+	return updateVersionStatusForTenantRegistry(ctx, updater.Target(), defaultImage, updater.versionFunc, dockerCfg, updater.dynakube)
 }
 
 func (updater *oneAgentUpdater) CheckForDowngrade(latestVersion string) (bool, error) {

src/controllers/dynakube/version/synthetic.go

@@ -65,5 +65,5 @@ func (updater syntheticUpdater) CheckForDowngrade(latestVersion string) (bool, e
 
 func (updater *syntheticUpdater) UseTenantRegistry(ctx context.Context, dockerCfg *dockerconfig.DockerConfig) error {
 	defaultImage := updater.dynakube.DefaultSyntheticImage()
-	return updateVersionStatusForTenantRegistry(ctx, updater.Target(), defaultImage, updater.versionFunc, dockerCfg)
+	return updateVersionStatusForTenantRegistry(ctx, updater.Target(), defaultImage, updater.versionFunc, dockerCfg, updater.dynakube)
 }

src/controllers/dynakube/version/updater.go

@@ -40,7 +40,7 @@ func (reconciler *Reconciler) run(ctx context.Context, updater versionStatusUpda
 	customImage := updater.CustomImage()
 	if customImage != "" {
 		log.Info("updating version status according to custom image", "updater", updater.Name())
-		err = setImageIDWithDigest(ctx, updater.Target(), customImage, reconciler.versionFunc, dockerCfg)
+		err = setImageIDWithDigest(ctx, updater.Target(), customImage, reconciler.versionFunc, dockerCfg, reconciler.dynakube)
 		return err
 	}
 
@@ -68,7 +68,7 @@ func (reconciler *Reconciler) run(ctx context.Context, updater versionStatusUpda
 			return err
 		}
 
-		err = setImageIDWithDigest(ctx, updater.Target(), publicImage.String(), reconciler.versionFunc, dockerCfg)
+		err = setImageIDWithDigest(ctx, updater.Target(), publicImage.String(), reconciler.versionFunc, dockerCfg, reconciler.dynakube)
 		if err != nil {
 			log.Info("could not update version status according to the public registry", "updater", updater.Name())
 			return err
@@ -94,12 +94,13 @@ func determineSource(updater versionStatusUpdater) dynatracev1beta1.VersionSourc
 	return dynatracev1beta1.TenantRegistryVersionSource
 }
 
-func setImageIDWithDigest(
+func setImageIDWithDigest( //nolint:revive
 	ctx context.Context,
 	target *dynatracev1beta1.VersionStatus,
 	imageUri string,
 	imageVersionFunc ImageVersionFunc,
 	dockerCfg *dockerconfig.DockerConfig,
+	dynakube *dynatracev1beta1.DynaKube,
 ) error {
 	ref, err := reference.Parse(imageUri)
 	if err != nil {
@@ -115,8 +116,12 @@ func setImageIDWithDigest(
 	} else if taggedRef, ok := ref.(reference.NamedTagged); ok {
 		imageVersion, err := imageVersionFunc(ctx, imageUri, dockerCfg)
 		if err != nil {
+			if !dynakube.HasProxy() {
+				log.Info("failed to determine image version")
+				return err
+			}
 			target.ImageID = taggedRef.String()
-			log.Error(err, "failed to get image digest, falling back to tag")
+			log.Info("failed to determine image version because of proxy, falling back to tag")
 		} else {
 			canonRef, err := reference.WithDigest(taggedRef, imageVersion.Digest)
 			if err != nil {
@@ -139,12 +144,13 @@ func setImageIDWithDigest(
 	return nil
 }
 
-func updateVersionStatusForTenantRegistry(
+func updateVersionStatusForTenantRegistry( //nolint:revive
 	ctx context.Context,
 	target *dynatracev1beta1.VersionStatus,
 	imageUri string,
 	imageVersionFunc ImageVersionFunc,
 	dockerCfg *dockerconfig.DockerConfig,
+	dynakube *dynatracev1beta1.DynaKube,
 ) error {
 	ref, err := reference.Parse(imageUri)
 	if err != nil {
@@ -159,7 +165,11 @@ func updateVersionStatusForTenantRegistry(
 	if taggedRef, ok := ref.(reference.NamedTagged); ok {
 		imageVersion, err := imageVersionFunc(ctx, imageUri, dockerCfg)
 		if err != nil {
-			log.Error(err, "failed to determine image version, ignoring version")
+			if !dynakube.HasProxy() {
+				log.Info("failed to determine image version")
+				return err
+			}
+			log.Info("failed to determine image version because of proxy, ignoring version")
 		}
 		target.ImageID = taggedRef.String()
 		target.Version = imageVersion.Version

src/controllers/dynakube/version/updater_test.go

@@ -297,12 +297,20 @@ func TestUpdateVersionStatus(t *testing.T) {
 	}
 	testDockerCfg := &dockerconfig.DockerConfig{}
 
-	t.Run("failing to get digest should not cause error, should fall back to using the tag", func(t *testing.T) {
+	t.Run("failing to get digest should cause error", func(t *testing.T) {
 		registry := newEmptyFakeRegistry()
 		target := dynatracev1beta1.VersionStatus{}
-		err := setImageIDWithDigest(ctx, &target, testImage.String(), registry.ImageVersionExt, testDockerCfg)
+		err := setImageIDWithDigest(ctx, &target, testImage.String(), registry.ImageVersionExt, testDockerCfg, newClassicFullStackDynakube())
+		assert.Error(t, err)
+	})
+
+	t.Run("failing to get digest should not cause error if proxy is set", func(t *testing.T) {
+		registry := newEmptyFakeRegistry()
+		target := dynatracev1beta1.VersionStatus{}
+		dynakube := newClassicFullStackDynakube()
+		dynakube.Spec.Proxy = &dynatracev1beta1.DynaKubeProxy{Value: "http://username:password@host:port"}
+		err := setImageIDWithDigest(ctx, &target, testImage.String(), registry.ImageVersionExt, testDockerCfg, dynakube)
 		assert.NoError(t, err)
-		assert.Equal(t, testImage.String(), target.ImageID)
 	})
 
 	t.Run("set status", func(t *testing.T) {
@@ -312,7 +320,7 @@ func TestUpdateVersionStatus(t *testing.T) {
 			},
 		})
 		target := dynatracev1beta1.VersionStatus{}
-		err := setImageIDWithDigest(ctx, &target, testImage.String(), registry.ImageVersionExt, testDockerCfg)
+		err := setImageIDWithDigest(ctx, &target, testImage.String(), registry.ImageVersionExt, testDockerCfg, newClassicFullStackDynakube())
 		require.NoError(t, err)
 		assertVersionStatusEquals(t, registry, getTaggedReference(t, testImage.String()), target)
 	})
@@ -326,7 +334,7 @@ func TestUpdateVersionStatus(t *testing.T) {
 			t.Error("digest function was called unexpectedly")
 			return ImageVersion{}, nil
 		}
-		err := setImageIDWithDigest(ctx, &target, expectedID, boomFunc, testDockerCfg)
+		err := setImageIDWithDigest(ctx, &target, expectedID, boomFunc, testDockerCfg, newClassicFullStackDynakube())
 		require.NoError(t, err)
 		assert.Equal(t, expectedID, target.ImageID)
 	})
@@ -339,7 +347,7 @@ func TestUpdateVersionStatus(t *testing.T) {
 			t.Error("digest function was called unexpectedly")
 			return ImageVersion{}, nil
 		}
-		err := setImageIDWithDigest(ctx, &target, expectedID, boomFunc, testDockerCfg)
+		err := setImageIDWithDigest(ctx, &target, expectedID, boomFunc, testDockerCfg, newClassicFullStackDynakube())
 		require.NoError(t, err)
 		assert.Equal(t, expectedID, target.ImageID)
 	})