-
Notifications
You must be signed in to change notification settings - Fork 95
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
Decouple gslb from the kubernetes Ingress resource #1557
Conversation
b0871db
to
486fedc
Compare
d38e840
to
1ed9519
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abaguas incredible work, thank you so much!
I performed some exploratory testing with local setup and found the following issue:
- Deploy full local setup with the test version
K8GB_LOCAL_VERSION=test make deploy-full-local-setup
- Create Gslb in
test-gslb
namespace
k -n test-gslb apply -f terratest/examples/failover-ref-gslb.yaml
- Create second testing namespace
k create ns test-gslb2
- Create ingress in this namespace
k -n test-gslb2 apply -f terratest/examples/failover-ref-ingress.yaml
- Notice that Gslb and referenced Ingress are deployed to different namespaces
- At the same time Gslb is capable to select Ingress from another namespace
k -n test-gslb get gslb -o yaml
....
status:
geoTag: us
healthyRecords: {}
hosts: terratest-failover.cloud.example.com
loadBalancer:
exposedIps:
- 172.20.0.10
- 172.20.0.11
servers:
- host: terratest-failover.cloud.example.com
services:
- name: frontend-podinfo
namespace: test-gslb2
serviceHealth:
terratest-failover.cloud.example.com: NotFound
This is most probably undesired behavior as Gslb designed to be namespace-scoped
Indeed, great catch. I just reproduced the issue (thank you for the detailed instructions) and 03e1e4e fixes it |
This change makes the GSLB resource independent of a Kubernetes Ingress. This is a first step to allow integrations with other ingress Resources (e.g. istio virtual services for Istio Gateway, HTTP routes for Gateway API) k8gb-io#552 The change is fully backwards compatible, embedding an Ingress resources will still behave the same way it worked before. In addition to the code refacting a new ResourceRef field is introduced. This field allows referencing resources that are not embedded in the GSLB definition and opens the gates to reference any resource types. As an example, the configuration bellow allows a GSLB resource to load balance the application defined in the Ingress resource on the same namespace with labels `app: demo` ``` spec: resourceRef: ingress: matchLabels: app: demo ``` --- Implementation details A couple of functions crucial for business logic, namely `GslbIngressExposedIPs` and `getServiceHealthStatus`, need to read configuration present in the Ingress resource. Since the code would become too complicated once new ways to configure ingress are integrated, the format of the data they depend on was generalized from the Kubernetes Ingress resource to an ingress agnostic format. The processing of the data looks as follows: A new `GslbReferenceResolver` interface was created. An implementation of this interface is capable of understanding a type of ingress configuration (e.g.: kubernetes' ingress, istio's virtual service, gateway API's http route) and implements two functions: `GetServers` and `GetGslbExposedIPs`. These functions extract the backends of the applications and the IP addresses they are exposed on, respectively. Once a reconciliation operation is triggered a new `GslbReferenceResolver` is instatiated. Then, the list of servers and the exposed IPs are read and stored in the status of the GSLB resource. Finally, the rest of the logic remains the same, with the difference that functions implementing business logic read the configuration from the status instead of looking up the Kubernetes Ingress resource. --- Points for discussion: * Should the list of servers and exposed IPs be stored in the status of GSLB resource? An internal data structure would also work, however we would need to pass it as an argument to numerous functions. * There is already a `depresolver` interface. Even though the names look similar `depresolver` resolves startup configuration, while `refresolver` resolves runtime configuration. In addition, logging is useful to communicate with the users but a logger cannot be instantiated in the `depresolver` package because it would lead to circular dependencies. For these reasons, a new package was created instead of adding this logic to the `depresolver` package. Naming of the package can also be discussed, the proposal `refresolver` comes from the fact that it resolves references to other resources. Signed-off-by: abaguas <[email protected]>
Signed-off-by: abaguas <[email protected]>
Signed-off-by: abaguas <[email protected]>
Signed-off-by: abaguas <[email protected]>
Signed-off-by: abaguas <[email protected]>
I've rebased this PR with
make demo DEMO_URL=http://terratest-failover.cloud.example.com
...
200
"message": "eu",
[Sun Jun 30 12:18:49 UTC 2024] ...
200
"message": "us",
[Sun Jun 30 12:18:54 UTC 2024] ...
200
"message": "us",
[Sun Jun 30 12:18:59 UTC 2024] ...
200
"message": "us",
[Sun Jun 30 12:19:05 UTC 2024] ...
503
[Sun Jun 30 12:19:10 UTC 2024] ...
200
"message": "eu",
[Sun Jun 30 12:19:15 UTC 2024] ...
200
"message": "eu",
I trust it is ready to merge. Kudos and thanks again to @abaguas for your incredible contribution! 🥇 // waiting for terratest pipelines to be green before the final approval and merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All green, see the comment above. Merging in 👍
This change makes the GSLB resource independent of a Kubernetes Ingress. This is a first step to allow integrations with other ingress Resources (e.g. istio virtual services for Istio Gateway, HTTP routes for Gateway API) #552
The change is fully backwards compatible, embedding an Ingress resources will still behave the same way it worked before.
In addition to the code refacting a new ResourceRef field is introduced. This field allows referencing resources that are not embedded in the GSLB definition and opens the gates to reference any resource types. As an example, the configuration bellow allows a GSLB resource to load balance the application defined in the Ingress resource on the same namespace with labels
app: demo
Implementation details
A couple of functions crucial for business logic, namely
GslbIngressExposedIPs
andgetServiceHealthStatus
, need to read configuration present in the Ingress resource. Since the code would become too complicated once new ways to configure ingress are integrated, the format of the data they depend on was generalized from the Kubernetes Ingress resource to an ingress agnostic format. The logic to process the data is as follows:A new
GslbReferenceResolver
interface was created. An implementation of this interface is capable of understanding a type of ingress configuration (e.g.: kubernetes' ingress, istio's virtual service, gateway API's http route) and implements two functions:GetServers
andGetGslbExposedIPs
. These functions extract the backends of the applications and the IP addresses they are exposed on, respectively.Once a reconciliation operation is triggered a new
GslbReferenceResolver
is instatiated. Then, the list of servers and the exposed IPs are read and stored in the status of the GSLB resource.Finally, the rest of the logic remains the same, with the difference that functions implementing business logic read the configuration from the status instead of looking up the Kubernetes Ingress resource.
Design Decisions:
An internal data structure would also work, however we would need to pass it as an argument to numerous functions.
By storing it in the status the code is cleaner and the information is more easily accessible to the users.
There is already a
depresolver
interface. Even though the names look similardepresolver
resolves startup configuration, whilerefresolver
resolves runtime configuration. In addition, logging is useful to communicate with the users but a logger cannot be instantiated in thedepresolver
package because it would lead to circular dependencies. For these reasons, a new package was created instead of adding this logic to thedepresolver
package. Naming of the package can also be discussed, the proposalrefresolver
comes from the fact that it resolves references to other resources.Testing:
Terratest tests for referenced ingresses were added. These tests are the exact same as for embedded ingresses, to make sure the functionality is the same.
Since the test environment must be torn down by the
defer
calls, each test was encapsulated in a function that is called by the respective table test.