Add Kourier LoadBalancerIP Config #1343

theomessin · 2023-02-21T11:12:03Z

Proposed Changes

Adds new service-load-balancer-ip config for the Kourier Gateway.
This sets Kourier's Service loadBalancerIP to the configured value.
Several Cloud Providers use this setting to specify the IP address that a service gets assigned:

Some cloud providers allow you to specify the loadBalancerIP. In those cases, the load-balancer is created with the user-specified loadBalancerIP. If the loadBalancerIP field is not specified, the loadBalancer is set up with an ephemeral IP address. If you specify a loadBalancerIP but your cloud provider does not support the feature, the loadbalancerIP field that you set is ignored.

Release Note

NONE

linux-foundation-easycla · 2023-02-21T11:12:06Z

The committers listed above are authorized under a signed CLA.

✅ login: theomessin / name: Theodore Messinezis (566614e, ff1d262, 7fe9775, 38b3609, acec746, 6b0d948)

knative-prow · 2023-02-21T11:12:12Z

Welcome @theomessin! It looks like this is your first PR to knative/operator 🎉

knative-prow · 2023-02-21T11:12:13Z

Hi @theomessin. Thanks for your PR.

I'm waiting for a knative 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.

knative-prow

@theomessin: 0 warnings.

In response to this:

Fixes #1290

Proposed Changes

Adds new service-loadBalancerIP config for the Kourier Gateway.

This sets Kourier's Service loadBalancerIP to the configured value.

Several Cloud Providers use this setting to specify the IP address that a service gets assigned:

Some cloud providers allow you to specify the loadBalancerIP. In those cases, the load-balancer is created with the user-specified loadBalancerIP. If the loadBalancerIP field is not specified, the loadBalancer is set up with an ephemeral IP address. If you specify a loadBalancerIP but your cloud provider does not support the feature, the loadbalancerIP field that you set is ignored.

Release Note
NONE

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.

nak3 · 2023-03-01T12:34:43Z

/ok-to-test

nak3

Thank you for this PR @theomessin

But I realized that the LoadBalancerIP field has been deprecated in k8s 1.24. Is it possible to ask your cloud provider to support the annotation as suggested in the change log?

https://github.com/kubernetes/kubernetes/blob/master/CHANGELOG/CHANGELOG-1.24.md#deprecation

Deprecated Service.Spec.LoadBalancerIP. This field was under-specified and its meaning varies across implementations. As of Kubernetes v1.24, users are encouraged to use implementation-specific annotations when available. This field may be removed in a future API version.

theomessin · 2023-03-03T11:09:06Z

Hi @nak3. Yes, it has been marked as deprecated. We're using Google Kubernetes Engine and as far as I can tell they haven't implemented any alternatives as of yet. They still recommend we use loadBalancerIP [1].

Deprecated Service.Spec.LoadBalancerIP. This field was under-specified and its meaning varies across implementations. As of Kubernetes v1.24, users are encouraged to use implementation-specific annotations when available. This field may be removed in a future API version.

The field is still available and since there aren't any implementation-specific annotations when available I'd say the best thing to do is still add this in the operator. I think GCP is a pretty important cloud provider and we have no power over Google 🤷🏼

[1] https://cloud.google.com/kubernetes-engine/docs/concepts/service-load-balancer-parameters#spd-static-ip

pkg/reconciler/knativeserving/ingress/kourier.go

config/crd/bases/operator.knative.dev_knativeservings.yaml

theomessin · 2023-06-11T16:57:34Z

@nak3 any update on this?

nak3

Thank you! LGTM except for one comment.

config/crd/bases/operator.knative.dev_knativeservings.yaml

nak3 · 2023-06-13T00:25:38Z

/lgtm
/approve

/assign @houshengbo
Thank you! I think I don't have a permission to merge this so defer to @houshengbo

nak3 · 2023-06-23T01:04:47Z

bump @houshengbo
/cc @dprotaso @psschwei

codecov · 2023-06-23T12:57:16Z

Codecov Report

Patch coverage: 82.35% and project coverage change: +0.02% 🎉

Comparison is base (1a2ae4b) 79.25% compared to head (38b3609) 79.28%.

❗ Current head 38b3609 differs from pull request most recent head 6b0d948. Consider uploading reports for the commit 6b0d948 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1343      +/-   ##
==========================================
+ Coverage   79.25%   79.28%   +0.02%     
==========================================
  Files          40       40              
  Lines        1822     1810      -12     
==========================================
- Hits         1444     1435       -9     
+ Misses        275      273       -2     
+ Partials      103      102       -1

Files Changed	Coverage Δ
pkg/reconciler/knativeserving/ingress/kourier.go	`79.72% <82.35%> (+0.65%)`	⬆️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

theomessin · 2023-07-31T10:56:03Z

Are we only waiting on the lint failing? The error message doesn't mean anything to me...

Running [/home/runner/golangci-lint-1.52.0-linux-amd64/golangci-lint run --out-format=github-actions --new-from-patch=/tmp/tmp-1720-r08HIklCaYvx/pull.patch --new=false --new-from-rev= --go=1.20] in [] ...
  Error: unnecessary conversion (unconvert)

theomessin · 2023-08-22T22:29:01Z

@nak3 @houshengbo

pkg/reconciler/knativeserving/ingress/kourier_test.go

nak3 · 2023-08-23T01:33:41Z

As I commented #1343 (comment), I don't have a power to approve PR.

