Skip to content

pkg/payload: Log manifest exclusion #712

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

Merged
openshift-merge-robot merged 1 commit into from
Feb 9, 2022
Merged

pkg/payload: Log manifest exclusion #712

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 20 additions & 15 deletions pkg/payload/payload.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,19 +179,21 @@ func LoadUpdate(dir, releaseImage, excludeIdentifier string, includeTechPreview
errs = append(errs, fmt.Errorf("parse %s: %w", file.Name(), err))
continue
}
originalFilename := filepath.Base(file.Name())
for i := range ms {
ms[i].OriginalFilename = originalFilename
}

// Filter out manifests that should be excluded based on annotation
filteredMs := []manifest.Manifest{}
for _, manifest := range ms {
if shouldExclude(excludeIdentifier, includeTechPreview, profile, &manifest) {
if err := include(excludeIdentifier, includeTechPreview, profile, &manifest); err != nil {
klog.V(5).Infof("excluding %s group=%s kind=%s namespace=%s name=%s: %v\n", manifest.OriginalFilename, manifest.GVK.Group, manifest.GVK.Kind, manifest.Obj.GetNamespace(), manifest.Obj.GetName(), err)
continue
}
filteredMs = append(filteredMs, manifest)
}
ms = filteredMs
for i := range ms {
ms[i].OriginalFilename = filepath.Base(file.Name())
}
manifests = append(manifests, ms...)
manifests = append(manifests, filteredMs...)
}
}

Expand All @@ -213,31 +215,34 @@ func LoadUpdate(dir, releaseImage, excludeIdentifier string, includeTechPreview
return payload, nil
}

