update kubernetes/openshift endpoint validation

[yangcao77]

What does this PR do?

This PR updates the kubernetes and openshift component endpoint validation, only checks for name conflict and allows port conflict.

This PR also updates the validation rule and unit tests. Briefly reviewed the validation rule, the unit test was wrong testing port conflict within same component, but was not caught. Updated the error check to fix the issue.

Which issue(s) does this PR fix

#1049

PR acceptance criteria

Testing and documentation do not need to be complete in order for this PR to be approved. We just need to ensure tracking issues are opened.

• Open new test/doc issues under the devfile/api repo
• Check each criteria if:
• There is a separate tracking issue. Add the issue link under the criteria
  or
• test/doc updates are made as part of this PR
• If unchecked, explain why it's not needed

• Unit/Functional tests

• QE Integration test

• Documentation

• Client Impact

How to test changes / Special notes to the reviewer

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.15 🎉

Comparison is base (832b661) 35.64% compared to head (459031e) 35.79%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1098      +/-   ##
==========================================
+ Coverage   35.64%   35.79%   +0.15%     
==========================================
  Files          52       52              
  Lines        6669     6679      +10     
==========================================
+ Hits         2377     2391      +14     
+ Misses       4147     4144       -3     
+ Partials      145      144       -1     

Impacted Files	Coverage Δ
pkg/validation/components.go	90.13% <100.00%> (+2.79%)	⬆️
pkg/validation/endpoints.go	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[amisevsk]

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?

[yangcao77]

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

[amisevsk]

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}, ""),

[yangcao77]

added

[amisevsk]

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")
	}
}

[yangcao77]

updated

[amisevsk]

I believe this branch is still not reachable; if err == nil, then the type assertion above will not succeed (i.e. we'll always have ok == false)

[yangcao77]

this else branch is for line 551 -> if tt.wantErr!= nil, but ok==true. which means we want the error to be nil, but we got something returned from the ValidateComponents

[yangcao77]

I have updated to use t.Errorf instead of assert.Equal to avoid confusion.

[amisevsk]

I think the issue is that line 551 is never reached, though:

merr, ok := err.(*multierror.Error) // If err is nil, then we'll have merr == nil, ok == false
if ok {                             // ok == true implies err != nil
  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")
  }

[yangcao77]

the else statement is for if tt.wantErr != nil, and within the if ok block. Which means, ok && tt.wantErr == nil, implies the actual err returned from the validate function is not nil, but the test case expects the error to be nil. that's why the test need to error out.

merr, ok := err.(*multierror.Error) 
if ok {                           // err != nil
  if tt.wantErr != nil { // err != nil && wantErr != nil, need to process and check if each error matches
    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 { // this is a else statement of if wantErr != nil. so err != nil && wantErr == nil. and test to error out
    t.Errorf("Error should be nil, got %v", err)
  }

[amisevsk]

You're right -- maybe my brain just isn't working today 🙂. Sorry for the hassle.

To try and explain where I'm coming from -- I feel like it'd be simpler to check whether an error exists first before asserting the type. As it stands, the case "returns nil and we expect nil" is not explicitly checked, and is verified only by dodging two conditionals.

	Returns nil	Returns multierror.Error	Returns other error
Expect no error	Not checked directly	Checked line 558	Test succeeds even though an error was returned
Expect an error	Checked at line 561	Checked lines 551-556	Test fails but prints "Error should not be nil" even though error is not nil

I understand (after reading the validation code) that only multierror.Error is returned, but from reading the test case, this behavior is not clear, as there's no explicit assertion that any error returned from ValidateComponents has to be of type multierror.Error.

Either way, I'll approve PR so it can be merged.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: amisevsk, maysunfaisal, yangcao77

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment