Repository: redhat-openshift-ecosystem/openshift-preflight
Pull request #559 (2022-04-11 19:35:01)

Clarify the graphql layer checking logic
## Motivation

The BasedOnUBI check had a few opportunities where clarification could help future developers make changes, so this PR intends to clean some of those things.

The logic itself doesn't change much, but there are some method name changes for very specific reasons (detailed before). Otherwise, the code is functionally the same.

I think there's room to make further distinction between just "layers" and the uncompressed layer IDs that we're working with here, but I haven't figured out what makes the most sense there yet, so I'll have to revisit.

I mostly just wanted to change `CheckRedHatLayers` (see below).

## Change summary, by file

**certification/internal/policy/container/based_on_ubi.go**

- The comment describing the check was no longer correct. We perform a more granular check now than simply checking OS files to see if we're using the Red Hat UBI
- Several other method/function comments were put in place.
- The `CheckRedHatLayers` method has been renamed (starting at the interface, and all implementations I found). The `CheckRedHatLayers` name wasn't clear as to what we expected to get out of the method, so the new name `CertifiedImagesContainingLayers`, while longer, does include the inputs (Layers) and the output (CertifiedImages containing... well you get the idea).
- StaticCheck complained that checking `certImages` for nil before checking length is greater than 1 was redundant, so I complied.

**certification/pyxis/layers.go**

- Mostly comments here, and making sure the method name matched the interface's new definition.

**certification/pyxis/layers_test.go**
**certification/internal/policy/container/based_on_ubi_test.go**

- Nothing to note, other than making sure Fakes are in line.





Signed-off-by: Jose R. Gonzalez <jose@flutes.dev>

