Skip to content

Commit

Permalink
fix(storage): allow to use age=0 in OLM conditions (#6204)
Browse files Browse the repository at this point in the history 
Storage Go doesn't allow setting AgeInDays=0 after discussing with @tritone. This PR is the agreed upon path to support Age=0 in OLM conditions. It also notes that other int fields in OLM Conditions do not support 0 values.

Fixes: #6539, #6240
  • Loading branch information
@frankyn
frankyn committed Aug 26, 2022
1 parent 1937ba4 commit c85704f
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 61 deletions.
63 changes: 33 additions & 30 deletions storage/bucket.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -610,7 +610,12 @@ const (
//
// All configured conditions must be met for the associated action to be taken.
type LifecycleCondition struct {
// AllObjects is used to select all objects in a bucket by
// setting AgeInDays to 0.
AllObjects bool

// AgeInDays is the age of the object in days.
// If you want to set AgeInDays to `0` use AllObjects set to `true`.
AgeInDays int64

// CreatedBefore is the time the object was created.
Expand All @@ -628,10 +633,12 @@ type LifecycleCondition struct {

// DaysSinceCustomTime is the days elapsed since the CustomTime date of the
// object. This condition can only be satisfied if CustomTime has been set.
// Note: Using `0` as the value will be ignored by the library and not sent to the API.
DaysSinceCustomTime int64

// DaysSinceNoncurrentTime is the days elapsed since the noncurrent timestamp
// of the object. This condition is relevant only for versioned objects.
// Note: Using `0` as the value will be ignored by the library and not sent to the API.
DaysSinceNoncurrentTime int64

// Liveness specifies the object's liveness. Relevant only for versioned objects
Expand Down Expand Up @@ -663,6 +670,7 @@ type LifecycleCondition struct {
// If the value is N, this condition is satisfied when there are at least N
// versions (including the live version) newer than this version of the
// object.
// Note: Using `0` as the value will be ignored by the library and not sent to the API.
NumNewerVersions int64
}

Expand Down Expand Up @@ -1421,19 +1429,6 @@ func toCORSFromProto(rc []*storagepb.Bucket_Cors) []CORS {
return out
}

// Used to handle breaking change in Autogen Storage client OLM Age field
// from int64 to *int64 gracefully in the manual client
// TODO(#6240): Method should be removed once breaking change is made and introduced to this client
func setAgeCondition(age int64, ageField interface{}) {
c := reflect.ValueOf(ageField).Elem()
switch c.Kind() {
case reflect.Int64:
c.SetInt(age)
case reflect.Ptr:
c.Set(reflect.ValueOf(&age))
}
}

func toRawLifecycle(l Lifecycle) *raw.BucketLifecycle {
var rl raw.BucketLifecycle
if len(l.Rules) == 0 {
Expand All @@ -1455,7 +1450,15 @@ func toRawLifecycle(l Lifecycle) *raw.BucketLifecycle {
},
}

setAgeCondition(r.Condition.AgeInDays, &rr.Condition.Age)
// AllObjects takes precedent when both AllObjects and AgeInDays are set
// Rationale: If you've opted into using AllObjects, it makes sense that you
// understand the implications of how this option works with AgeInDays.
if r.Condition.AllObjects {
rr.Condition.Age = googleapi.Int64(0)
rr.Condition.ForceSendFields = []string{"Age"}
} else if r.Condition.AgeInDays > 0 {
rr.Condition.Age = googleapi.Int64(r.Condition.AgeInDays)
}

switch r.Condition.Liveness {
case LiveAndArchived:
Expand Down Expand Up @@ -1504,6 +1507,11 @@ func toProtoLifecycle(l Lifecycle) *storagepb.Bucket_Lifecycle {
},
}

// TODO(#6205): This may not be needed for gRPC
if r.Condition.AllObjects {
rr.Condition.AgeDays = proto.Int32(0)
}

switch r.Condition.Liveness {
case LiveAndArchived:
rr.Condition.IsLive = nil
Expand All @@ -1527,21 +1535,6 @@ func toProtoLifecycle(l Lifecycle) *storagepb.Bucket_Lifecycle {
return &rl
}

// Used to handle breaking change in Autogen Storage client OLM Age field
// from int64 to *int64 gracefully in the manual client
// TODO(#6240): Method should be removed once breaking change is made and introduced to this client
func getAgeCondition(ageField interface{}) int64 {
v := reflect.ValueOf(ageField)
if v.Kind() == reflect.Int64 {
return v.Interface().(int64)
} else if v.Kind() == reflect.Ptr {
if val, ok := v.Interface().(*int64); ok {
return *val
}
}
return 0
}

func toLifecycle(rl *raw.BucketLifecycle) Lifecycle {
var l Lifecycle
if rl == nil {
Expand All @@ -1562,7 +1555,12 @@ func toLifecycle(rl *raw.BucketLifecycle) Lifecycle {
NumNewerVersions: rr.Condition.NumNewerVersions,
},
}
r.Condition.AgeInDays = getAgeCondition(rr.Condition.Age)
if rr.Condition.Age != nil {
r.Condition.AgeInDays = *rr.Condition.Age
if *rr.Condition.Age == 0 {
r.Condition.AllObjects = true
}
}

if rr.Condition.IsLive == nil {
r.Condition.Liveness = LiveAndArchived
Expand Down Expand Up @@ -1608,6 +1606,11 @@ func toLifecycleFromProto(rl *storagepb.Bucket_Lifecycle) Lifecycle {
},
}

// TODO(#6205): This may not be needed for gRPC
if rr.GetCondition().GetAgeDays() == 0 {
r.Condition.AllObjects = true
}

if rr.GetCondition().IsLive == nil {
r.Condition.Liveness = LiveAndArchived
} else if rr.GetCondition().GetIsLive() {
Expand Down
55 changes: 24 additions & 31 deletions storage/bucket_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,7 @@ import (
raw "google.golang.org/api/storage/v1"
)

// TODO(#6539): re-enable tests after breaking change is released and AgeInDays is a int64*
func TestBucketAttrsToRawBucket(t *testing.T) {
t.Skip("TestBucketAttrsToRawBucket skipped: https://github.com/googleapis/google-cloud-go/issues/6539")
t.Parallel()
attrs := &BucketAttrs{
Name: "name",
Expand Down Expand Up @@ -119,6 +117,13 @@ func TestBucketAttrsToRawBucket(t *testing.T) {
Condition: LifecycleCondition{
AgeInDays: 20,
},
}, {
Action: LifecycleAction{
Type: DeleteAction,
},
Condition: LifecycleCondition{
AllObjects: true,
},
}},
},
}
Expand Down Expand Up @@ -163,7 +168,7 @@ func TestBucketAttrsToRawBucket(t *testing.T) {
StorageClass: "NEARLINE",
},
Condition: &raw.BucketLifecycleRuleCondition{
//Age: 10,
Age: googleapi.Int64(10),
IsLive: googleapi.Bool(true),
CreatedBefore: "2017-01-02",
MatchesStorageClass: []string{"STANDARD"},
Expand Down Expand Up @@ -199,7 +204,7 @@ func TestBucketAttrsToRawBucket(t *testing.T) {
Type: DeleteAction,
},
Condition: &raw.BucketLifecycleRuleCondition{
//Age: 10,
Age: googleapi.Int64(10),
MatchesPrefix: []string{"testPrefix"},
MatchesSuffix: []string{"testSuffix"},
NumNewerVersions: 3,
Expand All @@ -218,7 +223,16 @@ func TestBucketAttrsToRawBucket(t *testing.T) {
Type: AbortIncompleteMPUAction,
},
Condition: &raw.BucketLifecycleRuleCondition{
//Age: 20,
Age: googleapi.Int64(20),
},
},
{
Action: &raw.BucketLifecycleRuleAction{
Type: DeleteAction,
},
Condition: &raw.BucketLifecycleRuleCondition{
Age: googleapi.Int64(0),
ForceSendFields: []string{"Age"},
},
},
},
Expand Down Expand Up @@ -350,9 +364,7 @@ func TestBucketAttrsToRawBucket(t *testing.T) {
}
}

// TODO(#6539): re-enable tests after breaking change is released and AgeInDays is a int64*
func TestBucketAttrsToUpdateToRawBucket(t *testing.T) {
t.Skip("TestBucketAttrsToUpdateToRawBucket skipped: https://github.com/googleapis/google-cloud-go/issues/6539")
t.Parallel()
au := &BucketAttrsToUpdate{
VersioningEnabled: false,
Expand Down Expand Up @@ -408,12 +420,12 @@ func TestBucketAttrsToUpdateToRawBucket(t *testing.T) {
Lifecycle: &raw.BucketLifecycle{
Rule: []*raw.BucketLifecycleRule{
{
Action: &raw.BucketLifecycleRuleAction{Type: "Delete"},
//Condition: &raw.BucketLifecycleRuleCondition{Age: 30},
Action: &raw.BucketLifecycleRuleAction{Type: "Delete"},
Condition: &raw.BucketLifecycleRuleCondition{Age: googleapi.Int64(30)},
},
{
Action: &raw.BucketLifecycleRuleAction{Type: AbortIncompleteMPUAction},
//Condition: &raw.BucketLifecycleRuleCondition{Age: 13},
Action: &raw.BucketLifecycleRuleAction{Type: AbortIncompleteMPUAction},
Condition: &raw.BucketLifecycleRuleCondition{Age: googleapi.Int64(13)},
},
},
},
Expand Down Expand Up @@ -564,26 +576,7 @@ func TestBucketAttrsToUpdateToRawBucket(t *testing.T) {
}
}

func TestAgeConditionBackwardCompat(t *testing.T) {
var ti int64
var want int64 = 100
setAgeCondition(want, &ti)
if getAgeCondition(ti) != want {
t.Fatalf("got %v, want %v", getAgeCondition(ti), want)
}

var tp *int64
want = 10
setAgeCondition(want, &tp)
if getAgeCondition(tp) != want {
t.Fatalf("got %v, want %v", getAgeCondition(tp), want)
}

}

// TODO(#6539): re-enable tests after breaking change is released and AgeInDays is a int64*
func TestNewBucket(t *testing.T) {
t.Skip("TestNewBucket skipped: https://github.com/googleapis/google-cloud-go/issues/6539")
labels := map[string]string{"a": "b"}
matchClasses := []string{"STANDARD"}
aTime := time.Date(2017, 1, 2, 0, 0, 0, 0, time.UTC)
Expand All @@ -605,7 +598,7 @@ func TestNewBucket(t *testing.T) {
StorageClass: "NEARLINE",
},
Condition: &raw.BucketLifecycleRuleCondition{
// Age: 10,
Age: googleapi.Int64(10),
IsLive: googleapi.Bool(true),
CreatedBefore: "2017-01-02",
MatchesStorageClass: matchClasses,
Expand Down
11 changes: 11 additions & 0 deletions storage/integration_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -309,6 +309,13 @@ func TestIntegration_BucketCreateDelete(t *testing.T) {
MatchesSuffix: []string{"testSuffix"},
NumNewerVersions: 3,
},
}, {
Action: LifecycleAction{
Type: DeleteAction,
},
Condition: LifecycleCondition{
AllObjects: true,
},
}},
}

Expand Down Expand Up @@ -442,6 +449,10 @@ func TestIntegration_BucketLifecycle(t *testing.T) {
Action: LifecycleAction{Type: AbortIncompleteMPUAction},
Condition: LifecycleCondition{AgeInDays: 30},
},
{
Action: LifecycleAction{Type: DeleteAction},
Condition: LifecycleCondition{AllObjects: true},
},
},
}

Expand Down

0 comments on commit c85704f

Please sign in to comment.