fix svc long name #9245

tombokombo · 2022-11-02T01:26:12Z

What this PR does / why we need it:

Fix endpointslices service matching when service name is too long.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
CVE Report (Scanner found CVE and adding report)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation only

Which issue/s this PR fixes

fixes #9240

How Has This Been Tested?

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added unit and/or e2e tests to cover my changes.
All new and existing tests passed.
Added Release Notes.

Does my pull request need a release note?

Fix endpointslices service matching when service name is too long.

k8s-ci-robot · 2022-11-02T01:26:20Z

@tombokombo: This issue is currently awaiting triage.

If Ingress contributors 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.

k8s-ci-robot · 2022-11-02T01:26:22Z

Hi @tombokombo. Thanks for your PR.

I'm waiting for a kubernetes 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/test-infra repository.

tombokombo · 2022-11-02T01:31:53Z

/assign @tao12345666333

ElvinEfendi · 2022-11-02T20:38:23Z

/ok-to-test

ElvinEfendi · 2022-11-02T20:40:08Z

internal/ingress/controller/store/endpointslice.go

@@ -34,7 +34,12 @@ func (s *EndpointSliceLister) MatchByKey(key string) ([]*discoveryv1.EndpointSli
 	var eps []*discoveryv1.EndpointSlice
 	// filter endpointSlices owned by svc
 	for _, listKey := range s.ListKeys() {
-		if !strings.HasPrefix(listKey, key) {
+		if len(key) < 58 && !strings.HasPrefix(listKey, key) {


Where does 58 come from? Can it change elsewhere and resurface this issue in another form?

Also as per #9240 (comment), this is merely optimization and the actual check is done using ownerReferences (which is great!). What if we skip this optimization completely, how bad would it be?

Where does 58 come from? Can it change elsewhere and resurface this issue in another form?

If the svc is too long, Kubernetes will use the first 58 characters + a random string to form the name of the endpointslices

Yeah, I'm concerned about that - what if in next k8s version they change it to say 48 characters?

What is the performance concern here? Is s.GetByKey(listKey) costly? How costly? I'd guess it does not even make an API call and just fetches the object from the cache store.

@ElvinEfendi slices are using meta generated name "feature" https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/endpointslice/utils.go#L142
this one is full lenght, still not truncated. Generated name is later used for name creation by k8s. As per docs
https://pkg.go.dev/k8s.io/apiserver/pkg/storage/names#NameGenerator.GenerateName
not exceed the length of a standard Kubernetes name (63 characters) . Mentioned 63 chars needs to contain random generated part. Look at https://github.com/kubernetes/apiserver/blob/master/pkg/storage/names/generate.go#L45.
and truncating ifself https://github.com/kubernetes/apiserver/blob/master/pkg/storage/names/generate.go#L51
As you can see, its hardcoded, not optional, at least for now, but I can reference it ( see next commit ). Problem could rise if it becomes optional, but it will be compile error after go k8s deps upgrade. In edge case, that const will still be there ( reference will work ) and it somehow becomes optional, higher num would still work, but less effectively. The case with less characters, will brake whole k8s world, cannot even imagine consequences....

Regarding optimization, I did test with 10k slices in one namespace and 10k in another. With this optimization, we get rid of second namespace, because keys starts with namespace. Calling MatchByKey 10k times, results are 86.85sec and 36.42sec with this optimization. Using real world numbers, 500 slices + 500 slices different ns, 500 calls results in 120.881986ms and optimized 57.799922ms. It make sense to me at least to get rid of different namespaces.

@tombokombo thanks for those numbers, the optimization does make sense in that case.

Also 👍🏼 for referencing MaxGeneratedNameLength instead of hardcoding it. It looks good now.

ElvinEfendi · 2022-11-02T20:59:25Z

Thanks @tombokombo! It'd be great to also have an e2e regression test for this bug.

tao12345666333 · 2022-11-03T00:34:45Z

The fix LGTM, I also hope you can add e2e test case

ElvinEfendi · 2022-11-03T21:00:04Z

internal/ingress/controller/store/endpointslice_test.go

+		}
+		if len(eps) != 1 {
+			t.Errorf("expected one slice %v, error, got %d slices", endpointSlice, len(eps))
+		}


It'd also be good to assert that the 1 eps we expect is the eps we want (with name test-backend-http-test-http-test-http-test-http-test-http-bar88 and namespace ns).

tombokombo · 2022-11-03T22:30:07Z

@ElvinEfendi @tao12345666333 hopefully there is nothing more to discuss. I'm going to provide e2e test soon.

tao12345666333 · 2022-11-04T00:10:52Z

Thanks I think it's ok, wait for your e2e case

ElvinEfendi · 2022-11-05T13:23:13Z

LGTM - please rebase / squash the commits.

I'll let @tao12345666333 to take a final look and approve.

Signed-off-by: tombokombo <tombo@sysart.tech>

tao12345666333

/lgtm
/approve

Thanks!

k8s-ci-robot · 2022-11-05T21:20:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tao12345666333, tombokombo

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~~ [tao12345666333]

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

This can be reverted when kubernetes/ingress-nginx#9245 is released

Signed-off-by: tombokombo <tombo@sysart.tech> Signed-off-by: tombokombo <tombo@sysart.tech>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 2, 2022

k8s-ci-robot requested review from ElvinEfendi and strongjz November 2, 2022 01:26

k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Nov 2, 2022

k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 2, 2022

k8s-ci-robot added needs-priority size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 2, 2022

k8s-ci-robot assigned tao12345666333 Nov 2, 2022

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 Nov 2, 2022

ElvinEfendi reviewed Nov 2, 2022

View reviewed changes

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 3, 2022

ElvinEfendi reviewed Nov 3, 2022

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 5, 2022

fix svc long name

ad8e828

Signed-off-by: tombokombo <tombo@sysart.tech>

tombokombo force-pushed the fix/endpslices-long-svc-name branch from 5e5272b to ad8e828 Compare November 5, 2022 15:44

tao12345666333 reviewed Nov 5, 2022

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 5, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 5, 2022

k8s-ci-robot merged commit 490ecff into kubernetes:main Nov 5, 2022

magentalabs-serviceagent-1 pushed a commit to OS2mo/os2mo-helm-chart that referenced this pull request Nov 8, 2022

[#53251] start.sh: pin nginx ingress to 1.3.1

cb42192

This can be reverted when kubernetes/ingress-nginx#9245 is released

magentalabs-serviceagent-1 pushed a commit to OS2mo/os2mo-helm-chart that referenced this pull request Nov 8, 2022

[#53251] start.sh: pin nginx ingress to 1.3.1

7de4500

This can be reverted when kubernetes/ingress-nginx#9245 is released

longwuyuan mentioned this pull request Nov 9, 2022

Ingress-nginx return 503 error if service name is longer than 58 chars #9276

Closed

hairmare mentioned this pull request Nov 22, 2022

feat(infra-apps): upgrade ingress from 4.2.* to 4.4.0 adfinis/helm-charts#859

Merged

6 tasks

jaehnri pushed a commit to jaehnri/ingress-nginx that referenced this pull request Jan 2, 2023

fix svc long name (kubernetes#9245)

2db32cf

Signed-off-by: tombokombo <tombo@sysart.tech> Signed-off-by: tombokombo <tombo@sysart.tech>

longwuyuan mentioned this pull request May 19, 2023

Reoccurrence of Service does not have any active Endpoint [when it actually does] #9932

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix svc long name #9245

fix svc long name #9245

tombokombo commented Nov 2, 2022

k8s-ci-robot commented Nov 2, 2022

k8s-ci-robot commented Nov 2, 2022

tombokombo commented Nov 2, 2022

ElvinEfendi commented Nov 2, 2022

ElvinEfendi Nov 2, 2022

ElvinEfendi Nov 2, 2022

tao12345666333 Nov 2, 2022

ElvinEfendi Nov 3, 2022

tombokombo Nov 3, 2022 •

edited

Loading

ElvinEfendi Nov 3, 2022

ElvinEfendi commented Nov 2, 2022

tao12345666333 commented Nov 3, 2022

ElvinEfendi Nov 3, 2022

tombokombo commented Nov 3, 2022

tao12345666333 commented Nov 4, 2022

ElvinEfendi commented Nov 5, 2022

tao12345666333 left a comment

k8s-ci-robot commented Nov 5, 2022

fix svc long name #9245

fix svc long name #9245

Conversation

tombokombo commented Nov 2, 2022

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

Does my pull request need a release note?

k8s-ci-robot commented Nov 2, 2022

k8s-ci-robot commented Nov 2, 2022

tombokombo commented Nov 2, 2022

ElvinEfendi commented Nov 2, 2022

ElvinEfendi Nov 2, 2022

Choose a reason for hiding this comment

ElvinEfendi Nov 2, 2022

Choose a reason for hiding this comment

tao12345666333 Nov 2, 2022

Choose a reason for hiding this comment

ElvinEfendi Nov 3, 2022

Choose a reason for hiding this comment

tombokombo Nov 3, 2022 • edited Loading

Choose a reason for hiding this comment

ElvinEfendi Nov 3, 2022

Choose a reason for hiding this comment

ElvinEfendi commented Nov 2, 2022

tao12345666333 commented Nov 3, 2022

ElvinEfendi Nov 3, 2022

Choose a reason for hiding this comment

tombokombo commented Nov 3, 2022

tao12345666333 commented Nov 4, 2022

ElvinEfendi commented Nov 5, 2022

tao12345666333 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 5, 2022

tombokombo Nov 3, 2022 •

edited

Loading