From b7a1616e766b4608cedf9eafb100aaa954cf7692 Mon Sep 17 00:00:00 2001 From: The Magician Date: Tue, 22 Aug 2023 10:02:26 -0700 Subject: [PATCH] Fix issues with unpopulated gc_rules and handling of Xd duration (#8686) (#6131) * Fix issues with unpopulated gc_rules and handling of Xd duration * Fix copt/paste issues * Extract duration_parsing_helper.go * Extract duration_parsing_helper.go * Add unit tests * Add time.UNIT * Fixed duration helper * Add better handling for days and hours * Add better handling for days and hours * fixed issue with diffs between gc_rules * minor fix * Add tests * package name fix * fix tests * fix tests * fix tests * fix tests * fix tests Signed-off-by: Modular Magician --- .changelog/8686.txt | 6 + .../bigtable/duration_parsing_helper.go | 223 ++++++++++++++++++ .../bigtable/duration_parsing_helper_test.go | 157 ++++++++++++ .../bigtable/resource_bigtable_gc_policy.go | 42 +++- ...source_bigtable_gc_policy_internal_test.go | 56 +++++ .../resource_bigtable_gc_policy_test.go | 8 +- 6 files changed, 484 insertions(+), 8 deletions(-) create mode 100644 .changelog/8686.txt create mode 100644 google-beta/services/bigtable/duration_parsing_helper.go create mode 100644 google-beta/services/bigtable/duration_parsing_helper_test.go diff --git a/.changelog/8686.txt b/.changelog/8686.txt new file mode 100644 index 0000000000..cca1539aed --- /dev/null +++ b/.changelog/8686.txt @@ -0,0 +1,6 @@ +```release-note:bug +bigtable: fixed permadiff in `google_bigtable_gc_policy.gc_rules` when `mode` is specified +``` +```release-note:bug +bigtable: fixed permadiff in `google_bigtable_gc_policy.gc_rules` when `max_age` is specified using increments larger than hours +``` diff --git a/google-beta/services/bigtable/duration_parsing_helper.go b/google-beta/services/bigtable/duration_parsing_helper.go new file mode 100644 index 0000000000..daabbe9f5d --- /dev/null +++ b/google-beta/services/bigtable/duration_parsing_helper.go @@ -0,0 +1,223 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 +package bigtable + +import ( + "errors" + "time" +) + +var unitMap = map[string]uint64{ + "ns": uint64(time.Nanosecond), + "us": uint64(time.Microsecond), + "µs": uint64(time.Microsecond), // U+00B5 = micro symbol + "μs": uint64(time.Microsecond), // U+03BC = Greek letter mu + "ms": uint64(time.Millisecond), + "s": uint64(time.Second), + "m": uint64(time.Minute), + "h": uint64(time.Hour), + "d": uint64(time.Hour) * 24, + "w": uint64(time.Hour) * 24 * 7, +} + +// ParseDuration parses a duration string. +// A duration string is a possibly signed sequence of +// decimal numbers, each with optional fraction and a unit suffix, +// such as "300ms", "-1.5h" or "2h45m". +// Valid time units are "ns", "us" (or "µs"), "ms", "s", "m", "h". +func ParseDuration(s string) (time.Duration, error) { + // [-+]?([0-9]*(\.[0-9]*)?[a-z]+)+ + orig := s + var d uint64 + neg := false + + // Consume [-+]? + if s != "" { + c := s[0] + if c == '-' || c == '+' { + neg = c == '-' + s = s[1:] + } + } + // Special case: if all that is left is "0", this is zero. + if s == "0" { + return 0, nil + } + if s == "" { + return 0, errors.New("time: invalid duration " + quote(orig)) + } + for s != "" { + var ( + v, f uint64 // integers before, after decimal point + scale float64 = 1 // value = v + f/scale + ) + + var err error + + // The next character must be [0-9.] + if !(s[0] == '.' || '0' <= s[0] && s[0] <= '9') { + return 0, errors.New("time: invalid duration " + quote(orig)) + } + // Consume [0-9]* + pl := len(s) + v, s, err = leadingInt(s) + if err != nil { + return 0, errors.New("time: invalid duration " + quote(orig)) + } + pre := pl != len(s) // whether we consumed anything before a period + + // Consume (\.[0-9]*)? + post := false + if s != "" && s[0] == '.' { + s = s[1:] + pl := len(s) + f, scale, s = leadingFraction(s) + post = pl != len(s) + } + if !pre && !post { + // no digits (e.g. ".s" or "-.s") + return 0, errors.New("time: invalid duration " + quote(orig)) + } + + // Consume unit. + i := 0 + for ; i < len(s); i++ { + c := s[i] + if c == '.' || '0' <= c && c <= '9' { + break + } + } + if i == 0 { + return 0, errors.New("time: missing unit in duration " + quote(orig)) + } + u := s[:i] + s = s[i:] + unit, ok := unitMap[u] + if !ok { + return 0, errors.New("time: unknown unit " + quote(u) + " in duration " + quote(orig)) + } + if v > 1<<63/unit { + // overflow + return 0, errors.New("time: invalid duration " + quote(orig)) + } + v *= unit + if f > 0 { + // float64 is needed to be nanosecond accurate for fractions of hours. + // v >= 0 && (f*unit/scale) <= 3.6e+12 (ns/h, h is the largest unit) + v += uint64(float64(f) * (float64(unit) / scale)) + if v > 1<<63 { + // overflow + return 0, errors.New("time: invalid duration " + quote(orig)) + } + } + d += v + if d > 1<<63 { + return 0, errors.New("time: invalid duration " + quote(orig)) + } + } + if neg { + return -time.Duration(d), nil + } + if d > 1<<63-1 { + return 0, errors.New("time: invalid duration " + quote(orig)) + } + return time.Duration(d), nil +} + +var errLeadingInt = errors.New("time: bad [0-9]*") // never printed + +// leadingInt consumes the leading [0-9]* from s. +func leadingInt[bytes []byte | string](s bytes) (x uint64, rem bytes, err error) { + i := 0 + for ; i < len(s); i++ { + c := s[i] + if c < '0' || c > '9' { + break + } + if x > 1<<63/10 { + // overflow + return 0, rem, errLeadingInt + } + x = x*10 + uint64(c) - '0' + if x > 1<<63 { + // overflow + return 0, rem, errLeadingInt + } + } + return x, s[i:], nil +} + +// leadingFraction consumes the leading [0-9]* from s. +// It is used only for fractions, so does not return an error on overflow, +// it just stops accumulating precision. +func leadingFraction(s string) (x uint64, scale float64, rem string) { + i := 0 + scale = 1 + overflow := false + for ; i < len(s); i++ { + c := s[i] + if c < '0' || c > '9' { + break + } + if overflow { + continue + } + if x > (1<<63-1)/10 { + // It's possible for overflow to give a positive number, so take care. + overflow = true + continue + } + y := x*10 + uint64(c) - '0' + if y > 1<<63 { + overflow = true + continue + } + x = y + scale *= 10 + } + return x, scale, s[i:] +} + +// These are borrowed from unicode/utf8 and strconv and replicate behavior in +// that package, since we can't take a dependency on either. +const ( + lowerhex = "0123456789abcdef" + runeSelf = 0x80 + runeError = '\uFFFD' +) + +func quote(s string) string { + buf := make([]byte, 1, len(s)+2) // slice will be at least len(s) + quotes + buf[0] = '"' + for i, c := range s { + if c >= runeSelf || c < ' ' { + // This means you are asking us to parse a time.Duration or + // time.Location with unprintable or non-ASCII characters in it. + // We don't expect to hit this case very often. We could try to + // reproduce strconv.Quote's behavior with full fidelity but + // given how rarely we expect to hit these edge cases, speed and + // conciseness are better. + var width int + if c == runeError { + width = 1 + if i+2 < len(s) && s[i:i+3] == string(runeError) { + width = 3 + } + } else { + width = len(string(c)) + } + for j := 0; j < width; j++ { + buf = append(buf, `\x`...) + buf = append(buf, lowerhex[s[i+j]>>4]) + buf = append(buf, lowerhex[s[i+j]&0xF]) + } + } else { + if c == '"' || c == '\\' { + buf = append(buf, '\\') + } + buf = append(buf, string(c)...) + } + } + buf = append(buf, '"') + return string(buf) +} diff --git a/google-beta/services/bigtable/duration_parsing_helper_test.go b/google-beta/services/bigtable/duration_parsing_helper_test.go new file mode 100644 index 0000000000..7b283fd95a --- /dev/null +++ b/google-beta/services/bigtable/duration_parsing_helper_test.go @@ -0,0 +1,157 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 +package bigtable_test + +import ( + "math" + "math/rand" + "strings" + "testing" + "time" + + "github.com/hashicorp/terraform-provider-google-beta/google-beta/services/bigtable" +) + +var parseDurationTests = []struct { + in string + want time.Duration +}{ + // simple + {"0", 0}, + {"5s", 5 * time.Second}, + {"30s", 30 * time.Second}, + {"1478s", 1478 * time.Second}, + // sign + {"-5s", -5 * time.Second}, + {"+5s", 5 * time.Second}, + {"-0", 0}, + {"+0", 0}, + // decimal + {"5.0s", 5 * time.Second}, + {"5.6s", 5*time.Second + 600*time.Millisecond}, + {"5.s", 5 * time.Second}, + {".5s", 500 * time.Millisecond}, + {"1.0s", 1 * time.Second}, + {"1.00s", 1 * time.Second}, + {"1.004s", 1*time.Second + 4*time.Millisecond}, + {"1.0040s", 1*time.Second + 4*time.Millisecond}, + {"100.00100s", 100*time.Second + 1*time.Millisecond}, + // different units + {"10ns", 10 * time.Nanosecond}, + {"11us", 11 * time.Microsecond}, + {"12µs", 12 * time.Microsecond}, // U+00B5 + {"12μs", 12 * time.Microsecond}, // U+03BC + {"13ms", 13 * time.Millisecond}, + {"14s", 14 * time.Second}, + {"15m", 15 * time.Minute}, + {"16h", 16 * time.Hour}, + {"5d", 5 * 24 * time.Hour}, + // composite durations + {"3h30m", 3*time.Hour + 30*time.Minute}, + {"10.5s4m", 4*time.Minute + 10*time.Second + 500*time.Millisecond}, + {"-2m3.4s", -(2*time.Minute + 3*time.Second + 400*time.Millisecond)}, + {"1h2m3s4ms5us6ns", 1*time.Hour + 2*time.Minute + 3*time.Second + 4*time.Millisecond + 5*time.Microsecond + 6*time.Nanosecond}, + {"39h9m14.425s", 39*time.Hour + 9*time.Minute + 14*time.Second + 425*time.Millisecond}, + // large value + {"52763797000ns", 52763797000 * time.Nanosecond}, + // more than 9 digits after decimal point, see https://golang.org/issue/6617 + {"0.3333333333333333333h", 20 * time.Minute}, + // 9007199254740993 = 1<<53+1 cannot be stored precisely in a float64 + {"9007199254740993ns", (1<<53 + 1) * time.Nanosecond}, + // largest duration that can be represented by int64 in nanoseconds + {"9223372036854775807ns", (1<<63 - 1) * time.Nanosecond}, + {"9223372036854775.807us", (1<<63 - 1) * time.Nanosecond}, + {"9223372036s854ms775us807ns", (1<<63 - 1) * time.Nanosecond}, + {"-9223372036854775808ns", -1 << 63 * time.Nanosecond}, + {"-9223372036854775.808us", -1 << 63 * time.Nanosecond}, + {"-9223372036s854ms775us808ns", -1 << 63 * time.Nanosecond}, + // largest negative value + {"-9223372036854775808ns", -1 << 63 * time.Nanosecond}, + // largest negative round trip value, see https://golang.org/issue/48629 + {"-2562047h47m16.854775808s", -1 << 63 * time.Nanosecond}, + // huge string; issue 15011. + {"0.100000000000000000000h", 6 * time.Minute}, + // This value tests the first overflow check in leadingFraction. + {"0.830103483285477580700h", 49*time.Minute + 48*time.Second + 372539827*time.Nanosecond}, +} + +func TestParseDuration(t *testing.T) { + for _, tc := range parseDurationTests { + d, err := bigtable.ParseDuration(tc.in) + if err != nil || d != tc.want { + t.Errorf("bigtable.ParseDuration(%q) = %v, %v, want %v, nil", tc.in, d, err, tc.want) + } + } +} + +var parseDurationErrorTests = []struct { + in string + expect string +}{ + // invalid + {"", `""`}, + {"3", `"3"`}, + {"-", `"-"`}, + {"s", `"s"`}, + {".", `"."`}, + {"-.", `"-."`}, + {".s", `".s"`}, + {"+.s", `"+.s"`}, + {"\x85\x85", `"\x85\x85"`}, + {"\xffff", `"\xffff"`}, + {"hello \xffff world", `"hello \xffff world"`}, + {"\uFFFD", `"\xef\xbf\xbd"`}, // utf8.RuneError + {"\uFFFD hello \uFFFD world", `"\xef\xbf\xbd hello \xef\xbf\xbd world"`}, // utf8.RuneError + // overflow + {"9223372036854775810ns", `"9223372036854775810ns"`}, + {"9223372036854775808ns", `"9223372036854775808ns"`}, + {"-9223372036854775809ns", `"-9223372036854775809ns"`}, + {"9223372036854776us", `"9223372036854776us"`}, + {"3000000h", `"3000000h"`}, + {"9223372036854775.808us", `"9223372036854775.808us"`}, + {"9223372036854ms775us808ns", `"9223372036854ms775us808ns"`}, +} + +func TestParseDurationErrors(t *testing.T) { + for _, tc := range parseDurationErrorTests { + _, err := bigtable.ParseDuration(tc.in) + if err == nil { + t.Errorf("bigtable.ParseDuration(%q) = _, nil, want _, non-nil", tc.in) + } else if !strings.Contains(err.Error(), tc.expect) { + t.Errorf("bigtable.ParseDuration(%q) = _, %q, error does not contain %q", tc.in, err, tc.expect) + } + } +} + +func TestParseDurationRoundTrip(t *testing.T) { + // https://golang.org/issue/48629 + max0 := time.Duration(math.MaxInt64) + max1, err := bigtable.ParseDuration(max0.String()) + if err != nil || max0 != max1 { + t.Errorf("round-trip failed: %d => %q => %d, %v", max0, max0.String(), max1, err) + } + + min0 := time.Duration(math.MinInt64) + min1, err := bigtable.ParseDuration(min0.String()) + if err != nil || min0 != min1 { + t.Errorf("round-trip failed: %d => %q => %d, %v", min0, min0.String(), min1, err) + } + + for i := 0; i < 100; i++ { + // Resolutions finer than milliseconds will result in + // imprecise round-trips. + d0 := time.Duration(rand.Int31()) * time.Millisecond + s := d0.String() + d1, err := bigtable.ParseDuration(s) + if err != nil || d0 != d1 { + t.Errorf("round-trip failed: %d => %q => %d, %v", d0, s, d1, err) + } + } +} + +func BenchmarkParseDuration(b *testing.B) { + for i := 0; i < b.N; i++ { + bigtable.ParseDuration("9007199254.740993ms") + bigtable.ParseDuration("9007199254740993ns") + } +} diff --git a/google-beta/services/bigtable/resource_bigtable_gc_policy.go b/google-beta/services/bigtable/resource_bigtable_gc_policy.go index cdce223456..30621a47f6 100644 --- a/google-beta/services/bigtable/resource_bigtable_gc_policy.go +++ b/google-beta/services/bigtable/resource_bigtable_gc_policy.go @@ -7,6 +7,7 @@ import ( "encoding/json" "fmt" "log" + "reflect" "strings" "time" @@ -39,7 +40,7 @@ func resourceBigtableGCPolicyCustomizeDiffFunc(diff tpgresource.TerraformResourc if oldDuration == "" && newDuration != "" { // flatten the old days and the new duration to duration... if they are // equal then do nothing. - do, err := time.ParseDuration(newDuration.(string)) + do, err := ParseDuration(newDuration.(string)) if err != nil { return err } @@ -58,6 +59,36 @@ func resourceBigtableGCPolicyCustomizeDiffFunc(diff tpgresource.TerraformResourc return nil } +func gcRulesDiffSuppress(oldRules, newRules interface{}) bool { + var oldPolicyRaw map[string]interface{} + if err := json.Unmarshal([]byte(oldRules.(string)), &oldPolicyRaw); err != nil { + return false + } + oldPolicy, err := getGCPolicyFromJSON(oldPolicyRaw /*isTopLevel=*/, true) + if err != nil { + return false + } + var newPolicyRaw map[string]interface{} + if err := json.Unmarshal([]byte(newRules.(string)), &newPolicyRaw); err != nil { + return false + } + newPolicy, err := getGCPolicyFromJSON(newPolicyRaw /*isTopLevel=*/, true) + if err != nil { + return false + } + oldPolicyString, err := GcPolicyToGCRuleString(oldPolicy /*isTopLevel=*/, true) + if err != nil { + return false + } + newPolicyString, err := GcPolicyToGCRuleString(newPolicy /*isTopLevel=*/, true) + if err != nil { + return false + } + if reflect.DeepEqual(oldPolicyString, newPolicyString) { + return true + } + return false +} func resourceBigtableGCPolicyCustomizeDiff(_ context.Context, d *schema.ResourceDiff, meta interface{}) error { return resourceBigtableGCPolicyCustomizeDiffFunc(d) @@ -109,6 +140,9 @@ func ResourceBigtableGCPolicy() *schema.Resource { json, _ := structure.NormalizeJsonString(v) return json }, + DiffSuppressFunc: func(k, old, new string, _ *schema.ResourceData) bool { + return gcRulesDiffSuppress(old, new) + }, }, "mode": { Type: schema.TypeString, @@ -304,7 +338,7 @@ func resourceBigtableGCPolicyRead(d *schema.ResourceData, meta interface{}) erro // Only set `gc_rules`` when the legacy fields are not set. We are not planning to support legacy fields. maxAge := d.Get("max_age") maxVersion := d.Get("max_version") - if d.Get("mode") == "" && len(maxAge.([]interface{})) == 0 && len(maxVersion.([]interface{})) == 0 { + if len(maxAge.([]interface{})) == 0 && len(maxVersion.([]interface{})) == 0 { gcRuleString, err := GcPolicyToGCRuleString(fi.FullGCPolicy, true) if err != nil { return err @@ -506,7 +540,7 @@ func getGCPolicyFromJSON(inputPolicy map[string]interface{}, isTopLevel bool) (b if childPolicy["max_age"] != nil { maxAge := childPolicy["max_age"].(string) - duration, err := time.ParseDuration(maxAge) + duration, err := ParseDuration(maxAge) if err != nil { return nil, fmt.Errorf("invalid duration string: %v", maxAge) } @@ -588,7 +622,7 @@ func validateNestedPolicy(p map[string]interface{}, isTopLevel bool) error { func getMaxAgeDuration(values map[string]interface{}) (time.Duration, error) { d := values["duration"].(string) if d != "" { - return time.ParseDuration(d) + return ParseDuration(d) } days := values["days"].(int) diff --git a/google-beta/services/bigtable/resource_bigtable_gc_policy_internal_test.go b/google-beta/services/bigtable/resource_bigtable_gc_policy_internal_test.go index 198a3842bd..410fb76e82 100644 --- a/google-beta/services/bigtable/resource_bigtable_gc_policy_internal_test.go +++ b/google-beta/services/bigtable/resource_bigtable_gc_policy_internal_test.go @@ -4,6 +4,7 @@ package bigtable import ( "encoding/json" + "fmt" "testing" "github.com/hashicorp/terraform-provider-google-beta/google-beta/tpgresource" @@ -144,6 +145,47 @@ func TestUnitBigtableGCPolicy_getGCPolicyFromJSON(t *testing.T) { } } +func TestGCRulesDiffSuppress(t *testing.T) { + format := "{\"rules\": [{\"max_age\":\"%s\"}]}" + cases := map[string]struct { + Old, New string + ExpectDiffSuppress bool + }{ + "compound": { + Old: "1d1h", + New: "25h", + ExpectDiffSuppress: true, + }, + "s->h": { + Old: "3600s", + New: "1h", + ExpectDiffSuppress: true, + }, + "s->m": { + Old: "3600s", + New: "60m", + ExpectDiffSuppress: true, + }, + "compound-diff": { + Old: "1d1h", + New: "26h", + ExpectDiffSuppress: false, + }, + + "s->h-diff": { + Old: "3601s", + New: "1h", + ExpectDiffSuppress: false, + }, + } + + for tn, tc := range cases { + if gcRulesDiffSuppress(fmt.Sprintf(format, tc.Old), fmt.Sprintf(format, tc.New)) != tc.ExpectDiffSuppress { + t.Errorf("bad: %s, %q => %q expect DiffSuppress to return %t", tn, tc.Old, tc.New, tc.ExpectDiffSuppress) + } + } +} + type testUnitBigtableGCPolicyCustomizeDiffTestcase struct { testName string arraySize int @@ -181,6 +223,20 @@ var testUnitBigtableGCPolicyCustomizeDiffTestcases = []testUnitBigtableGCPolicyC newDuration: "72h", cleared: true, }, + { + testName: "DaysToDurationEqUsingDayDuration", + arraySize: 1, + oldDays: 3, + newDuration: "3d", + cleared: true, + }, + { + testName: "DaysToDurationEqUsingCompoundWeekDayDuration", + arraySize: 1, + oldDays: 8, + newDuration: "1w1d", + cleared: true, + }, { testName: "DaysToDurationNotEq", arraySize: 1, diff --git a/google-beta/services/bigtable/resource_bigtable_gc_policy_test.go b/google-beta/services/bigtable/resource_bigtable_gc_policy_test.go index ee2a5acd74..c991626734 100644 --- a/google-beta/services/bigtable/resource_bigtable_gc_policy_test.go +++ b/google-beta/services/bigtable/resource_bigtable_gc_policy_test.go @@ -176,7 +176,7 @@ func TestAccBigtableGCPolicy_gcRulesPolicy(t *testing.T) { familyName := fmt.Sprintf("tf-test-%s", acctest.RandString(t, 10)) gcRulesOriginal := "{\"mode\":\"intersection\",\"rules\":[{\"max_age\":\"10h\"},{\"max_version\":2}]}" - gcRulesUpdate := "{\"mode\":\"intersection\",\"rules\":[{\"max_age\":\"16h\"},{\"max_version\":1}]}" + gcRulesUpdate := "{\"mode\":\"intersection\",\"rules\":[{\"max_age\":\"40h\"},{\"max_version\":1}]}" acctest.VcrTest(t, resource.TestCase{ PreCheck: func() { acctest.AccTestPreCheck(t) }, @@ -255,14 +255,14 @@ var testUnitGcPolicyToGCRuleStringTestCases = []testUnitGcPolicyToGCRuleString{ errorExpected: false, }, { - name: "MaxVersionPolicyNotTopeLevel", + name: "MaxVersionPolicyNotTopLevel", policy: bigtable.MaxVersionsPolicy(1), topLevel: false, want: `{"max_version":1}`, errorExpected: false, }, { - name: "MaxAgePolicyNotTopeLevel", + name: "MaxAgePolicyNotTopLevel", policy: bigtable.MaxAgePolicy(time.Hour), topLevel: false, want: `{"max_age":"1h"}`, @@ -795,7 +795,7 @@ func testAccBigtableGCPolicy_gcRulesUpdate(instanceName, tableName, family strin table = google_bigtable_table.table.name column_family = "%s" - gc_rules = "{\"mode\":\"intersection\", \"rules\":[{\"max_age\":\"16h\"},{\"max_version\":1}]}" + gc_rules = "{\"mode\":\"intersection\", \"rules\":[{\"max_age\":\"1d16h\"},{\"max_version\":1}]}" } `, instanceName, instanceName, tableName, family, family) }