Co-authored-by: Kenjiro Nakayama <[email protected]>

nak3 · 2023-08-25T06:01:27Z

/lgtm
/approve

(again I think I don't have a power to merge..)

psschwei

I don't know the operator but trust @nak3 's judgment
/approve
/hold
In case @houshengbo wants to weigh in, but feel free to unhold in a day or two

theomessin · 2023-09-11T13:23:56Z

/retest

theomessin · 2023-09-11T13:43:41Z

@nak3 @psschwei @houshengbo Can we please get this merged? The longer it sits the more work I have to keep doing to resolve conflicts every time main changes...

nak3 · 2023-09-12T02:12:20Z

/lgtm
/approve

knative-prow · 2023-09-12T02:12:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nak3, psschwei, theomessin

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

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

nak3 · 2023-09-12T02:13:13Z

/hold cancel

knative-prow bot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 21, 2023

knative-prow bot requested review from aliok and matzew February 21, 2023 11:12

knative-prow bot reviewed Feb 21, 2023

View reviewed changes

knative-prow bot 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 Mar 1, 2023

nak3 suggested changes Mar 1, 2023

View reviewed changes

knative-prow bot assigned nak3 Mar 1, 2023

nak3 reviewed Mar 3, 2023

View reviewed changes

pkg/reconciler/knativeserving/ingress/kourier.go Outdated Show resolved Hide resolved

config/crd/bases/operator.knative.dev_knativeservings.yaml Outdated Show resolved Hide resolved

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 17, 2023

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2023

theomessin requested a review from nak3 May 25, 2023 23:15

theomessin added 2 commits June 10, 2023 16:57

Add Kourier LoadBalancerIP Config

566614e

Refactor configureGWServiceType to configureGatewayService

ff1d262

nak3 reviewed Jun 12, 2023

View reviewed changes

config/crd/bases/operator.knative.dev_knativeservings.yaml Outdated Show resolved Hide resolved

Rename service-loadBalancerIP to service-load-balancer-ip

7fe9775

knative-prow bot assigned houshengbo Jun 13, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jun 13, 2023

knative-prow bot requested review from dprotaso and psschwei June 23, 2023 01:04

nak3 reviewed Aug 23, 2023

View reviewed changes

pkg/reconciler/knativeserving/ingress/kourier_test.go Outdated Show resolved Hide resolved

Update pkg/reconciler/knativeserving/ingress/kourier_test.go

38b3609

Co-authored-by: Kenjiro Nakayama <[email protected]>

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 25, 2023

psschwei reviewed Aug 25, 2023

View reviewed changes

knative-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 25, 2023

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 29, 2023

Merge branch 'main' into main

acec746

knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 11, 2023

knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 11, 2023

Can we please get this merged?

6b0d948

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2023

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2023

knative-prow bot merged commit c42751f into knative:main Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Kourier LoadBalancerIP Config #1343

Add Kourier LoadBalancerIP Config #1343

theomessin commented Feb 21, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 21, 2023 •

edited

Loading

knative-prow bot commented Feb 21, 2023

knative-prow bot commented Feb 21, 2023

knative-prow bot left a comment

nak3 commented Mar 1, 2023

nak3 left a comment

theomessin commented Mar 3, 2023 •

edited

Loading

theomessin commented Jun 11, 2023

nak3 left a comment

nak3 commented Jun 13, 2023

nak3 commented Jun 23, 2023

codecov bot commented Jun 23, 2023 •

edited

Loading

theomessin commented Jul 31, 2023

theomessin commented Aug 22, 2023

nak3 commented Aug 23, 2023

nak3 commented Aug 25, 2023

psschwei left a comment

theomessin commented Sep 11, 2023

theomessin commented Sep 11, 2023

nak3 commented Sep 12, 2023

knative-prow bot commented Sep 12, 2023

nak3 commented Sep 12, 2023

Add Kourier LoadBalancerIP Config #1343

Add Kourier LoadBalancerIP Config #1343

Conversation

theomessin commented Feb 21, 2023 • edited Loading

Proposed Changes

linux-foundation-easycla bot commented Feb 21, 2023 • edited Loading

knative-prow bot commented Feb 21, 2023

knative-prow bot commented Feb 21, 2023

knative-prow bot left a comment

Choose a reason for hiding this comment

Proposed Changes

nak3 commented Mar 1, 2023

nak3 left a comment

Choose a reason for hiding this comment

theomessin commented Mar 3, 2023 • edited Loading

theomessin commented Jun 11, 2023

nak3 left a comment

Choose a reason for hiding this comment

nak3 commented Jun 13, 2023

nak3 commented Jun 23, 2023

codecov bot commented Jun 23, 2023 • edited Loading

Codecov Report

theomessin commented Jul 31, 2023

theomessin commented Aug 22, 2023

nak3 commented Aug 23, 2023

nak3 commented Aug 25, 2023

psschwei left a comment

Choose a reason for hiding this comment

theomessin commented Sep 11, 2023

theomessin commented Sep 11, 2023

nak3 commented Sep 12, 2023

knative-prow bot commented Sep 12, 2023

nak3 commented Sep 12, 2023

theomessin commented Feb 21, 2023 •

edited

Loading

linux-foundation-easycla bot commented Feb 21, 2023 •

edited

Loading

theomessin commented Mar 3, 2023 •

edited

Loading

codecov bot commented Jun 23, 2023 •

edited

Loading