Skip to content

Commit

Permalink
Remove dead checks in image promoter
Browse files Browse the repository at this point in the history 
These are only called by tests. This also hides a flag that was unused
(for image size).

Signed-off-by: Jon Johnson <jonjohnsonjr@gmail.com>
  • Loading branch information
@jonjohnsonjr
jonjohnsonjr committed Mar 28, 2023
1 parent e04b754 commit 13977d5
Show file tree
Hide file tree
Showing 8 changed files with 8 additions and 624 deletions.
13 changes: 6 additions & 7 deletions cmd/kpromo/cmd/cip/cip.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,12 +191,16 @@ network from a registry, it reads from the local manifests only`,
"pass '--account=...' to all gcloud calls",
)

// This flag does nothing, but we don't want to remove it in case it breaks someone.
// Instead, we just keep it hidden.
unused := 0
CipCmd.PersistentFlags().IntVar(
&runOpts.MaxImageSize,
&unused,
"max-image-size",
options.DefaultOptions.MaxImageSize,
0,
"the maximum image size (in MiB) allowed for promotion",
)
CipCmd.PersistentFlags().MarkHidden("max-image-size")

CipCmd.PersistentFlags().StringVar(
&runOpts.SignerAccount,
Expand Down Expand Up @@ -226,11 +230,6 @@ network from a registry, it reads from the local manifests only`,
"maximum number of concurrent signature operations",
)

// TODO: Set this in a function instead
if runOpts.MaxImageSize <= 0 {
runOpts.MaxImageSize = 2048
}

