skip checking nodeport on external addrs in conformance tests

[pacoxu]

What type of PR is this?
/kind bug
/sig testing
/sig network

What this PR does / why we need it:
In some user scenarios, the external IP nodeport is for access from APPs outside this cluster/zone.

• However, in most scenarios, external IP is just for SSH or special use(Ingress Gateway, etc.)

Different users have different proposals.

most of the network tests now run from the cluster, no need to reach externalIPs.

Per #90764 (comment), I prefer to remove it before a clear definition for the external IPs.

move ExternalIP from Conformance entirely until it has a clear definition

Which issue(s) this PR fixes:
Fixes #90764

Special notes for your reviewer:
And @BenTheElder suggestion is like adding a feature filter. I will work on it if this is needed.

Per #98791 (comment), I will add new test with [Feature:ExternalNodeAccess] and remove current case. [WIP]

Does this PR introduce a user-facing change?:

None

[k8s-ci-robot]

@pacoxu: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[pacoxu]

/cc aojea

[aojea]

let me ask about this, I´m not sure how @BenTheElder @spiffxp prefer to parameterise these things?

[pacoxu]

I'm not sure whether it is rude to remove this directly.

BTW, we should decide the default behavior: skip or check?

[aojea]

ah yeah, but there is a miriad of flags, skips, checks, ... just what I prefer to wait for them so we can align all the test to behave the same

[BenTheElder]

I think this (parameters) are usually done with a flag? though this seems awfully specific and more like a different test entirely.

Also conformance tests should not have variable behavior, so IMO the correct answer is actually to split this out into another test and give it tag to filter on if we're keeping this test.

Changes to conformance tests behavior should also be approved by the conformance subproject.

[BenTheElder]

so like [Feature:ExternalNodeAccess] on the new tests or something.

But also why are we changing this? commenting back on the issue.
Just because some clusters can fail it doesn't necessarily mean we change conformance 😕

[pacoxu]

for the PR, I will update as you suggest later after the holiday(maybe a week later😄)

for the issue, we can disscuss it in #90764.

[fejta]

/uncc

[pacoxu]

I updated the PR. We may remove it as this is not clear.( It should be removed for "secured clusters" )

no need to reach externalIPs.

Per #90764 (comment), I prefer to remove it before a clear definition for the external IPs.

[aojea]

I prefer to remove it before a clear definition for the external IPs.

I personally, would move ExternalIP from Conformance entirely until it has a clear definition

from conformance, not from sig-network tests 😄

but that is a call that conformance people has to do, my point is that verifying in Conformance something that is not well-defined is not very conformant 🙃

[pacoxu]

I prefer to remove it before a clear definition for the external IPs.

I personally, would move ExternalIP from Conformance entirely until it has a clear definition

from conformance, not from sig-network tests 😄

but that is a call that conformance people has to do, my point is that verifying in Conformance something that is not well-defined is not very conformant 🙃

I think that I got it. Sorry for misunderstanding. 😂

• test case with framework.ConformanceIt() will be included in conformance test.
• test case with framework.It() will not be included

What we should do is removing it from Conformance test cases and check it in other cases.

[aojea]

let's hold until the discussion ends, Conformance has to make the call, and then we'll talk with sig-testing to see what is the best way to implement it

[aojea]

I think that this makes it, but I think that it will be better to add it as a new field part of the TestJig

// TestJig is a test jig to help service testing.
type TestJig struct {
	Client    clientset.Interface
	Namespace string
	Name      string
	ID        string
	Labels    map[string]string
        ExternalIPs bool
}

something like TestJig.ExternalIPs=true or TestJig.Conformance=true, so consumers of the TestJig can define it's behaviour in the object.

It sounds more scalable and intuitive to me, what do you think?

[spiffxp]

Yeah I like that approach. I prefer ExternalIPs as the field name, it's more explicit about what's being enabled or disabled.

[aojea]

/lgtm

[spiffxp]

/approve
/lgtm
/milestone v1.21

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pacoxu, spiffxp

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

The pull request process is described here

Needs approval from an approver in each of these files:

• test/e2e/framework/OWNERS [spiffxp]
• test/e2e/network/OWNERS [spiffxp]

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