[Bugfix] Fix 3.6 -> 3.7 upgrade (#910)

Author: ajanikow

CHANGELOG.md

@@ -10,6 +10,8 @@
 - (Cleanup) Reorganize main reconciliation context
 - (Bugfix) Unreachable condition
 - (Feature) Allow to disable external port (sidecar managed connection)
+- (Bugfix) Fix 3.6 -> 3.7 Upgrade procedure
+- (Bugfix) Add missing finalizer
 
 ## [1.2.7](https://github.com/arangodb/kube-arangodb/tree/1.2.7) (2022-01-17)
 - Add Plan BackOff functionality

pkg/deployment/context_impl.go

@@ -289,45 +289,66 @@ func (d *Deployment) getAuth() (driver.Authentication, error) {
 	}
 
 	var secret string
-	if i := d.apiObject.Status.CurrentImage; i == nil || !features.JWTRotation().Supported(i.ArangoDBVersion, i.Enterprise) {
-		s, err := d.GetCachedStatus().SecretReadInterface().Get(context.Background(), d.apiObject.Spec.Authentication.GetJWTSecretName(), meta.GetOptions{})
-		if err != nil {
-			return nil, errors.Newf("JWT Secret is missing")
-		}
+	var found bool
 
-		jwt, ok := s.Data[constants.SecretKeyToken]
-		if !ok {
-			return nil, errors.Newf("JWT Secret is invalid")
-		}
+	// Check if we can find token in folder
+	if i := d.apiObject.Status.CurrentImage; i == nil || features.JWTRotation().Supported(i.ArangoDBVersion, i.Enterprise) {
+		secret, found = d.getJWTFolderToken()
+	}
+
+	// Fallback to token
+	if !found {
+		secret, found = d.getJWTToken()
+	}
 
-		secret = string(jwt)
-	} else {
+	if !found {
+		return nil, errors.Newf("JWT Secret is invalid")
+	}
+
+	jwt, err := jwt.CreateArangodJwtAuthorizationHeader(secret, "kube-arangodb")
+	if err != nil {
+		return nil, errors.WithStack(err)
+	}
+
+	return driver.RawAuthentication(jwt), nil
+}
+
+func (d *Deployment) getJWTFolderToken() (string, bool) {
+	if i := d.apiObject.Status.CurrentImage; i == nil || features.JWTRotation().Supported(i.ArangoDBVersion, i.Enterprise) {
 		s, err := d.GetCachedStatus().SecretReadInterface().Get(context.Background(), pod.JWTSecretFolder(d.GetName()), meta.GetOptions{})
 		if err != nil {
 			d.deps.Log.Error().Err(err).Msgf("Unable to get secret")
-			return nil, errors.Newf("JWT Folder Secret is missing")
+			return "", false
 		}
 
 		if len(s.Data) == 0 {
-			return nil, errors.Newf("JWT Folder Secret is empty")
+			return "", false
 		}
 
 		if q, ok := s.Data[pod.ActiveJWTKey]; ok {
-			secret = string(q)
+			return string(q), true
 		} else {
 			for _, q := range s.Data {
-				secret = string(q)
-				break
+				return string(q), true
 			}
 		}
 	}
 
-	jwt, err := jwt.CreateArangodJwtAuthorizationHeader(secret, "kube-arangodb")
+	return "", false
+}
+
+func (d *Deployment) getJWTToken() (string, bool) {
+	s, err := d.GetCachedStatus().SecretReadInterface().Get(context.Background(), d.apiObject.Spec.Authentication.GetJWTSecretName(), meta.GetOptions{})
 	if err != nil {
-		return nil, errors.WithStack(err)
+		return "", false
 	}
 
-	return driver.RawAuthentication(jwt), nil
+	jwt, ok := s.Data[constants.SecretKeyToken]
+	if !ok {
+		return "", false
+	}
+
+	return string(jwt), true
 }
 
 // GetSyncServerClient returns a cached client for a specific arangosync server.

pkg/deployment/deployment.go

@@ -474,13 +474,17 @@ func (d *Deployment) updateCRStatus(ctx context.Context, force ...bool) error {
 	attempt := 0
 	for {
 		attempt++
+		q := patch.NewPatch(patch.ItemReplace(patch.NewPath("status"), d.status.last))
+
 		if d.apiObject.GetDeletionTimestamp() == nil {
-			ensureFinalizers(d.apiObject)
+			if ensureFinalizers(d.apiObject) {
+				q.ItemAdd(patch.NewPath("metadata", "finalizers"), d.apiObject.Finalizers)
+			}
 		}
 
 		var newAPIObject *api.ArangoDeployment
 		err := globals.GetGlobalTimeouts().Kubernetes().RunWithTimeout(ctx, func(ctxChild context.Context) error {
-			p, err := patch.NewPatch(patch.ItemReplace(patch.NewPath("status"), d.status.last)).Marshal()
+			p, err := q.Marshal()
 			if err != nil {
 				return err
 			}

pkg/deployment/deployment_finalizers.go

@@ -39,15 +39,17 @@ import (
 )
 
 // ensureFinalizers adds all required finalizers to the given deployment (in memory).
-func ensureFinalizers(depl *api.ArangoDeployment) {
+func ensureFinalizers(depl *api.ArangoDeployment) bool {
 	for _, f := range depl.GetFinalizers() {
 		if f == constants.FinalizerDeplRemoveChildFinalizers {
 			// Finalizer already set
-			return
+			return false
 		}
 	}
 	// Set finalizers
 	depl.SetFinalizers(append(depl.GetFinalizers(), constants.FinalizerDeplRemoveChildFinalizers))
+
+	return true
 }
 
 // runDeploymentFinalizers goes through the list of ArangoDeployoment finalizers to see if they can be removed.

pkg/deployment/member/state.go

@@ -24,6 +24,7 @@ import (
 	"context"
 	"sync"
 
+	"github.com/arangodb/go-driver"
 	api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1"
 	"github.com/arangodb/kube-arangodb/pkg/deployment/reconciler"
 	"github.com/arangodb/kube-arangodb/pkg/util/globals"
@@ -80,9 +81,11 @@ func (s *stateInspector) RefreshState(ctx context.Context, members api.Deploymen
 			return
 		}
 
-		if _, err := c.Version(nctx); err != nil {
+		if v, err := c.Version(nctx); err != nil {
 			results[id].Reachable = err
 			return
+		} else {
+			results[id].Version = v
 		}
 	})
 
@@ -110,6 +113,8 @@ func (s *stateInspector) MemberState(id string) (State, bool) {
 
 type State struct {
 	Reachable error
+
+	Version driver.VersionInfo
 }
 
 func (s State) IsReachable() bool {

pkg/deployment/reconcile/action_arango_member_update_pod_spec.go

@@ -28,6 +28,7 @@ import (
 	"github.com/rs/zerolog/log"
 
 	api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1"
+	"github.com/arangodb/kube-arangodb/pkg/util"
 	"github.com/rs/zerolog"
 )
 
@@ -86,6 +87,7 @@ func (a *actionArangoMemberUpdatePodSpec) Start(ctx context.Context) (bool, erro
 
 	if m.Endpoint == nil || *m.Endpoint != endpoint {
 		// Update endpoint
+		m.Endpoint = util.NewString(endpoint)
 		if err := status.Members.Update(m, a.action.Group); err != nil {
 			log.Error().Err(err).Msg("Unable to update endpoint")
 			return false, err

pkg/deployment/reconcile/plan_builder_rotate_upgrade.go

@@ -23,13 +23,13 @@ package reconcile
 import (
 	"context"
 
-	"github.com/arangodb/go-driver"
 	"github.com/arangodb/kube-arangodb/pkg/deployment/rotation"
 
 	"github.com/arangodb/kube-arangodb/pkg/deployment/features"
 
 	"github.com/arangodb/kube-arangodb/pkg/deployment/resources"
 
+	"github.com/arangodb/go-driver"
 	upgraderules "github.com/arangodb/go-upgrade-rules"
 	"github.com/arangodb/kube-arangodb/pkg/apis/deployment"
 	api "github.com/arangodb/kube-arangodb/pkg/apis/deployment/v1"
@@ -119,6 +119,26 @@ func createRotateOrUpgradePlanInternal(log zerolog.Logger, apiObject k8sutil.API
 	decision := createRotateOrUpgradeDecision(log, spec, status, context)
 
 	if decision.IsUpgrade() {
+
+		for _, m := range status.Members.AsList() {
+			// Pre-check
+			d := decision[m.Member.ID]
+			if !d.upgrade {
+				continue
+			}
+
+			// We have member to upgrade
+			if d.upgradeDecision.Hold {
+				// Holding upgrade
+				continue
+			}
+
+			if !d.upgradeDecision.UpgradeAllowed {
+				context.CreateEvent(k8sutil.NewUpgradeNotAllowedEvent(apiObject, d.upgradeDecision.FromVersion, d.upgradeDecision.ToVersion, d.upgradeDecision.FromLicense, d.upgradeDecision.ToLicense))
+				return nil, false
+			}
+		}
+
 		// Upgrade phase
 		// During upgrade always get first member which needs to be upgraded
 		for _, m := range status.Members.AsList() {
@@ -175,7 +195,7 @@ func createRotateOrUpgradePlanInternal(log zerolog.Logger, apiObject k8sutil.API
 			}
 
 			if m.Member.Conditions.IsTrue(api.ConditionTypeRestart) {
-				return createRotateMemberPlan(log, m.Member, m.Group, "Restart flag present"), false
+				return createRotateMemberPlan(log, m.Member, m.Group, spec, "Restart flag present"), false
 			}
 			arangoMember, ok := cachedStatus.ArangoMember(m.Member.ArangoMemberName(apiObject.GetName(), m.Group))
 			if !ok {
@@ -402,18 +422,15 @@ func createUpgradeMemberPlan(log zerolog.Logger, member api.MemberStatus,
 		Str("reason", reason).
 		Str("action", string(upgradeAction)).
 		Msg("Creating upgrade plan")
-	var plan = api.Plan{
-		api.NewAction(api.ActionTypeCleanTLSKeyfileCertificate, group, member.ID, "Remove server keyfile and enforce renewal/recreation"),
+
+	plan := createRotateMemberPlanWithAction(member, group, upgradeAction, spec, reason)
+
+	if member.Image == nil || member.Image.Image != spec.GetImage() {
+		plan = plan.Before(api.NewAction(api.ActionTypeSetMemberCurrentImage, group, member.ID, reason).SetImage(spec.GetImage()))
 	}
 	if status.CurrentImage == nil || status.CurrentImage.Image != spec.GetImage() {
-		plan = plan.After(api.NewAction(api.ActionTypeSetCurrentImage, group, "", reason).SetImage(spec.GetImage()))
+		plan = plan.Before(api.NewAction(api.ActionTypeSetCurrentImage, group, "", reason).SetImage(spec.GetImage()))
 	}
-	if member.Image == nil || member.Image.Image != spec.GetImage() {
-		plan = plan.After(api.NewAction(api.ActionTypeSetMemberCurrentImage, group, member.ID, reason).SetImage(spec.GetImage()))
-	}
-
-	plan = plan.After(api.NewAction(upgradeAction, group, member.ID, reason),
-		api.NewAction(api.ActionTypeWaitForMemberUp, group, member.ID))
 
 	return withSecureWrap(member, group, spec, plan...)
 }

pkg/deployment/reconcile/plan_builder_utils.go

@@ -34,20 +34,32 @@ import (
 // createRotateMemberPlan creates a plan to rotate (stop-recreate-start) an existing
 // member.
 func createRotateMemberPlan(log zerolog.Logger, member api.MemberStatus,
-	group api.ServerGroup, reason string) api.Plan {
+	group api.ServerGroup, spec api.DeploymentSpec, reason string) api.Plan {
 	log.Debug().
 		Str("id", member.ID).
 		Str("role", group.AsRole()).
 		Str("reason", reason).
 		Msg("Creating rotation plan")
-	plan := api.Plan{
+	return createRotateMemberPlanWithAction(member, group, api.ActionTypeRotateMember, spec, reason)
+}
+
+// createRotateMemberPlanWithAction creates a plan to rotate (stop-<action>>-start) an existing
+// member.
+func createRotateMemberPlanWithAction(member api.MemberStatus,
+	group api.ServerGroup, action api.ActionType, spec api.DeploymentSpec, reason string) api.Plan {
+
+	var plan = api.Plan{
 		api.NewAction(api.ActionTypeCleanTLSKeyfileCertificate, group, member.ID, "Remove server keyfile and enforce renewal/recreation"),
-		api.NewAction(api.ActionTypeResignLeadership, group, member.ID, reason),
+	}
+	plan = withSecureWrap(member, group, spec, plan...)
+
+	plan = plan.After(
 		withMemberPodUID(member, api.NewAction(api.ActionTypeKillMemberPod, group, member.ID, reason)),
-		withMemberPodUID(member, api.NewAction(api.ActionTypeRotateMember, group, member.ID, reason)),
+		withMemberPodUID(member, api.NewAction(action, group, member.ID, reason)),
 		api.NewAction(api.ActionTypeWaitForMemberUp, group, member.ID),
 		api.NewAction(api.ActionTypeWaitForMemberInSync, group, member.ID),
-	}
+	)
+
 	return plan
 }