CipCmd.PersistentFlags().IntVar(
&runOpts.SeverityThreshold,
"vuln-severity-threshold",
Expand Down
30 changes: 0 additions & 30 deletions docs/checks.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,36 +81,6 @@ func (f * foo) Run() error {

## Current Checks

### ImageRemovalCheck

Images that have been promoted are pushed to production; and once pushed to
production, they should never be removed. The `ImageRemovalCheck` checks if
any images are removed in the pull request by comparing the state of the
promoter manifests in the pull request's branch to the default development
branch. Two sets of Promotion Edges are generated (one for both the default
development branch and pull request) and then compared to make sure that every
destination image (defined by its image tag and digest) found in the default
development branch is found in the pull request.

This method for detecting removed images should ensure that pull requests are
only rejected if an image is completely removed from production, while still
allowing edge cases. One example edge case is where a user has already
promoted an image foo from registry A to registry B. Then in a later pull
request, the user promotes the same image foo from registry C to registry B.
Although image foo is removed from registry A, this pull request should be
accepted because the same image is still being promoted, albeit from a new
location.

### ImageSizeCheck

The larger an image is, the more likely it is to be a security risk; and in
general, it is bad practice to use unnecessarily large images. The
`ImageSizeCheck` checks if any images to be promoted are larger than the
defined maximum image size. The api calls that get image information from GCR
also give information on the image size in bytes. These image sizes are
recorded and then checked to make sure they are under the maximum size
defined by the *-max-image-size* in MiB.

### ImageVulnerabilityCheck

Since promoted images are pushed to production and production images are
Expand Down
230 changes: 1 addition & 229 deletions internal/legacy/dockerregistry/checks.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,238 +29,10 @@ import (
"github.com/sirupsen/logrus"
"google.golang.org/api/iterator"
grafeaspb "google.golang.org/genproto/googleapis/grafeas/v1"
gogit "gopkg.in/src-d/go-git.v4"

"sigs.k8s.io/promo-tools/v3/internal/legacy/dockerregistry/schema"
"sigs.k8s.io/promo-tools/v3/internal/legacy/stream"
)

// MBToBytes converts a value from MiB to Bytes.
func MBToBytes(value int) int {
const mbToBytesShift = 20
return value << mbToBytesShift
}

// BytesToMB converts a value from Bytes to MiB.
func BytesToMB(value int) int {
const bytesToMBShift = 20
return value >> bytesToMBShift
}

/* TODO(unused): MARKED FOR DELETION
func getGitShaFromEnv(envVar string) (plumbing.Hash, error) {
potenitalSHA := os.Getenv(envVar)
const gitShaLength = 40
if len(potenitalSHA) != gitShaLength {
return plumbing.Hash{},
fmt.Errorf("length of SHA is %d characters, should be %d",
len(potenitalSHA), gitShaLength)
}
_, err := hex.DecodeString(potenitalSHA)
if err != nil {
return plumbing.Hash{}, fmt.Errorf("not a valid SHA: %v", err)
}
return plumbing.NewHash(potenitalSHA), nil
}
*/

/* TODO(unused): MARKED FOR DELETION
// MKRealImageRemovalCheck returns an instance of ImageRemovalCheck.
func MKRealImageRemovalCheck(
gitRepoPath string,
edges map[PromotionEdge]interface{},
) (*ImageRemovalCheck, error) {
// The "PULL_BASE_SHA" and "PULL_PULL_SHA" environment variables are given
// by the PROW job running the promoter container and represent the Git SHAs
// for the master branch and the pull request branch respectively.
masterSHA, err := getGitShaFromEnv("PULL_BASE_SHA")
if err != nil {
return nil, fmt.Errorf("the PULL_BASE_SHA environment variable "+
"is invalid: %v", err)
}
pullRequestSHA, err := getGitShaFromEnv("PULL_PULL_SHA")
if err != nil {
return nil, fmt.Errorf("the PULL_PULL_SHA environment variable "+
"is invalid: %v", err)
}
return &ImageRemovalCheck{
gitRepoPath,
masterSHA,
pullRequestSHA,
edges,
},
nil
}
*/

// Run executes ImageRemovalCheck on a set of promotion edges.
// Returns an error if the pull request removes images from the
// promoter manifests.
func (check *ImageRemovalCheck) Run() error {
r, err := gogit.PlainOpen(check.GitRepoPath)
if err != nil {
return fmt.Errorf("could not open the Git repo: %v", err)
}
w, err := r.Worktree()
if err != nil {
return fmt.Errorf("could not create Git worktree: %v", err)
}

// The Prow job that this check is running in has already cloned the
// git repo for us so we can just checkout the master branch to get the
// master branch's version of the promoter manifests.
err = w.Checkout(&gogit.CheckoutOptions{
Hash: check.MasterSHA,
Force: true,
})
if err != nil {
return fmt.Errorf("could not checkout the master branch of the Git"+
" repo: %v", err)
}

mfests, err := schema.ParseThinManifestsFromDir(check.GitRepoPath, false)
if err != nil {
return fmt.Errorf("could not parse manifests from the directory: %v",
err)
}
masterEdges, err := ToPromotionEdges(mfests)
if err != nil {
return fmt.Errorf("could not generate promotion edges from promoter"+
" manifests: %v", err)
}

// Reset the current directory back to the pull request branch so that this
// check doesn't leave lasting effects that could affect subsequent checks.
err = w.Checkout(&gogit.CheckoutOptions{
Hash: check.PullRequestSHA,
Force: true,
})
if err != nil {
return fmt.Errorf("could not checkout the pull request branch of the"+
" Git repo %v: %v",
check.GitRepoPath, err)
}

return check.Compare(masterEdges, check.PullEdges)
}

// Compare is a function of the ImageRemovalCheck that handles
// the comparison of the pull requests's set of promotion edges and
// the master branch's set of promotion edges.
func (check *ImageRemovalCheck) Compare(
edgesMaster map[PromotionEdge]interface{},
edgesPullRequest map[PromotionEdge]interface{},
) error {
// Generate a set of all destination images that appear in
// the pull request's set of promotion edges.
destinationImages := make(map[PromotionEdge]interface{})
for edge := range edgesPullRequest {
destinationImages[PromotionEdge{
DstImageTag: edge.DstImageTag,
Digest: edge.Digest,
}] = nil
}

// Check that every destination image in the master branch's set of
// promotion edges exists in the pull request's set of promotion edges.
removedImages := make([]string, 0)
for edge := range edgesMaster {
_, found := destinationImages[PromotionEdge{
DstImageTag: edge.DstImageTag,
Digest: edge.Digest,
}]
if !found {
removedImages = append(removedImages,
string(edge.DstImageTag.Name))
}
}

if len(removedImages) > 0 {
return fmt.Errorf("the following images were removed in this pull "+
"request: %v", strings.Join(removedImages, ", "))
}
return nil
}

// Error is a function of ImageSizeError and implements the error interface.
func (err ImageSizeError) Error() string {
errStr := ""
if len(err.OversizedImages) > 0 {
errStr += fmt.Sprintf("The following images were over the max file "+
"size of %dMiB:\n%v\n", err.MaxImageSize,
err.joinImageSizesToString(err.OversizedImages))
}
if len(err.InvalidImages) > 0 {
errStr += fmt.Sprintf("The following images had an invalid file size "+
"of 0 bytes or less:\n%v\n",
err.joinImageSizesToString(err.InvalidImages))
}
return errStr
}

func (err ImageSizeError) joinImageSizesToString(
imageSizes map[string]int,
) string {
imageSizesStr := ""
imageNames := make([]string, 0)
for k := range imageSizes {
imageNames = append(imageNames, k)
}
sort.Strings(imageNames)
for i, imageName := range imageNames {
imageSizesStr += imageName + " (" +
fmt.Sprint(BytesToMB(imageSizes[imageName])) + " MiB)"
if i < len(imageNames)-1 {
imageSizesStr += "\n"
}
}
return imageSizesStr
}

/* TODO(unused): MARKED FOR DELETION
// MKRealImageSizeCheck returns an instance of ImageSizeCheck which
// checks that all images to be promoted are under a max size.
func MKRealImageSizeCheck(
maxImageSize int,
edges map[PromotionEdge]interface{},
digestImageSize DigestImageSize,
) *ImageSizeCheck {
return &ImageSizeCheck{
maxImageSize,
digestImageSize,
edges,
}
}
*/

// Run is a function of ImageSizeCheck and checks that all
// images to be promoted are under the max file size.
func (check *ImageSizeCheck) Run() error {
maxImageSizeByte := MBToBytes(check.MaxImageSize)
oversizedImages := make(map[string]int)
invalidImages := make(map[string]int)
for edge := range check.PullEdges {
imageSize := check.DigestImageSize[edge.Digest]
imageName := string(edge.DstImageTag.Name)
if imageSize > maxImageSizeByte {
oversizedImages[imageName] = imageSize
}
if imageSize <= 0 {
invalidImages[imageName] = imageSize
}
}

if len(oversizedImages) > 0 || len(invalidImages) > 0 {
return ImageSizeError{
check.MaxImageSize,
oversizedImages,
invalidImages,
}
}

return nil
}

// MKImageVulnCheck returns an instance of ImageVulnCheck which
// checks against images that have known vulnerabilities.
func MKImageVulnCheck(
Expand Down Expand Up @@ -398,7 +170,7 @@ func (check *ImageVulnCheck) Run() error {
return nil
}

// Error is a function of ImageSizeError and implements the error interface.
// Error is a function of ImageVulnError and implements the error interface.
func (err ImageVulnError) Error() string {
// TODO: Why are we not checking errors here?
//nolint:errcheck,errchkjson
Expand Down
Loading

0 comments on commit 13977d5

Please sign in to comment.