From 82fedb9325a539fb5e7918dc6c98311b6a5e8787 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Tue, 11 Apr 2023 00:06:24 +0800 Subject: [PATCH 1/4] update endpoint validation Signed-off-by: Stephanie --- pkg/validation/components.go | 7 ++++--- pkg/validation/components_test.go | 15 +++++++++++++++ pkg/validation/endpoints.go | 12 +++++++++++- pkg/validation/validation-rule.md | 2 +- 4 files changed, 31 insertions(+), 5 deletions(-) diff --git a/pkg/validation/components.go b/pkg/validation/components.go index e72d33255..997e4daa1 100644 --- a/pkg/validation/components.go +++ b/pkg/validation/components.go @@ -149,8 +149,8 @@ func ValidateComponents(components []v1alpha2.Component) (returnedErr error) { returnedErr = multierror.Append(returnedErr, resolveErrorMessageWithImportAttributes(err, component.Attributes)) } } - - err := validateEndpoints(component.Openshift.Endpoints, processedEndPointPort, processedEndPointName) + currentComponentEndPointPort := make(map[int]bool) + err := validateDuplicatedName(component.Openshift.Endpoints, processedEndPointName, currentComponentEndPointPort) if len(err) > 0 { for _, endpointErr := range err { returnedErr = multierror.Append(returnedErr, resolveErrorMessageWithImportAttributes(endpointErr, component.Attributes)) @@ -163,7 +163,8 @@ func ValidateComponents(components []v1alpha2.Component) (returnedErr error) { returnedErr = multierror.Append(returnedErr, resolveErrorMessageWithImportAttributes(err, component.Attributes)) } } - err := validateEndpoints(component.Kubernetes.Endpoints, processedEndPointPort, processedEndPointName) + currentComponentEndPointPort := make(map[int]bool) + err := validateDuplicatedName(component.Kubernetes.Endpoints, processedEndPointName, currentComponentEndPointPort) if len(err) > 0 { for _, endpointErr := range err { returnedErr = multierror.Append(returnedErr, resolveErrorMessageWithImportAttributes(endpointErr, component.Attributes)) diff --git a/pkg/validation/components_test.go b/pkg/validation/components_test.go index 2657c6a39..5b6023484 100644 --- a/pkg/validation/components_test.go +++ b/pkg/validation/components_test.go @@ -333,6 +333,21 @@ func TestValidateComponents(t *testing.T) { }, wantErr: []string{sameTargetPortErr}, }, + { + name: "Invalid Kube components with the same endpoint names", + components: []v1alpha2.Component{ + generateDummyKubernetesComponent("name1", []v1alpha2.Endpoint{endpointUrl18080}, ""), + generateDummyKubernetesComponent("name2", []v1alpha2.Endpoint{endpointUrl18081}, ""), + }, + wantErr: []string{sameEndpointNameErr}, + }, + { + name: "Valid Kube components with the same endpoint target ports", + components: []v1alpha2.Component{ + generateDummyContainerComponent("name1", nil, []v1alpha2.Endpoint{endpointUrl18080}, nil, v1alpha2.Annotation{}, false), + generateDummyKubernetesComponent("name2", []v1alpha2.Endpoint{endpointUrl28080}, ""), + }, + }, { name: "Valid containers with valid resource requirement", components: []v1alpha2.Component{ diff --git a/pkg/validation/endpoints.go b/pkg/validation/endpoints.go index 698b6ecd5..83992a28b 100644 --- a/pkg/validation/endpoints.go +++ b/pkg/validation/endpoints.go @@ -10,6 +10,14 @@ import "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" func validateEndpoints(endpoints []v1alpha2.Endpoint, processedEndPointPort map[int]bool, processedEndPointName map[string]bool) (errList []error) { currentComponentEndPointPort := make(map[int]bool) + errList = validateDuplicatedName(endpoints, processedEndPointName, currentComponentEndPointPort) + portErrorList := validateDuplicatedPort(processedEndPointPort, currentComponentEndPointPort) + errList = append(errList, portErrorList...) + + return errList +} + +func validateDuplicatedName(endpoints []v1alpha2.Endpoint, processedEndPointName map[string]bool, currentComponentEndPointPort map[int]bool) (errList []error) { for _, endPoint := range endpoints { if _, ok := processedEndPointName[endPoint.Name]; ok { errList = append(errList, &InvalidEndpointError{name: endPoint.Name}) @@ -17,13 +25,15 @@ func validateEndpoints(endpoints []v1alpha2.Endpoint, processedEndPointPort map[ processedEndPointName[endPoint.Name] = true currentComponentEndPointPort[endPoint.TargetPort] = true } + return errList +} +func validateDuplicatedPort(processedEndPointPort map[int]bool, currentComponentEndPointPort map[int]bool) (errList []error) { for targetPort := range currentComponentEndPointPort { if _, ok := processedEndPointPort[targetPort]; ok { errList = append(errList, &InvalidEndpointError{port: targetPort}) } processedEndPointPort[targetPort] = true } - return errList } diff --git a/pkg/validation/validation-rule.md b/pkg/validation/validation-rule.md index c8ff0cccb..216e1b2ef 100644 --- a/pkg/validation/validation-rule.md +++ b/pkg/validation/validation-rule.md @@ -14,7 +14,7 @@ The validation will be done as part of schema validation, the rule will be intro ### Endpoints: - all the endpoint names are unique across components -- endpoint ports must be unique across components -- two components cannot have the same target port, but one component may have two endpoints with the same target port. This restriction does not apply to container components with `dedicatedPod` set to `true`. +- endpoint ports must be unique across container components -- two container components cannot have the same target port, but one container component may have two endpoints with the same target port. This restriction does not apply to container components with `dedicatedPod` set to `true`. ### Commands: From a71fefceba40d02c6a38e41cefabf2fb42101289 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Tue, 11 Apr 2023 09:35:36 +0800 Subject: [PATCH 2/4] update unit test Signed-off-by: Stephanie --- pkg/validation/components_test.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/validation/components_test.go b/pkg/validation/components_test.go index 5b6023484..0d1d2b71b 100644 --- a/pkg/validation/components_test.go +++ b/pkg/validation/components_test.go @@ -327,11 +327,10 @@ func TestValidateComponents(t *testing.T) { wantErr: []string{sameTargetPortErr}, }, { - name: "Invalid container with same target ports in a single component", + name: "Valid container with same target ports in a single component", components: []v1alpha2.Component{ generateDummyContainerComponent("name1", nil, []v1alpha2.Endpoint{endpointUrl18080, endpointUrl28080}, nil, v1alpha2.Annotation{}, false), }, - wantErr: []string{sameTargetPortErr}, }, { name: "Invalid Kube components with the same endpoint names", @@ -553,8 +552,10 @@ func TestValidateComponents(t *testing.T) { assert.Regexp(t, tt.wantErr[i], merr.Errors[i].Error(), "Error message should match") } } - } else { + } else if tt.wantErr == nil { assert.Equal(t, nil, err, "Error should be nil") + } else if tt.wantErr != nil { + assert.NotEqual(t, nil, err, "Error should not be nil") } }) } From d5e6745f2420a93933c01b12f1f25a8708edf670 Mon Sep 17 00:00:00 2001 From: Stephanie Date: Wed, 12 Apr 2023 10:48:07 +0800 Subject: [PATCH 3/4] update the test error check to be more clear Signed-off-by: Stephanie --- pkg/validation/components_test.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/pkg/validation/components_test.go b/pkg/validation/components_test.go index 0d1d2b71b..dfa62c277 100644 --- a/pkg/validation/components_test.go +++ b/pkg/validation/components_test.go @@ -546,16 +546,19 @@ func TestValidateComponents(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := ValidateComponents(tt.components) - if merr, ok := err.(*multierror.Error); ok && tt.wantErr != nil { - if assert.Equal(t, len(tt.wantErr), len(merr.Errors), "Error list length should match") { - for i := 0; i < len(merr.Errors); i++ { - assert.Regexp(t, tt.wantErr[i], merr.Errors[i].Error(), "Error message should match") + merr, ok := err.(*multierror.Error) + if ok { + if tt.wantErr != nil { + if assert.Equal(t, len(tt.wantErr), len(merr.Errors), "Error list length should match") { + for i := 0; i < len(merr.Errors); i++ { + assert.Regexp(t, tt.wantErr[i], merr.Errors[i].Error(), "Error message should match") + } } + } else { + assert.Equal(t, nil, err, "Error should be nil") } - } else if tt.wantErr == nil { - assert.Equal(t, nil, err, "Error should be nil") } else if tt.wantErr != nil { - assert.NotEqual(t, nil, err, "Error should not be nil") + t.Errorf("Error should not be nil, want %v, got %v", tt.wantErr, err) } }) } From 459031e0038270d62fe425f8ba51e7a45c259f4a Mon Sep 17 00:00:00 2001 From: Stephanie Date: Wed, 12 Apr 2023 23:33:09 +0800 Subject: [PATCH 4/4] add unit test for two kube components Signed-off-by: Stephanie --- pkg/validation/components_test.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/pkg/validation/components_test.go b/pkg/validation/components_test.go index dfa62c277..b6213beea 100644 --- a/pkg/validation/components_test.go +++ b/pkg/validation/components_test.go @@ -341,12 +341,19 @@ func TestValidateComponents(t *testing.T) { wantErr: []string{sameEndpointNameErr}, }, { - name: "Valid Kube components with the same endpoint target ports", + name: "Valid Kube component with the same endpoint target ports as the container component's", components: []v1alpha2.Component{ generateDummyContainerComponent("name1", nil, []v1alpha2.Endpoint{endpointUrl18080}, nil, v1alpha2.Annotation{}, false), generateDummyKubernetesComponent("name2", []v1alpha2.Endpoint{endpointUrl28080}, ""), }, }, + { + name: "Invalid Kube components with the same endpoint names", + components: []v1alpha2.Component{ + generateDummyKubernetesComponent("name1", []v1alpha2.Endpoint{endpointUrl18080}, ""), + generateDummyKubernetesComponent("name2", []v1alpha2.Endpoint{endpointUrl28080}, ""), + }, + }, { name: "Valid containers with valid resource requirement", components: []v1alpha2.Component{ @@ -555,7 +562,7 @@ func TestValidateComponents(t *testing.T) { } } } else { - assert.Equal(t, nil, err, "Error should be nil") + t.Errorf("Error should be nil, got %v", err) } } else if tt.wantErr != nil { t.Errorf("Error should not be nil, want %v, got %v", tt.wantErr, err)