fix(upgrade): validate current release isn't required if it has failed (#3253)

* fix(upgrade): validate current required failed releases

* Update pkg-new/validation/errors.go

Co-authored-by: Alex Parker <7272359+ajp-io@users.noreply.github.com>

---------

Co-authored-by: Alex Parker <7272359+ajp-io@users.noreply.github.com>

Author: JGAntunes

cmd/installer/cli/upgrade.go

@@ -655,6 +655,7 @@ func validateIsReleaseUpgradable(ctx context.Context, upgradeConfig *upgradeConf
 	if upgradeConfig.currentAppVersion != nil {
 		opts.CurrentAppVersion = upgradeConfig.currentAppVersion.VersionLabel
 		opts.CurrentAppSequence = upgradeConfig.currentAppVersion.ChannelSequence
+		opts.CurrentAppStatus = upgradeConfig.currentAppVersion.Status
 	}
 
 	// Add target app version info

pkg-new/replicatedapi/client.go

@@ -177,12 +177,12 @@ func basicAuth(username, password string) string {
 }
 
 // GetPendingReleases fetches pending releases from the Replicated API
-func (c *client) GetPendingReleases(ctx context.Context, channelID string, currentSequence int64, opts *PendingReleasesOptions) (*PendingReleasesResponse, error) {
+func (c *client) GetPendingReleases(ctx context.Context, channelID string, channelSequence int64, opts *PendingReleasesOptions) (*PendingReleasesResponse, error) {
 	u := fmt.Sprintf("%s/release/%s/pending", c.replicatedAppURL, c.license.Spec.AppSlug)
 
 	params := url.Values{}
 	params.Set("selectedChannelId", channelID)
-	params.Set("channelSequence", fmt.Sprintf("%d", currentSequence))
+	params.Set("channelSequence", fmt.Sprintf("%d", channelSequence))
 	params.Set("isSemverSupported", fmt.Sprintf("%t", opts.IsSemverSupported))
 	if opts.SortOrder != "" {
 		params.Set("sortOrder", string(opts.SortOrder))
@@ -196,6 +196,11 @@ func (c *client) GetPendingReleases(ctx context.Context, channelID string, curre
 
 	req.Header.Set("Accept", "application/json")
 
+	// If we provide the current channel sequence, set it as an header since that's the way market API currently expects it
+	if opts.CurrentChannelSequence != 0 {
+		req.Header.Set("X-Replicated-CurrentChannelSequence", fmt.Sprintf("%d", opts.CurrentChannelSequence))
+	}
+
 	resp, err := c.httpClient.Do(req)
 	if err != nil {
 		return nil, fmt.Errorf("execute request: %w", err)

pkg-new/replicatedapi/types.go

@@ -15,8 +15,9 @@ const SortOrderDescending SortOrder = "desc"
 
 // PendingReleasesOptions represents options for fetching pending releases
 type PendingReleasesOptions struct {
-	IsSemverSupported bool
-	SortOrder         SortOrder
+	IsSemverSupported      bool
+	SortOrder              SortOrder
+	CurrentChannelSequence int64
 }
 
 // ChannelRelease represents a single release in a channel

pkg-new/validation/errors.go

@@ -26,6 +26,15 @@ func NewRequiredReleasesError(requiredVersions []string, targetVersion string) *
 	}
 }
 
+// NewCurrentReleaseFailedError creates a ValidationError indicating that the current
+// installed release is required and it's in a failed state
+func NewCurrentReleaseFailedError(currentVersion string, targetVersion string) *ValidationError {
+	return &ValidationError{
+		Message: fmt.Sprintf("Cannot upgrade to version %s, because the required version %s did not install successfully. Version %s must be fully installed before upgrading to version %s.",
+			targetVersion, currentVersion, currentVersion, targetVersion),
+	}
+}
+
 // NewAppVersionDowngradeError creates a ValidationError indicating that the target
 // app version is older than the current version
 func NewAppVersionDowngradeError(currentVersion, targetVersion string) *ValidationError {

pkg-new/validation/upgradable.go

@@ -17,14 +17,16 @@ var k8sBuildRegex = regexp.MustCompile(`k8s-(\d+\.\d+)`)
 
 // UpgradableOptions holds configuration for validating release deployability
 type UpgradableOptions struct {
-	CurrentAppVersion  string
-	CurrentAppSequence int64
-	CurrentECVersion   string
-	TargetAppVersion   string
-	TargetAppSequence  int64
-	TargetECVersion    string
-	License            *kotsv1beta1.License
-	requiredReleases   []string
+	CurrentAppVersion        string
+	CurrentAppSequence       int64
+	CurrentECVersion         string
+	CurrentAppStatus         string
+	TargetAppVersion         string
+	TargetAppSequence        int64
+	TargetECVersion          string
+	License                  *kotsv1beta1.License
+	currentReleaseIsRequired bool
+	requiredReleases         []string
 }
 
 // WithAirgapRequiredReleases extracts the required releases from airgap metadata to be used for validation
@@ -42,8 +44,13 @@ func (opts *UpgradableOptions) WithAirgapRequiredReleases(metadata *airgap.Airga
 			if err != nil {
 				return fmt.Errorf("failed to parse airgap spec required release update cursor %s: %w", release.UpdateCursor, err)
 			}
-			// We've hit a release that is less than or equal to the current installed release, we can stop
-			if sequence <= opts.CurrentAppSequence {
+			// We've hit the current app sequence, mark that it's required and return
+			if sequence == opts.CurrentAppSequence {
+				opts.currentReleaseIsRequired = true
+				return nil
+			}
+			// We've hit a release that is less than the current installed release, we can stop
+			if sequence < opts.CurrentAppSequence {
 				return nil
 			}
 			opts.requiredReleases = append(opts.requiredReleases, release.VersionLabel)
@@ -52,17 +59,19 @@ func (opts *UpgradableOptions) WithAirgapRequiredReleases(metadata *airgap.Airga
 	return nil
 }
 
-// WithOnlineRequiredReleases fetches the pending releases from the current app sequence and extracts the required releases until the target app sequence
+// WithOnlineRequiredReleases fetches all the releases from the current app sequence (including it) and extracts the required releases until the target app sequence
 func (opts *UpgradableOptions) WithOnlineRequiredReleases(ctx context.Context, replAPIClient replicatedapi.Client) error {
 	if opts.License == nil {
 		return fmt.Errorf("license is required to check online upgrade required releases")
 	}
+	// We want to get current app sequence inclusive. In oder for us to do that in a way that works for both channel sequences and semver we need to set the CurrentChannelSequence to current app sequence and the channel sequence provided to the method to current app sequence - 1
 	options := &replicatedapi.PendingReleasesOptions{
-		IsSemverSupported: opts.License.Spec.IsSemverRequired,
-		SortOrder:         replicatedapi.SortOrderAscending,
+		IsSemverSupported:      opts.License.Spec.IsSemverRequired,
+		SortOrder:              replicatedapi.SortOrderAscending,
+		CurrentChannelSequence: opts.CurrentAppSequence,
 	}
 	// Get pending releases from the current app sequence in asceding order
-	pendingReleases, err := replAPIClient.GetPendingReleases(ctx, opts.License.Spec.ChannelID, opts.CurrentAppSequence, options)
+	pendingReleases, err := replAPIClient.GetPendingReleases(ctx, opts.License.Spec.ChannelID, opts.CurrentAppSequence-1, options)
 	if err != nil {
 		return fmt.Errorf("failed to get pending releases while checking required releases for upgrade: %w", err)
 	}
@@ -81,7 +90,13 @@ func (opts *UpgradableOptions) handlePendingReleases(pendingReleases []replicate
 			break
 		}
 		if release.IsRequired {
-			opts.requiredReleases = append(opts.requiredReleases, release.VersionLabel)
+			// Mark that the current app release is required if channel sequence matches
+			if release.ChannelSequence == opts.CurrentAppSequence {
+				opts.currentReleaseIsRequired = true
+				// Else append to required releases
+			} else {
+				opts.requiredReleases = append(opts.requiredReleases, release.VersionLabel)
+			}
 		}
 	}
 }
@@ -94,7 +109,7 @@ func ValidateIsReleaseUpgradable(ctx context.Context, opts UpgradableOptions) er
 	}
 
 	// Check 2: Required releases
-	if err := validateRequiredReleases(ctx, opts); err != nil {
+	if err := validateRequiredReleases(opts); err != nil {
 		return err
 	}
 
@@ -111,8 +126,13 @@ func ValidateIsReleaseUpgradable(ctx context.Context, opts UpgradableOptions) er
 	return nil
 }
 
-// validateRequiredReleases checks if any required releases are being skipped
-func validateRequiredReleases(ctx context.Context, opts UpgradableOptions) error {
+// validateRequiredReleases checks if:
+// - any required releases are being skipped
+// - current app status is failed and it's a required version
+func validateRequiredReleases(opts UpgradableOptions) error {
+	if opts.CurrentAppStatus == "failed" && opts.currentReleaseIsRequired {
+		return NewCurrentReleaseFailedError(opts.CurrentAppVersion, opts.TargetAppVersion)
+	}
 	if len(opts.requiredReleases) > 0 {
 		return NewRequiredReleasesError(opts.requiredReleases, opts.TargetAppVersion)
 	}

pkg-new/validation/upgradable_test.go

@@ -64,12 +64,13 @@ func newTestAirgapMetadataWithSequences(releases []airgapReleaseData) *airgap.Ai
 
 func TestWithAirgapRequiredReleases(t *testing.T) {
 	tests := []struct {
-		name             string
-		metadata         *airgap.AirgapMetadata
-		currentSequence  int64
-		expectedReleases []string
-		expectError      bool
-		errorContains    string
+		name                             string
+		metadata                         *airgap.AirgapMetadata
+		currentSequence                  int64
+		expectedReleases                 []string
+		expectedCurrentReleaseIsRequired bool
+		expectError                      bool
+		errorContains                    string
 	}{
 		{
 			name:             "no required releases",
@@ -102,25 +103,36 @@ func TestWithAirgapRequiredReleases(t *testing.T) {
 			expectError:      false,
 		},
 		{
-			name: "stops at current sequence",
+			name: "all releases older than current",
 			metadata: newTestAirgapMetadataWithSequences([]airgapReleaseData{
-				{versionLabel: "1.4.0", updateCursor: "400"},
-				{versionLabel: "1.3.0", updateCursor: "300"},
 				{versionLabel: "1.2.0", updateCursor: "200"},
+				{versionLabel: "1.1.0", updateCursor: "100"},
 			}),
 			currentSequence:  300,
-			expectedReleases: []string{"1.4.0"},
+			expectedReleases: []string{},
 			expectError:      false,
 		},
 		{
-			name: "all releases older than current",
+			name: "current release is required",
+			metadata: newTestAirgapMetadataWithSequences([]airgapReleaseData{
+				{versionLabel: "1.0.0", updateCursor: "300"},
+			}),
+			currentSequence:                  300,
+			expectedReleases:                 []string{},
+			expectError:                      false,
+			expectedCurrentReleaseIsRequired: true,
+		},
+		{
+			name: "current release is required and there's other required releases",
 			metadata: newTestAirgapMetadataWithSequences([]airgapReleaseData{
+				{versionLabel: "1.4.0", updateCursor: "400"},
+				{versionLabel: "1.3.0", updateCursor: "300"},
 				{versionLabel: "1.2.0", updateCursor: "200"},
-				{versionLabel: "1.1.0", updateCursor: "100"},
 			}),
-			currentSequence:  300,
-			expectedReleases: []string{},
-			expectError:      false,
+			currentSequence:                  300,
+			expectedReleases:                 []string{"1.4.0"},
+			expectedCurrentReleaseIsRequired: true,
+			expectError:                      false,
 		},
 		{
 			name:            "nil metadata",
@@ -167,18 +179,20 @@ func TestWithAirgapRequiredReleases(t *testing.T) {
 				} else {
 					assert.Equal(t, tt.expectedReleases, opts.requiredReleases)
 				}
+				assert.Equal(t, tt.expectedCurrentReleaseIsRequired, opts.currentReleaseIsRequired)
 			}
 		})
 	}
 }
 
 func TestHandlePendingReleases(t *testing.T) {
 	tests := []struct {
-		name             string
-		pendingReleases  []replicatedapi.ChannelRelease
-		currentSequence  int64
-		targetSequence   int64
-		expectedReleases []string
+		name                             string
+		pendingReleases                  []replicatedapi.ChannelRelease
+		currentSequence                  int64
+		targetSequence                   int64
+		expectedReleases                 []string
+		expectedCurrentReleaseIsRequired bool
 	}{
 		{
 			name: "no required releases",
@@ -252,6 +266,29 @@ func TestHandlePendingReleases(t *testing.T) {
 			targetSequence:   100,
 			expectedReleases: []string{},
 		},
+		{
+			name: "current sequence is required",
+			pendingReleases: []replicatedapi.ChannelRelease{
+				{ChannelSequence: 99, VersionLabel: "1.0.0", IsRequired: true},
+			},
+			currentSequence:                  99,
+			targetSequence:                   100,
+			expectedReleases:                 []string{},
+			expectedCurrentReleaseIsRequired: true,
+		},
+		{
+			name: "current sequence is required and there's other required releases",
+			pendingReleases: []replicatedapi.ChannelRelease{
+				{ChannelSequence: 99, VersionLabel: "1.0.0", IsRequired: true},
+				{ChannelSequence: 101, VersionLabel: "1.1.0", IsRequired: true},
+				{ChannelSequence: 102, VersionLabel: "1.2.0", IsRequired: false},
+				{ChannelSequence: 103, VersionLabel: "1.3.0", IsRequired: true},
+			},
+			currentSequence:                  99,
+			targetSequence:                   104,
+			expectedReleases:                 []string{"1.1.0", "1.3.0"},
+			expectedCurrentReleaseIsRequired: true,
+		},
 	}
 
 	for _, tt := range tests {
@@ -268,6 +305,7 @@ func TestHandlePendingReleases(t *testing.T) {
 			} else {
 				assert.Equal(t, tt.expectedReleases, opts.requiredReleases)
 			}
+			assert.Equal(t, tt.expectedCurrentReleaseIsRequired, opts.currentReleaseIsRequired)
 		})
 	}
 }
@@ -399,6 +437,40 @@ func TestValidateIsReleaseUpgradable(t *testing.T) {
 			expectError:         true,
 			expectValidationErr: true,
 		},
+		{
+			name: "current release failed and is required",
+			opts: UpgradableOptions{
+				CurrentAppVersion:        "1.0.0",
+				CurrentAppSequence:       100,
+				CurrentAppStatus:         "failed",
+				CurrentECVersion:         "2.0.0+k8s-1.29",
+				TargetAppVersion:         "1.5.0",
+				TargetAppSequence:        500,
+				TargetECVersion:          "2.1.0+k8s-1.29",
+				License:                  newTestLicense(true),
+				requiredReleases:         []string{},
+				currentReleaseIsRequired: true,
+			},
+			expectError:         true,
+			expectValidationErr: true,
+		},
+		{
+			name: "current release failed and is deployed",
+			opts: UpgradableOptions{
+				CurrentAppVersion:        "1.0.0",
+				CurrentAppSequence:       100,
+				CurrentAppStatus:         "deployed",
+				CurrentECVersion:         "2.0.0+k8s-1.29",
+				TargetAppVersion:         "1.5.0",
+				TargetAppSequence:        500,
+				TargetECVersion:          "2.1.0+k8s-1.29",
+				License:                  newTestLicense(true),
+				requiredReleases:         []string{},
+				currentReleaseIsRequired: true,
+			},
+			expectError:         false,
+			expectValidationErr: false,
+		},
 		{
 			name: "ec version downgrade",
 			opts: UpgradableOptions{