[go: up one dir, main page]
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ingress: Add loadBalancerIP and loadBalancerClass #22670

Merged
merged 1 commit into from
Feb 2, 2023
Merged

ingress: Add loadBalancerIP and loadBalancerClass #22670

merged 1 commit into from
Feb 2, 2023

Conversation

oliver-ni
Copy link
Contributor
@oliver-ni oliver-ni commented Dec 11, 2022 •
edited by sayboras
Loading

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

@oliver-ni oliver-ni requested review from a team as code owners December 11, 2022 10:48
@maintainer-s-little-helper 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
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Dec 11, 2022
@github-actions
Copy link

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.

@github-actions 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
@simonfelding
Copy link
Contributor

Bump! Really want this.

@maintainer-s-little-helper
Copy link

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

@maintainer-s-little-helper 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
@maintainer-s-little-helper 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 oliver-ni requested review from squeed and sayboras and removed request for gandro and squeed January 19, 2023 18:27
@github-actions 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 aanm added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Jan 27, 2023
@aanm aanm requested a review from squeed January 27, 2023 19:26
@maintainer-s-little-helper 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
sayboras
sayboras approved these changes Jan 30, 2023
View reviewed changes
Copy link
Member
@sayboras sayboras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 -}}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 sayboras changed the title Add loadBalancerIP and loadBalancerClass options for shared ingressController service ingress: Add loadBalancerIP and loadBalancerClass Jan 30, 2023
@sayboras sayboras added the area/helm Impacts helm charts and user deployment experience label Jan 30, 2023
@sayboras sayboras added the area/servicemesh GH issues or PRs regarding servicemesh label Jan 30, 2023
@sayboras sayboras force-pushed the patch-1 branch 2 times, most recently from e87eeac to 2d29102 Compare January 30, 2023 00:28
@sayboras sayboras requested a review from a team as a code owner January 30, 2023 00:28
@sayboras sayboras requested a review from qmonnet January 30, 2023 00:28
@sayboras
Copy link
Member

/test

@oliver-ni @sayboras
ingress: Add loadBalancerIP and loadBalancerClass
841f4d2 
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
Copy link
Member
sayboras commented Jan 30, 2023 •
edited by maintainer-s-little-helper bot
Loading

/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
Copy link
Contributor
squeed commented Feb 1, 2023

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.

@squeed 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
Copy link
Member
pchaigno commented Feb 2, 2023

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.

@pchaigno pchaigno merged commit b8786b3 into cilium:master Feb 2, 2023
@tklauser tklauser mentioned this pull request Jun 28, 2023
Merged
3 tasks
@tklauser tklauser added backport-pending/1.13 backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed needs-backport/1.13 labels Jun 28, 2023
@gentoo-root gentoo-root mentioned this pull request Jul 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@squeed squeed squeed approved these changes

@sayboras sayboras sayboras approved these changes

@qmonnet qmonnet qmonnet approved these changes

Assignees
No one assigned
Labels
area/helm Impacts helm charts and user deployment experience area/servicemesh GH issues or PRs regarding servicemesh backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
cilium v1.13.5
Status: Released
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@oliver-ni @simonfelding @sayboras @squeed @pchaigno @qmonnet @tklauser @aanm