openshift-ci[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/redhat-openshift-ecosystem/openshift-preflight/pull/559#" title="Author self-approved">komish</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=redhat-openshift-ecosystem%2Fopenshift-preflight).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/redhat-openshift-ecosystem/openshift-preflight/blob/main/OWNERS)~~ [komish]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/certification/internal/policy/container/base_on_ubi.go b/certification/internal/policy/container/base_on_ubi.go
index e19448a..ea687f3 100644
--- a/certification/internal/policy/container/base_on_ubi.go
+++ b/certification/internal/policy/container/base_on_ubi.go
@@ -9,15 +9,13 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
-// BasedOnUBICheck evaluates if the provided image is based on the Red Hat Universal Base Image
-// by inspecting the contents of the `/etc/os-release` and identifying if the ID is `rhel` and the
-// Name value is `Red Hat Enterprise Linux`
+// BasedOnUBICheck evaluates if the provided image is based on the Red Hat Universal Base Image.
 type BasedOnUBICheck struct {
 	LayerHashCheckEngine layerHashChecker
 }
 
 type layerHashChecker interface {
-	CheckRedHatLayers(ctx context.Context, layerHashes []cranev1.Hash) ([]pyxis.CertImage, error)
+	CertifiedImagesContainingLayers(ctx context.Context, uncompressedLayerHashes []cranev1.Hash) ([]pyxis.CertImage, error)
 }
 
 func NewBasedOnUbiCheck(layerHashChecker layerHashChecker) *BasedOnUBICheck {
@@ -33,6 +31,7 @@ func (p *BasedOnUBICheck) Validate(ctx context.Context, imgRef certification.Ima
 	return p.validate(ctx, layerHashes)
 }
 
+// getImageLayers returns the root filesystem DiffIDs of the image.
 func (p *BasedOnUBICheck) getImageLayers(ctx context.Context, image cranev1.Image) ([]cranev1.Hash, error) {
 	configFile, err := image.ConfigFile()
 	if err != nil {
@@ -42,13 +41,15 @@ func (p *BasedOnUBICheck) getImageLayers(ctx context.Context, image cranev1.Imag
 	return configFile.RootFS.DiffIDs, nil
 }
 
-func (p *BasedOnUBICheck) checkRedHatLayers(ctx context.Context, layerHashes []cranev1.Hash) (bool, error) {
-	certImages, err := p.LayerHashCheckEngine.CheckRedHatLayers(ctx, layerHashes)
+// certifiedImagesFound checks to make sure images exist in Red Hat Pyxis containing the uncompressed
+// top layer IDs of the image under test.
+func (p *BasedOnUBICheck) certifiedImagesFound(ctx context.Context, layerHashes []cranev1.Hash) (bool, error) {
+	certImages, err := p.LayerHashCheckEngine.CertifiedImagesContainingLayers(ctx, layerHashes)
 	if err != nil {
 		log.Error("Error when querying pyxis for uncompressed top layer ids", err)
 		return false, err
 	}
-	if certImages != nil && len(certImages) >= 1 {
+	if len(certImages) >= 1 {
 		return true, nil
 	}
 	log.Error("No matching layer ids found in pyxis db. Please verify if the image is based on a recent UBI image")
@@ -56,7 +57,7 @@ func (p *BasedOnUBICheck) checkRedHatLayers(ctx context.Context, layerHashes []c
 }
 
 func (p *BasedOnUBICheck) validate(ctx context.Context, layerHashes []cranev1.Hash) (bool, error) {
-	hasUBIHash, err := p.checkRedHatLayers(ctx, layerHashes)
+	hasUBIHash, err := p.certifiedImagesFound(ctx, layerHashes)
 	if err != nil {
 		log.Error("Unable to verify layer hashes", err)
 		return false, err
diff --git a/certification/pyxis/layers.go b/certification/pyxis/layers.go
index 2d6d0f4..6c18175 100644
--- a/certification/pyxis/layers.go
+++ b/certification/pyxis/layers.go
@@ -11,12 +11,16 @@ import (
 	log "github.com/sirupsen/logrus"
 )
 
-func (p *pyxisClient) CheckRedHatLayers(ctx context.Context, layerHashes []cranev1.Hash) ([]CertImage, error) {
-	layerIds := make([]graphql.String, 0, len(layerHashes))
-	for _, layer := range layerHashes {
+// CertifiedImagesContainingLayers takes uncompressedLayerHashes and queries to a Red Hat Pyxis,
+// returning existing certified images from registry.access.redhat.com that contain any of the
+// IDs as its uncompressed top layer id.
+func (p *pyxisClient) CertifiedImagesContainingLayers(ctx context.Context, uncompressedLayerHashes []cranev1.Hash) ([]CertImage, error) {
+	layerIds := make([]graphql.String, 0, len(uncompressedLayerHashes))
+	for _, layer := range uncompressedLayerHashes {
 		layerIds = append(layerIds, graphql.String(layer.String()))
 	}
 
+	// our graphQL query
 	var query struct {
 		FindImages struct {
 			ContainerImage []struct {
@@ -29,14 +33,17 @@ func (p *pyxisClient) CheckRedHatLayers(ctx context.Context, layerHashes []crane
 			} `graphql:"error"`
 			Total graphql.Int
 			Page  graphql.Int
+			// filter to make sure we get exact results
 		} `graphql:"find_images(filter: {and:[{repositories:{registry:{in:$registries}}}{uncompressed_top_layer_id:{in:$contImageLayers}}]})"`
 	}
 
+	// variables to feed to our graphql filter
 	variables := map[string]interface{}{
 		"contImageLayers": layerIds,
 		"registries":      []graphql.String{"registry.access.redhat.com"},
 	}
 
+	// make our query
 	httpClient, ok := p.Client.(*http.Client)
 	if !ok {
 		return nil, errors.ErrInvalidHttpClient


Test patch:
diff --git a/certification/internal/policy/container/base_on_ubi_test.go b/certification/internal/policy/container/base_on_ubi_test.go
index 13030f3..af994d4 100644
--- a/certification/internal/policy/container/base_on_ubi_test.go
+++ b/certification/internal/policy/container/base_on_ubi_test.go
@@ -21,7 +21,7 @@ func ConfigFile() (*cranev1.ConfigFile, error) {
 
 type fakeLayerHashChecker struct{}
 
-func (flhc *fakeLayerHashChecker) CheckRedHatLayers(ctx context.Context, layers []cranev1.Hash) ([]pyxis.CertImage, error) {
+func (flhc *fakeLayerHashChecker) CertifiedImagesContainingLayers(ctx context.Context, layers []cranev1.Hash) ([]pyxis.CertImage, error) {
 	var matchingImages []pyxis.CertImage
 	matchingImages = append(matchingImages, pyxis.CertImage{})
 	return matchingImages, nil
@@ -29,14 +29,14 @@ func (flhc *fakeLayerHashChecker) CheckRedHatLayers(ctx context.Context, layers
 
 type fakeLayerHashCheckerNoMatch struct{}
 
-func (flhc *fakeLayerHashCheckerNoMatch) CheckRedHatLayers(ctx context.Context, layers []cranev1.Hash) ([]pyxis.CertImage, error) {
+func (flhc *fakeLayerHashCheckerNoMatch) CertifiedImagesContainingLayers(ctx context.Context, layers []cranev1.Hash) ([]pyxis.CertImage, error) {
 	var matchingImages []pyxis.CertImage
 	return matchingImages, nil
 }
 
 type fakeLayerHashCheckerTimeout struct{}
 
-func (flhc *fakeLayerHashCheckerTimeout) CheckRedHatLayers(ctx context.Context, layers []cranev1.Hash) ([]pyxis.CertImage, error) {
+func (flhc *fakeLayerHashCheckerTimeout) CertifiedImagesContainingLayers(ctx context.Context, layers []cranev1.Hash) ([]pyxis.CertImage, error) {
 	return nil, http.ErrHandlerTimeout
 }
 
diff --git a/certification/pyxis/layers_test.go b/certification/pyxis/layers_test.go
index e7a5ea3..1ec3c7e 100644
--- a/certification/pyxis/layers_test.go
+++ b/certification/pyxis/layers_test.go
@@ -21,7 +21,7 @@ var _ = Describe("Pyxis CheckRedHatLayers", func() {
 		})
 		Context("and a layer is a known good layer", func() {
 			It("should be a good layer", func() {
-				certImages, err := pyxisClient.CheckRedHatLayers(ctx, []cranev1.Hash{{}})
+				certImages, err := pyxisClient.CertifiedImagesContainingLayers(ctx, []cranev1.Hash{{}})
 				Expect(err).ToNot(HaveOccurred())
 				Expect(certImages).ToNot(BeNil())
 				Expect(certImages).ToNot(BeZero())