func shouldExclude(excludeIdentifier string, includeTechPreview bool, profile string, manifest *manifest.Manifest) bool {
func include(excludeIdentifier string, includeTechPreview bool, profile string, manifest *manifest.Manifest) error {
annotations := manifest.Obj.GetAnnotations()
if annotations == nil {
return true
return errors.New("no annotations")
}

excludeAnnotation := fmt.Sprintf("exclude.release.openshift.io/%s", excludeIdentifier)
if annotations[excludeAnnotation] == "true" {
return true
if v := annotations[excludeAnnotation]; v == "true" {
return fmt.Errorf("%s=%s", excludeAnnotation, v)
}

featureGateAnnotationValue, featureGateAnnotationExists := annotations["release.openshift.io/feature-gate"]
featureGateAnnotation := "release.openshift.io/feature-gate"
Copy link

@vrutkovs vrutkovs Dec 13, 2021

Choose a reason for hiding this comment

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

Move it to const?

Copy link
Member Author

@wking wking Dec 21, 2021

Choose a reason for hiding this comment

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

This is our one occurrence of this string outside of comments and tests. Did you want the tests to consume the constant, once we have it? They are hard-coding TechPreviewNoUpgrade today too:

$ git grep release.openshift.io/feature-gate | cat
...
pkg/payload/payload_test.go:                            "release.openshift.io/feature-gate":                           "TechPreviewNoUpgrade",
...

and there's already an openshift/api constant for that, so I expected the test-suite to prefer string literals to catch thing like "we typo'ed the constant value". And if we expect external consumers, I'd rather create the constant in openshift/api, or openshift/library-go instead of asking external consumers to import the CVO's pkg/payload. Thoughts?

featureGateAnnotationValue, featureGateAnnotationExists := annotations[featureGateAnnotation]
if featureGateAnnotationValue == "TechPreviewNoUpgrade" && !includeTechPreview {
Copy link

@vrutkovs vrutkovs Dec 13, 2021

Choose a reason for hiding this comment

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

Same - lets define it as a const to simplify lookup / re-import if needed

return true
return fmt.Errorf("tech-preview excluded, and %s=%s", featureGateAnnotation, featureGateAnnotationValue)
}
// never include the manifest if the feature-gate annotation is outside of allowed values (only TechPreviewNoUpgrade is currently allowed)
if featureGateAnnotationExists && featureGateAnnotationValue != "TechPreviewNoUpgrade" {
return true
return fmt.Errorf("unrecognized value %s=%s", featureGateAnnotation, featureGateAnnotationValue)
}

profileAnnotation := fmt.Sprintf("include.release.openshift.io/%s", profile)
if val, ok := annotations[profileAnnotation]; ok && val == "true" {
return false
return nil
} else if ok {
return fmt.Errorf("unrecognized value %s=%s", profileAnnotation, val)
}
return true
return fmt.Errorf("%s unset", profileAnnotation)
}

// ValidateDirectory checks if a directory can be a candidate update by
Expand Down
28 changes: 14 additions & 14 deletions pkg/payload/payload_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package payload

import (
"errors"
"io/ioutil"
"path/filepath"
"reflect"
Expand All @@ -17,7 +18,7 @@ import (
"github.com/openshift/library-go/pkg/manifest"
)

func Test_loadUpdatePayload(t *testing.T) {
func TestLoadUpdate(t *testing.T) {
type args struct {
dir string
releaseImage string
Expand Down Expand Up @@ -132,15 +133,15 @@ func mustRead(path string) []byte {
return data
}

func Test_Exclude(t *testing.T) {
func Test_include(t *testing.T) {
tests := []struct {
name string
exclude string
includeTechPreview bool
profile string
annotations map[string]interface{}

isExcluded bool
expected error
}{
{
name: "exclusion identifier set",
Expand All @@ -149,13 +150,13 @@ func Test_Exclude(t *testing.T) {
annotations: map[string]interface{}{
"exclude.release.openshift.io/identifier": "true",
"include.release.openshift.io/self-managed-high-availability": "true"},
isExcluded: true,
expected: errors.New("exclude.release.openshift.io/identifier=true"),
},
{
name: "profile selection works",
profile: "single-node",
annotations: map[string]interface{}{"include.release.openshift.io/self-managed-high-availability": "true"},
isExcluded: true,
expected: errors.New("include.release.openshift.io/single-node unset"),
},
{
name: "profile selection works included",
Expand All @@ -169,7 +170,7 @@ func Test_Exclude(t *testing.T) {
"include.release.openshift.io/self-managed-high-availability": "true",
"release.openshift.io/feature-gate": "TechPreviewNoUpgrade",
},
isExcluded: true,
expected: errors.New("tech-preview excluded, and release.openshift.io/feature-gate=TechPreviewNoUpgrade"),
},
{
name: "correct techpreview value is included if techpreview on",
Expand All @@ -179,7 +180,6 @@ func Test_Exclude(t *testing.T) {
"include.release.openshift.io/self-managed-high-availability": "true",
"release.openshift.io/feature-gate": "TechPreviewNoUpgrade",
},
isExcluded: false,
},
{
name: "incorrect techpreview value is not excluded if techpreview off",
Expand All @@ -188,7 +188,7 @@ func Test_Exclude(t *testing.T) {
"include.release.openshift.io/self-managed-high-availability": "true",
"release.openshift.io/feature-gate": "Other",
},
isExcluded: true,
expected: errors.New("unrecognized value release.openshift.io/feature-gate=Other"),
},
{
name: "incorrect techpreview value is not excluded if techpreview on",
Expand All @@ -198,19 +198,19 @@ func Test_Exclude(t *testing.T) {
"include.release.openshift.io/self-managed-high-availability": "true",
"release.openshift.io/feature-gate": "Other",
},
isExcluded: true,
expected: errors.New("unrecognized value release.openshift.io/feature-gate=Other"),
},
{
name: "default profile selection excludes without annotation",
profile: DefaultClusterProfile,
annotations: map[string]interface{}{},
isExcluded: true,
expected: errors.New("include.release.openshift.io/self-managed-high-availability unset"),
},
{
name: "default profile selection excludes with no annotation",
profile: DefaultClusterProfile,
annotations: nil,
isExcluded: true,
expected: errors.New("no annotations"),
},
}
for _, tt := range tests {
Expand All @@ -219,15 +219,15 @@ func Test_Exclude(t *testing.T) {
if tt.annotations != nil {
metadata["annotations"] = tt.annotations
}
ret := shouldExclude(tt.exclude, tt.includeTechPreview, tt.profile, &manifest.Manifest{
err := include(tt.exclude, tt.includeTechPreview, tt.profile, &manifest.Manifest{
Obj: &unstructured.Unstructured{
Object: map[string]interface{}{
"metadata": metadata,
},
},
})
if ret != tt.isExcluded {
t.Errorf("(exclude: %v, profile: %v, annotations: %v) %v != %v", tt.exclude, tt.profile, tt.annotations, tt.isExcluded, ret)
if !reflect.DeepEqual(err, tt.expected) {
t.Errorf("(exclude: %v, profile: %v, annotations: %v) expected %v, but got %v", tt.exclude, tt.profile, tt.annotations, tt.expected, err)
}
})
}
Expand Down