Add conformance tests for service ports #78

tpantelis · 2024-09-30T14:24:38Z

When a service is exported from two clusters, ensure the clusterset service exposes the union of service ports declared on its constituent services. Also if port properties don't match, ensure the conflict resolution policy is properly applied.

Creation of K8s resources was refactored to functions rather than defined in global vars to allow the resources to be customized when created on multiple clusters.

Fixes #71

k8s-ci-robot · 2024-09-30T14:24:48Z

Hi @tpantelis. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

tpantelis · 2024-09-30T14:25:04Z

/cc @MrFreezeex

MrFreezeex

Looks good although I think the conformance port tests would need a bit more edge case test on for conflicting port but this can very well be in another PR (I can do it too if you want)
/lgtm
/ok-to-test

skitt · 2024-09-30T14:52:41Z

conformance/service_import.go

+				serviceImport := t.awaitServiceImport(&clients[0], t.helloService.Name, func(serviceImport *v1alpha1.ServiceImport) bool {
+					return len(serviceImport.Spec.Ports) == 3
+				})
+				Expect(serviceImport).NotTo(BeNil(), "ServiceImport was not found")


This returns nil if the verification function fails, doesn’t it?

Suggested change

Expect(serviceImport).NotTo(BeNil(), "ServiceImport was not found")

Expect(serviceImport).NotTo(BeNil(), "ServiceImport was not found or did not have the expected number of ports")

awaitServiceImport only returns nil if the ServiceImport is not found. If the verification function isn't satisfied then awaitServiceImport times out but still succeeds and returns the ServiceImport, enabling more granular and exact assertions by the caller with richer descriptions and avoiding vaguer description as suggested above.

skitt · 2024-09-30T14:53:58Z

conformance/service_import.go

+					serviceImport := t.awaitServiceImport(&clients[0], t.helloService.Name, func(serviceImport *v1alpha1.ServiceImport) bool {
+						return len(serviceImport.Spec.Ports) == len(t.helloService.Spec.Ports)
+					})
+					Expect(serviceImport).NotTo(BeNil(), "ServiceImport was not found")


Same comment as above.

tpantelis · 2024-09-30T15:31:19Z

Looks good although I think the conformance port tests would need a bit more edge case test on for conflicting port but this can very well be in another PR (I can do it too if you want) /lgtm /ok-to-test

yeah I saw your comments on the related issue. They're good points but I'm not sure we need to get that granular with these tests. Perhaps it's sufficient to just handle a simple conflict scenario to verify the conflict policy is applied correctly and leave more detailed and/or edge cases to the testing on the implementation's side.

MrFreezeex · 2024-09-30T15:42:13Z

yeah I saw your comments on the related issue. They're good points but I'm not sure we need to get that granular with these tests. Perhaps it's sufficient to just handle a simple conflict scenario to verify the conflict policy is applied correctly and leave more detailed and/or edge cases to the testing on the implementation's side.

Well the thing is that if you are not careful when you do the union of the ports on the implementation you are likely to construct a ServiceImport with invalid ports (meaning that the API will reject a similar Service with those ports). In a state where this is not handled you would probably only see this in error logs from your controller.

So I do think that it's a good thing to have those in the conformance test in some way as it would ensure that those cases are handled properly by the implementers.

tpantelis · 2024-09-30T16:07:57Z

yeah I saw your comments on the related issue. They're good points but I'm not sure we need to get that granular with these tests. Perhaps it's sufficient to just handle a simple conflict scenario to verify the conflict policy is applied correctly and leave more detailed and/or edge cases to the testing on the implementation's side.

...

So I do think that it's a good thing to have those in the conformance test in some way as it would ensure that those case are handled properly by the implementers.

yeah it's a fair point and discussion to have but that seems to be more of taking on the role of exhaustive E2E tests. I think we should keep the focus on MCS API/spec compliance/conformance. Validity of service ports and edge cases/caveats thereof is more core K8s and certainly can be mentioned in the MCS spec or at least linked to in core K8s docs.

MrFreezeex · 2024-09-30T16:24:48Z

think we should keep the focus on MCS API/spec compliance/conformance. Validity of service ports and edge cases/caveats thereof is more core K8s and certainly can be mentioned in the MCS spec or at least linked to in core K8s docs.

Well to me getting the controller to produce an invalid ServiceImport is a conformance problem. I don't think that the conformance tests should only tests the happy path/easy cases.

