Skip to content

Commit

Permalink
fix: associate unexected errs w/ rules; always include validation res…
Browse files Browse the repository at this point in the history 
…ult details (#219)

## Issue
<!-- Link to the github issue this PR address, ie: #123 -->

## Description
Fix multiple issues with validation results:
- validation results for unexpected errors weren't setting
ValidationRule or ValidationType, and hence couldn't be associated with
a particular rule in the OciValidator
- validation results were omitting details that had been added when
errors occurred
- positive details weren't being added to validation results in all
cases

---------

Signed-off-by: Tyler Gillson <tyler.gillson@gmail.com>
  • Loading branch information
@TylerGillson
TylerGillson authored Jul 19, 2024
1 parent 2bb485b commit 891a318
Show file tree
Hide file tree
Showing 5 changed files with 87 additions and 68 deletions.
2 changes: 1 addition & 1 deletion Dockerfile.devspace
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
# Build the manager binary
FROM --platform=$TARGETPLATFORM golang:alpine3.18 as builder
FROM --platform=$TARGETPLATFORM golang:alpine3.19 as builder
ARG TARGETOS
ARG TARGETARCH

Expand Down
18 changes: 10 additions & 8 deletions internal/controller/ocivalidator_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -100,17 +100,19 @@ func (r *OciValidatorReconciler) Reconcile(ctx context.Context, req ctrl.Request

// OCI Registry rules
for _, rule := range validator.Spec.OciRegistryRules {
vrr := val.BuildValidationResult(rule)

username, password, err := r.secretKeyAuth(req, rule)
if err != nil {
l.Error(err, "failed to get secret auth", "ruleName", rule.Name)
resp.AddResult(nil, err)
resp.AddResult(vrr, err)
continue
}

pubKeys, err := r.signaturePubKeys(req, rule)
if err != nil {
l.Error(err, "failed to get signature verification public keys", "ruleName", rule.Name)
resp.AddResult(nil, err)
resp.AddResult(vrr, err)
continue
}

Expand All @@ -122,13 +124,13 @@ func (r *OciValidatorReconciler) Reconcile(ctx context.Context, req ctrl.Request
)
if err != nil {
l.Error(err, "failed to create OCI client", "ruleName", rule.Name)
resp.AddResult(nil, err)
resp.AddResult(vrr, err)
continue
}

ociRuleService := val.NewOciRuleService(r.Log, val.WithOCIClient(ociClient))

vrr, err := ociRuleService.ReconcileOciRegistryRule(rule)
vrr, err = ociRuleService.ReconcileOciRegistryRule(rule)
if err != nil {
l.Error(err, "failed to reconcile OCI Registry rule", "ruleName", rule.Name)
}
Expand Down Expand Up @@ -161,11 +163,11 @@ func (r *OciValidatorReconciler) secretKeyAuth(req ctrl.Request, rule v1alpha1.O
nn := ktypes.NamespacedName{Name: rule.Auth.SecretName, Namespace: req.Namespace}

if err := r.Get(context.Background(), nn, authSecret); err != nil {
return "", "", fmt.Errorf("failed to fetch auth secret %s/%s for rule %s: %w", nn.Name, nn.Namespace, rule.Name(), err)
return "", "", fmt.Errorf("failed to fetch auth secret %s/%s for rule %s: %w", nn.Namespace, nn.Name, rule.Name(), err)
}

if len(authSecret.Data) == 0 {
return "", "", fmt.Errorf("secret %s/%s has no data", nn.Name, nn.Namespace)
return "", "", fmt.Errorf("secret %s/%s has no data", nn.Namespace, nn.Name)
}

var username, password string
Expand Down Expand Up @@ -197,7 +199,7 @@ func (r *OciValidatorReconciler) signaturePubKeys(req ctrl.Request, rule v1alpha

if err := r.Get(context.Background(), nn, pubKeysSecret); err != nil {
return nil, fmt.Errorf("failed to fetch public keys secret %s/%s for rule %s: %w",
nn.Name, nn.Namespace, rule.Name(), err,
nn.Namespace, nn.Name, rule.Name(), err,
)
}

Expand All @@ -208,7 +210,7 @@ func (r *OciValidatorReconciler) signaturePubKeys(req ctrl.Request, rule v1alpha
}
}
if len(pubKeys) == 0 {
return nil, fmt.Errorf("no public keys found in secret %s/%s for rule: %s", nn.Name, nn.Namespace, rule.Name())
return nil, fmt.Errorf("no public keys found in secret %s/%s for rule: %s", nn.Namespace, nn.Name, rule.Name())
}

return pubKeys, nil
Expand Down
38 changes: 20 additions & 18 deletions internal/validators/oci_validator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,28 +49,28 @@ func WithOCIClient(client *oci.Client) Option {
// ReconcileOciRegistryRule reconciles an OCI registry rule from the OCIValidator config.
func (s *OciRuleService) ReconcileOciRegistryRule(rule v1alpha1.OciRegistryRule) (*types.ValidationRuleResult, error) {
l := s.log.V(0).WithValues("rule", rule.Name(), "host", rule.Host)
vr := buildValidationResult(rule)
errs := make([]error, 0)
details := make([]string, 0)
vr := BuildValidationResult(rule)

var err error
ctx := context.Background()

errMsg := "failed to validate repositories in registry"
if len(rule.Artifacts) == 0 {
errMsg := "failed to validate repositories in registry"
d, e := s.validateRepos(ctx, rule, vr)
details = append(details, d...)
errs = append(errs, e...)
details, errs := s.validateRepos(ctx, rule, vr)

if len(e) > 0 {
l.Error(e[len(e)-1], errMsg)
s.updateResult(vr, errs, errMsg, details...)
return vr, errors.New(errMsg)
if len(errs) > 0 {
l.Error(errs[len(errs)-1], errMsg)
err = errors.New(errMsg)
}
s.updateResult(vr, errs, errMsg, details...)

return vr, nil
return vr, err
}

errMsg := "failed to validate artifact in registry"
details := make([]string, 0)
errs := make([]error, 0)
errMsg = "failed to validate artifact in registry"

for _, artifact := range rule.Artifacts {
ref, err := generateRef(rule.Host, artifact.Ref, vr)
if err != nil {
Expand All @@ -87,12 +87,13 @@ func (s *OciRuleService) ReconcileOciRegistryRule(rule v1alpha1.OciRegistryRule)
details = append(details, d...)
errs = append(errs, e...)
}
s.updateResult(vr, errs, errMsg, details...)

if len(errs) > 0 {
return vr, errors.New(errMsg)
err = errors.New(errMsg)
}
return vr, nil
s.updateResult(vr, errs, errMsg, details...)

return vr, err
}

// validateArtifact validates a single artifact within an OCI registry. Used when either a path to a repo or artifact(s) are specified in an OciRegistryRule.
Expand Down Expand Up @@ -121,6 +122,7 @@ func (s *OciRuleService) validateReference(ctx context.Context, ref name.Referen
errs = append(errs, err)
return details, errs
}
details = append(details, fmt.Sprintf("pulled and validated artifact %s", ref.Name()))

// verify image signature (optional)
if sv.SecretName != "" {
Expand Down Expand Up @@ -212,8 +214,8 @@ func (s *OciRuleService) updateResult(vr *types.ValidationRuleResult, errs []err
vr.Condition.Details = append(vr.Condition.Details, details...)
}

// buildValidationResult builds a default ValidationResult for a given validation type.
func buildValidationResult(rule v1alpha1.OciRegistryRule) *types.ValidationRuleResult {
// BuildValidationResult builds a default ValidationResult for a given validation type.
func BuildValidationResult(rule v1alpha1.OciRegistryRule) *types.ValidationRuleResult {
state := vapi.ValidationSucceeded
latestCondition := vapi.DefaultValidationCondition()
latestCondition.Details = make([]string, 0)
Expand Down
93 changes: 55 additions & 38 deletions internal/validators/oci_validator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package validators
import (
"context"
"fmt"
"net"
"net/http/httptest"
"net/url"
"testing"
Expand All @@ -26,39 +27,44 @@ const (
)

var (
vrr = buildValidationResult(v1alpha1.OciRegistryRule{})
vrr = BuildValidationResult(v1alpha1.OciRegistryRule{})
)

func TestGenerateRef(t *testing.T) {
testCases := []struct {
name string
registry string
artifact string
validationRuleResult *types.ValidationRuleResult
expectedRefName string
expectErr bool
}{
{
name: "Pass: valid artifact with SHA",
registry: registryName,
artifact: "artifact@sha256:ddbac6e7732bf90a4e674a01bf279ce27ea8691530b8d209e6fe77477e0fa406",
validationRuleResult: vrr,
expectedRefName: "registry/artifact@sha256:ddbac6e7732bf90a4e674a01bf279ce27ea8691530b8d209e6fe77477e0fa406",
expectErr: false,
},
{
name: "Pass: valid artifact with semver tag",
registry: registryName,
artifact: "artifact:v1.0.0",
validationRuleResult: vrr,
expectedRefName: "registry/artifact:v1.0.0",
expectErr: false,
},
{
name: "Pass: valid artifact with latest tag",
registry: registryName,
artifact: "artifact",
validationRuleResult: vrr,
expectedRefName: "registry/artifact:latest",
expectErr: false,
},
{
name: "Fail: invalid artifact",
registry: registryName,
artifact: "invalidArtifact",
validationRuleResult: vrr,
Expand All @@ -68,20 +74,23 @@ func TestGenerateRef(t *testing.T) {
}

for _, tc := range testCases {
ref, err := generateRef(tc.registry, tc.artifact, tc.validationRuleResult)

if tc.expectErr {
assert.NotNil(t, err)
} else {
assert.Contains(t, ref.Name(), tc.expectedRefName)
assert.NoError(t, err)
}
t.Run(tc.name, func(t *testing.T) {
ref, err := generateRef(tc.registry, tc.artifact, tc.validationRuleResult)

if tc.expectErr {
assert.NotNil(t, err)
} else {
assert.Contains(t, ref.Name(), tc.expectedRefName)
assert.NoError(t, err)
}
})
}
}

func TestValidateReference(t *testing.T) {
s := httptest.NewServer(registry.New())
defer s.Close()
port := s.Listener.Addr().(*net.TCPAddr).Port

url, err := uploadArtifact(s, validArtifact)
if err != nil {
Expand Down Expand Up @@ -112,15 +121,15 @@ func TestValidateReference(t *testing.T) {
ref: validRef,
layerValidation: false,
pubKeys: nil,
expectedDetail: "",
expectedDetail: fmt.Sprintf("pulled and validated artifact 127.0.0.1:%d/test/oci-image:latest", port),
expectErr: false,
},
{
name: "Pass: valid ref, layer validation",
ref: validRef,
layerValidation: true,
pubKeys: nil,
expectedDetail: "",
expectedDetail: fmt.Sprintf("pulled and validated artifact 127.0.0.1:%d/test/oci-image:latest", port),
expectErr: false,
},
{
Expand Down Expand Up @@ -150,10 +159,11 @@ func TestValidateReference(t *testing.T) {
sv: v1alpha1.SignatureVerification{
SecretName: "secret",
},
expectErr: true,
expectedDetail: fmt.Sprintf("pulled and validated artifact 127.0.0.1:%d/test/oci-image:latest", port),
expectErr: true,
},
{
name: "Pass: valid ref with layer and signature verification",
name: "Fail: valid ref, signature verification enabled with invalid public key",
ref: validRef,
layerValidation: true,
pubKeys: [][]byte{
Expand Down Expand Up @@ -197,6 +207,7 @@ iNa765seE3jYC3MGUe5h52393Dhy7B5bXGsg6EfPpNYamlAEWjxCpHF3Lg==
func TestValidateRepos(t *testing.T) {
s1 := httptest.NewServer(registry.New())
defer s1.Close()
port := s1.Listener.Addr().(*net.TCPAddr).Port

urlWithArtifact, err := uploadArtifact(s1, validArtifact)
if err != nil {
Expand All @@ -211,52 +222,58 @@ func TestValidateRepos(t *testing.T) {
}

testCases := []struct {
name string
host string
expectedDetail string
expectErr bool
}{
{
name: "Pass: valid registry",
host: urlWithArtifact.Host,
expectedDetail: "",
expectedDetail: fmt.Sprintf("pulled and validated artifact 127.0.0.1:%d/test/oci-image:latest", port),
expectErr: false,
},
{
name: "Fail: invalid registry: no repositories",
host: urlNoArtifact.Host,
expectedDetail: "no repositories found in registry",
expectErr: false,
},
{
name: "Fail: invalid registry",
host: "invalidHost",
expectedDetail: "failed to list repositories in registry",
expectErr: true,
},
}

for _, tc := range testCases {
ociClient, err := oci.NewOCIClient(oci.WithAnonymousAuth())
if err != nil {
t.Error(err)
}
ociService := NewOciRuleService(logr.Logger{}, WithOCIClient(ociClient))

rule := v1alpha1.OciRegistryRule{
Host: tc.host,
SignatureVerification: v1alpha1.SignatureVerification{},
}
details, errs := ociService.validateRepos(context.Background(), rule, &types.ValidationRuleResult{})

if tc.expectedDetail == "" {
assert.Empty(t, details)
} else {
assert.Len(t, details, 1)
assert.Contains(t, details[0], tc.expectedDetail)
}

if !tc.expectErr {
assert.Empty(t, errs)
} else {
assert.Len(t, errs, 1)
}
t.Run(tc.name, func(t *testing.T) {
ociClient, err := oci.NewOCIClient(oci.WithAnonymousAuth())
if err != nil {
t.Error(err)
}
ociService := NewOciRuleService(logr.Logger{}, WithOCIClient(ociClient))

rule := v1alpha1.OciRegistryRule{
Host: tc.host,
SignatureVerification: v1alpha1.SignatureVerification{},
}
details, errs := ociService.validateRepos(context.Background(), rule, &types.ValidationRuleResult{})

if tc.expectedDetail == "" {
assert.Empty(t, details)
} else {
assert.Len(t, details, 1)
assert.Contains(t, details[0], tc.expectedDetail)
}

if !tc.expectErr {
assert.Empty(t, errs)
} else {
assert.Len(t, errs, 1)
}
})
}
}

Expand Down
4 changes: 1 addition & 3 deletions pkg/oci/oci_client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,10 +246,8 @@ func (c *Client) VerifySignature(ctx context.Context, ref name.Reference) ([]str
errs = append(errs, err)
continue
}

if hasValidSignature {
details = nil
errs = nil
details = append(details, fmt.Sprintf("verified signature for %s", ref))
return details, errs
}
}
Expand Down

0 comments on commit 891a318

Please sign in to comment.