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

adding required annotations check and necessary tests #1086

Merged
merged 1 commit into from
Dec 12, 2023
Merged

adding required annotations check and necessary tests #1086

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
1 change: 1 addition & 0 deletions certification/results.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,5 @@ type Results struct {
Passed []Result
Failed []Result
Errors []Result
Warned []Result
}
7 changes: 7 additions & 0 deletions internal/check/certification.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,13 @@ import (
"github.com/redhat-openshift-ecosystem/openshift-preflight/internal/image"
)

// Indicates the possible levels that can be utilized in the implementation of a check.
const (
LevelBest = "best"
LevelOptional = "optional"
LevelWarn = "warn"
)

// Check as an interface containing all methods necessary
// to use and identify a given check.
type Check interface {
Expand Down
14 changes: 13 additions & 1 deletion internal/csv/csv.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,19 @@ import (
corev1 "k8s.io/api/core/v1"
)

const InfrastructureFeaturesAnnotation = "operators.openshift.io/infrastructure-features"
const (
InfrastructureFeaturesAnnotation = "operators.openshift.io/infrastructure-features"
DisconnectedAnnotation = "features.operators.openshift.io/disconnected"
FIPSCompliantAnnotation = "features.operators.openshift.io/fips-compliant"
ProxyAwareAnnotation = "features.operators.openshift.io/proxy-aware"
TLSProfilesAnnotation = "features.operators.openshift.io/tls-profiles"
TokenAuthAWSAnnotation = "features.operators.openshift.io/token-auth-aws"
TokenAuthAzureAnnotation = "features.operators.openshift.io/token-auth-azure"
TokenAuthGCPAnnotation = "features.operators.openshift.io/token-auth-gcp"
Comment on lines +19 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a potentially unrelated question.. but why are these becoming required? Are we installing operators on all providers all at once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if a given operator would/could support token auth for all cloud providers or not. My guess would be that is unlikely, just based on our dealings with STS in the ACK project. IMO It's more so for the below:

The current format of the infrastructure annotations is unsuited for robust checks and effective enforcement. We need a new format that allows to cleanly differentiate between mere absence of a data or actual lack of support for a certain infrastructure feature.

CNFAnnotation = "features.operators.openshift.io/cnf"
CNIAnnotation = "features.operators.openshift.io/cni"
CSIAnnotation = "features.operators.openshift.io/csi"
)

// SupportsDisconnected accepts a stringified list of supported features
// and returns true if "disconnected" is listed as a supported feature.
Expand Down
29 changes: 18 additions & 11 deletions internal/engine/engine.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,33 +216,39 @@ func (c *craneEngine) ExecuteChecks(ctx context.Context) error {

// execute checks
logger.V(log.DBG).Info("executing checks")
for _, check := range c.checks {
for _, executedCheck := range c.checks {
c.results.TestedImage = c.image
komish marked this conversation as resolved.
Show resolved Hide resolved

logger.V(log.DBG).Info("running check", "check", check.Name())
if check.Metadata().Level == "optional" {
logger.Info(fmt.Sprintf("Check %s is not currently being enforced.", check.Name()))
logger.V(log.DBG).Info("running check", "check", executedCheck.Name())
if executedCheck.Metadata().Level == check.LevelOptional || executedCheck.Metadata().Level == check.LevelWarn {
logger.Info(fmt.Sprintf("Check %s is not currently being enforced.", executedCheck.Name()))
}

// run the validation
checkStartTime := time.Now()
checkPassed, err := check.Validate(ctx, c.imageRef)
checkPassed, err := executedCheck.Validate(ctx, c.imageRef)
checkElapsedTime := time.Since(checkStartTime)

if err != nil {
logger.WithValues("result", "ERROR", "err", err.Error()).Info("check completed", "check", check.Name())
c.results.Errors = appendUnlessOptional(c.results.Errors, certification.Result{Check: check, ElapsedTime: checkElapsedTime})
logger.WithValues("result", "ERROR", "err", err.Error()).Info("check completed", "check", executedCheck.Name())
c.results.Errors = appendUnlessOptional(c.results.Errors, certification.Result{Check: executedCheck, ElapsedTime: checkElapsedTime})
continue
}

if !checkPassed {
logger.WithValues("result", "FAILED").Info("check completed", "check", check.Name())
c.results.Failed = appendUnlessOptional(c.results.Failed, certification.Result{Check: check, ElapsedTime: checkElapsedTime})
// if a test doesn't pass but is of level warn include it in warning results, instead of failed results
if executedCheck.Metadata().Level == check.LevelWarn {
logger.WithValues("result", "WARNING").Info("check completed", "check", executedCheck.Name())
c.results.Warned = appendUnlessOptional(c.results.Warned, certification.Result{Check: executedCheck, ElapsedTime: checkElapsedTime})
continue
}
logger.WithValues("result", "FAILED").Info("check completed", "check", executedCheck.Name())
c.results.Failed = appendUnlessOptional(c.results.Failed, certification.Result{Check: executedCheck, ElapsedTime: checkElapsedTime})
continue
}

logger.WithValues("result", "PASSED").Info("check completed", "check", check.Name())
c.results.Passed = appendUnlessOptional(c.results.Passed, certification.Result{Check: check, ElapsedTime: checkElapsedTime})
logger.WithValues("result", "PASSED").Info("check completed", "check", executedCheck.Name())
c.results.Passed = appendUnlessOptional(c.results.Passed, certification.Result{Check: executedCheck, ElapsedTime: checkElapsedTime})
}

if len(c.results.Errors) > 0 || len(c.results.Failed) > 0 {
Expand Down Expand Up @@ -674,6 +680,7 @@ func InitializeOperatorChecks(ctx context.Context, p policy.Policy, cfg Operator
operatorpol.NewSecurityContextConstraintsCheck(),
&operatorpol.RelatedImagesCheck{},
operatorpol.FollowsRestrictedNetworkEnablementGuidelines{},
operatorpol.RequiredAnnotations{},
}, nil
}

Expand Down
24 changes: 23 additions & 1 deletion internal/engine/engine_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,24 @@ var _ = Describe("Execute Checks tests", func() {
check.HelpText{},
)

warningCheckPassing := check.NewGenericCheck(
"warnCheckPassing",
func(context.Context, image.ImageReference) (bool, error) {
return true, nil
},
check.Metadata{Level: check.LevelWarn},
check.HelpText{},
)

warningCheckFailing := check.NewGenericCheck(
"warnCheckFailing",
func(context.Context, image.ImageReference) (bool, error) {
return false, nil
},
check.Metadata{Level: check.LevelWarn},
check.HelpText{},
)

emptyConfig := runtime.Config{}
engine = craneEngine{
dockerConfig: emptyConfig.DockerConfig,
Expand All @@ -118,6 +136,8 @@ var _ = Describe("Execute Checks tests", func() {
failedCheck,
optionalCheckPassing,
optionalCheckFailing,
warningCheckPassing,
warningCheckFailing,
},
isBundle: false,
isScratch: false,
Expand All @@ -127,9 +147,10 @@ var _ = Describe("Execute Checks tests", func() {
It("should succeed", func() {
err := engine.ExecuteChecks(testcontext)
Expect(err).ToNot(HaveOccurred())
Expect(engine.results.Passed).To(HaveLen(1))
Expect(engine.results.Passed).To(HaveLen(2))
Expect(engine.results.Failed).To(HaveLen(1))
Expect(engine.results.Errors).To(HaveLen(1))
Expect(engine.results.Warned).To(HaveLen(1))
Expect(engine.results.CertificationHash).To(BeEmpty())
})
Context("it is a bundle", func() {
Expand Down Expand Up @@ -330,6 +351,7 @@ var _ = Describe("Check Name Queries", func() {
"SecurityContextConstraintsInCSV",
"AllImageRefsInRelatedImages",
"FollowsRestrictedNetworkEnablementGuidelines",
"RequiredAnnotations",
}),
Entry("scratch container policy", ScratchContainerPolicy, []string{
"HasLicense",
Expand Down
28 changes: 23 additions & 5 deletions internal/formatters/junitxml.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ type JUnitTestSuite struct {
XMLName xml.Name `xml:"testsuite"`
Tests int `xml:"tests,attr"`
Failures int `xml:"failures,attr"`
Warnings int `xml:"warnings,attr"`
Time string `xml:"time,attr"`
Name string `xml:"name,attr"`
Properties []JUnitProperty `xml:"properties>property,omitempty"`
Expand All @@ -30,7 +31,8 @@ type JUnitTestCase struct {
Name string `xml:"name,attr"`
Time string `xml:"time,attr"`
SkipMessage *JUnitSkipMessage `xml:"skipped,omitempty"`
Failure *JUnitFailure `xml:"failure,omitempty"`
Failure *JUnitMessage `xml:"failure,omitempty"`
Warning *JUnitMessage `xml:"warning,omitempty"`
SystemOut string `xml:"system-out,omitempty"`
Message string `xml:",chardata"`
}
Expand All @@ -44,18 +46,19 @@ type JUnitProperty struct {
Value string `xml:"value,attr"`
}

type JUnitFailure struct {
type JUnitMessage struct {
Message string `xml:"message,attr"`
Type string `xml:"type,attr"`
Contents string `xml:",chardata"`
}

func junitXMLFormatter(ctx context.Context, r certification.Results) ([]byte, error) {
func junitXMLFormatter(_ context.Context, r certification.Results) ([]byte, error) {
response := getResponse(r)
suites := JUnitTestSuites{}
testsuite := JUnitTestSuite{
Tests: len(r.Errors) + len(r.Failed) + len(r.Passed),
Tests: len(r.Errors) + len(r.Failed) + len(r.Passed) + len(r.Warned),
Failures: len(r.Errors) + len(r.Failed),
Warnings: len(r.Warned),
Time: "0s",
Name: "Red Hat Certification",
Properties: []JUnitProperty{},
Expand All @@ -80,7 +83,7 @@ func junitXMLFormatter(ctx context.Context, r certification.Results) ([]byte, er
Classname: response.Image,
Name: result.Name(),
Time: result.ElapsedTime.String(),
Failure: &JUnitFailure{
Failure: &JUnitMessage{
Message: "Failed",
Type: "",
Contents: fmt.Sprintf("%s: Suggested Fix: %s", result.Help().Message, result.Help().Suggestion),
Expand All @@ -90,6 +93,21 @@ func junitXMLFormatter(ctx context.Context, r certification.Results) ([]byte, er
totalDuration += result.ElapsedTime
}

for _, result := range r.Warned {
testCase := JUnitTestCase{
Classname: response.Image,
Name: result.Name(),
Time: result.ElapsedTime.String(),
Warning: &JUnitMessage{
Message: "Warn",
Type: "",
Contents: fmt.Sprintf("%s: Suggested Fix: %s", result.Help().Message, result.Help().Suggestion),
},
}
testsuite.TestCases = append(testsuite.TestCases, testCase)
totalDuration += result.ElapsedTime
}

testsuite.Time = fmt.Sprintf("%f", totalDuration.Seconds())
suites.Suites = append(suites.Suites, testsuite)

Expand Down
32 changes: 32 additions & 0 deletions internal/formatters/junitxml_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,38 @@ var _ = Describe("JUnitXML Formatter", func() {
ElapsedTime: 0,
},
},
Warned: []certification.Result{
{
Check: check.NewGenericCheck(
"WarningCheckPass",
func(ctx context.Context, ir image.ImageReference) (bool, error) { return true, nil },
check.Metadata{
Description: "description",
KnowledgeBaseURL: "kburl",
CheckURL: "checkurl",
},
check.HelpText{
Message: "helptext",
Suggestion: "suggestion",
}),
ElapsedTime: 0,
},
{
Check: check.NewGenericCheck(
"WarningCheckFail",
func(ctx context.Context, ir image.ImageReference) (bool, error) { return false, nil },
check.Metadata{
Description: "description",
KnowledgeBaseURL: "kburl",
CheckURL: "checkurl",
},
check.HelpText{
Message: "helptext",
Suggestion: "suggestion",
}),
ElapsedTime: 0,
},
},
}
})
It("should format without error", func() {
Expand Down
29 changes: 23 additions & 6 deletions internal/formatters/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ func getResponse(r certification.Results) UserResponse {
passedChecks := make([]checkExecutionInfo, 0, len(r.Passed))
failedChecks := make([]checkExecutionInfo, 0, len(r.Failed))
erroredChecks := make([]checkExecutionInfo, 0, len(r.Errors))
warnedChecks := make([]checkExecutionInfo, 0, len(r.Warned))

if len(r.Passed) > 0 {
for _, check := range r.Passed {
Expand Down Expand Up @@ -47,15 +48,30 @@ func getResponse(r certification.Results) UserResponse {
}
}

if len(r.Warned) > 0 {
for _, check := range r.Warned {
warnedChecks = append(warnedChecks, checkExecutionInfo{
Name: check.Name(),
ElapsedTime: float64(check.ElapsedTime.Milliseconds()),
Description: check.Metadata().Description,
Help: check.Help().Message,
Suggestion: check.Help().Suggestion,
KnowledgeBaseURL: check.Metadata().KnowledgeBaseURL,
CheckURL: check.Metadata().CheckURL,
})
}
}

response := UserResponse{
Image: r.TestedImage,
Passed: r.PassedOverall,
LibraryInfo: version.Version,
CertificationHash: r.CertificationHash,
Results: resultsText{
Passed: passedChecks,
Failed: failedChecks,
Errors: erroredChecks,
Passed: passedChecks,
Failed: failedChecks,
Errors: erroredChecks,
Warnings: warnedChecks,
},
}

Expand All @@ -73,9 +89,10 @@ type UserResponse struct {

// resultsText represents the results of check execution against the asset.
type resultsText struct {
Passed []checkExecutionInfo `json:"passed" xml:"passed"`
Failed []checkExecutionInfo `json:"failed" xml:"failed"`
Errors []checkExecutionInfo `json:"errors" xml:"errors"`
Passed []checkExecutionInfo `json:"passed" xml:"passed"`
Failed []checkExecutionInfo `json:"failed" xml:"failed"`
Errors []checkExecutionInfo `json:"errors" xml:"errors"`
Warnings []checkExecutionInfo `json:"warning,omitempty" xml:"warning,omitempty"`
}

// checkExecutionInfo contains all possible output fields that a user might see in their result.
Expand Down
Loading