From 49defde437eb06f6ff249aa055646f52c52836f7 Mon Sep 17 00:00:00 2001 From: Mateusz Kuziemko Date: Tue, 1 Feb 2022 12:58:16 +0100 Subject: [PATCH] Validate input parameters names on Implementation manifest (#610) --- pkg/sdk/renderer/argo/dedicated_renderer.go | 12 +- pkg/sdk/renderer/argo/helpers.go | 3 +- pkg/sdk/renderer/argo/renderer.go | 5 +- pkg/sdk/renderer/argo/typeinstance_handler.go | 6 +- pkg/sdk/validation/manifest/helpers_test.go | 10 +- .../validation/manifest/json_remote_helper.go | 1 + .../manifest/json_remote_implementation.go | 109 +++++++++++- .../json_remote_implementation_export_test.go | 11 +- .../json_remote_implementation_test.go | 155 ++++++++++++++++++ 9 files changed, 299 insertions(+), 13 deletions(-) diff --git a/pkg/sdk/renderer/argo/dedicated_renderer.go b/pkg/sdk/renderer/argo/dedicated_renderer.go index 8a44d3d69..d89669ec0 100644 --- a/pkg/sdk/renderer/argo/dedicated_renderer.go +++ b/pkg/sdk/renderer/argo/dedicated_renderer.go @@ -75,7 +75,11 @@ func newDedicatedRenderer(maxDepth int, policyEnforcedCli PolicyEnforcedHubClien return r } -func (r *dedicatedRenderer) WrapEntrypointWithRootStep(workflow *Workflow) (*Workflow, *WorkflowStep) { +func (r *dedicatedRenderer) WrapEntrypointWithRootStep(workflow *Workflow) (*Workflow, *WorkflowStep, error) { + if workflow == nil || workflow.WorkflowSpec == nil || workflow.Entrypoint == "" { + return nil, nil, errors.New("workflow and its entrypoint cannot be empty") + } + r.entrypointStep = &WorkflowStep{ WorkflowStep: &wfv1.WorkflowStep{ Name: "start-entrypoint", @@ -98,7 +102,7 @@ func (r *dedicatedRenderer) WrapEntrypointWithRootStep(workflow *Workflow) (*Wor workflow.Entrypoint = r.rootTemplate.Name workflow.Templates = append(workflow.Templates, r.rootTemplate) - return workflow, r.entrypointStep + return workflow, r.entrypointStep, nil } func (r *dedicatedRenderer) AppendAdditionalInputTypeInstances(typeInstances []types.InputTypeInstanceRef) { @@ -396,7 +400,9 @@ func (r *dedicatedRenderer) UnmarshalWorkflowFromImplementation(prefix string, i if err != nil { return nil, nil, errors.Wrap(err, "while unmarshaling Argo Workflow from OCF Implementation") } - + if workflow == nil || workflow.WorkflowSpec == nil || workflow.Entrypoint == "" { + return nil, nil, errors.New("workflow and its entrypoint cannot be empty") + } artifactsNameMapping := map[string]string{} for i := range workflow.Templates { diff --git a/pkg/sdk/renderer/argo/helpers.go b/pkg/sdk/renderer/argo/helpers.go index e8c8718b7..8298f8ee0 100644 --- a/pkg/sdk/renderer/argo/helpers.go +++ b/pkg/sdk/renderer/argo/helpers.go @@ -19,7 +19,8 @@ func interfaceRefToHub(in types.InterfaceRef) hubpublicgraphql.InterfaceReferenc } } -func getEntrypointWorkflowIndex(w *Workflow) (int, error) { +// GetEntrypointWorkflowIndex returns workflow entrypoint index +func GetEntrypointWorkflowIndex(w *Workflow) (int, error) { if w == nil { return 0, NewWorkflowNilError() } diff --git a/pkg/sdk/renderer/argo/renderer.go b/pkg/sdk/renderer/argo/renderer.go index 5f4f45e6b..831728fdd 100644 --- a/pkg/sdk/renderer/argo/renderer.go +++ b/pkg/sdk/renderer/argo/renderer.go @@ -116,7 +116,10 @@ func (r *Renderer) Render(ctx context.Context, input *RenderInput) (*RenderOutpu } // 3.1 Add our own root step and replace entrypoint - rootWorkflow, entrypointStep := dedicatedRenderer.WrapEntrypointWithRootStep(rootWorkflow) + rootWorkflow, entrypointStep, err := dedicatedRenderer.WrapEntrypointWithRootStep(rootWorkflow) + if err != nil { + return nil, errors.Wrap(err, "while wrapping entrypoint with root step") + } // 4. Add user input if err := dedicatedRenderer.AddUserInputSecretRefIfProvided(rootWorkflow); err != nil { diff --git a/pkg/sdk/renderer/argo/typeinstance_handler.go b/pkg/sdk/renderer/argo/typeinstance_handler.go index 2978067b3..43644d8ad 100644 --- a/pkg/sdk/renderer/argo/typeinstance_handler.go +++ b/pkg/sdk/renderer/argo/typeinstance_handler.go @@ -38,7 +38,7 @@ func (r *TypeInstanceHandler) AddInputTypeInstances(rootWorkflow *Workflow, inst return nil } - idx, err := getEntrypointWorkflowIndex(rootWorkflow) + idx, err := GetEntrypointWorkflowIndex(rootWorkflow) if err != nil { return err } @@ -204,7 +204,7 @@ func (r *TypeInstanceHandler) AddUploadTypeInstancesStep(rootWorkflow *Workflow, }, } - idx, err := getEntrypointWorkflowIndex(rootWorkflow) + idx, err := GetEntrypointWorkflowIndex(rootWorkflow) if err != nil { return err } @@ -295,7 +295,7 @@ func (r *TypeInstanceHandler) AddUpdateTypeInstancesStep(rootWorkflow *Workflow, }, } - idx, err := getEntrypointWorkflowIndex(rootWorkflow) + idx, err := GetEntrypointWorkflowIndex(rootWorkflow) if err != nil { return err } diff --git a/pkg/sdk/validation/manifest/helpers_test.go b/pkg/sdk/validation/manifest/helpers_test.go index 2c8967ca6..362cd08a3 100644 --- a/pkg/sdk/validation/manifest/helpers_test.go +++ b/pkg/sdk/validation/manifest/helpers_test.go @@ -13,8 +13,9 @@ import ( ) type fakeHub struct { - checkManifestsFn func(ctx context.Context, manifestRefs []gqlpublicapi.ManifestReference) (map[gqlpublicapi.ManifestReference]bool, error) - knownTypes []*gqlpublicapi.Type + checkManifestsFn func(ctx context.Context, manifestRefs []gqlpublicapi.ManifestReference) (map[gqlpublicapi.ManifestReference]bool, error) + knownTypes []*gqlpublicapi.Type + interfaceRevision *gqlpublicapi.InterfaceRevision } func (h *fakeHub) ListTypes(_ context.Context, opts ...public.TypeOption) ([]*gqlpublicapi.Type, error) { @@ -47,6 +48,10 @@ func (h *fakeHub) CheckManifestRevisionsExist(ctx context.Context, manifestRefs return h.checkManifestsFn(ctx, manifestRefs) } +func (h *fakeHub) FindInterfaceRevision(_ context.Context, _ gqlpublicapi.InterfaceReference, _ ...public.InterfaceRevisionOption) (*gqlpublicapi.InterfaceRevision, error) { + return h.interfaceRevision, nil +} + func fixHub(t *testing.T, knownListTypes []*gqlpublicapi.Type, manifests map[gqlpublicapi.ManifestReference]bool, err error) *fakeHub { hub := fixHubForManifestsExistence(t, manifests, err) hub.knownTypes = knownListTypes @@ -69,6 +74,7 @@ func fixHubForManifestsExistence(t *testing.T, result map[gqlpublicapi.ManifestR return result, err }, + interfaceRevision: &gqlpublicapi.InterfaceRevision{}, } return hub } diff --git a/pkg/sdk/validation/manifest/json_remote_helper.go b/pkg/sdk/validation/manifest/json_remote_helper.go index 33b2302a0..72edc8c2a 100644 --- a/pkg/sdk/validation/manifest/json_remote_helper.go +++ b/pkg/sdk/validation/manifest/json_remote_helper.go @@ -13,6 +13,7 @@ import ( // Hub is an interface for Hub GraphQL client methods needed for the remote validation. type Hub interface { CheckManifestRevisionsExist(ctx context.Context, manifestRefs []gqlpublicapi.ManifestReference) (map[gqlpublicapi.ManifestReference]bool, error) + FindInterfaceRevision(ctx context.Context, ref gqlpublicapi.InterfaceReference, opts ...public.InterfaceRevisionOption) (*gqlpublicapi.InterfaceRevision, error) ListTypes(ctx context.Context, opts ...public.TypeOption) ([]*gqlpublicapi.Type, error) } diff --git a/pkg/sdk/validation/manifest/json_remote_implementation.go b/pkg/sdk/validation/manifest/json_remote_implementation.go index f8e03a953..5d5a62357 100644 --- a/pkg/sdk/validation/manifest/json_remote_implementation.go +++ b/pkg/sdk/validation/manifest/json_remote_implementation.go @@ -11,6 +11,10 @@ import ( gqlpublicapi "capact.io/capact/pkg/hub/api/graphql/public" "capact.io/capact/pkg/hub/client/public" "capact.io/capact/pkg/sdk/apis/0.0.1/types" + wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" + + "capact.io/capact/pkg/sdk/renderer/argo" + "k8s.io/utils/strings/slices" "github.com/dustin/go-humanize/english" "github.com/pkg/errors" @@ -47,7 +51,11 @@ func (v *RemoteImplementationValidator) Do(ctx context.Context, _ types.Manifest if err != nil { return ValidationResult{}, errors.Wrap(err, "while unmarshalling JSON into Implementation type") } - validateFns := []validateFn{v.checkManifestRevisionsExist, v.checkRequiresParentNodes} + validateFns := []validateFn{ + v.checkManifestRevisionsExist, + v.checkRequiresParentNodes, + v.validateInputArtifactsNames, + } for _, fn := range validateFns { validationResults, err := fn(ctx, entity) @@ -115,10 +123,107 @@ func (v *RemoteImplementationValidator) checkManifestRevisionsExist(ctx context. }) } } - return checkManifestRevisionsExist(ctx, v.hub, manifestRefsToCheck) } +func (v *RemoteImplementationValidator) validateInputArtifactsNames(ctx context.Context, entity types.Implementation) (ValidationResult, error) { + var validationErrs []error + var interfacesInputNames []string + var implAdditionalInput []string + var workflowArtifacts []wfv1.Artifact + + //1. get interface input names + for _, implementsItem := range entity.Spec.Implements { + interfaceInput, err := v.fetchInterfaceInput(ctx, gqlpublicapi.InterfaceReference{ + Path: implementsItem.Path, + Revision: implementsItem.Revision, + }, v.hub) + if err != nil { + return ValidationResult{}, errors.Wrap(err, "while fetching Interface inputs") + } + for _, inputParameter := range interfaceInput.Parameters { + interfacesInputNames = append(interfacesInputNames, inputParameter.Name) + } + for _, inputTypeInstance := range interfaceInput.TypeInstances { + interfacesInputNames = append(interfacesInputNames, inputTypeInstance.Name) + } + } + + //2. get implementation additional inputs + if entity.Spec.AdditionalInput != nil { + for name := range entity.Spec.AdditionalInput.Parameters { + implAdditionalInput = append(implAdditionalInput, name) + } + for name := range entity.Spec.AdditionalInput.TypeInstances { + implAdditionalInput = append(implAdditionalInput, name) + } + } + + //3. get inputs from entrypoint workflow template + workflow, err := v.decodeImplArgsToArgoWorkflow(entity.Spec.Action.Args) + if err != nil { + return ValidationResult{}, errors.Wrap(err, "while decoding Implementation arguments to Argo workflow") + } + if workflow != nil && workflow.WorkflowSpec != nil { + idx, err := argo.GetEntrypointWorkflowIndex(workflow) + if err != nil { + return ValidationResult{}, errors.Wrap(err, "while getting entrypoint index from workflow") + } + workflowArtifacts = workflow.Templates[idx].Inputs.Artifacts + } + + //4. verify if the inputs from Implementation and Interface match with Argo workflow artifacts + for _, artifact := range workflowArtifacts { + existsInInterface := slices.Contains(interfacesInputNames, artifact.Name) + existsInAdditionalInput := slices.Contains(implAdditionalInput, artifact.Name) + + if existsInInterface && + artifact.Optional { + validationErrs = append(validationErrs, fmt.Errorf("invalid workflow input artifact %q: it shouldn't be optional as it is defined as Interface input", artifact.Name)) + } + if existsInAdditionalInput && !artifact.Optional { + validationErrs = append(validationErrs, fmt.Errorf("invalid workflow input artifact %q: it should be optional, as it is defined as Implementation additional input", artifact.Name)) + } + if !existsInInterface && !existsInAdditionalInput { + validationErrs = append(validationErrs, fmt.Errorf("unknown workflow input artifact %q: there is no such input neither in Interface input, nor Implementation additional input", artifact.Name)) + } + } + + return ValidationResult{Errors: validationErrs}, nil +} + +func (v *RemoteImplementationValidator) fetchInterfaceInput(ctx context.Context, interfaceRef gqlpublicapi.InterfaceReference, hub Hub) (gqlpublicapi.InterfaceInput, error) { + iface, err := hub.FindInterfaceRevision(ctx, interfaceRef, public.WithInterfaceRevisionFields(public.InterfaceRevisionInputFields)) + if err != nil { + return gqlpublicapi.InterfaceInput{}, errors.Wrap(err, "while looking for Interface definition") + } + if iface == nil { + return gqlpublicapi.InterfaceInput{}, fmt.Errorf("interface %s:%s was not found in Hub", interfaceRef.Path, interfaceRef.Revision) + } + + if iface.Spec == nil || iface.Spec.Input == nil { + return gqlpublicapi.InterfaceInput{}, nil + } + + return *iface.Spec.Input, nil +} + +func (v *RemoteImplementationValidator) decodeImplArgsToArgoWorkflow(implArgs map[string]interface{}) (*argo.Workflow, error) { + var decodedImplArgs = struct { + Workflow argo.Workflow `json:"workflow"` + }{} + + b, err := json.Marshal(implArgs) + if err != nil { + return nil, errors.Wrap(err, "while marshaling Implementation arguments") + } + + if err := json.Unmarshal(b, &decodedImplArgs); err != nil { + return nil, errors.Wrap(err, "while unmarshalling Implementation arguments to Argo Workflow") + } + return &decodedImplArgs.Workflow, nil +} + func (v *RemoteImplementationValidator) checkRequiresParentNodes(ctx context.Context, entity types.Implementation) (ValidationResult, error) { parentNodeTypesToCheck := ParentNodesAssociation{} for requiresKey, reqItem := range entity.Spec.Requires { diff --git a/pkg/sdk/validation/manifest/json_remote_implementation_export_test.go b/pkg/sdk/validation/manifest/json_remote_implementation_export_test.go index 0c9815bc4..c9a315498 100644 --- a/pkg/sdk/validation/manifest/json_remote_implementation_export_test.go +++ b/pkg/sdk/validation/manifest/json_remote_implementation_export_test.go @@ -1,9 +1,18 @@ package manifest -import "context" +import ( + "context" + + "capact.io/capact/pkg/sdk/apis/0.0.1/types" +) // CheckParentNodesAssociation is just a hack to export the internal method for testing purposes. // The *_test.go files are not compiled into final binary, and as it's under _test.go it's also not accessible for other non-testing packages. func (v *RemoteImplementationValidator) CheckParentNodesAssociation(ctx context.Context, relations ParentNodesAssociation) (ValidationResult, error) { return v.checkParentNodesAssociation(ctx, relations) } + +//ValidateInputArtifactsNames exports validateInputArtifactsNames method for testing purposes. +func (v *RemoteImplementationValidator) ValidateInputArtifactsNames(ctx context.Context, entity types.Implementation) (ValidationResult, error) { + return v.validateInputArtifactsNames(ctx, entity) +} diff --git a/pkg/sdk/validation/manifest/json_remote_implementation_test.go b/pkg/sdk/validation/manifest/json_remote_implementation_test.go index d18fa5813..892776c30 100644 --- a/pkg/sdk/validation/manifest/json_remote_implementation_test.go +++ b/pkg/sdk/validation/manifest/json_remote_implementation_test.go @@ -3,15 +3,136 @@ package manifest_test import ( "context" "errors" + "fmt" "testing" gqlpublicapi "capact.io/capact/pkg/hub/api/graphql/public" + "capact.io/capact/pkg/sdk/apis/0.0.1/types" + "capact.io/capact/pkg/sdk/renderer/argo" "capact.io/capact/pkg/sdk/validation/manifest" + wfv1 "github.com/argoproj/argo-workflows/v3/pkg/apis/workflow/v1alpha1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestValidateInputArtifactsNames(t *testing.T) { + inputParametersName := "input-parameters" + additionalParameterName := "additional-parameters" + tests := map[string]struct { + ifaceInput *gqlpublicapi.InterfaceInput + implAdditionalInput *types.AdditionalInput + argoArtifacts []wfv1.Artifact + exprectedErrors []error + }{ + "When Argo input does not exist in the Interface and Implementation": { + ifaceInput: &gqlpublicapi.InterfaceInput{ + Parameters: []*gqlpublicapi.InputParameter{}, + }, + implAdditionalInput: &types.AdditionalInput{ + Parameters: map[string]types.AdditionalInputParameter{ + additionalParameterName: {}, + }, + }, + argoArtifacts: []wfv1.Artifact{ + { + Name: inputParametersName, + }, + { + Name: additionalParameterName, + Optional: true, + }, + }, + exprectedErrors: []error{fmt.Errorf("unknown workflow input artifact \"%s\": there is no such input neither in Interface input, nor Implementation additional input", inputParametersName)}, + }, + "When Argo input has optional input that does exist in Interface ": { + ifaceInput: &gqlpublicapi.InterfaceInput{ + Parameters: []*gqlpublicapi.InputParameter{ + { + Name: inputParametersName, + }, + }, + }, + implAdditionalInput: &types.AdditionalInput{}, + argoArtifacts: []wfv1.Artifact{ + { + Name: inputParametersName, + Optional: true, + }, + }, + exprectedErrors: []error{fmt.Errorf("invalid workflow input artifact \"%s\": it shouldn't be optional as it is defined as Interface input", inputParametersName)}, + }, + "When Argo input is not optional but exists in Implementation additional inputs": { + ifaceInput: &gqlpublicapi.InterfaceInput{ + Parameters: []*gqlpublicapi.InputParameter{ + { + Name: inputParametersName, + }, + }, + }, + implAdditionalInput: &types.AdditionalInput{ + Parameters: map[string]types.AdditionalInputParameter{ + additionalParameterName: {}, + }, + }, + argoArtifacts: []wfv1.Artifact{ + { + Name: additionalParameterName, + Optional: false, + }, + }, + exprectedErrors: []error{fmt.Errorf("invalid workflow input artifact \"%s\": it should be optional, as it is defined as Implementation additional input", additionalParameterName)}, + }, + "When Argo inputs are correctly set": { + ifaceInput: &gqlpublicapi.InterfaceInput{ + Parameters: []*gqlpublicapi.InputParameter{ + { + Name: inputParametersName, + }, + }, + }, + implAdditionalInput: &types.AdditionalInput{ + Parameters: map[string]types.AdditionalInputParameter{ + additionalParameterName: {}, + }, + }, + argoArtifacts: []wfv1.Artifact{ + { + Name: inputParametersName, + Optional: false, + }, + { + Name: additionalParameterName, + Optional: true, + }, + }, + exprectedErrors: nil, + }, + } + for tn, tc := range tests { + t.Run(tn, func(t *testing.T) { + // given + ctx := context.Background() + hubCli := fakeHub{ + interfaceRevision: &gqlpublicapi.InterfaceRevision{ + Spec: &gqlpublicapi.InterfaceSpec{ + Input: tc.ifaceInput, + }, + }, + } + validator := manifest.NewRemoteImplementationValidator(&hubCli) + implementation := fixImplementation(tc.argoArtifacts, tc.implAdditionalInput) + + // when + result, err := validator.ValidateInputArtifactsNames(ctx, implementation) + + // then + require.NoError(t, err) + assert.Equal(t, tc.exprectedErrors, result.Errors) + }) + } +} + func TestCheckParentNodesAssociation(t *testing.T) { tests := map[string]struct { knownTypes []*gqlpublicapi.Type @@ -121,3 +242,37 @@ func fixGQLType(path, rev, parent string) *gqlpublicapi.Type { }}, } } +func fixImplementation(inputArtifacts []wfv1.Artifact, implAdditionalInput *types.AdditionalInput) types.Implementation { + workflow := argo.Workflow{ + WorkflowSpec: &wfv1.WorkflowSpec{ + Entrypoint: "test", + }, + Templates: []*argo.Template{ + { + Template: &wfv1.Template{ + Name: "test", + Inputs: wfv1.Inputs{ + Artifacts: inputArtifacts, + }, + }, + }, + }, + } + + return types.Implementation{ + Spec: types.ImplementationSpec{ + Implements: []types.Implement{ + { + Path: "cap.interface.test.impl", + Revision: "0.1.0", + }, + }, + Action: types.Action{ + Args: map[string]interface{}{ + "workflow": workflow, + }, + }, + AdditionalInput: implAdditionalInput, + }, + } +}