Incorporate review feedback from vaikas

Signed-off-by: Matt Moore <[email protected]>

pkg/policy/policy.go

@@ -16,6 +16,7 @@ package policy
 
 import (
 	"context"
+	"fmt"
 	"io"
 	"net/http"
 	"os"
@@ -29,8 +30,9 @@ type Verification struct {
 	// of the listed policies.  It allows the values: allow, deny, and warn.
 	NoMatchPolicy string `yaml:"no-match-policy,omitempty"`
 
-	// Policies specifies a collection of policies to use to cover the base
-	// images used as part of evaluation.  See "policy" below for usage.
+	// Policies specifies a set of Sources for fetching policies to use to cover
+	// images used as part of evaluation.  For more information about what each
+	// Source supports, see its usage.
 	// Policies can be nil so that we can distinguish between an explicitly
 	// specified empty list and when policies is unspecified.
 	Policies *[]Source `yaml:"policies,omitempty"`
@@ -71,65 +73,76 @@ func (v *Verification) Validate(ctx context.Context) (errs *apis.FieldError) {
 	return errs
 }
 
-func (pd *Source) Validate(ctx context.Context) (errs *apis.FieldError) {
+func (pd *Source) Validate(ctx context.Context) *apis.FieldError {
 	// Check that exactly one of the fields is set.
 	set := sets.NewString()
 	if pd.Data != "" {
 		set.Insert("data")
-		_, _, err := ParseClusterImagePolicies(ctx, pd.Data)
-		if err != nil {
-			errs = errs.Also(apis.ErrInvalidValue(err.Error(), "data"))
-		}
 	}
 	if pd.Path != "" {
 		set.Insert("path")
-		raw, err := os.ReadFile(pd.Path)
-		if err != nil {
-			errs = errs.Also(&apis.FieldError{
-				Message: err.Error(),
-				Paths:   []string{"path"},
-			})
-		} else {
-			_, _, err := ParseClusterImagePolicies(ctx, string(raw))
-			if err != nil {
-				errs = errs.Also(apis.ErrInvalidValue(err.Error(), "path"))
-			}
-		}
 	}
 	if pd.URL != "" {
 		set.Insert("url")
-		resp, err := http.Get(pd.URL)
-		if err != nil {
-			errs = errs.Also(&apis.FieldError{
-				Message: err.Error(),
-				Paths:   []string{"url"},
-			})
-		} else {
-			defer resp.Body.Close()
-			raw, err := io.ReadAll(resp.Body)
-			if err != nil {
-				errs = errs.Also(&apis.FieldError{
-					Message: err.Error(),
-					Paths:   []string{"url"},
-				})
-			} else {
-				_, _, err := ParseClusterImagePolicies(ctx, string(raw))
-				if err != nil {
-					errs = errs.Also(apis.ErrInvalidValue(err.Error(), "url"))
-				}
-			}
-		}
 	}
-
+	// This returns eagerly to avoid confusing `oneof` validation with errors
+	// along multiple paths of the oneof.
 	switch set.Len() {
 	case 0:
-		errs = errs.Also(apis.ErrMissingOneOf("data", "path", "url"))
+		return apis.ErrMissingOneOf("data", "path", "url")
 	case 1:
 		// What we want.
 	default:
 		// This will be unreachable until we add more than one thing
 		// to our oneof.
-		errs = errs.Also(apis.ErrMultipleOneOf(set.List()...))
+		return apis.ErrMultipleOneOf(set.List()...)
+	}
+	// We know (from the switch above) there is exactly one field name.
+	field, _ := set.PopAny()
+
+	content, err := pd.fetch(ctx)
+	if err != nil {
+		return &apis.FieldError{
+			Message: err.Error(),
+			Paths:   []string{field},
+		}
+	}
+	if _, _, err := ParseClusterImagePolicies(ctx, content); err != nil {
+		return apis.ErrInvalidValue(err.Error(), field)
+	}
+	return nil
+}
+
+func (pd *Source) fetch(ctx context.Context) (string, error) {
+	switch {
+	case pd.Data != "":
+		return pd.Data, nil
+
+	case pd.Path != "":
+		raw, err := os.ReadFile(pd.Path)
+		if err != nil {
+			return "", err
+		}
+		return string(raw), nil
+
+	case pd.URL != "":
+		req, err := http.NewRequestWithContext(ctx, http.MethodGet, pd.URL, nil)
+		if err != nil {
+			return "", err
+		}
+		resp, err := http.DefaultClient.Do(req)
+		if err != nil {
+			return "", err
+		}
+		defer resp.Body.Close()
+		raw, err := io.ReadAll(resp.Body)
+		if err != nil {
+			return "", err
+		}
+		return string(raw), nil
+
+	default:
+		// This should never happen for a validated policy.
+		return "", fmt.Errorf("unsupported policy shape: %v", pd)
 	}
-	return errs
 }

pkg/policy/validate.go

@@ -39,7 +39,8 @@ var (
 )
 
 // Validate decodes a provided YAML document containing zero or more objects
-// and performs limited validation on them.
+// and performs limited validation on them, after applying defaulting (to
+// simulate the mutating webhook running before the validating webhook).
 func Validate(ctx context.Context, document string) (warns error, err error) {
 	if len(document) == 0 {
 		return nil, ErrEmptyDocument

pkg/policy/verifier.go

@@ -18,9 +18,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"io"
-	"net/http"
-	"os"
 
 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/name"
@@ -37,12 +34,22 @@ import (
 // the policies backing this interface.
 type Verifier interface {
 	// Verify checks that the provided reference satisfies the backing policies.
-	Verify(context.Context, name.Reference, authn.Keychain) error
+	//
+	// For policies specifying `match:` criteria with apiVersion/kind, the
+	// TypeMeta should be associated with `ctx` here using:
+	//    webhook.GetIncludeTypeMeta(ctx)
+	//
+	// For policies specifying `match:` criteria with label selectors, the
+	// ObjectMeta should be associated with `ctx` here using:
+	//    webhook.GetIncludeObjectMeta(ctx)
+	Verify(context.Context, name.Reference, authn.Keychain, ...ociremote.Option) error
 }
 
 // WarningWriter is used to surface warning messages in a manner that
 // is customizable by callers that's suitable for their execution
-// environment.
+// environment.  The signature is intended to match the standard format string
+// signature (e.g. Printf, Infof, Logf, Errorf, Fatalf, ...), so functions like
+// log.Printf or t.Errorf can be passed here directly.
 type WarningWriter func(string, ...interface{})
 
 // Compile turns a Verification into an executable Verifier.
@@ -72,33 +79,9 @@ func gather(ctx context.Context, v Verification, ww WarningWriter) (*config.Imag
 	}
 
 	for i, p := range pol {
-		var content string
-		switch {
-		case p.Data != "":
-			content = p.Data
-
-		case p.Path != "":
-			raw, err := os.ReadFile(p.Path)
-			if err != nil {
-				return nil, err
-			}
-			content = string(raw)
-
-		case p.URL != "":
-			resp, err := http.Get(p.URL)
-			if err != nil {
-				return nil, err
-			}
-			defer resp.Body.Close()
-			raw, err := io.ReadAll(resp.Body)
-			if err != nil {
-				return nil, err
-			}
-			content = string(raw)
-
-		default:
-			// This should never happen for a validated policy.
-			return nil, fmt.Errorf("unsupported policy shape: %v", p)
+		content, err := p.fetch(ctx)
+		if err != nil {
+			return nil, err
 		}
 
 		l, warns, err := ParseClusterImagePolicies(ctx, content)
@@ -148,7 +131,7 @@ type impl struct {
 var _ Verifier = (*impl)(nil)
 
 // Verify implements Verifier
-func (i *impl) Verify(ctx context.Context, ref name.Reference, kc authn.Keychain) error {
+func (i *impl) Verify(ctx context.Context, ref name.Reference, kc authn.Keychain, opts ...ociremote.Option) error {
 	tm := getTypeMeta(ctx)
 	om := getObjectMeta(ctx)
 	matches, err := i.ipc.GetMatchingPolicies(ref.Name(), tm.Kind, tm.APIVersion, om.Labels)
@@ -170,9 +153,11 @@ func (i *impl) Verify(ctx context.Context, ref name.Reference, kc authn.Keychain
 		}
 	}
 
+	// Add the keychain to our (optional) list of options.
+	opts = append(opts, ociremote.WithRemoteOptions(remote.WithAuthFromKeychain(kc)))
+
 	for _, p := range matches {
-		_, errs := webhook.ValidatePolicy(ctx, "" /* namespace */, ref, p,
-			kc, ociremote.WithRemoteOptions(remote.WithAuthFromKeychain(kc)))
+		_, errs := webhook.ValidatePolicy(ctx, "" /* namespace */, ref, p, kc, opts...)
 		for _, err := range errs {
 			var fe *apis.FieldError
 			if errors.As(err, &fe) {