From 507deaabe39ed5570ac5c31acbfdd668f503547c Mon Sep 17 00:00:00 2001 From: Mahmoud Ismail Date: Thu, 26 Jan 2023 11:01:03 +0100 Subject: [PATCH] [CLOUD-83] Fixes for upgrade from 3.0 to 3.1 (#234) --- CHANGELOG.md | 1 + hopsworksai/internal/api/apis.go | 5 +- hopsworksai/internal/api/apis_test.go | 4 +- hopsworksai/internal/api/model.go | 3 +- hopsworksai/resource_cluster.go | 48 +++- hopsworksai/resource_cluster_test.go | 330 ++++++++++++++++++++++++++ 6 files changed, 379 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 752810d..47cb7ee 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ NOTES: BREAKING CHANGES: ENHANCEMENTS: +* resource/hopsworksai_cluster: Enforce setting `ecr_registry_account_id` and `acr_registry_name` during upgrade from 3.0 to 3.1 BUG FIXES: diff --git a/hopsworksai/internal/api/apis.go b/hopsworksai/internal/api/apis.go index 24a7b38..2a7277d 100644 --- a/hopsworksai/internal/api/apis.go +++ b/hopsworksai/internal/api/apis.go @@ -268,9 +268,10 @@ func GetSupportedVersions(ctx context.Context, apiClient APIHandler, cloud Cloud return response.Payload.Versions, nil } -func UpgradeCluster(ctx context.Context, apiClient APIHandler, clusterId string, upgradeToVersion string) error { +func UpgradeCluster(ctx context.Context, apiClient APIHandler, clusterId string, upgradeToVersion string, dockerRegistryAccount string) error { req := UpgradeClusterRequest{ - Version: upgradeToVersion, + Version: upgradeToVersion, + DockerRegistryAccount: dockerRegistryAccount, } payload, err := json.Marshal(req) if err != nil { diff --git a/hopsworksai/internal/api/apis_test.go b/hopsworksai/internal/api/apis_test.go index eea6f2e..8b245e3 100644 --- a/hopsworksai/internal/api/apis_test.go +++ b/hopsworksai/internal/api/apis_test.go @@ -2290,7 +2290,7 @@ func TestUpgradeCluster(t *testing.T) { }, } - if err := UpgradeCluster(context.TODO(), apiClient, "cluster-id-1", "v2"); err != nil { + if err := UpgradeCluster(context.TODO(), apiClient, "cluster-id-1", "v2", ""); err != nil { t.Fatalf("should not throw an error, but got %s", err) } } @@ -2314,7 +2314,7 @@ func TestUpgradeCluster_API_error(t *testing.T) { }, } - if err := UpgradeCluster(context.TODO(), apiClient, "cluster-id-1", "v2"); err == nil || err.Error() != "failure to start upgrade" { + if err := UpgradeCluster(context.TODO(), apiClient, "cluster-id-1", "v2", ""); err == nil || err.Error() != "failure to start upgrade" { t.Fatalf("should throw an error, but got %s", err) } } diff --git a/hopsworksai/internal/api/model.go b/hopsworksai/internal/api/model.go index ebd4713..1eb22ab 100644 --- a/hopsworksai/internal/api/model.go +++ b/hopsworksai/internal/api/model.go @@ -579,7 +579,8 @@ type GetSupportedVersionsResponse struct { } type UpgradeClusterRequest struct { - Version string `json:"version"` + Version string `json:"version"` + DockerRegistryAccount string `json:"dockerRegistryAccount,omitempty"` } type NodeInfo struct { diff --git a/hopsworksai/resource_cluster.go b/hopsworksai/resource_cluster.go index 05bfb7c..f996fc1 100644 --- a/hopsworksai/resource_cluster.go +++ b/hopsworksai/resource_cluster.go @@ -916,7 +916,6 @@ func awsAttributesSchema() *schema.Resource { Type: schema.TypeString, Optional: true, Computed: true, - ForceNew: true, ValidateFunc: validation.StringMatch(regexp.MustCompile(`^\d{12}$`), "Invalid ECR account id"), }, "ebs_encryption": { @@ -1062,7 +1061,6 @@ func azureAttributesSchema() *schema.Resource { Description: "The name of the ACR registry.", Type: schema.TypeString, Optional: true, - ForceNew: true, }, }, } @@ -1140,6 +1138,14 @@ func resourceClusterCreate(ctx context.Context, d *schema.ResourceData, meta int return resourceClusterRead(ctx, d, meta) } +func getECRRegistryAccountIdFromInstanceProfile(instanceProfile string) string { + submatches := instanceProfileRegex().FindStringSubmatch(instanceProfile) + if len(submatches) == 3 { + return submatches[1] + } + return "" +} + func createAWSCluster(d *schema.ResourceData, baseRequest *api.CreateCluster) *api.CreateAWSCluster { req := api.CreateAWSCluster{ CreateCluster: *baseRequest, @@ -1168,10 +1174,7 @@ func createAWSCluster(d *schema.ResourceData, baseRequest *api.CreateCluster) *a if registry, okR := d.GetOk("aws_attributes.0.ecr_registry_account_id"); okR { req.EcrRegistryAccountId = registry.(string) } else { - submatches := instanceProfileRegex().FindStringSubmatch(req.InstanceProfileArn) - if len(submatches) == 3 { - req.EcrRegistryAccountId = submatches[1] - } + req.EcrRegistryAccountId = getECRRegistryAccountIdFromInstanceProfile(req.InstanceProfileArn) } } @@ -1450,7 +1453,29 @@ func resourceClusterUpdate(ctx context.Context, d *schema.ResourceData, meta int upgradeInProgressToVersion, upgradeInProgressToVersionOk := d.GetOk("upgrade_in_progress.0.to_version") if !upgradeInProgressFromVersionOk && !upgradeInProgressToVersionOk { - if err := api.UpgradeCluster(ctx, client, clusterId, toVersion); err != nil { + clusterVersion, _ := version.NewVersion(toVersion) + versionWithDefaultECR, _ := version.NewVersion("3.0.0") + + dockerRegistryAccount := "" + if clusterVersion.GreaterThan(versionWithDefaultECR) { + if _, ok := d.GetOk("aws_attributes"); ok { + if v, okR := d.GetOk("aws_attributes.0.ecr_registry_account_id"); okR { + dockerRegistryAccount = v.(string) + } else { + dockerRegistryAccount = getECRRegistryAccountIdFromInstanceProfile(d.Get("aws_attributes.0.instance_profile_arn").(string)) + } + } + + if _, ok := d.GetOk("azure_attributes"); ok { + if v, okR := d.GetOk("azure_attributes.0.acr_registry_name"); okR { + dockerRegistryAccount = v.(string) + } else { + return diag.Errorf("To upgrade from %s to %s, you need to create an acr registry and configure it by setting attribute acr_registry_name", fromVersion, toVersion) + } + } + } + + if err := api.UpgradeCluster(ctx, client, clusterId, toVersion, dockerRegistryAccount); err != nil { return diag.FromErr(err) } if err := resourceClusterWaitForRunningAfterUpgrade(ctx, client, d.Timeout(schema.TimeoutUpdate), clusterId); err != nil { @@ -1654,6 +1679,15 @@ func resourceClusterUpdate(ctx context.Context, d *schema.ResourceData, meta int } } } + + if d.HasChange("aws_attributes.0.ecr_registry_account_id") { + return diag.Errorf("You cannot change the ecr_registry_account_id after cluster creation") + } + + if d.HasChange("azure_attributes.0.acr_registry_name") { + return diag.Errorf("You cannot change the acr_registry_name after cluster creation") + } + return resourceClusterRead(ctx, d, meta) } diff --git a/hopsworksai/resource_cluster_test.go b/hopsworksai/resource_cluster_test.go index 3512b41..c74934b 100644 --- a/hopsworksai/resource_cluster_test.go +++ b/hopsworksai/resource_cluster_test.go @@ -5987,3 +5987,333 @@ func testClusterCreate_AZURE_set_ACR_without_AKS(t *testing.T, version string, e } r.Apply(t, context.TODO()) } + +func TestClusterUpdate_upgrade_AWS_3_0_to_3_1(t *testing.T) { + t.Parallel() + r := test.ResourceFixture{ + HttpOps: []test.Operation{ + { + Method: http.MethodGet, + Path: "/api/clusters/cluster-id-1", + Response: `{ + "apiVersion": "v1", + "status": "ok", + "code": 200, + "payload":{ + "cluster": { + "id": "cluster-id-1", + "name": "cluster-name-1", + "state" : "running", + "provider": "AWS", + "version": "3.0.0", + "ports":{ + "featureStore": false, + "onlineFeatureStore": false, + "kafka": false, + "ssh": false + } + } + } + }`, + }, + { + Method: http.MethodPost, + Path: "/api/clusters/cluster-id-1/upgrade", + ExpectRequestBody: `{ + "version": "3.1.0", + "dockerRegistryAccount": "123456789101" + }`, + Response: `{ + "apiVersion": "v1", + "status": "ok", + "code": 200 + }`, + }, + }, + Resource: clusterResource(), + OperationContextFunc: clusterResource().UpdateContext, + Id: "cluster-id-1", + Update: true, + State: map[string]interface{}{ + "version": "3.1.0", + "aws_attributes": []interface{}{ + map[string]interface{}{ + "region": "region-1", + "instance_profile_arn": "arn:aws:iam::123456789101:instance-profile/profile", + "bucket": []interface{}{ + map[string]interface{}{ + "name": "bucket-1", + }, + }, + }, + }, + "open_ports": []interface{}{ + map[string]interface{}{ + "ssh": false, + "kafka": false, + "feature_store": false, + "online_feature_store": false, + }, + }, + }, + } + r.Apply(t, context.TODO()) +} + +func TestClusterUpdate_upgrade_AZURE_3_0_to_3_1(t *testing.T) { + t.Parallel() + r := test.ResourceFixture{ + HttpOps: []test.Operation{ + { + Method: http.MethodGet, + Path: "/api/clusters/cluster-id-1", + Response: `{ + "apiVersion": "v1", + "status": "ok", + "code": 200, + "payload":{ + "cluster": { + "id": "cluster-id-1", + "name": "cluster-name-1", + "state" : "running", + "provider": "AWS", + "version": "3.0.0", + "ports":{ + "featureStore": false, + "onlineFeatureStore": false, + "kafka": false, + "ssh": false + } + } + } + }`, + }, + { + Method: http.MethodPost, + Path: "/api/clusters/cluster-id-1/upgrade", + ExpectRequestBody: `{ + "version": "3.1.0", + "dockerRegistryAccount": "my-acr" + }`, + Response: `{ + "apiVersion": "v1", + "status": "ok", + "code": 200 + }`, + }, + }, + Resource: clusterResource(), + OperationContextFunc: clusterResource().UpdateContext, + Id: "cluster-id-1", + Update: true, + State: map[string]interface{}{ + "version": "3.1.0", + "azure_attributes": []interface{}{ + map[string]interface{}{ + "location": "location-1", + "resource_group": "resource-group-1", + "user_assigned_managed_identity": "user-identity-1", + "container": []interface{}{ + map[string]interface{}{ + "storage_account": "storage-account-1", + }, + }, + "acr_registry_name": "my-acr", + }, + }, + "open_ports": []interface{}{ + map[string]interface{}{ + "ssh": false, + "kafka": false, + "feature_store": false, + "online_feature_store": false, + }, + }, + }, + } + r.Apply(t, context.TODO()) +} + +func TestClusterUpdate_upgrade_AZURE_3_0_to_3_1_error(t *testing.T) { + t.Parallel() + r := test.ResourceFixture{ + HttpOps: []test.Operation{ + { + Method: http.MethodGet, + Path: "/api/clusters/cluster-id-1", + Response: `{ + "apiVersion": "v1", + "status": "ok", + "code": 200, + "payload":{ + "cluster": { + "id": "cluster-id-1", + "name": "cluster-name-1", + "state" : "running", + "provider": "AWS", + "version": "3.0.0", + "ports":{ + "featureStore": false, + "onlineFeatureStore": false, + "kafka": false, + "ssh": false + } + } + } + }`, + }, + }, + Resource: clusterResource(), + OperationContextFunc: clusterResource().UpdateContext, + Id: "cluster-id-1", + Update: true, + State: map[string]interface{}{ + "version": "3.1.0", + "azure_attributes": []interface{}{ + map[string]interface{}{ + "location": "location-1", + "resource_group": "resource-group-1", + "user_assigned_managed_identity": "user-identity-1", + "container": []interface{}{ + map[string]interface{}{ + "storage_account": "storage-account-1", + }, + }, + }, + }, + "open_ports": []interface{}{ + map[string]interface{}{ + "ssh": false, + "kafka": false, + "feature_store": false, + "online_feature_store": false, + }, + }, + }, + ExpectError: "To upgrade from 3.0.0 to 3.1.0, you need to create an acr registry and configure it by setting attribute acr_registry_name", + } + r.Apply(t, context.TODO()) +} + +func TestClusterUpdate_AWS_update_ecr_error(t *testing.T) { + t.Parallel() + r := test.ResourceFixture{ + HttpOps: []test.Operation{ + { + Method: http.MethodGet, + Path: "/api/clusters/cluster-id-1", + Response: `{ + "apiVersion": "v1", + "status": "ok", + "code": 200, + "payload":{ + "cluster": { + "id": "cluster-id-1", + "name": "cluster-name-1", + "state" : "running", + "provider": "AWS", + "version": "3.0.0", + "ports":{ + "featureStore": false, + "onlineFeatureStore": false, + "kafka": false, + "ssh": false + } + } + } + }`, + }, + }, + Resource: clusterResource(), + OperationContextFunc: clusterResource().UpdateContext, + Id: "cluster-id-1", + Update: true, + State: map[string]interface{}{ + "version": "3.0.0", + "aws_attributes": []interface{}{ + map[string]interface{}{ + "region": "region-1", + "instance_profile_arn": "arn:aws:iam::123456789101:instance-profile/profile", + "bucket": []interface{}{ + map[string]interface{}{ + "name": "bucket-1", + }, + }, + "ecr_registry_account_id": "1123232323", + }, + }, + "open_ports": []interface{}{ + map[string]interface{}{ + "ssh": false, + "kafka": false, + "feature_store": false, + "online_feature_store": false, + }, + }, + }, + ExpectError: "You cannot change the ecr_registry_account_id after cluster creation", + } + r.Apply(t, context.TODO()) +} + +func TestClusterUpdate_AZURE_update_acr_error(t *testing.T) { + t.Parallel() + r := test.ResourceFixture{ + HttpOps: []test.Operation{ + { + Method: http.MethodGet, + Path: "/api/clusters/cluster-id-1", + Response: `{ + "apiVersion": "v1", + "status": "ok", + "code": 200, + "payload":{ + "cluster": { + "id": "cluster-id-1", + "name": "cluster-name-1", + "state" : "running", + "provider": "AWS", + "version": "3.0.0", + "ports":{ + "featureStore": false, + "onlineFeatureStore": false, + "kafka": false, + "ssh": false + } + } + } + }`, + }, + }, + Resource: clusterResource(), + OperationContextFunc: clusterResource().UpdateContext, + Id: "cluster-id-1", + Update: true, + State: map[string]interface{}{ + "version": "3.0.0", + "azure_attributes": []interface{}{ + map[string]interface{}{ + "location": "location-1", + "resource_group": "resource-group-1", + "user_assigned_managed_identity": "user-identity-1", + "container": []interface{}{ + map[string]interface{}{ + "storage_account": "storage-account-1", + }, + }, + "acr_registry_name": "my-acr", + }, + }, + "open_ports": []interface{}{ + map[string]interface{}{ + "ssh": false, + "kafka": false, + "feature_store": false, + "online_feature_store": false, + }, + }, + }, + ExpectError: "You cannot change the acr_registry_name after cluster creation", + } + r.Apply(t, context.TODO()) +}