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.

Sign up for GitHub

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

Jump to bottom

fix: modify olm-status-descriptor-test in scorecard #4009

Merged
merged 1 commit into from
Oct 22, 2020
Merged

fix: modify olm-status-descriptor-test in scorecard #4009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 16 additions & 0 deletions changelog/fragments/scorecard-status-descriptor-test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
# entries is a list of entries to include in
# release notes and/or the migration guide
entries:
- description: >
Modify `olm-status-descriptors-test` to only validate if the status-descriptors are present in CRD.
# kind is one of:
# - addition
# - change
# - deprecation
# - removal
# - bugfix
kind: "bugfix"
# Is this a breaking change?
breaking: false
38 changes: 26 additions & 12 deletions internal/scorecard/tests/bundle_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,8 @@ var _ = Describe("Basic and OLM tests", func() {

Describe("Test Status and Spec Descriptors", func() {
var (
cr unstructured.Unstructured
descriptor string
csv operatorsv1alpha1.ClusterServiceVersion
cr unstructured.Unstructured
csv operatorsv1alpha1.ClusterServiceVersion
)

csv = operatorsv1alpha1.ClusterServiceVersion{
Expand All @@ -157,8 +156,6 @@ var _ = Describe("Basic and OLM tests", func() {
},
}

descriptor = "status"

It("should pass when csv with owned cr and required fields is present", func() {
cr = unstructured.Unstructured{
Object: map[string]interface{}{
Expand All @@ -175,16 +172,33 @@ var _ = Describe("Basic and OLM tests", func() {
Group: "test.example.com",
})

result = checkOwnedCSVDescriptors(cr, &csv, descriptor, result)
result = checkOwnedCSVStatusDescriptor(cr, &csv, result)
Expect(result.State).To(Equal(scapiv1alpha3.PassState))
})

It("should pass when CR Object Descriptor is nil", func() {
cr := unstructured.Unstructured{
Object: nil,
}
cr.SetGroupVersionKind(schema.GroupVersionKind{
Kind: "TestKind",
Group: "test.example.com",
})

result = checkOwnedCSVStatusDescriptor(cr, &csv, result)
Expect(result.State).To(Equal(scapiv1alpha3.PassState))
})

It("should fail when CR Object Descriptor is nil", func() {
It("should fail when CR Object Descriptor is nil and CRD with given GVK cannot be found", func() {
cr := unstructured.Unstructured{
Object: nil,
}
cr.SetGroupVersionKind(schema.GroupVersionKind{
Kind: "TestKindNotPresent",
Group: "testnotpresent.example.com",
})

result = checkOwnedCSVDescriptors(cr, &csv, descriptor, result)
result = checkOwnedCSVStatusDescriptor(cr, &csv, result)
Expect(result.State).To(Equal(scapiv1alpha3.FailState))
})

Expand All @@ -197,7 +211,7 @@ var _ = Describe("Basic and OLM tests", func() {
},
}

result = checkOwnedCSVDescriptors(cr, &csv, descriptor, result)
result = checkOwnedCSVStatusDescriptor(cr, &csv, result)
Expect(result.State).To(Equal(scapiv1alpha3.FailState))
})

Expand All @@ -210,7 +224,7 @@ var _ = Describe("Basic and OLM tests", func() {
},
}

result = checkOwnedCSVDescriptors(cr, &csv, descriptor, result)
result = checkOwnedCSVStatusDescriptor(cr, &csv, result)
Expect(result.State).To(Equal(scapiv1alpha3.FailState))
})
It("should pass when required descriptor field is present in CR", func() {
Expand All @@ -229,7 +243,7 @@ var _ = Describe("Basic and OLM tests", func() {
Group: "test.example.com",
})

result = checkOwnedCSVDescriptors(cr, &csv, descriptor, result)
result = checkOwnedCSVSpecDescriptors(cr, &csv, result)
Expect(result.State).To(Equal(scapiv1alpha3.PassState))
})
It("should fail when required spec descriptor field is not present in CR", func() {
Expand All @@ -241,7 +255,7 @@ var _ = Describe("Basic and OLM tests", func() {
},
}

result = checkOwnedCSVDescriptors(cr, &csv, descriptor, result)
result = checkOwnedCSVSpecDescriptors(cr, &csv, result)
Expect(result.State).To(Equal(scapiv1alpha3.FailState))
})
It("should fail when CRs do not have spec field specified", func() {
Expand Down
73 changes: 48 additions & 25 deletions internal/scorecard/tests/olm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,44 @@ func checkCSVDescriptors(bundle *apimanifests.Bundle, r scapiv1alpha3.TestResult
}
r.Log += fmt.Sprintf("Loaded %d Custom Resources from alm-examples\n", len(crs))

// if the descriptor is status then check the status
// otherwise check the spec
for _, cr := range crs {
r = checkOwnedCSVDescriptors(cr, bundle.CSV, descriptor, r)
if descriptor == statusDescriptor {
r = checkOwnedCSVStatusDescriptor(cr, bundle.CSV, r)
} else {
r = checkOwnedCSVSpecDescriptors(cr, bundle.CSV, r)
}
}
return r
}

func checkOwnedCSVStatusDescriptor(cr unstructured.Unstructured, csv *operatorsv1alpha1.ClusterServiceVersion,
r scapiv1alpha3.TestResult) scapiv1alpha3.TestResult {

var crd *operatorsv1alpha1.CRDDescription

if cr.Object[statusDescriptor] == nil {
r.Suggestions = append(r.Suggestions, fmt.Sprintf("Status field can be added to the CR %s", cr.GetName()))
Copy link
Member

Choose a reason for hiding this comment

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

I thought the whole point of this PR was to remove this check entirely @varshaprasad96?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since having Status field is suggested according the k8s-api conventions, have modified this check to add suggestions. Also this unit test ensures that if Object descriptor is nil, the scorecard test still passes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of failing when status is not present in CR, suggestions are added. And we directly proceed with checking if the statusDescriptor is present in CRD or not. If the users input as described in #3999 is given with no status field in CR, this test would pass.

Copy link
Member

Choose a reason for hiding this comment

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

A CR from alm-examples should never have a status according to #3999 (comment), so we shouldn't be suggesting that one be added. Whether the CRD defines a status field for a resource in its validation is another story.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm..okay. Because based on the comment, I tried creating a resource through console UI - with and without status field. And it worked in both the cases. And to verify if such CSVs exist with status, just randomly went through the example community operators and found this.

But considering we remove that validation check for that, can we remove the suggestion, or do you suggest we remove the entire [StatusDescriptor](

operator-sdk/internal/scorecard/tests/olm.go

Lines 232 to 284 in 9d27e22

func checkOwnedCSVDescriptors(cr unstructured.Unstructured, csv *operatorsv1alpha1.ClusterServiceVersion,
descriptor string, r scapiv1alpha3.TestResult) scapiv1alpha3.TestResult {
if cr.Object[descriptor] == nil {
r.State = scapiv1alpha3.FailState
return r
}
block := cr.Object[descriptor].(map[string]interface{})
var crd *operatorsv1alpha1.CRDDescription
for _, owned := range csv.Spec.CustomResourceDefinitions.Owned {
if owned.Kind == cr.GetKind() {
crd = &owned
break
}
}
if crd == nil {
msg := fmt.Sprintf("Failed to find an owned CRD for CR %s with GVK %s", cr.GetName(), cr.GroupVersionKind().String())
r.Errors = append(r.Errors, msg)
r.State = scapiv1alpha3.FailState
return r
}
if descriptor == statusDescriptor {
for key := range block {
for _, statDesc := range crd.StatusDescriptors {
if statDesc.Path == key {
delete(block, key)
break
}
}
}
}
if descriptor == specDescriptor {
for key := range block {
for _, specDesc := range crd.SpecDescriptors {
if specDesc.Path == key {
delete(block, key)
break
}
}
}
}
for key := range block {
r.Errors = append(r.Errors, fmt.Sprintf("%s does not have a %s descriptor", key, descriptor))
r.Suggestions = append(r.Suggestions, fmt.Sprintf("Add a %s descriptor for %s", descriptor, key))
r.State = scapiv1alpha3.FailState
}
return r
}
) validation test?

Shouldn't we still check if statusDescriptor is present in CRD?

Copy link
Member

Choose a reason for hiding this comment

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

I shouldn't have said "never" above. The alm-examples can have status blocks, but they should be treated as not existing. Therefore I'm only suggesting the cr.Object[statusDescriptor] == nil condition be removed. Everything else is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it. Will create a follow up PR for this.

}

for _, owned := range csv.Spec.CustomResourceDefinitions.Owned {
if owned.Kind == cr.GetKind() {
crd = &owned
break
}
}

if crd == nil {
msg := fmt.Sprintf("Failed to find an owned CRD for CR %s with GVK %s", cr.GetName(), cr.GroupVersionKind().String())
r.Errors = append(r.Errors, msg)
r.State = scapiv1alpha3.FailState
return r
}

if len(crd.StatusDescriptors) == 0 {
r.Errors = append(r.Errors, fmt.Sprintf("%s does not have a status descriptor", crd.Name))
r.State = scapiv1alpha3.FailState
}

return r
Expand All @@ -229,15 +265,14 @@ func checkCSVDescriptors(bundle *apimanifests.Bundle, r scapiv1alpha3.TestResult
// TODO This is the validation we did in v1, but it looks like it only validates fields that
// are in the example CRs, if you have a field in your CRD that isn't present in one of your examples,
// I don't think it will be validated.
func checkOwnedCSVDescriptors(cr unstructured.Unstructured, csv *operatorsv1alpha1.ClusterServiceVersion,
descriptor string, r scapiv1alpha3.TestResult) scapiv1alpha3.TestResult {

if cr.Object[descriptor] == nil {
func checkOwnedCSVSpecDescriptors(cr unstructured.Unstructured, csv *operatorsv1alpha1.ClusterServiceVersion,
r scapiv1alpha3.TestResult) scapiv1alpha3.TestResult {
if cr.Object[specDescriptor] == nil {
r.State = scapiv1alpha3.FailState
return r
}

block := cr.Object[descriptor].(map[string]interface{})
block := cr.Object[specDescriptor].(map[string]interface{})

var crd *operatorsv1alpha1.CRDDescription
for _, owned := range csv.Spec.CustomResourceDefinitions.Owned {
Expand All @@ -254,30 +289,18 @@ func checkOwnedCSVDescriptors(cr unstructured.Unstructured, csv *operatorsv1alph
return r
}

if descriptor == statusDescriptor {
for key := range block {
for _, statDesc := range crd.StatusDescriptors {
if statDesc.Path == key {
delete(block, key)
break
}
}
}
}
if descriptor == specDescriptor {
for key := range block {
for _, specDesc := range crd.SpecDescriptors {
if specDesc.Path == key {
delete(block, key)
break
}
for key := range block {
for _, specDesc := range crd.SpecDescriptors {
if specDesc.Path == key {
delete(block, key)
break
}
}
}

for key := range block {
r.Errors = append(r.Errors, fmt.Sprintf("%s does not have a %s descriptor", key, descriptor))
r.Suggestions = append(r.Suggestions, fmt.Sprintf("Add a %s descriptor for %s", descriptor, key))
r.Errors = append(r.Errors, fmt.Sprintf("%s does not have a %s descriptor", key, specDescriptor))
r.Suggestions = append(r.Suggestions, fmt.Sprintf("Add a %s descriptor for %s", specDescriptor, key))
r.State = scapiv1alpha3.FailState
}
return r
Expand Down