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.

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

[BUG] GSLB is not updated when Ingress has change #932

Closed
kuritka opened this issue Aug 5, 2022 · 2 comments · Fixed by #948 or #1652
Closed

[BUG] GSLB is not updated when Ingress has change #932

kuritka opened this issue Aug 5, 2022 · 2 comments · Fixed by #948 or #1652
Labels
bug Something isn't working
Milestone

Comments

@kuritka
Copy link
Collaborator

kuritka commented Aug 5, 2022

We found a bug where GSLB does not update itself (specifically annotations section) when the ingress has changed.

The problem with the annotations was that the rancher kept changing the annotations and k8gb kept changing them back. They were stuck in a loop.

The bug is located here:

log.Info().
Str("ingress", a.GetName()).
Msg("Ingress does not exist anymore. Skipping Glsb creation...")
return

  • implement an update functionality of the GSLB as the ingress changes.
  • Implement the necessary tests to make sure that the GSLB update is done exactly as we need.
@kuritka kuritka added the bug Something isn't working label Aug 5, 2022
kuritka added a commit that referenced this issue Sep 5, 2022
related to #932

The setup manager is quite complex. We are currently solving a bug related to the fact
that changes in ingress also change the GSLB and can cause cycles #932. I am doing the
necessary refactoring (code is better structured, but the functionality remains the same),
to make it easier to make the necessary changes..

 - split SetupWithManager and Reconciliation into two files
 - encapsulate createGslbFromIngress
 - simplify ingressHandler
 - extracting parse strategy func

Signed-off-by: kuritka <kuritka@gmail.com>
kuritka added a commit that referenced this issue Sep 5, 2022
related to #932

The setup manager is quite complex. We are currently solving a bug related to the fact
that changes in ingress also change the GSLB and can cause cycles #932. I am doing the
necessary refactoring (code is better structured, but the functionality remains the same),
to make it easier to make the necessary changes..

 - split SetupWithManager and Reconciliation into two files
 - encapsulate createGslbFromIngress
 - simplify ingressHandler
 - extracting parse strategy func

Signed-off-by: kuritka <kuritka@gmail.com>
kuritka added a commit that referenced this issue Sep 6, 2022
* Refactoring SetupManager
related to #932

The setup manager is quite complex. We are currently solving a bug related to the fact
that changes in ingress also change the GSLB and can cause cycles #932. I am doing the
necessary refactoring (code is better structured, but the functionality remains the same),
to make it easier to make the necessary changes..

 - split SetupWithManager and Reconciliation into two files
 - encapsulate createGslbFromIngress
 - simplify ingressHandler
 - extracting parse strategy func

Signed-off-by: kuritka <kuritka@gmail.com>

* Update controllers/gslb_controller_setup.go

Co-authored-by: Jirka Kremser <535866+jkremser@users.noreply.github.com>
Signed-off-by: MichalK <kuritka@gmail.com>

Signed-off-by: kuritka <kuritka@gmail.com>
Signed-off-by: MichalK <kuritka@gmail.com>
Co-authored-by: Jirka Kremser <535866+jkremser@users.noreply.github.com>
@kuritka kuritka reopened this Sep 9, 2022
kuritka added a commit that referenced this issue Sep 9, 2022
related to #923,#932

I'm going to introduce new powers for Manager and Client. I'm putting all mocks into one directory.

Signed-off-by: kuritka <kuritka@gmail.com>
kuritka added a commit that referenced this issue Sep 9, 2022
related to #923,#932

I'm going to introduce new mocks for Manager and Client. I'm putting all mocks into one directory.

Signed-off-by: kuritka <kuritka@gmail.com>
kuritka added a commit that referenced this issue Sep 12, 2022
related to #923,#932

I'm going to introduce new mocks for Manager and Client. I'm putting all mocks into one directory.

Signed-off-by: kuritka <kuritka@gmail.com>
@somaritane somaritane added this to the 1.0 milestone Oct 9, 2022
@somaritane somaritane moved this to To do in k8gb Oct 9, 2022
@somaritane somaritane added this to k8gb Oct 9, 2022
@somaritane somaritane moved this from To do to Done in k8gb Oct 26, 2022
@somaritane somaritane moved this from Done to In progress in k8gb Oct 26, 2022
@kuritka
Copy link
Collaborator Author

