From a9932b73c0a24578ac07e9593a12d2ae300ab370 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Tue, 15 Sep 2020 14:48:54 -0400 Subject: [PATCH 1/7] add container name validation during creation and push Signed-off-by: Stephanie --- pkg/odo/cli/component/create.go | 10 +++++++++- pkg/odo/cli/component/push.go | 23 +++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/pkg/odo/cli/component/create.go b/pkg/odo/cli/component/create.go index 0cccd4229b3..997799a9426 100644 --- a/pkg/odo/cli/component/create.go +++ b/pkg/odo/cli/component/create.go @@ -18,6 +18,7 @@ import ( "github.com/openshift/odo/pkg/component" "github.com/openshift/odo/pkg/config" "github.com/openshift/odo/pkg/devfile" + adaptersCommon "github.com/openshift/odo/pkg/devfile/adapters/common" "github.com/openshift/odo/pkg/devfile/parser" "github.com/openshift/odo/pkg/devfile/parser/data/common" "github.com/openshift/odo/pkg/envinfo" @@ -835,13 +836,13 @@ func (co *CreateOptions) Validate() (err error) { if err != nil { return err } - // Only validate namespace if pushtarget isn't docker if !pushtarget.IsPushTargetDocker() { err := util.ValidateK8sResourceName("component namespace", co.devfileMetadata.componentNamespace) if err != nil { return err } + } spinner.End(true) @@ -1062,6 +1063,13 @@ func (co *CreateOptions) devfileRun() (err error) { if err != nil { return errors.Wrap(err, "unable to parse devfile") } + containerComponents := adaptersCommon.GetDevfileContainerComponents(devObj.Data) + for _, comp := range containerComponents { + err := util.ValidateK8sResourceName("container name", comp.Name) + if err != nil { + return err + } + } err = downloadStarterProject(devObj, co.devfileMetadata.starter, co.interactive) if err != nil { diff --git a/pkg/odo/cli/component/push.go b/pkg/odo/cli/component/push.go index 5ed4d43e87a..a6b09b5d814 100644 --- a/pkg/odo/cli/component/push.go +++ b/pkg/odo/cli/component/push.go @@ -10,6 +10,7 @@ import ( ktemplates "k8s.io/kubectl/pkg/util/templates" "github.com/openshift/odo/pkg/component" + adaptersCommon "github.com/openshift/odo/pkg/devfile/adapters/common" "github.com/openshift/odo/pkg/devfile/parser" "github.com/openshift/odo/pkg/log" projectCmd "github.com/openshift/odo/pkg/odo/cli/project" @@ -175,6 +176,28 @@ func (po *PushOptions) Validate() (err error) { // If Devfile is present we do not need to validate the below S2I checks // TODO: Perhaps one day move Devfile validation to here instead? if util.CheckPathExists(po.DevfilePath) { + spinner := log.Spinner("Validating K8s Resources Name") + defer spinner.End(false) + err = util.ValidateK8sResourceName("component name", po.EnvSpecificInfo.GetName()) + if err != nil { + return err + } + containerComponents := adaptersCommon.GetDevfileContainerComponents(po.Devfile.Data) + for _, comp := range containerComponents { + err := util.ValidateK8sResourceName("container name", comp.Name) + if err != nil { + return err + } + } + // Only validate namespace if pushtarget isn't docker + if !pushtarget.IsPushTargetDocker() { + err := util.ValidateK8sResourceName("component namespace", po.EnvSpecificInfo.GetNamespace()) + if err != nil { + return err + } + } + + spinner.End(true) return nil } From 8870d8af835510f48ef4c8bb0d42040a0b563f4b Mon Sep 17 00:00:00 2001 From: Stephanie Date: Tue, 15 Sep 2020 19:45:09 -0400 Subject: [PATCH 2/7] fix integration test failure Signed-off-by: Stephanie --- tests/integration/devfile/cmd_devfile_env_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/devfile/cmd_devfile_env_test.go b/tests/integration/devfile/cmd_devfile_env_test.go index 690062590fd..0e42bcbb008 100644 --- a/tests/integration/devfile/cmd_devfile_env_test.go +++ b/tests/integration/devfile/cmd_devfile_env_test.go @@ -13,7 +13,7 @@ import ( var _ = Describe("odo devfile env command tests", func() { const ( testName = "testname" - testNamepace = "testNamepace" + testNamepace = "testnamepace" testDebugPort = "8888" fakeParameter = "fakeParameter" ) From 40011ab40594e90c7341503ee5a746a0752310a2 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Tue, 15 Sep 2020 23:35:21 -0400 Subject: [PATCH 3/7] add integration tests Signed-off-by: Stephanie --- tests/integration/devfile/cmd_devfile_create_test.go | 6 ++++++ tests/integration/devfile/cmd_devfile_push_test.go | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/tests/integration/devfile/cmd_devfile_create_test.go b/tests/integration/devfile/cmd_devfile_create_test.go index bd3fa3c93cd..5a5f5f04ecd 100644 --- a/tests/integration/devfile/cmd_devfile_create_test.go +++ b/tests/integration/devfile/cmd_devfile_create_test.go @@ -197,6 +197,12 @@ var _ = Describe("odo devfile create command tests", func() { It("should fail to create the devfile component with --devfile points to different devfile", func() { helper.CmdShouldFail("odo", "create", "nodejs", "--devfile", "/path/to/file") }) + + It("should fail to create the devfile component if the container name is too long", func() { + helper.ReplaceString("devfile.yaml", "runtime", "runtimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntime") + output := helper.CmdShouldFail("odo", "create", "--devfile", "./devfile.yaml") + Expect(output).Should(ContainSubstring("Contain at most 63 characters")) + }) }) Context("When devfile exists not in user's working directory and user specify the devfile path via --devfile", func() { diff --git a/tests/integration/devfile/cmd_devfile_push_test.go b/tests/integration/devfile/cmd_devfile_push_test.go index a282d2853ef..e26be45090b 100644 --- a/tests/integration/devfile/cmd_devfile_push_test.go +++ b/tests/integration/devfile/cmd_devfile_push_test.go @@ -649,6 +649,17 @@ var _ = Describe("odo devfile push command tests", func() { }) + It("should fail to push the devfile component if the container name is too long", func() { + + helper.CmdShouldPass("odo", "create", "nodejs", "--project", namespace, cmpName) + + helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), context) + helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "nodejs", "devfile.yaml"), filepath.Join(context, "devfile.yaml")) + helper.ReplaceString("devfile.yaml", "runtime", "runtimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntime") + output := helper.CmdShouldFail("odo", "push", "--context", context) + Expect(output).Should(ContainSubstring("Contain at most 63 characters")) + }) + }) Context("Verify files are correctly synced", func() { From aafdfaa9366503aef3bc8be21603e0f6240337e5 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Thu, 17 Sep 2020 12:31:53 -0400 Subject: [PATCH 4/7] use unit test instead of integration test Signed-off-by: Stephanie --- pkg/devfile/validate/validate.go | 15 ++ pkg/devfile/validate/validate_test.go | 129 ++++++++++++++++++ pkg/odo/cli/component/create.go | 11 +- pkg/odo/cli/component/push.go | 11 +- .../devfile/cmd_devfile_create_test.go | 6 - .../devfile/cmd_devfile_push_test.go | 11 -- 6 files changed, 152 insertions(+), 31 deletions(-) create mode 100644 pkg/devfile/validate/validate_test.go diff --git a/pkg/devfile/validate/validate.go b/pkg/devfile/validate/validate.go index 970fd28662f..e262f977cd6 100644 --- a/pkg/devfile/validate/validate.go +++ b/pkg/devfile/validate/validate.go @@ -3,7 +3,10 @@ package validate import ( "fmt" + adaptersCommon "github.com/openshift/odo/pkg/devfile/adapters/common" + "github.com/openshift/odo/pkg/devfile/parser/data" "github.com/openshift/odo/pkg/devfile/parser/data/common" + "github.com/openshift/odo/pkg/util" "k8s.io/klog" v200 "github.com/openshift/odo/pkg/devfile/parser/data/2.0.0" @@ -35,3 +38,15 @@ func ValidateDevfileData(data interface{}) error { return nil } + +// ValidateContatinerName validates whether the container name is valid for K8 +func ValidateContatinerName(devfileData data.DevfileData) error { + containerComponents := adaptersCommon.GetDevfileContainerComponents(devfileData) + for _, comp := range containerComponents { + err := util.ValidateK8sResourceName("container name", comp.Name) + if err != nil { + return err + } + } + return nil +} diff --git a/pkg/devfile/validate/validate_test.go b/pkg/devfile/validate/validate_test.go new file mode 100644 index 00000000000..b632ed6ca6f --- /dev/null +++ b/pkg/devfile/validate/validate_test.go @@ -0,0 +1,129 @@ +package validate + +import ( + "testing" + + "github.com/openshift/odo/pkg/devfile/parser/data" + versionsCommon "github.com/openshift/odo/pkg/devfile/parser/data/common" + "github.com/openshift/odo/pkg/testingutil" +) + +func TestValidateContatinerName(t *testing.T) { + + tests := []struct { + name string + devfileData data.DevfileData + wantErr bool + }{ + { + name: "Case 1: Valid container name", + devfileData: &testingutil.TestDevfileData{ + Components: []versionsCommon.DevfileComponent{ + { + Name: "runtime", + Container: &versionsCommon.Container{ + Image: "quay.io/nodejs-12", + Endpoints: []versionsCommon.Endpoint{ + { + Name: "port-3000", + TargetPort: 3000, + }, + }, + }, + }, + }, + }, + wantErr: false, + }, + { + name: "Case 2: long container name", + devfileData: &testingutil.TestDevfileData{ + Components: []versionsCommon.DevfileComponent{ + { + Name: "runtimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntime", + Container: &versionsCommon.Container{ + Image: "quay.io/nodejs-12", + Endpoints: []versionsCommon.Endpoint{ + { + Name: "port-3000", + TargetPort: 3000, + }, + }, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "Case 3: special character in container name", + devfileData: &testingutil.TestDevfileData{ + Components: []versionsCommon.DevfileComponent{ + { + Name: "run@time", + Container: &versionsCommon.Container{ + Image: "quay.io/nodejs-12", + Endpoints: []versionsCommon.Endpoint{ + { + Name: "port-3000", + TargetPort: 3000, + }, + }, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "Case 4: numeric container name", + devfileData: &testingutil.TestDevfileData{ + Components: []versionsCommon.DevfileComponent{ + { + Name: "12345", + Container: &versionsCommon.Container{ + Image: "quay.io/nodejs-12", + Endpoints: []versionsCommon.Endpoint{ + { + Name: "port-3000", + TargetPort: 3000, + }, + }, + }, + }, + }, + }, + wantErr: true, + }, + { + name: "Case 5: container name with capitalised character", + devfileData: &testingutil.TestDevfileData{ + Components: []versionsCommon.DevfileComponent{ + { + Name: "runTime", + Container: &versionsCommon.Container{ + Image: "quay.io/nodejs-12", + Endpoints: []versionsCommon.Endpoint{ + { + Name: "port-3000", + TargetPort: 3000, + }, + }, + }, + }, + }, + }, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateContatinerName(tt.devfileData) + if !tt.wantErr && err != nil { + t.Errorf("unexpected error: %v", err) + } + + }) + } + +} diff --git a/pkg/odo/cli/component/create.go b/pkg/odo/cli/component/create.go index 997799a9426..b3478fc4e5a 100644 --- a/pkg/odo/cli/component/create.go +++ b/pkg/odo/cli/component/create.go @@ -18,9 +18,9 @@ import ( "github.com/openshift/odo/pkg/component" "github.com/openshift/odo/pkg/config" "github.com/openshift/odo/pkg/devfile" - adaptersCommon "github.com/openshift/odo/pkg/devfile/adapters/common" "github.com/openshift/odo/pkg/devfile/parser" "github.com/openshift/odo/pkg/devfile/parser/data/common" + "github.com/openshift/odo/pkg/devfile/validate" "github.com/openshift/odo/pkg/envinfo" "github.com/openshift/odo/pkg/kclient" "github.com/openshift/odo/pkg/log" @@ -1063,12 +1063,9 @@ func (co *CreateOptions) devfileRun() (err error) { if err != nil { return errors.Wrap(err, "unable to parse devfile") } - containerComponents := adaptersCommon.GetDevfileContainerComponents(devObj.Data) - for _, comp := range containerComponents { - err := util.ValidateK8sResourceName("container name", comp.Name) - if err != nil { - return err - } + err = validate.ValidateDevfileData(devObj.Data) + if err != nil { + return err } err = downloadStarterProject(devObj, co.devfileMetadata.starter, co.interactive) diff --git a/pkg/odo/cli/component/push.go b/pkg/odo/cli/component/push.go index 432dd771b78..1216549d96f 100644 --- a/pkg/odo/cli/component/push.go +++ b/pkg/odo/cli/component/push.go @@ -10,8 +10,8 @@ import ( ktemplates "k8s.io/kubectl/pkg/util/templates" "github.com/openshift/odo/pkg/component" - adaptersCommon "github.com/openshift/odo/pkg/devfile/adapters/common" "github.com/openshift/odo/pkg/devfile/parser" + "github.com/openshift/odo/pkg/devfile/validate" "github.com/openshift/odo/pkg/log" projectCmd "github.com/openshift/odo/pkg/odo/cli/project" "github.com/openshift/odo/pkg/odo/genericclioptions" @@ -182,12 +182,9 @@ func (po *PushOptions) Validate() (err error) { if err != nil { return err } - containerComponents := adaptersCommon.GetDevfileContainerComponents(po.Devfile.Data) - for _, comp := range containerComponents { - err := util.ValidateK8sResourceName("container name", comp.Name) - if err != nil { - return err - } + err = validate.ValidateContatinerName(po.Devfile.Data) + if err != nil { + return err } // Only validate namespace if pushtarget isn't docker if !pushtarget.IsPushTargetDocker() { diff --git a/tests/integration/devfile/cmd_devfile_create_test.go b/tests/integration/devfile/cmd_devfile_create_test.go index db2d860b8b6..542b370fae1 100644 --- a/tests/integration/devfile/cmd_devfile_create_test.go +++ b/tests/integration/devfile/cmd_devfile_create_test.go @@ -198,12 +198,6 @@ var _ = Describe("odo devfile create command tests", func() { helper.CmdShouldFail("odo", "create", "nodejs", "--devfile", "/path/to/file") }) - It("should fail to create the devfile component if the container name is too long", func() { - helper.ReplaceString("devfile.yaml", "runtime", "runtimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntime") - output := helper.CmdShouldFail("odo", "create", "--devfile", "./devfile.yaml") - Expect(output).Should(ContainSubstring("Contain at most 63 characters")) - }) - It("should fail to create the devfile component for an invalid devfile", func() { // Delete the devfile that was copied in as part of setup helper.DeleteFile("devfile.yaml") diff --git a/tests/integration/devfile/cmd_devfile_push_test.go b/tests/integration/devfile/cmd_devfile_push_test.go index 5c8f89149ee..1f8ed9c1bf6 100644 --- a/tests/integration/devfile/cmd_devfile_push_test.go +++ b/tests/integration/devfile/cmd_devfile_push_test.go @@ -648,17 +648,6 @@ var _ = Describe("odo devfile push command tests", func() { helper.MatchAllInOutput(output, []string{"no POM in this directory"}) }) - It("should fail to push the devfile component if the container name is too long", func() { - - helper.CmdShouldPass("odo", "create", "nodejs", "--project", namespace, cmpName) - - helper.CopyExample(filepath.Join("source", "devfiles", "nodejs", "project"), context) - helper.CopyExampleDevFile(filepath.Join("source", "devfiles", "nodejs", "devfile.yaml"), filepath.Join(context, "devfile.yaml")) - helper.ReplaceString("devfile.yaml", "runtime", "runtimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntimeruntime") - output := helper.CmdShouldFail("odo", "push", "--context", context) - Expect(output).Should(ContainSubstring("Contain at most 63 characters")) - }) - }) Context("Verify files are correctly synced", func() { From f92f9cbe4dbf7e47fd62f90e85af70030d1799bf Mon Sep 17 00:00:00 2001 From: Stephanie Date: Mon, 21 Sep 2020 13:59:35 -0400 Subject: [PATCH 5/7] fix integration test failure Signed-off-by: Stephanie --- tests/integration/devfile/cmd_devfile_env_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/devfile/cmd_devfile_env_test.go b/tests/integration/devfile/cmd_devfile_env_test.go index ddb05e9dd2b..95b436b97dd 100644 --- a/tests/integration/devfile/cmd_devfile_env_test.go +++ b/tests/integration/devfile/cmd_devfile_env_test.go @@ -13,7 +13,7 @@ import ( var _ = Describe("odo devfile env command tests", func() { const ( testName = "testname" - testProject = "testProject" + testProject = "testproject" testDebugPort = "8888" fakeParameter = "fakeParameter" ) From f68b19e412abea6f0642277bbb4e1cc279f1ffdf Mon Sep 17 00:00:00 2001 From: Stephanie Date: Tue, 22 Sep 2020 08:32:21 -0400 Subject: [PATCH 6/7] fix typo Signed-off-by: Stephanie --- pkg/devfile/validate/validate.go | 4 ++-- pkg/devfile/validate/validate_test.go | 4 ++-- pkg/odo/cli/component/push.go | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/devfile/validate/validate.go b/pkg/devfile/validate/validate.go index 613c6c99cc2..41dfc2205c0 100644 --- a/pkg/devfile/validate/validate.go +++ b/pkg/devfile/validate/validate.go @@ -48,8 +48,8 @@ func ValidateDevfileData(data interface{}) error { } -// ValidateContatinerName validates whether the container name is valid for K8 -func ValidateContatinerName(devfileData data.DevfileData) error { +// ValidateContainerName validates whether the container name is valid for K8 +func ValidateContainerName(devfileData data.DevfileData) error { containerComponents := adaptersCommon.GetDevfileContainerComponents(devfileData) for _, comp := range containerComponents { err := util.ValidateK8sResourceName("container name", comp.Name) diff --git a/pkg/devfile/validate/validate_test.go b/pkg/devfile/validate/validate_test.go index b632ed6ca6f..71195ae2126 100644 --- a/pkg/devfile/validate/validate_test.go +++ b/pkg/devfile/validate/validate_test.go @@ -8,7 +8,7 @@ import ( "github.com/openshift/odo/pkg/testingutil" ) -func TestValidateContatinerName(t *testing.T) { +func TestValidateContainerName(t *testing.T) { tests := []struct { name string @@ -118,7 +118,7 @@ func TestValidateContatinerName(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := ValidateContatinerName(tt.devfileData) + err := ValidateContainerName(tt.devfileData) if !tt.wantErr && err != nil { t.Errorf("unexpected error: %v", err) } diff --git a/pkg/odo/cli/component/push.go b/pkg/odo/cli/component/push.go index 8349ebc2eae..a70260a699b 100644 --- a/pkg/odo/cli/component/push.go +++ b/pkg/odo/cli/component/push.go @@ -182,7 +182,7 @@ func (po *PushOptions) Validate() (err error) { if err != nil { return err } - err = validate.ValidateContatinerName(po.Devfile.Data) + err = validate.ValidateContainerName(po.Devfile.Data) if err != nil { return err } From 132713148cbb04846871405906f60829629b9189 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Fri, 25 Sep 2020 14:35:56 -0400 Subject: [PATCH 7/7] move validation check to adapter --- .../adapters/kubernetes/component/adapter.go | 16 +++++++++++++++ pkg/odo/cli/component/push.go | 20 ------------------- 2 files changed, 16 insertions(+), 20 deletions(-) diff --git a/pkg/devfile/adapters/kubernetes/component/adapter.go b/pkg/devfile/adapters/kubernetes/component/adapter.go index 61aadaab3fe..82431a49a06 100644 --- a/pkg/devfile/adapters/kubernetes/component/adapter.go +++ b/pkg/devfile/adapters/kubernetes/component/adapter.go @@ -8,6 +8,7 @@ import ( componentlabels "github.com/openshift/odo/pkg/component/labels" "github.com/openshift/odo/pkg/envinfo" + "github.com/openshift/odo/pkg/util" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -22,6 +23,7 @@ import ( "github.com/openshift/odo/pkg/devfile/adapters/kubernetes/storage" "github.com/openshift/odo/pkg/devfile/adapters/kubernetes/utils" versionsCommon "github.com/openshift/odo/pkg/devfile/parser/data/common" + "github.com/openshift/odo/pkg/devfile/validate" "github.com/openshift/odo/pkg/kclient" "github.com/openshift/odo/pkg/log" odoutil "github.com/openshift/odo/pkg/odo/util" @@ -122,6 +124,20 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) { // Validate the devfile build and run commands log.Info("\nValidation") s := log.Spinner("Validating the devfile") + err = util.ValidateK8sResourceName("component name", a.ComponentName) + if err != nil { + return err + } + err = validate.ValidateContainerName(a.Devfile.Data) + if err != nil { + return err + } + + err = util.ValidateK8sResourceName("component namespace", parameters.EnvSpecificInfo.GetNamespace()) + if err != nil { + return err + } + pushDevfileCommands, err := common.ValidateAndGetPushDevfileCommands(a.Devfile.Data, a.devfileBuildCmd, a.devfileRunCmd) if err != nil { s.End(false) diff --git a/pkg/odo/cli/component/push.go b/pkg/odo/cli/component/push.go index a70260a699b..286368dcedc 100644 --- a/pkg/odo/cli/component/push.go +++ b/pkg/odo/cli/component/push.go @@ -11,7 +11,6 @@ import ( "github.com/openshift/odo/pkg/component" "github.com/openshift/odo/pkg/devfile/parser" - "github.com/openshift/odo/pkg/devfile/validate" "github.com/openshift/odo/pkg/log" projectCmd "github.com/openshift/odo/pkg/odo/cli/project" "github.com/openshift/odo/pkg/odo/genericclioptions" @@ -176,25 +175,6 @@ func (po *PushOptions) Validate() (err error) { // If Devfile is present we do not need to validate the below S2I checks // TODO: Perhaps one day move Devfile validation to here instead? if util.CheckPathExists(po.DevfilePath) { - spinner := log.Spinner("Validating devfile component") - defer spinner.End(false) - err = util.ValidateK8sResourceName("component name", po.EnvSpecificInfo.GetName()) - if err != nil { - return err - } - err = validate.ValidateContainerName(po.Devfile.Data) - if err != nil { - return err - } - // Only validate namespace if pushtarget isn't docker - if !pushtarget.IsPushTargetDocker() { - err := util.ValidateK8sResourceName("component namespace", po.EnvSpecificInfo.GetNamespace()) - if err != nil { - return err - } - } - - spinner.End(true) return nil }