And even past that if there are potential caveat that affect most implementations why not add it there too? It helps level up all the implementations by providing them cases that they didn't necessarily think of...

tpantelis · 2024-09-30T16:45:45Z

think we should keep the focus on MCS API/spec compliance/conformance. Validity of service ports and edge cases/caveats thereof is more core K8s and certainly can be mentioned in the MCS spec or at least linked to in core K8s docs.

Well to me getting the controller to produce an invalid ServiceImport is a conformance problem. I don't think that the conformance tests should only tests the happy path/easy cases.

And even past that if there are potential caveat that affect most implementations why not add it there too? It helps level up all the implementations by providing them cases that they didn't necessarily think of...

Good points, although should we assume that what constitutes a valid clusterset ServiceImport necessarily matches what constitutes a valid cluster Service?... I mean is the union of the ports necessarily valid? Say cluster1 exports service port TCP/100 but cluster2 doesn't - what should happen if a client is resolved to cluster2's service IP and tries to access port TCP/100? I guess this depends on the implementation (Submariner internally exposes the intersection of the ports).

Anyway, we can certainly add more tests to cover cases as you suggest to conform to the spec. But, right now, the spec only talks about conflicting port names, hence why I implemented that scenario in the tests. Perhaps we should first update the spec as to exactly what scenarios constitute a ServiceImport port conflict and then consider adding additional conformance tests.

MrFreezeex · 2024-09-30T16:58:05Z

I mean is the union of the ports necessarily valid? Say cluster1 exports service port TCP/100 but cluster2 doesn't - what should happen if a client is resolved to cluster2's service IP and tries to access port TCP/100? I guess this depends on the implementation (Submariner internally exposes the intersection of the ports).

Well the KEP state explicitly that it should be the union so I believe Submariner won't be conformant in that sense to the current KEP :/.

Anyway, we can certainly add more tests to cover cases as you suggest to conform to the spec. But, right now, the spec only talks about conflicting port names, hence why I implemented that scenario in the tests. Perhaps we should first update the spec as to exactly what scenarios constitute a ServiceImport port conflict and then consider adding additional conformance tests.

Yes it's true that the KEP could be updated although to me this is almost common sense that the ports in ServiceImport should respect the ports rules of regular Services. If we want to keep a test very very generic it could not even test for conflict directly but just throws a few ports at it and tests that the resulting ports are valid (aka no duplicated name and no duplicated ports with the same port number and proto).

tpantelis · 2024-09-30T17:43:21Z

I mean is the union of the ports necessarily valid? Say cluster1 exports service port TCP/100 but cluster2 doesn't - what should happen if a client is resolved to cluster2's service IP and tries to access port TCP/100? I guess this depends on the implementation (Submariner internally exposes the intersection of the ports).

Well the KEP state explicitly that it should be the union so I believe Submariner won't be conformant in that sense to the current KEP :/.

Well, Submariner does expose the union as per the spec wrt the ServiceImport CR but, under the hood, it's the intersection that's used b/c it wouldn't be valid otherwise (unless the DNS query specifies the cluster name). I don't know how other implementations handle this. I brought this up at the sig meeting a while back and, FWIR, it was acknowledged by the original authors that the union could be problematic but that's what was decided at that time.

Yes it's true that the KEP could be updated although to me this is almost common sense that the ports in ServiceImport should respect the ports rules of regular Services. If we want to keep a test very very generic it could not even test for conflict directly but just throws a few ports at it and tests that the resulting ports are valid (aka no duplicated name and no duplicated ports with the same port number and proto).

Perhaps it is common sense but I was just going by the current spec. However it may not be common knowledge what the exact ports rules are for regular services. It sounds like you performed actual testing to find that out. I still think it would be useful to update the spec to make it clear exactly what constitutes a valid ServiceImport port. Maybe the original authors were aware of the exact service rules but, for some reason, decided to just enumerate port name equality for ServiceImport.

MrFreezeex · 2024-09-30T19:20:52Z

Perhaps it is common sense but I was just going by the current spec. However it may not be common knowledge what the exact ports rules are for regular services. It sounds like you performed actual testing to find that out. I still think it would be useful to update the spec to make it clear exactly what constitutes a valid ServiceImport port. Maybe the original authors were aware of the exact service rules but, for some reason, decided to just enumerate port name equality for ServiceImport.

Yes I agree the rules are not common knowledge indeed! Here is the PR clarifying that: kubernetes/enhancements#4887

