From 9e221d02588afef7387691bd73a4c201f395b9a1 Mon Sep 17 00:00:00 2001 From: Katarzyna Lach Date: Wed, 27 Dec 2023 14:27:57 +0000 Subject: [PATCH] Add check that Alpha and Beta are subsets of GA fields Extend CheckSchema function with Assertion that Alpha and Beta type are subsets of GA. --- pkg/cloud/api/check.go | 41 +++ pkg/cloud/api/check_test.go | 264 ++++++++++++++++++ .../converter_test_types/converter-structs.go | 52 ++++ pkg/cloud/api/diff.go | 38 ++- pkg/cloud/api/diff_test.go | 102 +++++++ pkg/cloud/api/mutable_resource.go | 36 ++- pkg/cloud/api/resource_test.go | 47 ++++ 7 files changed, 568 insertions(+), 12 deletions(-) create mode 100644 pkg/cloud/api/converter_test_types/converter-structs.go diff --git a/pkg/cloud/api/check.go b/pkg/cloud/api/check.go index f05e702f..12197b2f 100644 --- a/pkg/cloud/api/check.go +++ b/pkg/cloud/api/check.go @@ -171,3 +171,44 @@ func checkSchema(t reflect.Type) error { return nil } + +// CheckStructuralSubset checks if type From is the subset of To type. Type is a +// subset of another if it contains fields with the same type and name. This +// function is not symmetric. Path parameter is used for better error reporting. +func CheckStructuralSubset(p Path, from, to reflect.Type) error { + if from.Kind() != to.Kind() { + return fmt.Errorf("%s has different type: %v != %v", p, from.Kind(), to.Kind()) + } + if isBasicT(from) { + return nil + } + switch from.Kind() { + case reflect.Pointer: + return CheckStructuralSubset(p.Pointer(), from.Elem(), to.Elem()) + + case reflect.Struct: + for i := 0; i < from.NumField(); i++ { + af := from.Field(i) + bf, exist := to.FieldByName(af.Name) + if !exist { + return fmt.Errorf("%s: type %T does not have field %v", p.String(), to, af.Name) + } + if err := CheckStructuralSubset(p.Field(af.Name), af.Type, bf.Type); err != nil { + return err + } + } + return nil + + case reflect.Slice, reflect.Array: + return CheckStructuralSubset(p.AnySliceIndex(), from.Elem(), to.Elem()) + + case reflect.Map: + path := p.AnyMapIndex() + err := CheckStructuralSubset(path, from.Key(), to.Key()) + if err != nil { + return err + } + return CheckStructuralSubset(path, from.Elem(), to.Elem()) + } + return fmt.Errorf("%s Unsupported type %v", p.String(), from.Kind()) +} diff --git a/pkg/cloud/api/check_test.go b/pkg/cloud/api/check_test.go index 76d2d10e..8209f929 100644 --- a/pkg/cloud/api/check_test.go +++ b/pkg/cloud/api/check_test.go @@ -19,6 +19,8 @@ package api import ( "reflect" "testing" + + teststruct "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/api/converter_test_types" ) func TestCheckFieldsAreSet(t *testing.T) { @@ -272,3 +274,265 @@ func TestCheckSchema(t *testing.T) { }) } } + +func TestConvertToT(t *testing.T) { + t.Parallel() + intVar := int(1) + type sti struct { + I int + LS []string + } + type stiB struct { + I int + LS string + } + + type St struct { + I int + St sti + PSt *sti + PS *string + LS []string + M map[string]string + } + type StA struct { + I int + RT *sti + PS *string + LS []string + M map[string]string + } + type stB struct { + I int + St sti + RT *sti + } + + for _, tc := range []struct { + name string + a any + b any + wantErr bool + }{ + { + name: "basic - same", + a: int(1), + b: int(1), + }, + { + name: "basic - different", + a: int(1), + b: "1", + wantErr: true, + }, + { + name: "basic - pointer", + a: int(1), + b: &intVar, + wantErr: true, + }, + { + name: "array - same", + a: [2]int{1, 2}, + b: [2]int{1, 2}, + }, + { + name: "array - different", + a: [2]int{1, 2}, + b: [2]string{"1", "2"}, + wantErr: true, + }, + { + name: "array - slice", + a: [2]int{1, 2}, + b: []int{}, + wantErr: true, + }, + { + name: "slice - same", + a: []int{}, + b: []int{}, + }, + { + name: "slice - different", + a: []int{}, + b: []string{}, + wantErr: true, + }, + { + name: "slice - literal", + a: []int{}, + b: int(1), + wantErr: true, + }, + { + name: "slice - struct", + a: []St{}, + b: St{}, + wantErr: true, + }, + { + name: "slice with pointer - same", + a: []*St{}, + b: []*St{}, + }, + { + name: "slice with pointer - different", + a: []*St{}, + b: []*StA{}, + wantErr: true, + }, + { + name: "slice with pointer - slice with struct", + a: []*St{}, + b: []St{}, + wantErr: true, + }, + { + name: "slice with pointer - pointer to struct", + a: []*St{}, + b: &St{}, + wantErr: true, + }, + { + name: "struct - same", + a: St{}, + b: St{}, + }, + { + name: "struct - missing field", + a: St{}, + b: StA{}, + wantErr: true, + }, + { + name: "struct - subType", + a: teststruct.St{}, + b: St{}, + }, + { + name: "struct - inner struct different", + a: teststruct.StA{}, + b: StA{}, + wantErr: true, + }, + { + name: "pointer - same", + a: &St{}, + b: &St{}, + }, + { + name: "pointer - different", + a: &St{}, + b: &StA{}, + wantErr: true, + }, + { + name: "pointer - inner struct different", + a: &teststruct.StA{}, + b: &StA{}, + wantErr: true, + }, + { + name: "pointer - type", + a: &St{}, + b: St{}, + wantErr: true, + }, + { + name: "map - same", + a: map[string]St{}, + b: map[string]St{}, + }, + { + name: "map - different key", + a: map[string]St{}, + b: map[int]St{}, + wantErr: true, + }, + { + name: "map - different value", + a: map[string]int{}, + b: map[string]St{}, + wantErr: true, + }, + { + name: "map - value as a pointer", + a: map[string]*St{}, + b: map[string]St{}, + wantErr: true, + }, + { + name: "map - key as a pointer", + a: map[*St]string{}, + b: map[*St]string{}, + }, + { + name: "map - pointer to map", + a: map[int]string{}, + b: &map[int]string{}, + wantErr: true, + }, + { + name: "map - slice of key type", + a: map[int]string{}, + b: []int{}, + wantErr: true, + }, + { + name: "map - slice of value type", + a: map[int]string{}, + b: []string{}, + wantErr: true, + }, + { + name: "map - basic key type", + a: map[int]string{}, + b: int(0), + wantErr: true, + }, + { + name: "map - basic value type", + a: map[int]string{}, + b: "string", + wantErr: true, + }, + { + name: "channel", + a: make(chan int), + b: make(chan int), + wantErr: true, + }, + { + name: "func", + a: func() {}, + b: func() {}, + wantErr: true, + }, + { + name: "bool", + a: true, + b: false, + }, + { + name: "bool - int", + a: true, + b: intVar, + wantErr: true, + }, + { + name: "bool - pointer", + a: true, + b: &intVar, + wantErr: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := CheckStructuralSubset(Path{}, reflect.TypeOf(tc.a), reflect.TypeOf(tc.b)) + gotErr := err != nil + if gotErr != tc.wantErr { + t.Fatalf("CheckStructuralSubset() = %v; gotErr = %t, want %t", err, gotErr, tc.wantErr) + } + }) + } +} diff --git a/pkg/cloud/api/converter_test_types/converter-structs.go b/pkg/cloud/api/converter_test_types/converter-structs.go new file mode 100644 index 00000000..cb5a92d1 --- /dev/null +++ b/pkg/cloud/api/converter_test_types/converter-structs.go @@ -0,0 +1,52 @@ +/* +Copyright 2024 Google LLC + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + +https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package convert_test_types + +// This file contains structs for testing conversion. Type to test conversion +// need to have the same names but different schema that's new file with +// duplicated structs are needed. +type St struct { + I int + PS *string + LS []string + M map[string]string +} + +type sti struct { + C int + LS []string +} + +type StA struct { + I int + RT *sti + PS *string + LS []string + M map[string]string +} + +type sti2 struct { + AI string + BI int +} +type StBI struct { + Name string + SelfLink string + SI *sti2 + NullFields []string + ForceSendFields []string +} diff --git a/pkg/cloud/api/diff.go b/pkg/cloud/api/diff.go index 248f1f9c..2f377dad 100644 --- a/pkg/cloud/api/diff.go +++ b/pkg/cloud/api/diff.go @@ -41,6 +41,18 @@ func diff[T any](a, b *T, trait *FieldTraits) (*DiffResult, error) { return d.result, nil } +func diffStructs[A any, B any](a *A, b *B) (*DiffResult, error) { + d := &differ[A]{ + traits: &FieldTraits{}, + result: &DiffResult{}, + } + err := d.do(Path{}, reflect.ValueOf(a), reflect.ValueOf(b)) + if err != nil { + return nil, err + } + return d.result, nil +} + // DiffResult gives a list of elements that differ. type DiffResult struct { Items []DiffItem @@ -55,10 +67,22 @@ func (r *DiffResult) add(state DiffItemState, p Path, a, b reflect.Value) { Path: p, } if a.IsValid() { - di.A = a.Interface() + // Interface() will panic if is called on unexported types in this case + // the best we can do is to pass its name to the result. + if a.CanInterface() { + di.A = a.Interface() + } else { + di.A = a.String() + } } if b.IsValid() { - di.B = b.Interface() + // Interface() will panic if is called on unexported types in this case + // the best we can do is to pass its name to the result. + if b.CanInterface() { + di.B = b.Interface() + } else { + di.B = b.String() + } } r.Items = append(r.Items, di) } @@ -124,18 +148,22 @@ func (d *differ[T]) do(p Path, av, bv reflect.Value) error { for i := 0; i < av.NumField(); i++ { afv := av.Field(i) aft := av.Type().Field(i) - bfv := bv.Field(i) - fp := p.Field(aft.Name) if aft.Name == "NullFields" || aft.Name == "ForceSendFields" { continue } + fp := p.Field(aft.Name) switch d.traits.fieldType(fp) { case FieldTypeOutputOnly, FieldTypeSystem: continue } + bfv := bv.FieldByName(aft.Name) + if !bfv.IsValid() { + d.result.add(DiffItemOnlyInA, p, av, bv) + continue + } if err := d.do(fp, afv, bfv); err != nil { return fmt.Errorf("differ struct %p: %w", fp, err) } @@ -178,7 +206,7 @@ func (d *differ[T]) do(p Path, av, bv reflect.Value) error { for _, amk := range av.MapKeys() { amv := av.MapIndex(amk) bmv := bv.MapIndex(amk) - mp := p.MapIndex(amk.Interface()) + mp := p.MapIndex(amk) if !bmv.IsValid() { d.result.add(DiffItemDifferent, mp, amv, bmv) diff --git a/pkg/cloud/api/diff_test.go b/pkg/cloud/api/diff_test.go index 8cba72c1..7f446a90 100644 --- a/pkg/cloud/api/diff_test.go +++ b/pkg/cloud/api/diff_test.go @@ -190,3 +190,105 @@ func TestDiff(t *testing.T) { }) } } + +func TestDiffForStructWithUnexportedFields(t *testing.T) { + t.Parallel() + + type sti struct { + A int + b []string + } + + type st struct { + I int + S sti + + ls []string + M map[string]string + m map[string]string + si sti + } + + for _, tc := range []struct { + name string + a st + b st + wantDiff bool + wantErr bool + }{ + { + name: "empty", + a: st{}, + b: st{}, + wantDiff: false, + }, + { + name: "exported eq", + a: st{I: 1}, + b: st{I: 1}, + wantDiff: false, + }, + { + name: "exported not eq", + a: st{I: 1}, + b: st{I: 3}, + wantDiff: true, + }, + { + name: "unexported map eq", + a: st{m: map[string]string{"a": "b"}}, + b: st{m: map[string]string{"a": "b"}}, + wantDiff: false, + }, + { + name: "exported map not eq", + a: st{M: map[string]string{"a": "b"}}, + b: st{M: map[string]string{"a": "c"}}, + wantDiff: true, + }, + { + name: "unexported map not eq", + a: st{m: map[string]string{"a": "b"}}, + b: st{m: map[string]string{"c": "b"}}, + wantDiff: true, + }, + { + name: "unexported struct eq", + a: st{si: sti{A: 5}}, + b: st{si: sti{A: 5}}, + wantDiff: false, + }, + { + name: "unexported struct not eq", + a: st{si: sti{A: 5}}, + b: st{si: sti{A: 6}}, + wantDiff: true, + }, + { + name: "unexported embedded in struct eq", + a: st{S: sti{A: 5, b: []string{"6"}}}, + b: st{S: sti{A: 5, b: []string{"6"}}}, + wantDiff: false, + }, + { + name: "unexported embedded in struct not eq", + a: st{S: sti{A: 5, b: []string{"6"}}}, + b: st{S: sti{A: 5, b: []string{"7"}}}, + wantDiff: true, + }, + } { + t.Run(tc.name, func(t *testing.T) { + r, err := diff(&tc.a, &tc.b, nil) + gotErr := err != nil + if gotErr != tc.wantErr { + t.Fatalf("Diff() = %v; gotErr = %t, want %t", err, gotErr, tc.wantErr) + } + if gotErr { + return + } + if r.HasDiff() != tc.wantDiff { + t.Errorf("HasDiff = %t, want %t. diff = %s", r.HasDiff(), tc.wantDiff, pretty.Sprint(r)) + } + }) + } +} diff --git a/pkg/cloud/api/mutable_resource.go b/pkg/cloud/api/mutable_resource.go index e727347d..c1a72069 100644 --- a/pkg/cloud/api/mutable_resource.go +++ b/pkg/cloud/api/mutable_resource.go @@ -191,18 +191,40 @@ func (u *mutableResource[GA, Alpha, Beta]) CheckSchema() error { if err != nil { return err } - err = checkSchema(reflect.TypeOf(&u.alpha)) - if err != nil { - return err + ga, _ := u.ToGA() + + if !isPlaceholderType(u.alpha) { + err = checkSchema(reflect.TypeOf(&u.alpha)) + if err != nil { + return err + } + alpha, _ := u.ToAlpha() + err = checkSubsetOf(ga, alpha) + if err != nil { + return fmt.Errorf("checkSubsetOf(%T, %T) = %v, want nil", ga, alpha, err) + } } - err = checkSchema(reflect.TypeOf(&u.beta)) - if err != nil { - return err + if !isPlaceholderType(u.beta) { + err = checkSchema(reflect.TypeOf(&u.beta)) + if err != nil { + return err + } + beta, _ := u.ToBeta() + err = checkSubsetOf(ga, beta) + + if err != nil { + return fmt.Errorf("checkSubsetOf(%T, %T) = %v, want nil", ga, beta, err) + } } - // TODOD(kl52752) Add validation that GA is a subset of Beta and Alpha. + return nil } +func checkSubsetOf[T1 any, T2 any](t1 *T1, t2 *T2) error { + + return CheckStructuralSubset(Path{}, reflect.TypeOf(t1), reflect.TypeOf(t2)) +} + func (u *mutableResource[GA, Alpha, Beta]) ResourceID() *cloud.ResourceID { return u.resourceID } const ( diff --git a/pkg/cloud/api/resource_test.go b/pkg/cloud/api/resource_test.go index ad99b4a6..e3bdcf38 100644 --- a/pkg/cloud/api/resource_test.go +++ b/pkg/cloud/api/resource_test.go @@ -20,6 +20,7 @@ import ( "testing" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud" + teststruct "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/api/converter_test_types" "github.com/GoogleCloudPlatform/k8s-cloud-provider/pkg/cloud/meta" "github.com/google/go-cmp/cmp" ) @@ -708,6 +709,14 @@ func TestResourceSetX(t *testing.T) { func TestResourceCheckSchema(t *testing.T) { t.Parallel() + type sti struct { + AI string + BI string + } + type sti2 struct { + AI string + CI int + } type st struct { Name string @@ -732,6 +741,29 @@ func TestResourceCheckSchema(t *testing.T) { NullFields []string ForceSendFields []string } + type stC struct { + Name string + SelfLink string + C int + NullFields []string + ForceSendFields []string + } + type StBI struct { + Name string + SelfLink string + SI *sti2 + NullFields []string + ForceSendFields []string + } + type stCI struct { + Name string + SelfLink string + SI *sti + C int + NullFields []string + ForceSendFields []string + } + type invalid struct { I chan int NullFields []string @@ -780,6 +812,21 @@ func TestResourceCheckSchema(t *testing.T) { res: newTestResource[PlaceholderType, stA, stB](nil), wantErr: true, }, + { + name: "invalid schema alpha is not a subset of GA", + res: newTestResource[st, stC, stB](nil), + wantErr: true, + }, + { + name: "invalid schema beta is not a subset of GA", + res: newTestResource[st, stA, stC](nil), + wantErr: true, + }, + { + name: "invalid schema - embedded struct mismatch", + res: newTestResource[StBI, StBI, teststruct.StBI](nil), + wantErr: true, + }, } { t.Run(tc.name, func(t *testing.T) { err := tc.res.CheckSchema()