Merge pull request #569 from appgate/upgrade-skip-bug

fix: upgrade complete skipping appliances

.goreleaser.yml

@@ -2,6 +2,7 @@
 project_name: sdpctl
 env:
   - SNAPSHOT_VERSION={{ .Now.Format "2006.01.02" }}
+  - SDPCTL_LOG_LEVEL={{ if .IsSnapshot }}debug{{ else }}info{{ end }}
 before:
   hooks:
     - make clean

cmd/appliance/upgrade/complete.go

@@ -198,7 +198,11 @@ func upgradeCompleteRun(cmd *cobra.Command, args []string, opts *upgradeComplete
 	if err != nil {
 		return err
 	}
-	plan, err := appliancepkg.NewUpgradePlan(rawAppliances, *initialStats, controlHost, filter, orderBy, descending)
+	upgradeStatusMap, err := a.UpgradeStatusMap(ctx, rawAppliances)
+	if err != nil {
+		return err
+	}
+	plan, err := appliancepkg.NewUpgradePlan(rawAppliances, initialStats, upgradeStatusMap, controlHost, filter, orderBy, descending)
 	if err != nil {
 		return err
 	}

cmd/appliance/upgrade/complete_test.go

@@ -80,6 +80,22 @@ func TestUpgradeCompleteCommand(t *testing.T) {
 			name:       "test no appliances ready",
 			cli:        "upgrade complete --no-interactive",
 			appliances: []string{appliancepkg.TestApplianceUnpreparedPrimary, appliancepkg.TestApplianceControllerNotPrepared},
+			customStubs: []httpmock.Stub{
+				{
+					URL: "/admin/appliances/{appliance}/upgrade",
+					Responder: func(w http.ResponseWriter, r *http.Request) {
+						us := openapi.NewStatsAppliancesListAllOfUpgradeWithDefaults()
+						us.SetStatus(appliancepkg.UpgradeStatusIdle)
+						body, err := us.MarshalJSON()
+						if err != nil {
+							w.WriteHeader(http.StatusInternalServerError)
+							return
+						}
+						w.Header().Add("Content-Type", "application/json")
+						w.Write(body)
+					},
+				},
+			},
 			from:       "6.2.0",
 			to:         "6.2.1",
 			wantErr:    true,

pkg/appliance/checks.go

@@ -305,7 +305,7 @@ var (
 	ErrControllerVersionMismatch = errors.New("all controllers need to be prepared with the same version when doing a major or minor version upgrade.")
 )
 
-func CheckNeedsMultiControllerUpgrade(stats openapi.StatsAppliancesList, appliances []openapi.Appliance) ([]openapi.Appliance, error) {
+func CheckNeedsMultiControllerUpgrade(stats *openapi.StatsAppliancesList, appliances []openapi.Appliance) ([]openapi.Appliance, error) {
 	var (
 		errs                   *multierror.Error
 		preparedControllers    []openapi.Appliance

pkg/appliance/checks_test.go

@@ -673,7 +673,7 @@ func TestCheckNeedsMultiControllerUpgrade(t *testing.T) {
 				stats.Data = append(stats.Data, d.stat)
 				argAppliances = append(argAppliances, d.appliance)
 			}
-			got, err := CheckNeedsMultiControllerUpgrade(stats, argAppliances)
+			got, err := CheckNeedsMultiControllerUpgrade(&stats, argAppliances)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("CheckNeedsMultiControllerUpgrade() error = %v, wantErr %v", err, tt.wantErr)
 				return

pkg/appliance/mock.go

@@ -147,23 +147,16 @@ var (
 		w.WriteHeader(http.StatusOK)
 		fmt.Fprint(w, string(body))
 	}
-	mutatingResponder = func(callback func(count int) ([]byte, error)) http.HandlerFunc {
+	DefaultResponder = func(callback func(rw http.ResponseWriter, req *http.Request, count int)) http.HandlerFunc {
 		count := 0
 		return func(w http.ResponseWriter, r *http.Request) {
-			mutated, err := callback(count)
-			if err != nil {
-				panic(fmt.Sprintf("Internal testing error; request mutation failed %q", err))
-			}
-			count++
-			w.Header().Set("Content-Type", "application/json")
-			w.WriteHeader(http.StatusOK)
-			fmt.Fprint(w, string(mutated))
+			callback(w, r, count)
 		}
 	}
-	mutatingApplianceResponder = func(id string, callback func(id string, count int) ([]byte, error)) http.HandlerFunc {
+	mutatingResponder = func(callback func(count int) ([]byte, error)) http.HandlerFunc {
 		count := 0
 		return func(w http.ResponseWriter, r *http.Request) {
-			mutated, err := callback(id, count)
+			mutated, err := callback(count)
 			if err != nil {
 				panic(fmt.Sprintf("Internal testing error; request mutation failed %q", err))
 			}
@@ -217,6 +210,32 @@ func (cts *CollectiveTestStruct) GenerateStubs(appliances []openapi.Appliance, s
 	}
 	stubs = append(stubs, statsStub)
 
+	stubs = append(stubs, httpmock.Stub{
+		URL: "/admin/appliances/{appliance}/upgrade",
+		Responder: DefaultResponder(func(rw http.ResponseWriter, req *http.Request, count int) {
+			id := req.PathValue("appliance")
+			for _, s := range stats.GetData() {
+				if s.GetId() != id {
+					continue
+				}
+				us := s.GetUpgrade()
+				if count <= 0 {
+					us.SetStatus(UpgradeStatusReady)
+				} else if count == 1 {
+					us.SetStatus(UpgradeStatusIdle)
+					us.SetDetails("")
+				}
+				body, err := us.MarshalJSON()
+				if err != nil {
+					rw.WriteHeader(http.StatusInternalServerError)
+					return
+				}
+				rw.Header().Set("Content-Type", "application/json")
+				rw.Write(body)
+			}
+		}),
+	})
+
 	// global settings stub
 	globalSettingsStub := httpmock.Stub{
 		URL: "/admin/global-settings",
@@ -245,25 +264,6 @@ func (cts *CollectiveTestStruct) GenerateStubs(appliances []openapi.Appliance, s
 			URL:       fmt.Sprintf("/admin/appliances/%s/maintenance", a.GetId()),
 			Responder: changeRequestResponder,
 		})
-		stubs = append(stubs, httpmock.Stub{
-			URL: fmt.Sprintf("/admin/appliances/%s/upgrade", a.GetId()),
-			Responder: mutatingApplianceResponder(a.GetId(), func(id string, count int) ([]byte, error) {
-				for _, s := range stats.GetData() {
-					if s.GetId() != id {
-						continue
-					}
-					us := s.GetUpgrade()
-					if count <= 0 {
-						us.SetStatus(UpgradeStatusReady)
-					} else if count == 1 {
-						us.SetStatus(UpgradeStatusIdle)
-						us.SetDetails("")
-					}
-					return json.Marshal(us)
-				}
-				return nil, fmt.Errorf("failed to find stats for '%s'", id)
-			}),
-		})
 
 		backupID := uuid.NewString()
 		stubs = append(stubs, httpmock.Stub{

pkg/appliance/upgrade.go

@@ -45,11 +45,11 @@ type UpgradePlan struct {
 	Batches                 [][]openapi.Appliance
 	Skipping                []SkipUpgrade
 	BackupIds               []string
-	stats                   openapi.StatsAppliancesList
+	stats                   *openapi.StatsAppliancesList
 	adminHostname           string
 }
 
-func NewUpgradePlan(appliances []openapi.Appliance, stats openapi.StatsAppliancesList, adminHostname string, filter map[string]map[string]string, orderBy []string, descending bool) (*UpgradePlan, error) {
+func NewUpgradePlan(appliances []openapi.Appliance, stats *openapi.StatsAppliancesList, upgradeStatusMap map[string]UpgradeStatusResult, adminHostname string, filter map[string]map[string]string, orderBy []string, descending bool) (*UpgradePlan, error) {
 	plan := UpgradePlan{
 		adminHostname: adminHostname,
 		stats:         stats,
@@ -103,7 +103,7 @@ func NewUpgradePlan(appliances []openapi.Appliance, stats openapi.StatsAppliance
 	var errs *multierror.Error
 	for _, a := range finalApplianceList {
 		// Get current version and stats
-		stats, err := ApplianceStats(a, plan.stats)
+		stats, err := ApplianceStats(&a, plan.stats)
 		if err != nil {
 			errs = multierror.Append(errs, err)
 			plan.Skipping = append(plan.Skipping, SkipUpgrade{
@@ -123,15 +123,21 @@ func NewUpgradePlan(appliances []openapi.Appliance, stats openapi.StatsAppliance
 		}
 
 		// Get upgrade status and target version
-		upgradeStatus := stats.GetUpgrade()
-		if status, ok := upgradeStatus.GetStatusOk(); ok && *status != UpgradeStatusReady {
+		upgradeStatus, ok := upgradeStatusMap[a.GetId()]
+		if !ok {
+			plan.Skipping = append(plan.Skipping, SkipUpgrade{
+				Appliance: a,
+				Reason:    ErrNoApplianceStats,
+			})
+		}
+		if upgradeStatus.Status != UpgradeStatusReady {
 			plan.Skipping = append(plan.Skipping, SkipUpgrade{
 				Appliance: a,
 				Reason:    ErrSkipReasonNotPrepared,
 			})
 			continue
 		}
-		targetVersion, err := ParseVersionString(upgradeStatus.GetDetails())
+		targetVersion, err := ParseVersionString(upgradeStatus.Details)
 		if err != nil {
 			errs = multierror.Append(errs, err)
 			plan.Skipping = append(plan.Skipping, SkipUpgrade{
@@ -330,7 +336,7 @@ func (up *UpgradePlan) PrintPreCompleteSummary(out io.Writer) error {
 		t.AddHeader("Appliance", "Site", "Current version", "Prepared version", "Backup")
 	}
 	if up.PrimaryController != nil {
-		currentVersion, targetVersion := applianceVersions(*up.PrimaryController, up.stats)
+		currentVersion, targetVersion := applianceVersions(*up.PrimaryController, *up.stats)
 		tb := &bytes.Buffer{}
 		t := util.NewPrinter(tb, 4)
 		tableHeaders(t)
@@ -349,7 +355,7 @@ func (up *UpgradePlan) PrintPreCompleteSummary(out io.Writer) error {
 		t := util.NewPrinter(tb, 4)
 		tableHeaders(t)
 		for _, ctrl := range up.Controllers {
-			current, target := applianceVersions(ctrl, up.stats)
+			current, target := applianceVersions(ctrl, *up.stats)
 			t.AddLine(ctrl.GetName(), ctrl.GetSiteName(), current, target, shouldBackup(ctrl.GetId()))
 		}
 		t.Print()
@@ -364,7 +370,7 @@ func (up *UpgradePlan) PrintPreCompleteSummary(out io.Writer) error {
 		t := util.NewPrinter(tb, 4)
 		tableHeaders(t)
 		for _, lfls := range up.LogForwardersAndServers {
-			current, target := applianceVersions(lfls, up.stats)
+			current, target := applianceVersions(lfls, *up.stats)
 			t.AddLine(lfls.GetName(), lfls.GetSiteName(), current, target, shouldBackup(lfls.GetId()))
 		}
 		t.Print()
@@ -378,7 +384,7 @@ func (up *UpgradePlan) PrintPreCompleteSummary(out io.Writer) error {
 			t := util.NewPrinter(tb, 4)
 			tableHeaders(t)
 			for _, a := range c {
-				current, target := applianceVersions(a, up.stats)
+				current, target := applianceVersions(a, *up.stats)
 				// s := fmt.Sprintf("- %s: %s -> %s", a.GetName(), current, target)
 				t.AddLine(a.GetName(), a.GetSiteName(), current, target, shouldBackup(a.GetId()))
 			}
@@ -447,14 +453,14 @@ func (up *UpgradePlan) PrintPostCompleteSummary(out io.Writer, stats []openapi.S
 }
 
 func applianceVersions(a openapi.Appliance, s openapi.StatsAppliancesList) (currentVersion *version.Version, targetVersion *version.Version) {
-	stats, _ := ApplianceStats(a, s)
+	stats, _ := ApplianceStats(&a, &s)
 	currentVersion, _ = ParseVersionString(stats.GetVersion())
 	us := stats.GetUpgrade()
 	targetVersion, _ = ParseVersionString(us.GetDetails())
 	return
 }
 
-func ApplianceStats(a openapi.Appliance, stats openapi.StatsAppliancesList) (*openapi.StatsAppliancesListAllOfData, error) {
+func ApplianceStats(a *openapi.Appliance, stats *openapi.StatsAppliancesList) (*openapi.StatsAppliancesListAllOfData, error) {
 	for _, s := range stats.GetData() {
 		if s.GetId() == a.GetId() {
 			return &s, nil

pkg/appliance/upgrade_test.go

@@ -64,7 +64,7 @@ func TestMakeUpgradePlan(t *testing.T) {
 					{coll.Appliances["connectorA1"], coll.Appliances["gatewayA3"], coll.Appliances["logserver"], coll.Appliances["portalA1"]},
 				},
 				adminHostname: hostname,
-				stats:         *coll.Stats,
+				stats:         coll.Stats,
 			},
 		},
 		{
@@ -101,7 +101,7 @@ func TestMakeUpgradePlan(t *testing.T) {
 					{coll.Appliances["connectorA1"], coll.Appliances["gatewayA3"], coll.Appliances["logserver"], coll.Appliances["portalA1"]},
 				},
 				adminHostname: hostname,
-				stats:         *coll.Stats,
+				stats:         coll.Stats,
 			},
 		},
 		{
@@ -163,7 +163,21 @@ func TestMakeUpgradePlan(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := NewUpgradePlan(tt.args.appliances, *tt.args.stats, tt.args.ctrlHostname, tt.args.filter, tt.args.orderBy, tt.args.descending)
+			upgradeStatusMap := map[string]UpgradeStatusResult{}
+			for _, a := range tt.args.appliances {
+				for _, s := range tt.args.stats.GetData() {
+					if a.GetId() != s.GetId() {
+						continue
+					}
+					us := s.GetUpgrade()
+					upgradeStatusMap[a.GetId()] = UpgradeStatusResult{
+						Name:    a.GetName(),
+						Status:  us.GetStatus(),
+						Details: us.GetDetails(),
+					}
+				}
+			}
+			got, err := NewUpgradePlan(tt.args.appliances, tt.args.stats, upgradeStatusMap, tt.args.ctrlHostname, tt.args.filter, tt.args.orderBy, tt.args.descending)
 			if tt.wantErr {
 				assert.Error(t, err)
 			}
@@ -339,10 +353,27 @@ Appliances that will be skipped:
 		t.Run(tt.name, func(t *testing.T) {
 			coll := GenerateCollective(t, tt.in.hostname, tt.in.from, tt.in.to, tt.in.Appliances)
 			appliances := make([]openapi.Appliance, 0, len(tt.in.Appliances))
+			upgradeStatusMap := map[string]UpgradeStatusResult{}
 			for _, v := range tt.in.Appliances {
-				appliances = append(appliances, coll.Appliances[v])
+				appliance, ok := coll.Appliances[v]
+				if !ok {
+					t.Fatalf("internal testing error: appliance name does not match any appliance")
+					return
+				}
+				for _, stat := range coll.Stats.GetData() {
+					if stat.GetId() != appliance.GetId() {
+						continue
+					}
+					us := stat.GetUpgrade()
+					upgradeStatusMap[appliance.GetId()] = UpgradeStatusResult{
+						Name:    appliance.GetName(),
+						Status:  us.GetStatus(),
+						Details: us.GetDetails(),
+					}
+				}
+				appliances = append(appliances, appliance)
 			}
-			up, err := NewUpgradePlan(appliances, *coll.Stats, tt.in.hostname, tt.in.filter, tt.in.orderBy, tt.in.descending)
+			up, err := NewUpgradePlan(appliances, coll.Stats, upgradeStatusMap, tt.in.hostname, tt.in.filter, tt.in.orderBy, tt.in.descending)
 			if err != nil {
 				t.Fatalf("internal test error: %v", err)
 			}
@@ -417,10 +448,23 @@ WARNING: Upgrade was completed, but not all appliances are running the same vers
 			hostname := "appgate.test"
 			coll := GenerateCollective(t, hostname, tt.from, tt.to, tt.appliances)
 			appliances := make([]openapi.Appliance, 0, len(tt.appliances))
+			upgradeStatusMap := map[string]UpgradeStatusResult{}
 			for _, v := range tt.appliances {
-				appliances = append(appliances, coll.Appliances[v])
+				a := coll.Appliances[v]
+				for _, s := range coll.Stats.GetData() {
+					if s.GetId() != a.GetId() {
+						continue
+					}
+					us := s.GetUpgrade()
+					upgradeStatusMap[a.GetId()] = UpgradeStatusResult{
+						Name:    a.GetName(),
+						Status:  us.GetStatus(),
+						Details: us.GetDetails(),
+					}
+				}
+				appliances = append(appliances, a)
 			}
-			up, err := NewUpgradePlan(appliances, *coll.Stats, hostname, DefaultCommandFilter, nil, false)
+			up, err := NewUpgradePlan(appliances, coll.Stats, upgradeStatusMap, hostname, DefaultCommandFilter, nil, false)
 			if err != nil {
 				t.Fatalf("PrintPostCompleteSummary() - internal test error: %v", err)
 				return