When a service is exported from two clusters, ensure the clusterset service exposes the union of service ports declared on its constituent services. Also if port properties don't match, ensure the conflict resolution policy is properly applied. Creation of K8s resources was refactored to functions rather than defined in global vars to allow the resources to be customized when created on multiple clusters. Fixes kubernetes-sigs#71 Signed-off-by: Tom Pantelis <tompantelis@gmail.com>

tpantelis · 2024-10-01T19:30:02Z

The pull-mcs-api-e2e job is intermittently failing

tpantelis · 2024-10-01T19:30:14Z

/test pull-mcs-api-e2e

MrFreezeex

/lgtm

skitt · 2024-10-02T14:25:43Z

/approve

k8s-ci-robot · 2024-10-02T14:25:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrFreezeex, skitt, tpantelis

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:

~~OWNERS~~ [skitt]

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

k8s-ci-robot requested review from JeremyOT and skitt September 30, 2024 14:24

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 30, 2024

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 30, 2024

k8s-ci-robot requested a review from MrFreezeex September 30, 2024 14:25

MrFreezeex approved these changes Sep 30, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 30, 2024

k8s-ci-robot assigned MrFreezeex Sep 30, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 30, 2024

skitt reviewed Sep 30, 2024

View reviewed changes

tpantelis force-pushed the service-ports branch from 36a1076 to a7f3510 Compare October 1, 2024 18:49

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 1, 2024

tpantelis requested review from MrFreezeex and skitt October 2, 2024 13:29

MrFreezeex approved these changes Oct 2, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 2, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2024

k8s-ci-robot merged commit eff1ba8 into kubernetes-sigs:master Oct 2, 2024
6 checks passed

tpantelis mentioned this pull request Oct 3, 2024

REQUEST: New membership for tpantelis kubernetes/org#5205

Closed

11 tasks

tpantelis deleted the service-ports branch November 27, 2024 13:04

mikemorris mentioned this pull request Dec 11, 2024

KEP 1645: clarify port conflict rules kubernetes/enhancements#4887

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add conformance tests for service ports #78

Add conformance tests for service ports #78

tpantelis commented Sep 30, 2024

k8s-ci-robot commented Sep 30, 2024

tpantelis commented Sep 30, 2024

MrFreezeex left a comment

skitt Sep 30, 2024 •

edited

Loading

tpantelis Sep 30, 2024

skitt Sep 30, 2024

tpantelis commented Sep 30, 2024

MrFreezeex commented Sep 30, 2024 •

edited

Loading

tpantelis commented Sep 30, 2024

MrFreezeex commented Sep 30, 2024 •

edited

Loading

tpantelis commented Sep 30, 2024

MrFreezeex commented Sep 30, 2024

tpantelis commented Sep 30, 2024

MrFreezeex commented Sep 30, 2024

tpantelis commented Oct 1, 2024

tpantelis commented Oct 1, 2024

MrFreezeex left a comment

skitt commented Oct 2, 2024

k8s-ci-robot commented Oct 2, 2024

	Expect(serviceImport).NotTo(BeNil(), "ServiceImport was not found")
	Expect(serviceImport).NotTo(BeNil(), "ServiceImport was not found or did not have the expected number of ports")

Add conformance tests for service ports #78

Add conformance tests for service ports #78

Conversation

tpantelis commented Sep 30, 2024

k8s-ci-robot commented Sep 30, 2024

tpantelis commented Sep 30, 2024

MrFreezeex left a comment

Choose a reason for hiding this comment

skitt Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

tpantelis Sep 30, 2024

Choose a reason for hiding this comment

skitt Sep 30, 2024

Choose a reason for hiding this comment

tpantelis commented Sep 30, 2024

MrFreezeex commented Sep 30, 2024 • edited Loading

tpantelis commented Sep 30, 2024

MrFreezeex commented Sep 30, 2024 • edited Loading

tpantelis commented Sep 30, 2024

MrFreezeex commented Sep 30, 2024

tpantelis commented Sep 30, 2024

MrFreezeex commented Sep 30, 2024

tpantelis commented Oct 1, 2024

tpantelis commented Oct 1, 2024

MrFreezeex left a comment

Choose a reason for hiding this comment

skitt commented Oct 2, 2024

k8s-ci-robot commented Oct 2, 2024

skitt Sep 30, 2024 •

edited

Loading

MrFreezeex commented Sep 30, 2024 •

edited

Loading

MrFreezeex commented Sep 30, 2024 •

edited

Loading