-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
allow updating additional_zones, turn it into a set #152
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -106,10 +106,9 @@ func resourceContainerCluster() *schema.Resource { | |
}, | ||
|
||
"additional_zones": { | ||
Type: schema.TypeList, | ||
Type: schema.TypeSet, | ||
Optional: true, | ||
Computed: true, | ||
ForceNew: true, | ||
Elem: &schema.Schema{Type: schema.TypeString}, | ||
}, | ||
|
||
|
@@ -386,7 +385,7 @@ func resourceContainerClusterCreate(d *schema.ResourceData, meta interface{}) er | |
} | ||
|
||
if v, ok := d.GetOk("additional_zones"); ok { | ||
locationsList := v.([]interface{}) | ||
locationsList := v.(*schema.Set).List() | ||
locations := []string{} | ||
for _, v := range locationsList { | ||
location := v.(string) | ||
|
@@ -634,29 +633,61 @@ func resourceContainerClusterUpdate(d *schema.ResourceData, meta interface{}) er | |
|
||
zoneName := d.Get("zone").(string) | ||
clusterName := d.Get("name").(string) | ||
desiredNodeVersion := d.Get("node_version").(string) | ||
timeoutInMinutes := int(d.Timeout(schema.TimeoutUpdate).Minutes()) | ||
|
||
req := &container.UpdateClusterRequest{ | ||
Update: &container.ClusterUpdate{ | ||
DesiredNodeVersion: desiredNodeVersion, | ||
}, | ||
} | ||
op, err := config.clientContainer.Projects.Zones.Clusters.Update( | ||
project, zoneName, clusterName, req).Do() | ||
if err != nil { | ||
return err | ||
d.Partial(true) | ||
|
||
if d.HasChange("node_version") { | ||
desiredNodeVersion := d.Get("node_version").(string) | ||
|
||
req := &container.UpdateClusterRequest{ | ||
Update: &container.ClusterUpdate{ | ||
DesiredNodeVersion: desiredNodeVersion, | ||
}, | ||
} | ||
op, err := config.clientContainer.Projects.Zones.Clusters.Update( | ||
project, zoneName, clusterName, req).Do() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
// Wait until it's updated | ||
waitErr := containerOperationWait(config, op, project, zoneName, "updating GKE cluster version", timeoutInMinutes, 2) | ||
if waitErr != nil { | ||
return waitErr | ||
} | ||
|
||
log.Printf("[INFO] GKE cluster %s has been updated to %s", d.Id(), | ||
desiredNodeVersion) | ||
|
||
d.SetPartial("node_version") | ||
} | ||
|
||
// Wait until it's updated | ||
if d.HasChange("additional_zones") { | ||
azs := convertStringArr(d.Get("additional_zones").(*schema.Set).List()) | ||
locations := append(azs, zoneName) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm guessing there's no problem if a location is repeated multiple times (i.e. the API doesn't throw an error)? Otherwise it might make sense to do this call before converting it to a list There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! The API does throw an error. I just added a check like we have in |
||
req := &container.UpdateClusterRequest{ | ||
Update: &container.ClusterUpdate{ | ||
DesiredLocations: locations, | ||
}, | ||
} | ||
op, err := config.clientContainer.Projects.Zones.Clusters.Update( | ||
project, zoneName, clusterName, req).Do() | ||
if err != nil { | ||
return err | ||
} | ||
|
||
waitErr := containerOperationWait(config, op, project, zoneName, "updating GKE cluster", timeoutInMinutes, 2) | ||
if waitErr != nil { | ||
return waitErr | ||
// Wait until it's updated | ||
waitErr := containerOperationWait(config, op, project, zoneName, "updating GKE cluster locations", timeoutInMinutes, 2) | ||
if waitErr != nil { | ||
return waitErr | ||
} | ||
|
||
log.Printf("[INFO] GKE cluster %s locations have been updated to %v", d.Id(), | ||
locations) | ||
} | ||
|
||
log.Printf("[INFO] GKE cluster %s has been updated to %s", d.Id(), | ||
desiredNodeVersion) | ||
d.Partial(false) | ||
|
||
return resourceContainerClusterRead(d, meta) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
package google | ||
|
||
import ( | ||
"fmt" | ||
"log" | ||
"strconv" | ||
"strings" | ||
|
||
"github.com/hashicorp/terraform/helper/schema" | ||
"github.com/hashicorp/terraform/terraform" | ||
) | ||
|
||
func resourceContainerClusterMigrateState( | ||
v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) { | ||
if is.Empty() { | ||
log.Println("[DEBUG] Empty InstanceState; nothing to migrate.") | ||
return is, nil | ||
} | ||
|
||
switch v { | ||
case 0: | ||
log.Println("[INFO] Found Container Cluster State v0; migrating to v1") | ||
is, err := migrateClusterStateV0toV1(is) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can be shorted to just
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
if err != nil { | ||
return is, err | ||
} | ||
return is, nil | ||
default: | ||
return is, fmt.Errorf("Unexpected schema version: %d", v) | ||
} | ||
} | ||
|
||
func migrateClusterStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: capital 't' in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving it lowercase to match the other files |
||
log.Printf("[DEBUG] Attributes before migration: %#v", is.Attributes) | ||
|
||
newZones := []string{} | ||
|
||
for k, v := range is.Attributes { | ||
if !strings.HasPrefix(k, "additional_zones.") { | ||
continue | ||
} | ||
|
||
if k == "additional_zones.#" { | ||
continue | ||
} | ||
|
||
// Key is now of the form additional_zones.%d | ||
kParts := strings.Split(k, ".") | ||
|
||
// Sanity check: two parts should be there and <N> should be a number | ||
badFormat := false | ||
if len(kParts) != 2 { | ||
badFormat = true | ||
} else if _, err := strconv.Atoi(kParts[1]); err != nil { | ||
badFormat = true | ||
} | ||
|
||
if badFormat { | ||
return is, fmt.Errorf("migration error: found additional_zones key in unexpected format: %s", k) | ||
} | ||
|
||
newZones = append(newZones, v) | ||
delete(is.Attributes, k) | ||
} | ||
|
||
for _, v := range newZones { | ||
hash := schema.HashString(v) | ||
newKey := fmt.Sprintf("additional_zones.%d", hash) | ||
is.Attributes[newKey] = v | ||
} | ||
|
||
log.Printf("[DEBUG] Attributes after migration: %#v", is.Attributes) | ||
return is, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,75 @@ | ||
package google | ||
|
||
import ( | ||
"testing" | ||
|
||
"github.com/hashicorp/terraform/terraform" | ||
) | ||
|
||
func TestContainerClusterMigrateState(t *testing.T) { | ||
cases := map[string]struct { | ||
StateVersion int | ||
Attributes map[string]string | ||
Expected map[string]string | ||
Meta interface{} | ||
}{ | ||
"change additional_zones from list to set": { | ||
StateVersion: 0, | ||
Attributes: map[string]string{ | ||
"additional_zones.#": "1", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be 2? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup! Done. |
||
"additional_zones.0": "us-central1-c", | ||
"additional_zones.1": "us-central1-b", | ||
}, | ||
Expected: map[string]string{ | ||
"additional_zones.#": "1", | ||
"additional_zones.90274510": "us-central1-c", | ||
"additional_zones.1919306328": "us-central1-b", | ||
}, | ||
Meta: &Config{}, | ||
}, | ||
} | ||
|
||
for tn, tc := range cases { | ||
is := &terraform.InstanceState{ | ||
ID: "i-abc123", | ||
Attributes: tc.Attributes, | ||
} | ||
is, err := resourceContainerClusterMigrateState( | ||
tc.StateVersion, is, tc.Meta) | ||
|
||
if err != nil { | ||
t.Fatalf("bad: %s, err: %#v", tn, err) | ||
} | ||
|
||
for k, v := range tc.Expected { | ||
if is.Attributes[k] != v { | ||
t.Fatalf( | ||
"bad: %s\n\n expected: %#v -> %#v\n got: %#v -> %#v\n in: %#v", | ||
tn, k, v, k, is.Attributes[k], is.Attributes) | ||
} | ||
} | ||
} | ||
} | ||
|
||
func TestContainerClusterMigrateState_empty(t *testing.T) { | ||
var is *terraform.InstanceState | ||
var meta *Config | ||
|
||
// should handle nil | ||
is, err := resourceContainerClusterMigrateState(0, is, meta) | ||
|
||
if err != nil { | ||
t.Fatalf("err: %#v", err) | ||
} | ||
if is != nil { | ||
t.Fatalf("expected nil instancestate, got: %#v", is) | ||
} | ||
|
||
// should handle non-nil but empty | ||
is = &terraform.InstanceState{} | ||
is, err = resourceContainerClusterMigrateState(0, is, meta) | ||
|
||
if err != nil { | ||
t.Fatalf("err: %#v", err) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,9 @@ package google | |
|
||
import ( | ||
"fmt" | ||
"reflect" | ||
"sort" | ||
"strings" | ||
"testing" | ||
|
||
"strconv" | ||
|
@@ -63,13 +66,22 @@ func TestAccContainerCluster_withMasterAuth(t *testing.T) { | |
} | ||
|
||
func TestAccContainerCluster_withAdditionalZones(t *testing.T) { | ||
clusterName := fmt.Sprintf("cluster-test-%s", acctest.RandString(10)) | ||
|
||
resource.Test(t, resource.TestCase{ | ||
PreCheck: func() { testAccPreCheck(t) }, | ||
Providers: testAccProviders, | ||
CheckDestroy: testAccCheckContainerClusterDestroy, | ||
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccContainerCluster_withAdditionalZones, | ||
Config: testAccContainerCluster_withAdditionalZones(clusterName), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckContainerCluster( | ||
"google_container_cluster.with_additional_zones"), | ||
), | ||
}, | ||
{ | ||
Config: testAccContainerCluster_updateAdditionalZones(clusterName), | ||
Check: resource.ComposeTestCheckFunc( | ||
testAccCheckContainerCluster( | ||
"google_container_cluster.with_additional_zones"), | ||
|
@@ -236,6 +248,10 @@ func testAccCheckContainerClusterDestroy(s *terraform.State) error { | |
return nil | ||
} | ||
|
||
var setFields map[string]struct{} = map[string]struct{}{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could this just be a list/set of strings rather than having an unused struct{} value? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is basically how you do a set in go, since there's no native set type (there's the terraform-specific one but I don't want to rely on terraform implementation in tests), and lists don't have a contains function so you'd have to scan the whole thing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. :( OK |
||
"additional_zones": struct{}{}, | ||
} | ||
|
||
func testAccCheckContainerCluster(n string) resource.TestCheckFunc { | ||
return func(s *terraform.State) error { | ||
attributes, err := getResourceAttributes(n, s) | ||
|
@@ -354,6 +370,9 @@ func getResourceAttributes(n string, s *terraform.State) (map[string]string, err | |
|
||
func checkMatch(attributes map[string]string, attr string, gcp interface{}) string { | ||
if gcpList, ok := gcp.([]string); ok { | ||
if _, ok := setFields[attr]; ok { | ||
return checkSetMatch(attributes, attr, gcpList) | ||
} | ||
return checkListMatch(attributes, attr, gcpList) | ||
} | ||
if gcpMap, ok := gcp.(map[string]string); ok { | ||
|
@@ -366,6 +385,30 @@ func checkMatch(attributes map[string]string, attr string, gcp interface{}) stri | |
return "" | ||
} | ||
|
||
func checkSetMatch(attributes map[string]string, attr string, gcpList []string) string { | ||
num, err := strconv.Atoi(attributes[attr+".#"]) | ||
if err != nil { | ||
return fmt.Sprintf("Error in number conversion for attribute %s: %s", attr, err) | ||
} | ||
if num != len(gcpList) { | ||
return fmt.Sprintf("Cluster has mismatched %s size.\nTF Size: %d\nGCP Size: %d", attr, num, len(gcpList)) | ||
} | ||
|
||
// We don't know the exact keys of the elements, so go through the whole list looking for matching ones | ||
tfAttr := []string{} | ||
for k, v := range attributes { | ||
if strings.HasPrefix(k, attr) && !strings.HasSuffix(k, "#") { | ||
tfAttr = append(tfAttr, v) | ||
} | ||
} | ||
sort.Strings(tfAttr) | ||
sort.Strings(gcpList) | ||
if reflect.DeepEqual(tfAttr, gcpList) { | ||
return "" | ||
} | ||
return matchError(attr, tfAttr, gcpList) | ||
} | ||
|
||
func checkListMatch(attributes map[string]string, attr string, gcpList []string) string { | ||
num, err := strconv.Atoi(attributes[attr+".#"]) | ||
if err != nil { | ||
|
@@ -402,7 +445,7 @@ func checkMapMatch(attributes map[string]string, attr string, gcpMap map[string] | |
return "" | ||
} | ||
|
||
func matchError(attr, tf string, gcp interface{}) string { | ||
func matchError(attr, tf interface{}, gcp interface{}) string { | ||
return fmt.Sprintf("Cluster has mismatched %s.\nTF State: %+v\nGCP State: %+v", attr, tf, gcp) | ||
} | ||
|
||
|
@@ -438,9 +481,10 @@ resource "google_container_cluster" "with_master_auth" { | |
} | ||
}`, acctest.RandString(10)) | ||
|
||
var testAccContainerCluster_withAdditionalZones = fmt.Sprintf(` | ||
func testAccContainerCluster_withAdditionalZones(clusterName string) string { | ||
return fmt.Sprintf(` | ||
resource "google_container_cluster" "with_additional_zones" { | ||
name = "cluster-test-%s" | ||
name = "%s" | ||
zone = "us-central1-a" | ||
initial_node_count = 1 | ||
|
||
|
@@ -453,7 +497,28 @@ resource "google_container_cluster" "with_additional_zones" { | |
username = "mr.yoda" | ||
password = "adoy.rm" | ||
} | ||
}`, acctest.RandString(10)) | ||
}`, clusterName) | ||
} | ||
|
||
func testAccContainerCluster_updateAdditionalZones(clusterName string) string { | ||
return fmt.Sprintf(` | ||
resource "google_container_cluster" "with_additional_zones" { | ||
name = "%s" | ||
zone = "us-central1-a" | ||
initial_node_count = 1 | ||
|
||
additional_zones = [ | ||
"us-central1-f", | ||
"us-central1-b", | ||
"us-central1-c", | ||
] | ||
|
||
master_auth { | ||
username = "mr.yoda" | ||
password = "adoy.rm" | ||
} | ||
}`, clusterName) | ||
} | ||
|
||
var testAccContainerCluster_withVersion = fmt.Sprintf(` | ||
data "google_container_engine_versions" "central1a" { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
convertStringArr
is defined inresource_compute_target_pool.go
- is there a better place to put it?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put it in provider.go, I think that's the best option we have righ tnow.