Skip to content
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

tests: record fields that are never set #3435

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
154 changes: 154 additions & 0 deletions tests/apichecks/crds_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,14 @@ import (

"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/crd/crdloader"
"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/test"
testcontroller "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/test/controller"
testgcp "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/test/gcp"
"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/test/resourcefixture"
testvariable "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/test/resourcefixture/variable"
"sigs.k8s.io/yaml"

apiextensions "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/util/sets"
"k8s.io/klog/v2"
)
Expand Down Expand Up @@ -390,3 +397,150 @@ func TestCRDShortNames(t *testing.T) {
want := strings.Join(errs, "\n")
test.CompareGoldenFile(t, "testdata/exceptions/shortnames.txt", want)
}

// Run this test with WRITE_GOLDEN_OUTPUT set to update the exceptions list.
func TestCRDFieldPresenceInUnstructured(t *testing.T) {
crds, err := crdloader.LoadAllCRDs()
if err != nil {
t.Fatalf("error loading CRDs: %v", err)
}

unstructs := loadUnstructs(t)

var errs []string
for _, crd := range crds {
for _, version := range crd.Spec.Versions {

if version.Name == "v1alpha1" {
continue
}

visitCRDVersion(version, func(field *CRDField) {
fieldPath := field.FieldPath

// Only consider fields under `spec`
if !strings.HasPrefix(fieldPath, ".spec.") {
return
}

// skip the resource id field
if strings.HasSuffix(fieldPath, ".resourceID") {
return
}

// Check for "Ref" fields
if strings.HasSuffix(fieldPath, "Ref") {
hasExternal := false
hasName := false

// Check for specific related fields
for _, obj := range unstructs {
if hasField(obj.Object, fieldPath+".external") {
hasExternal = true
}
if hasField(obj.Object, fieldPath+".name") {
hasName = true
}
}

// Only report an error if neither external nor name is set
if !hasExternal && !hasName {
errs = append(errs, fmt.Sprintf("[missing_field] crd=%s version=%v: field %q is not set; neither 'external' nor 'name' are set", crd.Name, version.Name, fieldPath))
}
return
}

// Skip non-terminal fields (fields with children or slices)
if field.props != nil {
if len(field.props.Properties) > 0 || field.props.Type == "object" {
return
}
if field.props.Type == "array" && field.props.Items != nil {
return // Skip the array itself; focus on its elements
}
}

// Any XYZRef field was already handled and handling the children will just double count
if strings.Contains(fieldPath, "Ref") {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's an example of this? Because I think you've already checked strings.HasSuffix(fieldPath, "Ref") { above...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah for references in particular I decided to call it set if the field ending in Ref specifies either external or name.

For instance, when we visit projectRef we run the special handling to determine if either is set.

but then in our depth first traversal we also visit the fields children again and can end up in a situation where we call out that external is not set when name is:

        +       [missing_field] crd=alloydbbackups.alloydb.cnrm.cloud.google.com version=v1beta1: field ".spec.clusterNameRef.external" is not set in unstructured objects

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha!

Suggested change
if strings.Contains(fieldPath, "Ref") {
// Any fooRef field was already handled and handling the children will just double count
// when we visit fooRef.namespace, fooRef.name and fooRef.external etc
if strings.Contains(fieldPath, "Ref.") {

return
}

// Check if field exists in any unstructured object
missing := true
for _, obj := range unstructs {
if hasField(obj.Object, fieldPath) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh, can we have another linter that checks for fields we hallucinated :-) (I.e. checks the other way round) There might be a refactor where we introduce something like CollectFields(obj *unstructured.Unstructured) sets[string]

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting so the check is there to make sure that the fields in the list are defined in the CRD, right ?

Copy link
Collaborator

@justinsb justinsb Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I think we've got most of them, but there really shouldn't be an exception list here. Though if you implement this, I think it's reasonable to first creating an exception list and then we can farm them out by type!

missing = false
break
}
}

if missing {
errs = append(errs, fmt.Sprintf("[missing_field] crd=%s version=%v: field %q is not set in unstructured objects", crd.Name, version.Name, fieldPath))
}
})
}
}

sort.Strings(errs)
want := strings.Join(errs, "\n")
test.CompareGoldenFile(t, "testdata/exceptions/missingfields.txt", want)
}

func loadUnstructs(t *testing.T) []*unstructured.Unstructured {
t.Helper()
unstructs := []*unstructured.Unstructured{}
fixtures := resourcefixture.Load(t)

for _, fixture := range fixtures {
fixture := fixture
createResource := bytesToUnstructured(t, fixture.Create)
updateResource := bytesToUnstructured(t, fixture.Update)

unstructs = append(unstructs, createResource, updateResource)
}

return unstructs
}

var (
testID = testvariable.NewUniqueID()
testProject = testgcp.GCPProject{ProjectID: "test-skip", ProjectNumber: 123456789}
)

func bytesToUnstructured(t *testing.T, bytes []byte) *unstructured.Unstructured {
t.Helper()

updatedBytes := testcontroller.ReplaceTestVars(t, bytes, testID, testProject)
return ToUnstruct(t, updatedBytes)
}

// hasField checks if an unstructured object contains the given field path.
func hasField(obj map[string]interface{}, fieldPath string) bool {
parts := strings.Split(strings.TrimPrefix(fieldPath, "."), ".")
current := obj

for _, part := range parts {
if next, ok := current[part]; ok {
if nextMap, ok := next.(map[string]interface{}); ok {
current = nextMap
} else {
return true
}
} else {
return false
}
}
return false
}

func ToUnstruct(t *testing.T, bytes []byte) *unstructured.Unstructured {
t.Helper()

var obj map[string]interface{}
err := yaml.Unmarshal(bytes, &obj)
if err != nil {
t.Errorf("error unmarshalling bytes %s to unstruct: %v", string(bytes), err)
}

return &unstructured.Unstructured{Object: obj}
}
Loading
Loading