add lbName annotation #565

agadelshin · 2018-12-01T13:00:08Z

this PR create feature for #369

I've created annotation "kubernetes.io/ingress.loadbalancer-name" to share LB between ingresses. Work still in progress, but it contains a lot of changes, so please look at this PR to control progress.

k8s-ci-robot · 2018-12-01T13:00:15Z

Hi @pondohva. 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.

agadelshin · 2018-12-01T13:01:08Z

I still need to do smth with TLS configuration, but simple case with N ingresses with the same LB name in annotation works.

agadelshin · 2018-12-01T13:04:32Z

@rramkumar1 are you OK with this? I decided to create annotation, because it's the simplest way to have shared LB name for ingresses.

I think I need to create some logging to point out users with the same hostname or with the same static IP in different ingresses to this feature.

agadelshin · 2018-12-02T14:21:02Z

pkg/controller/controller_test.go

+
+func pathRuleExists(pathRules []*compute.PathRule, path string, service string) bool {
+	for _, pr := range pathRules {
+		//if pr.Service == service && slice.ContainsString(pr.Paths, path, nil) {


can't find how I can translate backend name like "global/backendServices/k8s-be-30002--aaaaa" to k8s service name

agadelshin · 2018-12-03T08:23:57Z

/assign @rramkumar1

could you please help me with this? I'm kinda stuck to make a decision

rramkumar1 · 2018-12-07T18:08:44Z

@pondohva Annotation seems like the only sensible way to do this. I'm taking a look now at your implementation

Will leave comments in my review on the areas you are stuck

k8s-ci-robot · 2018-12-09T10:03:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pondohva
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: rramkumar1

If they are not already assigned, you can assign the PR to them by writing /assign @rramkumar1 in a comment when ready.

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

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

rramkumar1

@pondohva Made a first pass. A couple high level points:

I'm not clear where the actual merging happens. Is that still a WIP?
I would suggest breaking down this problem into two sections. The first is the "origin" LB. You can think of this as the Ingress definition which "branches" are derived from. When you dequeue an "origin", you list all other Ingresses and find its "branches". Once you have each "branch", merge them all into the "origin" and then translate and sync the "origin" as you would normally.

rramkumar1 · 2018-12-07T18:16:44Z

pkg/context/context.go

@@ -54,6 +54,7 @@ type ControllerContext struct {
 	ControllerContextConfig

 	IngressInformer            cache.SharedIndexInformer
+	SecretInformer             cache.SharedIndexInformer


Why do you need to watch secrets?

pkg/loadbalancers/l7.go

pkg/controller/controller.go

agadelshin · 2018-12-09T20:09:18Z

@rramkumar1

yes, sure.
Thanks. I started to merge this in pkg/controller/controller.go:481, but I found that pass ingresslist instead of one ingress is not optimal decision. A lot of places in codebase expects 1 ingress in syncState. I'll think about this.

agadelshin · 2018-12-09T21:34:01Z

@rramkumar1 could you please look again? I'm not sure what I missed here. Can you give me some advice how can I test my changes? Usually I build docker image and setup GKE cluster to run some ingresses and check what happens, but I believe it's not optimal.

agadelshin · 2018-12-09T21:40:20Z

BTW, I haven't tested it yet, just run go test.

rramkumar1 · 2018-12-11T17:17:18Z

@pondohva I'm still having trouble understanding how your changes would even work. In particular, I don't see any sort of merging logic. Can you please explain your approach?

agadelshin · 2018-12-11T19:43:13Z

@rramkumar1 I merged urlmap only in https://github.com/kubernetes/ingress-gce/pull/565/files#diff-3c862eb54a8e0e161b534e0c67e5379eR520.

this ingresses:

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: basic-ingress1
  annotations:
    kubernetes.io/ingress.class: "gce"
    kubernetes.io/ingress.loadbalancer-name: "abc"
spec:
  rules:
  - host: host.example.com
    http:
      paths:
      - backend:
          serviceName: web
          servicePort: 8080
        path: /web1
---
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: basic-ingress2
  annotations:
    kubernetes.io/ingress.class: "gce"
    kubernetes.io/ingress.loadbalancer-name: "abc"
spec:
  rules:
  - host: host.example.com
    http:
      paths:
      - backend:
          serviceName: web2
          servicePort: 8080
        path: /web2

result:

$ kubectl get ing
NAME             HOSTS              ADDRESS          PORTS     AGE
basic-ingress1   host.example.com   35.244.156.139   80        4m
basic-ingress2   host.example.com   35.244.156.139   80        4m

agadelshin · 2018-12-12T13:45:19Z

@rramkumar1 I got your point. Added merge logic here https://github.com/kubernetes/ingress-gce/pull/565/files#diff-3c862eb54a8e0e161b534e0c67e5379eR377

agadelshin · 2019-01-03T12:16:42Z

@rramkumar1 could you please review last changes? anything else to do?

rramkumar1 · 2019-01-06T08:19:33Z

@pondohva Sorry I was on vacation and haven't had much time to look at this.

Given that this is a big change I would like to spend some more time looking at it. I'll try to have some comments in a week or so.

agadelshin · 2019-03-05T23:58:23Z

@rramkumar1 did you have some time to look at this PR? I'd like to implement this feature, but maybe I should rework smth?

rramkumar1 · 2019-03-27T15:53:22Z

@pondohva Overall the PR seems to get the job done but my feeling is that the logic is going to be really flimsy and error prone.

I'll leave this PR open but for the time being I don't think we will move forward with it.

mcfedr · 2019-08-23T13:45:20Z

@rramkumar1 This seems like an interesting concept, do you have any comments what it would take to move forward with it?

bowei · 2020-02-26T08:24:10Z

/ok-to-test

k8s-ci-robot · 2020-02-26T08:24:17Z

@agadelshin: PR needs rebase.

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 · 2020-02-26T08:26:25Z

@agadelshin: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-ingress-gce-test	`0e03a55`	link	`/test pull-ingress-gce-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

agadelshin · 2020-04-09T22:21:17Z

thank you @bowei for update. I’ll rebase PR and will try to figure out why tests failed.

Also I’d like to finalize it so if you have any suggestions how I can improve it, I’d appreciate.

agadelshin · 2020-04-10T14:24:56Z

oh, well, it's almost 1.5 years later. I believe it will be safer and better for learning to rewrite it :)

rramkumar1 · 2020-04-10T14:26:52Z

@agadelshin I think you will find interest in the new Gateway APIs which are more tailored to solve this problem. I don't think we will be able to merge any form of this PR to this repo

https://github.com/kubernetes-sigs/service-apis

agadelshin · 2020-04-10T14:32:14Z

@rramkumar1 got it! thank you, I'll look into Gateway APIs.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. 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 Dec 1, 2018

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 1, 2018

k8s-ci-robot requested review from rramkumar1 and thockin December 1, 2018 13:00

agadelshin commented Dec 2, 2018

View reviewed changes

k8s-ci-robot assigned rramkumar1 Dec 3, 2018

rramkumar1 reviewed Dec 9, 2018

View reviewed changes

agadelshin changed the title ~~[WIP] add lbName annotation~~ add lbName annotation Dec 9, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 9, 2018

add lbName annotation

0e03a55

k8s-ci-robot added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Mar 27, 2019

agadelshin closed this Apr 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add lbName annotation #565

add lbName annotation #565

agadelshin commented Dec 1, 2018

k8s-ci-robot commented Dec 1, 2018

agadelshin commented Dec 1, 2018

agadelshin commented Dec 1, 2018

agadelshin Dec 2, 2018

agadelshin commented Dec 3, 2018

rramkumar1 commented Dec 7, 2018 •

edited

Loading

k8s-ci-robot commented Dec 9, 2018

rramkumar1 left a comment

rramkumar1 Dec 7, 2018

agadelshin commented Dec 9, 2018

agadelshin commented Dec 9, 2018 •

edited

Loading

agadelshin commented Dec 9, 2018

rramkumar1 commented Dec 11, 2018

agadelshin commented Dec 11, 2018

agadelshin commented Dec 12, 2018

agadelshin commented Jan 3, 2019

rramkumar1 commented Jan 6, 2019

agadelshin commented Mar 5, 2019

rramkumar1 commented Mar 27, 2019 •

edited

Loading

mcfedr commented Aug 23, 2019

bowei commented Feb 26, 2020

k8s-ci-robot commented Feb 26, 2020

k8s-ci-robot commented Feb 26, 2020

agadelshin commented Apr 9, 2020

agadelshin commented Apr 10, 2020

rramkumar1 commented Apr 10, 2020

agadelshin commented Apr 10, 2020

add lbName annotation #565

add lbName annotation #565

Conversation

agadelshin commented Dec 1, 2018

k8s-ci-robot commented Dec 1, 2018

agadelshin commented Dec 1, 2018

agadelshin commented Dec 1, 2018

agadelshin Dec 2, 2018

Choose a reason for hiding this comment

agadelshin commented Dec 3, 2018

rramkumar1 commented Dec 7, 2018 • edited Loading

k8s-ci-robot commented Dec 9, 2018

rramkumar1 left a comment

Choose a reason for hiding this comment

rramkumar1 Dec 7, 2018

Choose a reason for hiding this comment

agadelshin commented Dec 9, 2018

agadelshin commented Dec 9, 2018 • edited Loading

agadelshin commented Dec 9, 2018

rramkumar1 commented Dec 11, 2018

agadelshin commented Dec 11, 2018

agadelshin commented Dec 12, 2018

agadelshin commented Jan 3, 2019

rramkumar1 commented Jan 6, 2019

agadelshin commented Mar 5, 2019

rramkumar1 commented Mar 27, 2019 • edited Loading

mcfedr commented Aug 23, 2019

bowei commented Feb 26, 2020

k8s-ci-robot commented Feb 26, 2020

k8s-ci-robot commented Feb 26, 2020

agadelshin commented Apr 9, 2020

agadelshin commented Apr 10, 2020

rramkumar1 commented Apr 10, 2020

agadelshin commented Apr 10, 2020

rramkumar1 commented Dec 7, 2018 •

edited

Loading

agadelshin commented Dec 9, 2018 •

edited

Loading

rramkumar1 commented Mar 27, 2019 •

edited

Loading