From 891a318738ac5f552b2bf9325754add5e811a69a Mon Sep 17 00:00:00 2001 From: Tyler Gillson Date: Fri, 19 Jul 2024 10:33:18 -0600 Subject: [PATCH] fix: associate unexected errs w/ rules; always include validation result details (#219) ## Issue ## 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 --- Dockerfile.devspace | 2 +- .../controller/ocivalidator_controller.go | 18 ++-- internal/validators/oci_validator.go | 38 ++++---- internal/validators/oci_validator_test.go | 93 +++++++++++-------- pkg/oci/oci_client.go | 4 +- 5 files changed, 87 insertions(+), 68 deletions(-) diff --git a/Dockerfile.devspace b/Dockerfile.devspace index a2416d0b..e6b69559 100644 --- a/Dockerfile.devspace +++ b/Dockerfile.devspace @@ -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 diff --git a/internal/controller/ocivalidator_controller.go b/internal/controller/ocivalidator_controller.go index 76dd4cc9..d7e89665 100644 --- a/internal/controller/ocivalidator_controller.go +++ b/internal/controller/ocivalidator_controller.go @@ -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 } @@ -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) } @@ -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 @@ -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, ) } @@ -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 diff --git a/internal/validators/oci_validator.go b/internal/validators/oci_validator.go index 88dd8313..3f351282 100644 --- a/internal/validators/oci_validator.go +++ b/internal/validators/oci_validator.go @@ -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 { @@ -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. @@ -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 != "" { @@ -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) diff --git a/internal/validators/oci_validator_test.go b/internal/validators/oci_validator_test.go index f03607b3..16e08a1b 100644 --- a/internal/validators/oci_validator_test.go +++ b/internal/validators/oci_validator_test.go @@ -3,6 +3,7 @@ package validators import ( "context" "fmt" + "net" "net/http/httptest" "net/url" "testing" @@ -26,11 +27,12 @@ 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 @@ -38,6 +40,7 @@ func TestGenerateRef(t *testing.T) { expectErr bool }{ { + name: "Pass: valid artifact with SHA", registry: registryName, artifact: "artifact@sha256:ddbac6e7732bf90a4e674a01bf279ce27ea8691530b8d209e6fe77477e0fa406", validationRuleResult: vrr, @@ -45,6 +48,7 @@ func TestGenerateRef(t *testing.T) { expectErr: false, }, { + name: "Pass: valid artifact with semver tag", registry: registryName, artifact: "artifact:v1.0.0", validationRuleResult: vrr, @@ -52,6 +56,7 @@ func TestGenerateRef(t *testing.T) { expectErr: false, }, { + name: "Pass: valid artifact with latest tag", registry: registryName, artifact: "artifact", validationRuleResult: vrr, @@ -59,6 +64,7 @@ func TestGenerateRef(t *testing.T) { expectErr: false, }, { + name: "Fail: invalid artifact", registry: registryName, artifact: "invalidArtifact", validationRuleResult: vrr, @@ -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 { @@ -112,7 +121,7 @@ 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, }, { @@ -120,7 +129,7 @@ func TestValidateReference(t *testing.T) { 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, }, { @@ -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{ @@ -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 { @@ -211,21 +222,25 @@ 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, @@ -233,30 +248,32 @@ func TestValidateRepos(t *testing.T) { } 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) + } + }) } } diff --git a/pkg/oci/oci_client.go b/pkg/oci/oci_client.go index 60b2e38c..623e1468 100644 --- a/pkg/oci/oci_client.go +++ b/pkg/oci/oci_client.go @@ -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 } }