Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

use the allowed upgrades list when validating version upgrades #240

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

use the allowed upgrades list when validating version upgrades #240

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
138 changes: 102 additions & 36 deletions internal/provider/cluster_resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"net/http"
"regexp"
"sort"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -193,7 +194,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 +234,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 +645,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 +669,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 +679,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
Comment on lines -703 to +688
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

}
}
if !versionValid {
Expand All @@ -716,23 +701,65 @@ 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
break
}
}

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.")
} else if err != nil {
resp.Diagnostics.AddError("Invalid CockroachDB version", err.Error())
return
}

if downgrade, _ := isDowngrade(stateVersion, planVersion); downgrade {
var targetVersionInfo client.ClusterMajorVersion
for _, v := range apiResp.Versions {
if v.Version == planVersion {
targetVersionInfo = v
break
}
}

var validDowngrade bool
for _, v := range targetVersionInfo.AllowedUpgrades {
if v == stateVersion {
validDowngrade = true
break
}
}
if !validDowngrade {
resp.Diagnostics.AddError("Invalid CockroachDB version",
fmt.Sprintf(
"Cannot roll back to to '%s'.",
planVersion))
return
}
}

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 +998,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, fmt.Errorf("'%s' is not a valid major CockroachDB version.", majorVersion)
}
// 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 +1098,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 +1110,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
63 changes: 35 additions & 28 deletions internal/provider/cluster_resource_test.go
View file
Open in desktop
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 Expand Up @@ -1135,19 +1135,25 @@ func TestIntegrationDedicatedClusterResource(t *testing.T) {
Versions: []client.ClusterMajorVersion{
{
Version: minSupportedClusterMajorVersion,
AllowedUpgrades: []string{
latestClusterMajorVersion,
},
},
{
Version: latestClusterMajorVersion,
Version: latestClusterMajorVersion,
AllowedUpgrades: []string{},
},
},
}, nil, nil)
s.EXPECT().UpdateCluster(gomock.Any(), clusterID, &client.UpdateClusterSpecification{UpgradeStatus: &upgradingCluster.UpgradeStatus}).
DoAndReturn(
func(context.Context, string, *client.UpdateClusterSpecification,
) (*client.Cluster, *http.Response, error) {
return &upgradingCluster, httpOk, nil
},
)

s.EXPECT().UpdateCluster(gomock.Any(), clusterID, &client.UpdateClusterSpecification{
CockroachVersion: ptr(latestClusterMajorVersion),
}).DoAndReturn(
func(context.Context, string, *client.UpdateClusterSpecification,
) (*client.Cluster, *http.Response, error) {
return &upgradingCluster, httpOk, nil
},
)

s.EXPECT().GetCluster(gomock.Any(), clusterID).
Return(&upgradingCluster, httpOk, nil)
Expand All @@ -1165,11 +1171,12 @@ func TestIntegrationDedicatedClusterResource(t *testing.T) {

// Finalize

s.EXPECT().UpdateCluster(gomock.Any(), clusterID, gomock.Any()).
DoAndReturn(func(context.Context, string, *client.UpdateClusterSpecification,
) (*client.Cluster, *http.Response, error) {
return &finalizedCluster, httpOk, nil
})
s.EXPECT().UpdateCluster(gomock.Any(), clusterID, &client.UpdateClusterSpecification{
UpgradeStatus: ptr(client.CLUSTERUPGRADESTATUSTYPE_FINALIZED),
}).DoAndReturn(func(context.Context, string, *client.UpdateClusterSpecification,
) (*client.Cluster, *http.Response, error) {
return &finalizedCluster, httpOk, nil
})

s.EXPECT().GetCluster(gomock.Any(), clusterID).
Return(&finalizedCluster, httpOk, nil).Times(6)
Expand Down
Loading
Loading