Skip to content

Commit

Permalink
use the allowed upgrades list when validating version upgrades
Browse files Browse the repository at this point in the history 
Beginning with v24.2, some releases are innovation releases which can be
skipped during upgrading. The ListClusterMajorVersions API returns
information on allowed upgrade paths, which we now use to validate
whether an upgrade is possible in `apply`. Additional validation
(cluster state, etc) is performed server-side within the API, so this
validation is mostly to catch the most common type of error with a
faster, more helpful error message, and not to be exhaustive.
  • Loading branch information
@dcrosta
dcrosta committed Sep 25, 2024
1 parent ed7e6ee commit f7cd25d
Show file tree
Hide file tree
Showing 6 changed files with 139 additions and 55 deletions.
137 changes: 101 additions & 36 deletions internal/provider/cluster_resource.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,12 @@ package provider

import (
"context"
"errors"
"fmt"
"net/http"
"regexp"
"sort"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -193,7 +195,7 @@ func (r *clusterResource) Schema(
PlanModifiers: []planmodifier.String{
stringplanmodifier.UseStateForUnknown(),
},
Validators: []validator.String{stringvalidator.OneOf(AllowedUpgradeTypeTypeEnumValueStrings...)},
Validators: []validator.String{stringvalidator.OneOf(AllowedUpgradeTypeTypeEnumValueStrings...)},
MarkdownDescription: "Dictates the behavior of CockroachDB major version upgrades. Manual upgrades are not supported on CockroachDB Basic. Manual or automatic upgrades are supported on CockroachDB Standard. If you omit the field, it defaults to `AUTOMATIC`. Allowed values are:" +
formatEnumMarkdownList(AllowedUpgradeTypeTypeEnumValueStrings),
},
Expand Down Expand Up @@ -233,8 +235,8 @@ func (r *clusterResource) Schema(
Description: "Number of virtual CPUs per node in the cluster.",
},
"private_network_visibility": schema.BoolAttribute{
Optional: true,
Computed: true,
Optional: true,
Computed: true,
MarkdownDescription: "Set to true to assign private IP addresses to nodes. Required for CMEK and other advanced networking features. Clusters created with this flag will have advanced security features enabled. This cannot be changed after cluster creation and incurs additional charges. See [Create an Advanced Cluster](https://www.cockroachlabs.com/docs/cockroachcloud/create-an-advanced-cluster.html#step-6-configure-advanced-security-features) and [Pricing](https://www.cockroachlabs.com/pricing/) for more information.",
PlanModifiers: []planmodifier.Bool{
boolplanmodifier.UseStateForUnknown(),
Expand Down Expand Up @@ -644,23 +646,6 @@ func (r *clusterResource) getUsageLimits(config *ServerlessClusterConfig) *Usage
return nil
}

// Comparator for two CRDB versions to make validation simpler. Assumes biannual releases of the form vYY.H.
// Result is the number of releases apart, negative if r is later and positive is l is later. For example,
// compareCrdbVersions("v22.1", "v22.2") = -1, compareCrdbVersions("v22.1", "v19.2") = 3
func compareCrdbVersions(l, r string, d *diag.Diagnostics) int {
var lMaj, lMin, rMaj, rMin int
if _, err := fmt.Sscanf(l, "v%d.%d", &lMaj, &lMin); err != nil {
d.AddError("Couldn't parse version number", fmt.Sprintf("Couldn't parse version '%s'.", l))
return 0
}
if _, err := fmt.Sscanf(r, "v%d.%d", &rMaj, &rMin); err != nil {
d.AddError("Couldn't parse version number", fmt.Sprintf("Couldn't parse version '%s'.", r))
return 0
}

return (rMaj*2 + rMin - 1) - (lMaj*2 + lMin - 1)
}

func (r *clusterResource) Update(
ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse,
) {
Expand All @@ -685,9 +670,8 @@ func (r *clusterResource) Update(
return
}

// CRDB Versions
// CRDB Upgrades/Downgrades
if IsKnown(plan.CockroachVersion) && plan.CockroachVersion != state.CockroachVersion {
// Validate that the target version is valid.
planVersion := plan.CockroachVersion.ValueString()
stateVersion := state.CockroachVersion.ValueString()
traceAPICall("ListMajorClusterVersions")
Expand All @@ -696,11 +680,13 @@ func (r *clusterResource) Update(
resp.Diagnostics.AddError("Couldn't retrieve CockroachDB version list", formatAPIErrorMessage(err))
return
}

// Target version must be a valid major version
var versionValid bool
for _, v := range apiResp.Versions {
if v.Version == planVersion {
versionValid = true
continue
break
}
}
if !versionValid {
Expand All @@ -716,23 +702,63 @@ func (r *clusterResource) Update(
return
}

upgradeStatus := client.CLUSTERUPGRADESTATUSTYPE_MAJOR_UPGRADE_RUNNING
cmp := compareCrdbVersions(stateVersion, planVersion, &resp.Diagnostics)
if cmp < 0 {
// Make sure we're rolling back to the previous version.
if cmp < -1 {
resp.Diagnostics.AddError("Invalid rollback version", "Can only roll back to the previous version.")
// Next, if we are upgrading, it must be a valid upgrade from the
// current version, according to the ListMajorClusterVersions API.
if upgrading, err := isUpgrade(stateVersion, planVersion); upgrading {
var currentVersionInfo client.ClusterMajorVersion
for _, v := range apiResp.Versions {
if v.Version == stateVersion {
currentVersionInfo = v
}
}

var validUpgrade bool
for _, v := range currentVersionInfo.AllowedUpgrades {
if v == planVersion {
validUpgrade = true
break
}
}
if !validUpgrade {
resp.Diagnostics.AddError("Invalid CockroachDB version",
fmt.Sprintf(
"Cannot change major version to '%s'. Valid upgrade versions include [%s]",
planVersion,
strings.Join(currentVersionInfo.AllowedUpgrades, "|")))
return
}
upgradeStatus = client.CLUSTERUPGRADESTATUSTYPE_ROLLBACK_RUNNING
} else if cmp > 1 {
resp.Diagnostics.AddError("Invalid upgrade version", "Can't skip versions. Upgrades must be performed one version at a time.")
if IsKnown(plan.UpgradeStatus) && plan.UpgradeStatus.ValueString() != string(client.CLUSTERUPGRADESTATUSTYPE_MAJOR_UPGRADE_RUNNING) {
resp.Diagnostics.AddError("Invalid upgrade_status",
fmt.Sprintf(
"cockroach_version ('%s') would upgrade cluster, but upgrade_status was '%s'. "+
"Remove upgrade_status when upgrading via cockroach_version.",
planVersion,
plan.UpgradeStatus.ValueString()))
return
}
} else if err != nil {
resp.Diagnostics.AddError("Invalid CockroachDB version", err.Error())
return
}

if downgrade, _ := isDowngrade(stateVersion, planVersion); downgrade {
if IsKnown(plan.UpgradeStatus) && plan.UpgradeStatus.ValueString() != string(client.CLUSTERUPGRADESTATUSTYPE_ROLLBACK_RUNNING) {
resp.Diagnostics.AddError("Invalid upgrade_status",
fmt.Sprintf(
"cockroach_version ('%s') would roll back cluster, but upgrade_status was '%s'. "+
"Remove upgrade_status when rolling back via cockroach_version.",
planVersion,
plan.UpgradeStatus.ValueString()))
return
}

// Rely on UpdateCluster to validate that we're downgrading to the rollback version;
// we don't have enough info to perform that check here.
}

traceAPICall("UpdateCluster")
clusterObj, _, err := r.provider.service.UpdateCluster(ctx, plan.ID.ValueString(), &client.UpdateClusterSpecification{
UpgradeStatus: &upgradeStatus,
CockroachVersion: &planVersion,
})
if err != nil {
resp.Diagnostics.AddError("Error updating cluster", formatAPIErrorMessage(err))
Expand Down Expand Up @@ -971,6 +997,45 @@ func simplifyClusterVersion(version string, planSpecifiesPreviewString bool) str
return fmt.Sprintf("v%s.%s", parts[1], parts[2])
}

var majorVersionRE = regexp.MustCompile(`^v(0|[1-9][0-9]*)\.(0|[1-9][0-9]*)$`)

func parseMajorVersion(majorVersion string) (int, int, error) {
parts := majorVersionRE.FindStringSubmatch(majorVersion)
if len(parts) == 0 {
return 0, 0, errors.New(fmt.Sprintf("'%s' is not a valid major CockroachDB version.", majorVersion))

Check failure on line 1005 in internal/provider/cluster_resource.go

View workflow job for this annotation

GitHub Actions / Golint 

S1028: should use fmt.Errorf(...) instead of errors.New(fmt.Sprintf(...)) (gosimple)
}
// regex guarantees we'll have parsable ints
major, _ := strconv.Atoi(parts[1])
minor, _ := strconv.Atoi(parts[2])
return major, minor, nil
}

// isUpgrade returns true if toMajorVersion is newer than fromMajorVersion
func isUpgrade(fromMajorVersion, toMajorVersion string) (bool, error) {
fromYear, fromOrdinal, err := parseMajorVersion(fromMajorVersion)
if err != nil {
return false, err
}
toYear, toOrdinal, err := parseMajorVersion(toMajorVersion)
if err != nil {
return false, err
}
return toYear > fromYear || (toYear == fromYear && toOrdinal > fromOrdinal), nil
}

// isDowngrade returns true if toMajorVersion is older than fromMajorVersion
func isDowngrade(fromMajorVersion, toMajorVersion string) (bool, error) {
fromYear, fromOrdinal, err := parseMajorVersion(fromMajorVersion)
if err != nil {
return false, err
}
toYear, toOrdinal, err := parseMajorVersion(toMajorVersion)
if err != nil {
return false, err
}
return toYear < fromYear || (toYear == fromYear && toOrdinal < fromOrdinal), nil
}

// Since the API response will always sort regions by name, we need to
// resort the list, so it matches up with the plan. If the response and
// plan regions don't match up, the sort won't work right, but we can
Expand Down Expand Up @@ -1032,7 +1097,7 @@ func loadClusterToTerraformState(

if clusterObj.Config.Serverless != nil {
serverlessConfig := &ServerlessClusterConfig{
RoutingId: types.StringValue(clusterObj.Config.Serverless.RoutingId),
RoutingId: types.StringValue(clusterObj.Config.Serverless.RoutingId),
UpgradeType: types.StringValue(string(clusterObj.Config.Serverless.UpgradeType)),
}

Expand All @@ -1044,8 +1109,8 @@ func loadClusterToTerraformState(
if usageLimits != nil {
serverlessConfig.UsageLimits = &UsageLimits{
ProvisionedVirtualCpus: types.Int64PointerValue(usageLimits.ProvisionedVirtualCpus),
RequestUnitLimit: types.Int64PointerValue(usageLimits.RequestUnitLimit),
StorageMibLimit: types.Int64PointerValue(usageLimits.StorageMibLimit),
RequestUnitLimit: types.Int64PointerValue(usageLimits.RequestUnitLimit),
StorageMibLimit: types.Int64PointerValue(usageLimits.StorageMibLimit),
}
} else if plan != nil && plan.ServerlessConfig != nil && plan.ServerlessConfig.UsageLimits != nil {
// There is no difference in behavior between UsageLimits = nil and
Expand Down
30 changes: 15 additions & 15 deletions internal/provider/cluster_resource_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ func TestAccServerlessUpgradeType(t *testing.T) {
checkUpgradeTypeResources := func(upgradeType client.UpgradeTypeType) resource.TestCheckFunc {
return resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr(serverlessResourceName, "serverless.upgrade_type", string(upgradeType)),
resource.TestCheckResourceAttr(serverlessDataSourceName , "serverless.upgrade_type", string(upgradeType)),
resource.TestCheckResourceAttr(serverlessDataSourceName, "serverless.upgrade_type", string(upgradeType)),
)
}
resource.Test(t, resource.TestCase{
Expand All @@ -101,56 +101,56 @@ func TestAccServerlessUpgradeType(t *testing.T) {
// Create a provisioned cluster with the default value for upgrade_type
{
Config: provisionedSingleRegionClusterStep(clusterName, "STANDARD", 6, nil).Config,
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
},
// Explicitly updating the value to MANUAL performs the update
{
Config: provisionedSingleRegionClusterStep(clusterName, "STANDARD", 6, ptr(client.UPGRADETYPETYPE_MANUAL)).Config,
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_MANUAL),
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_MANUAL),
},
// Removal of the optional value from the config makes no change
{
Config: provisionedSingleRegionClusterStep(clusterName, "STANDARD", 6, nil).Config,
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_MANUAL),
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_MANUAL),
},
// Change it back to automatic so we can downgrade the cluster to
// BASIC. Currently the ccapi doesn't allow downgrading to BASIC
// unless upgrade_type is AUTOMATIC already.
{
Config: provisionedSingleRegionClusterStep(clusterName, "STANDARD", 6, ptr(client.UPGRADETYPETYPE_AUTOMATIC)).Config,
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
},
// Downgrade to Basic, the upgrade_type remains AUTOMATIC
{
Config: onDemandSingleRegionClusterWithUnlimitedStep(clusterName, "BASIC", nil /* upgradeType */).Config,
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
},
// Setting the value to MANUAL is not allowed for Basic
{
Config: onDemandSingleRegionClusterWithUnlimitedStep(clusterName, "BASIC", ptr(client.UPGRADETYPETYPE_MANUAL)).Config,
Config: onDemandSingleRegionClusterWithUnlimitedStep(clusterName, "BASIC", ptr(client.UPGRADETYPETYPE_MANUAL)).Config,
ExpectError: regexp.MustCompile("plan type BASIC does not allow upgrade_type MANUAL"),
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
},
// Setting completely invalid value for upgrade_type
{
Config: onDemandSingleRegionClusterWithUnlimitedStep(clusterName, "BASIC", ptr(client.UpgradeTypeType("hi"))).Config,
Config: onDemandSingleRegionClusterWithUnlimitedStep(clusterName, "BASIC", ptr(client.UpgradeTypeType("hi"))).Config,
ExpectError: regexp.MustCompile("Attribute serverless.upgrade_type value must be one of"),
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
},
// Basic clusters can also accept a value of AUTOMATIC.
{
Config: onDemandSingleRegionClusterWithUnlimitedStep(clusterName, "BASIC", ptr(client.UPGRADETYPETYPE_AUTOMATIC)).Config,
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
},
// Destroy the cluster so we can create it again in the next step
{
Config: " ",
Destroy: true,
Config: " ",
Destroy: true,
},
// Basic clusters can also be created with a value of AUTOMATIC
{
Config: onDemandSingleRegionClusterWithUnlimitedStep(clusterName, "BASIC", ptr(client.UPGRADETYPETYPE_AUTOMATIC)).Config,
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
Check: checkUpgradeTypeResources(client.UPGRADETYPETYPE_AUTOMATIC),
},
},
})
Expand Down Expand Up @@ -194,7 +194,7 @@ func TestIntegrationServerlessClusterResource(t *testing.T) {
Plan: planType,
Config: client.ClusterConfig{
Serverless: &client.ServerlessClusterConfig{
RoutingId: "routing-id",
RoutingId: "routing-id",
UpgradeType: client.UPGRADETYPETYPE_AUTOMATIC,
},
},
Expand Down
2 changes: 1 addition & 1 deletion vendor/github.com/cockroachdb/cockroach-cloud-sdk-go/v3/pkg/client/model_any.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit f7cd25d

Please sign in to comment.