kuritka commented Nov 2, 2022

returning back on this task:

BUG reproducing manually

having ONLY simple podinfo app and k8gb deployed (using clean test environment https://github.com/kuritka/k8gb-demo-cncf-2022)

than, by applying ingress puting test application under k8gb control (application is loadbalancable):

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    x.y.io/ep: '[{"addresses":["1.2.3.4"],"port":80}]'
    k8gb.io/strategy: roundRobin
  name: gslb
  namespace: demo
spec:
  ingressClassName: nginx
  rules:
  - host: demo.cloud.example.com
    http:
      paths:
      - backend:
          service:
            name: frontend-podinfo
            port:
              name: http
        path: /
        pathType: Prefix

manually changing ingress k -n demo edit ing gslb and change annotation to x.y.io/ep: '[{"addresses":["2.2.2.2"],"port":8080}]'

command k -n demo get ing -o=jsonpath='{.items[*].metadata.annotations}{"\n"}' returns unchanged annotation
map[x.y.io/ep:[{"addresses":["1.2.3.4"],"port":80}]

kuritka added a commit that referenced this issue Nov 7, 2022
closes #932
related to #1018

 - Fixed implementation
In some cases, when the ingress was changed, reconciliation was triggered, which changed the annotations back to the values calculated with GSLB. I modified the merge function to filter which values should be synced and which should not. In fact, it is only `k8gb.io/strategy` and `k8gb.io/primary-geotag` that are changed on initialization.
 - utit tests update
 - terratest
terratests are focused on repeated patching of annotations in ingress and examining ingress when patching annotations in GSLB

Signed-off-by: kuritka <kuritka@gmail.com>
kuritka added a commit that referenced this issue Nov 7, 2022
closes #932
related to #1018

 - Fixed implementation
In some cases, when the ingress was changed, reconciliation was triggered, which changed the annotations back to the values calculated with GSLB. I modified the merge function to filter which values should be synced and which should not. In fact, it is only `k8gb.io/strategy` and `k8gb.io/primary-geotag` that are changed on initialization.
 - utit tests update
 - terratest
terratests are focused on repeated patching of annotations in ingress and examining ingress when patching annotations in GSLB

Signed-off-by: kuritka <kuritka@gmail.com>
kuritka added a commit that referenced this issue Nov 7, 2022
closes #932
related to #1018

 - Implementation: In some cases, when the ingress was changed, reconciliation was triggered, which changed the annotations back to the values calculated with GSLB. I modified the merge function to filter which values should be synced and which should not. In fact, it is only `k8gb.io/strategy` and `k8gb.io/primary-geotag` that are changed on initialization.
 - Utit tests update
 - Terratests are focused on repeated patching of annotations in ingress and examining ingress when patching annotations in GSLB

Signed-off-by: kuritka <kuritka@gmail.com>
@kuritka
Copy link
Collaborator Author

kuritka commented Mar 19, 2023

related to #1019

This error arises because GLSB and ingress are two sources for k8gb. If you overwrite one of them, k8gb takes the data from the second one and overwrites the first one. Another consequence is that if you delete one source, for example, it can be regenerated from the first one. If there is an inconsistency, one, the source is modified by the other and vice versa. This can cause several error cycles at the beginning until the reconciliation is stopped. It also depends on whether you create an ingress with GSLB or vice versa.

The best solution is to have only one source of truth (in our case it would be ingress, later gateway).

I started a working demo where I removed this : k8gb-lite. This required refactoring the watcher, throwing out the GSLB and rewriting the terratest. I put terratests for FO, RR, WRR against three clusters and did manual tests at the same time, everything works as expected.

@ytsarev ytsarev moved this from In progress to To do in k8gb Apr 5, 2023
@ytsarev ytsarev moved this from To do to In progress in k8gb Jul 20, 2024
@github-project-automation github-project-automation bot moved this from In progress to Done in k8gb Jul 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
2 participants