ingress: Add loadBalancerIP and loadBalancerClass #22670

oliver-ni · 2022-12-11T10:48:37Z

Please ensure your pull request adheres to the following guidelines:

For first time contributors, read Submitting a pull request
All code is covered by unit and/or runtime tests where feasible.
All commits contain a well written commit description including a title,
description and a Fixes: #XXX line if the commit addresses a particular
GitHub issue.
If your commit description contains a Fixes: <commit-id> tag, then
please add the commit author[s] as reviewer[s] to this issue.
All commits are signed off. See the section Developer’s Certificate of Origin
Provide a title or release-note blurb suitable for the release notes.
Thanks for contributing!

This pull request adds configuration options for the loadBalancerClass and loadBalancerIP fields in the Service object created for cilium's ingress controller when ingressController.loadbalancerMode is set to shared.

Previously, there was no option to configure these options, which makes it difficult for me to use the chart in my situation. I have to manually delete and recreate the service, since the field cannot even be edited with kubectl edit.

Sorry about the commit titles. If this change is approved, would appreciate a squash and merge with a better title.

ingress: Add loadBalancerIP and loadBalancerClass

install/kubernetes/cilium/values.yaml

install/kubernetes/cilium/templates/cilium-ingress-service.yaml

github-actions · 2023-01-12T02:00:36Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

simonfelding · 2023-01-19T13:51:54Z

Bump! Really want this.

maintainer-s-little-helper · 2023-01-19T18:25:26Z

Commits 9a70feef22e598cf19f7d694a64a0420d5855215, e18b4ce8d5fb2941a24d117f3e58f0fce998a44d do not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

sayboras

Sorry for the delay, the changes look good to me :), only one minor comment as per below (which can be taken care separately).

sayboras · 2023-01-30T00:18:07Z

install/kubernetes/cilium/templates/cilium-ingress-service.yaml

@@ -24,6 +24,14 @@ spec:
    protocol: TCP
    nodePort: {{ .Values.ingressController.service.secureNodePort }}
  type: {{ .Values.ingressController.service.type }}
+  {{- if semverCompare ">=1.24-0" .Capabilities.KubeVersion.Version -}}


these two fields are only applicable if ServiceType is LoadBalancer. Understand that the service type is added in dependently with this PR, so we can take care of this subsequently.

#22583

sayboras · 2023-01-30T00:34:44Z

/test

This commit is to add loadBalancerIP and loadBalancerClass for shared Ingress service. Kindly note the below points: - loadbalancerIP feature is depending on cloud provider controller - loadBalancerClass is only supported for k8s 1.24 and later. Signed-off-by: Oliver Ni <oliver.ni@gmail.com>

sayboras · 2023-01-30T01:11:25Z

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN

Failure Output

FAIL: Failed to reach 192.168.56.11:80 from testclient-host-f6hd5

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

squeed · 2023-02-01T21:47:01Z

Test failure seems like a flake connecting to the apisever for one single test in one run. I'm going to go and say that this is ok to merge.

The run was https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-4.19/638/, and the failure message was

error: unable to upgrade connection: Authorization error (user=kube-apiserver-kubelet-client, verb=create, resource=nodes, subresource=proxy)

Given that this change has no effect by default, it's safe to say this is a flake. Labeling as ok-to-merge.

pchaigno · 2023-02-02T09:36:46Z

Test failure seems like a flake connecting to the apisever for one single test in one run. I'm going to go and say that this is ok to merge.

The run was https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-4.19/638/, and the failure message was

error: unable to upgrade connection: Authorization error (user=kube-apiserver-kubelet-client, verb=create, resource=nodes, subresource=proxy)

FYI, that is known flake #15455, for which Cilium is known to be the culprit.

oliver-ni requested review from a team as code owners December 11, 2022 10:48

oliver-ni requested a review from squeed December 11, 2022 10:48

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 11, 2022

oliver-ni requested a review from gandro December 11, 2022 10:48

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Dec 11, 2022

squeed requested changes Dec 12, 2022

View reviewed changes

install/kubernetes/cilium/values.yaml Show resolved Hide resolved

sayboras reviewed Dec 12, 2022

View reviewed changes

install/kubernetes/cilium/templates/cilium-ingress-service.yaml Show resolved Hide resolved

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 12, 2023

maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jan 19, 2023

oliver-ni force-pushed the patch-1 branch from e18b4ce to a44c879 Compare January 19, 2023 18:26

maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Jan 19, 2023

oliver-ni requested review from squeed and sayboras and removed request for gandro and squeed January 19, 2023 18:27

github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 20, 2023

aanm added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jan 27, 2023

aanm requested a review from squeed January 27, 2023 19:26

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 27, 2023

aanm force-pushed the patch-1 branch from a44c879 to df4f951 Compare January 27, 2023 19:43

sayboras approved these changes Jan 30, 2023

View reviewed changes

sayboras force-pushed the patch-1 branch from df4f951 to 7a65b49 Compare January 30, 2023 00:24

sayboras changed the title ~~Add loadBalancerIP and loadBalancerClass options for shared ingressController service~~ ingress: Add loadBalancerIP and loadBalancerClass Jan 30, 2023

sayboras added the area/helm Impacts helm charts and user deployment experience label Jan 30, 2023

sayboras added the area/servicemesh GH issues or PRs regarding servicemesh label Jan 30, 2023

sayboras force-pushed the patch-1 branch 2 times, most recently from e87eeac to 2d29102 Compare January 30, 2023 00:28

sayboras requested a review from a team as a code owner January 30, 2023 00:28

sayboras requested a review from qmonnet January 30, 2023 00:28

sayboras force-pushed the patch-1 branch from 2d29102 to 841f4d2 Compare January 30, 2023 01:09

qmonnet approved these changes Jan 30, 2023

View reviewed changes

squeed approved these changes Feb 1, 2023

View reviewed changes

squeed added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 1, 2023

pchaigno merged commit b8786b3 into cilium:master Feb 2, 2023

sayboras added the needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch label Jun 26, 2023

tklauser mentioned this pull request Jun 28, 2023

v1.13 Backports 2023-06-28 #26528

Merged

3 tasks

gentoo-root mentioned this pull request Jul 26, 2023

Prepare for release v1.13.5 #27088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ingress: Add loadBalancerIP and loadBalancerClass #22670

ingress: Add loadBalancerIP and loadBalancerClass #22670

oliver-ni commented Dec 11, 2022 •

edited by sayboras

github-actions bot commented Jan 12, 2023

simonfelding commented Jan 19, 2023

maintainer-s-little-helper bot commented Jan 19, 2023

sayboras left a comment

sayboras Jan 30, 2023

sayboras commented Jan 30, 2023

sayboras commented Jan 30, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

squeed commented Feb 1, 2023

pchaigno commented Feb 2, 2023

ingress: Add loadBalancerIP and loadBalancerClass #22670

ingress: Add loadBalancerIP and loadBalancerClass #22670

Conversation

oliver-ni commented Dec 11, 2022 • edited by sayboras

github-actions bot commented Jan 12, 2023

simonfelding commented Jan 19, 2023

maintainer-s-little-helper bot commented Jan 19, 2023

sayboras left a comment

Choose a reason for hiding this comment

sayboras Jan 30, 2023

Choose a reason for hiding this comment

sayboras commented Jan 30, 2023

sayboras commented Jan 30, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

squeed commented Feb 1, 2023

pchaigno commented Feb 2, 2023

oliver-ni commented Dec 11, 2022 •

edited by sayboras

sayboras commented Jan 30, 2023 •

edited by maintainer-s-little-helper bot