From f0e276bb7b76455f8186d1c1b0da16ac88e7098e Mon Sep 17 00:00:00 2001 From: Dmitry Ukov Date: Mon, 18 Jan 2021 12:41:34 +0400 Subject: [PATCH] Implement basic validation for plan and phase Plan validation: 1. Each phase must be defined within phase document bundle. 2. Each phase does not return error for Validate method Phase validation: 1. Document bundle associated with the phase can be rendered without an error. 2. Associated executor must not return an error. Relates-to: #330 Change-Id: I08c5e8e42570f2cafdced5a02481b033414ffae0 --- pkg/phase/client.go | 29 +++- pkg/phase/client_test.go | 127 +++++++++++++++++- pkg/phase/helper_test.go | 4 +- .../valid_site/phases/kubeapply_phase.yaml | 10 ++ .../valid_site/phases/kustomization.yaml | 1 + .../testdata/valid_site/phases/phaseplan.yaml | 14 ++ 6 files changed, 178 insertions(+), 7 deletions(-) create mode 100644 pkg/phase/testdata/valid_site/phases/kubeapply_phase.yaml diff --git a/pkg/phase/client.go b/pkg/phase/client.go index 3bd8af535..686a2effe 100644 --- a/pkg/phase/client.go +++ b/pkg/phase/client.go @@ -16,6 +16,7 @@ package phase import ( "io" + "io/ioutil" "path/filepath" "k8s.io/apimachinery/pkg/runtime/schema" @@ -124,9 +125,19 @@ func (p *phase) Run(ro ifc.RunOptions) error { } // Validate makes sure that phase is properly configured -// TODO implement this func (p *phase) Validate() error { - return nil + // Check that we can render documents supplied to phase + err := p.Render(ioutil.Discard, false, ifc.RenderOptions{}) + if err != nil { + return err + } + + // Check that executor if properly configured + executor, err := p.Executor() + if err != nil { + return err + } + return executor.Validate() } // Render executor documents @@ -186,8 +197,18 @@ type plan struct { } // Validate makes sure that phase plan is properly configured -// TODO implement this -func (p *plan) Validate() error { return nil } +func (p *plan) Validate() error { + for _, step := range p.apiObj.Phases { + phaseRunner, err := p.phaseClient.PhaseByID(ifc.ID{Name: step.Name}) + if err != nil { + return err + } + if err = phaseRunner.Validate(); err != nil { + return err + } + } + return nil +} // Run function excutes Run method for each phase func (p *plan) Run(ro ifc.RunOptions) error { diff --git a/pkg/phase/client_test.go b/pkg/phase/client_test.go index 5b09ca3ff..41bdd0e7a 100644 --- a/pkg/phase/client_test.go +++ b/pkg/phase/client_test.go @@ -15,6 +15,7 @@ package phase_test import ( + "fmt" "io" "testing" @@ -135,6 +136,78 @@ func TestPhaseRun(t *testing.T) { } } +func TestPhaseValidate(t *testing.T) { + tests := []struct { + name string + errContains string + phaseID ifc.ID + configFunc func(t *testing.T) *config.Config + registryFunc phase.ExecutorRegistry + }{ + { + name: "Success fake executor", + configFunc: testConfig, + phaseID: ifc.ID{Name: "capi_init"}, + registryFunc: fakeRegistry, + }, + { + name: "Error no document entry point", + configFunc: testConfig, + phaseID: ifc.ID{Name: "no_entry_point"}, + registryFunc: fakeRegistry, + errContains: "documentEntryPoint is not defined for the phase 'no_entry_point' in namespace ''", + }, + { + name: "Error no executor", + configFunc: testConfig, + phaseID: ifc.ID{Name: "no_executor_phase"}, + registryFunc: fakeRegistry, + errContains: "Phase name 'no_executor_phase', namespace '' must have executorRef field defined in config", + }, + { + name: "Error executor validate", + configFunc: testConfig, + phaseID: ifc.ID{Name: "kube_apply"}, + registryFunc: func() map[schema.GroupVersionKind]ifc.ExecutorFactory { + gvk := schema.GroupVersionKind{ + Group: "airshipit.org", + Version: "v1alpha1", + Kind: "KubernetesApply", + } + return map[schema.GroupVersionKind]ifc.ExecutorFactory{ + gvk: func(config ifc.ExecutorConfig) (ifc.Executor, error) { + return fakeExecutor{ + validate: fmt.Errorf("validation error"), + }, nil + }, + } + }, + errContains: "validation error", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + conf := tt.configFunc(t) + helper, err := phase.NewHelper(conf) + require.NoError(t, err) + require.NotNil(t, helper) + client := phase.NewClient(helper, phase.InjectRegistry(tt.registryFunc)) + require.NotNil(t, client) + p, err := client.PhaseByID(tt.phaseID) + require.NoError(t, err) + err = p.Validate() + if tt.errContains != "" { + require.Error(t, err) + assert.Equal(t, tt.errContains, err.Error()) + } else { + require.NoError(t, err) + } + }) + } +} + // TODO develop tests, when we add phase object validation func TestClientByAPIObj(t *testing.T) { helper, err := phase.NewHelper(testConfig(t)) @@ -300,6 +373,57 @@ func TestPlanRun(t *testing.T) { }) } } +func TestPlanValidate(t *testing.T) { + testCases := []struct { + name string + errContains string + planID ifc.ID + configFunc func(t *testing.T) *config.Config + registryFunc phase.ExecutorRegistry + }{ + { + name: "Valid fake executor", + configFunc: testConfig, + planID: ifc.ID{Name: "init"}, + registryFunc: fakeRegistry, + }, + { + name: "Invalid fake executor", + configFunc: testConfig, + planID: ifc.ID{Name: "plan_invalid_phase"}, + registryFunc: fakeRegistry, + errContains: "documentEntryPoint is not defined for the phase 'no_entry_point' in namespace ''", + }, + { + name: "Phase does not exist", + configFunc: testConfig, + planID: ifc.ID{Name: "phase_not_exist"}, + registryFunc: fakeRegistry, + errContains: `document filtered by selector [Group="airshipit.org", Version="v1alpha1", ` + + `Kind="Phase", Name="non_existent_name"] found no documents`, + }, + } + for _, tc := range testCases { + tt := tc + t.Run(tt.name, func(t *testing.T) { + conf := tt.configFunc(t) + helper, err := phase.NewHelper(conf) + require.NoError(t, err) + require.NotNil(t, helper) + client := phase.NewClient(helper, phase.InjectRegistry(tt.registryFunc)) + require.NotNil(t, client) + p, err := client.PlanByID(tt.planID) + require.NoError(t, err) + err = p.Validate() + if tt.errContains != "" { + require.Error(t, err) + assert.Equal(t, tt.errContains, err.Error()) + } else { + require.NoError(t, err) + } + }) + } +} func fakeExecFactory(config ifc.ExecutorConfig) (ifc.Executor, error) { return fakeExecutor{}, nil @@ -308,6 +432,7 @@ func fakeExecFactory(config ifc.ExecutorConfig) (ifc.Executor, error) { var _ ifc.Executor = fakeExecutor{} type fakeExecutor struct { + validate error } func (e fakeExecutor) Render(_ io.Writer, _ ifc.RenderOptions) error { @@ -319,5 +444,5 @@ func (e fakeExecutor) Run(ch chan events.Event, _ ifc.RunOptions) { } func (e fakeExecutor) Validate() error { - return nil + return e.validate } diff --git a/pkg/phase/helper_test.go b/pkg/phase/helper_test.go index 6fc574b45..2a404dbde 100644 --- a/pkg/phase/helper_test.go +++ b/pkg/phase/helper_test.go @@ -189,7 +189,7 @@ func TestHelperListPhases(t *testing.T) { }{ { name: "Success phase list", - phaseLen: 4, + phaseLen: 5, config: testConfig, }, { @@ -240,7 +240,7 @@ func TestHelperListPlans(t *testing.T) { }{ { name: "Success plan list", - expectedLen: 3, + expectedLen: 5, config: testConfig, }, { diff --git a/pkg/phase/testdata/valid_site/phases/kubeapply_phase.yaml b/pkg/phase/testdata/valid_site/phases/kubeapply_phase.yaml new file mode 100644 index 000000000..edb788fe9 --- /dev/null +++ b/pkg/phase/testdata/valid_site/phases/kubeapply_phase.yaml @@ -0,0 +1,10 @@ +apiVersion: airshipit.org/v1alpha1 +kind: Phase +metadata: + name: kube_apply +config: + executorRef: + apiVersion: airshipit.org/v1alpha1 + kind: KubernetesApply + name: kubernetes-apply + documentEntryPoint: valid_site/phases \ No newline at end of file diff --git a/pkg/phase/testdata/valid_site/phases/kustomization.yaml b/pkg/phase/testdata/valid_site/phases/kustomization.yaml index fc7e6fbd1..d9edc4063 100644 --- a/pkg/phase/testdata/valid_site/phases/kustomization.yaml +++ b/pkg/phase/testdata/valid_site/phases/kustomization.yaml @@ -8,3 +8,4 @@ resources: - cluster_map.yaml - phase_no_docentrypoint.yaml - no_executor_phase.yaml + - kubeapply_phase.yaml diff --git a/pkg/phase/testdata/valid_site/phases/phaseplan.yaml b/pkg/phase/testdata/valid_site/phases/phaseplan.yaml index 8d06a7d6c..d5512aeea 100644 --- a/pkg/phase/testdata/valid_site/phases/phaseplan.yaml +++ b/pkg/phase/testdata/valid_site/phases/phaseplan.yaml @@ -22,3 +22,17 @@ metadata: name: some_plan phases: - name: some_phase +--- +apiVersion: airshipit.org/v1alpha1 +kind: PhasePlan +metadata: + name: plan_invalid_phase +phases: + - name: no_entry_point +--- +apiVersion: airshipit.org/v1alpha1 +kind: PhasePlan +metadata: + name: phase_not_exist +phases: + - name: non_existent_name