Skip to content

update kubernetes/openshift endpoint validation #1098

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 13, 2023
+50 −13
Merged

update kubernetes/openshift endpoint validation #1098

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
82fedb9
update endpoint validation
yangcao77 Apr 10, 2023
a71fefc
update unit test
yangcao77 Apr 11, 2023
d5e6745
update the test error check to be more clear
yangcao77 Apr 12, 2023
459031e
add unit test for two kube components
yangcao77 Apr 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions pkg/validation/components.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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))
Expand Down
22 changes: 19 additions & 3 deletions pkg/validation/components_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,11 +327,25 @@ 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",
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}, ""),
Comment on lines +346 to +347
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to double check -- is this test intended to use a container component and a kube component? I don't think duplicate endpoints between a container and kube component was ever an issue, was it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was an issue. previously we do not allow any port duplication across components

Copy link
Contributor

@amisevsk amisevsk Apr 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, makes sense. Perhaps it makes sense to add another test case for two Kubernetes components that use the same port number as well? The name of this test case suggests it's covering something like

generateDummyKubernetesComponent("name1", []v1alpha2.Endpoint{endpointUrl18080}, ""),
generateDummyKubernetesComponent("name2", []v1alpha2.Endpoint{endpointUrl28080}, ""),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

},
},
{
name: "Valid containers with valid resource requirement",
Expand Down Expand Up @@ -538,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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For clarity, it might make sense to do these checks first -- as it stands it looks like we check a subcase of tt.wantErr != nil first. Something like

if tt.wantErr == nil {
	assert.NoError(t, err, "Error should be nil")
	return
} else {
	if !assert.Error(t, err, "Should return error") {
		return
	}
}

merr, ok := err.(*multierror.Error)
if !ok {
	t.Fatalf("Unexpected error: %w", err)
}
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")
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

}
})
}
Expand Down
12 changes: 11 additions & 1 deletion pkg/validation/endpoints.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,20 +10,30 @@ 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})
}
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
}
2 changes: 1 addition & 1 deletion pkg/validation/validation-rule.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down