Introducing standard ingress relation support

weiiwang01 commented 1 year ago

This update integrates standard ingress relation support into the nginx-ingress-integrator charm. Concurrently, it phases out the legacy ingress relation.

This updated nginx-ingress-integrator charm isn't backward compatible with earlier versions.

Due to certain constraints of the nginx ingress controller, the nginx-ingress-integrator only supports the v2 version of the ingress relation with the ip field. However, please note that changes to include the ip field have not been merged as of now. See the pending changes here: canonical/traefik-k8s-operator#220

Other notable changes are:

The nginx-ingress-integrator will now only accept one relation at a time; either one nginx-route relation without any ingress relation or vice versa.
The configuration-only mode has been removed. Relations are required for the nginx-ingress-integrator to create ingress.
The default value for the rewrite-enabled configuration has been modified to false.
The describe-ingresses action is removed, since the action only prints definition of the ingress, while the charm manage four different types of resources. Only provide ingress definition might not be able to provide a full picture for user. Users should use kubectl for debugging.
The service IP addresses info in the charm status is remove, since the standard ingress uses headless service which doesn't have cluster IP address, this is to align the experience of ingress relation and nginx-route relation.

claudiubelu commented 1 year ago

From what I can tell, this basically introduces regressions when it comes to Nginx Ingress.

This limits the ingress relation to 1, which is opposite to what was requested by a user [1]. Secondly, it also bricks the usage of nginx/nginx-ingress Controller typically used in EKS deployments [2]:

The nginx/nginx-ingress Controller typically used in EKS doesn't handle
multiple Kubernetes Ingress Resources having the same hostname in the same
way as the k8s.gcr.io/ingress-nginx/controller:v1.1.0 Controller used in
microk8s. The former doesn't handle them at all, unless you specify certain
annotations, and an additional Kubernetes Ingress Resource [1]. But both
Controllers can handle one Kubernetes Ingress Resource containing multiple
hosts / routes.

Basically, multiple relations were introduced so only one Ingress resource would be created, which would contain all the necessary routes associated with a single service hostname, allowing us to bundle different services under the same name.

Additionally, limiting to only one relation per charm, would mean adding N instances of a charm for N charms that would require that relation, instead of having just one nginx-ingress-integrator charm. That makes it more of a hassle than it should to maintain and update. Plus, IMO, it makes deployments look messier and unappealing for future clients and potential adopters of Juju.

Hopefully these issues will be addressed somehow before moving forward.

[1] https://bugs.launchpad.net/charm-k8s-ingress/+bug/1955597 [2] https://git.launchpad.net/charm-k8s-ingress/commit/?id=59f1ca1b4dec1a3fc7cb659b554800ae15b943d3

weiiwang01 commented 1 year ago

From what I can tell, this basically introduces regressions when it comes to Nginx Ingress.

This limits the ingress relation to 1, which is opposite to what was requested by a user [1]. Secondly, it also bricks the usage of nginx/nginx-ingress Controller typically used in EKS deployments [2]:
The nginx/nginx-ingress Controller typically used in EKS doesn't handle
multiple Kubernetes Ingress Resources having the same hostname in the same
way as the k8s.gcr.io/ingress-nginx/controller:v1.1.0 Controller used in
microk8s. The former doesn't handle them at all, unless you specify certain
annotations, and an additional Kubernetes Ingress Resource [1]. But both
Controllers can handle one Kubernetes Ingress Resource containing multiple
hosts / routes.
Basically, multiple relations were introduced so only one Ingress resource would be created, which would contain all the necessary routes associated with a single service hostname, allowing us to bundle different services under the same name.

Additionally, limiting to only one relation per charm, would mean adding N instances of a charm for N charms that would require that relation, instead of having just one nginx-ingress-integrator charm. That makes it more of a hassle than it should to maintain and update. Plus, IMO, it makes deployments look messier and unappealing for future clients and potential adopters of Juju.

Hopefully these issues will be addressed somehow before moving forward.

[1] https://bugs.launchpad.net/charm-k8s-ingress/+bug/1955597 [2] https://git.launchpad.net/charm-k8s-ingress/commit/?id=59f1ca1b4dec1a3fc7cb659b554800ae15b943d3

@claudiubelu Thanks for your input! The rationale behind this change is that if multiple relations are connected to the same nginx-ingress-integrator charm, then the charm configuration will be applied to all the relations. This becomes especially tricky for configurations like tls-secret-name. For that reason, more often in our deployment, we simply deploy an nginx-ingress-integrator charm for each application. Since the nginx-ingress-integrator charm is a workload-less charm, there isn't too much overhead. And now, as we are some plans to add the tls-certificates relation to the nginx-ingress-integrator charm, it will further complicate the situation. We have also received user request to create multiple ingress resources for the same host to add different annotations to different paths: https://github.com/canonical/nginx-ingress-integrator-operator/issues/78. It's quite a challenge to satisfy both the Nginx's Ingress Controller users and Kubernetes' Nginx Ingress Controller users.

We are aware this is a significant breaking change. That's why this will not be merged into the main branch of the nginx-ingress-integrator. Instead, we will release the new version in a different channel on the charmhub, likely called v2.

github-actions[bot] commented 1 year ago

Test coverage for 303a8acd531b64051215930bca85db86260223d6

Name                        Stmts   Miss Branch BrPart  Cover   Missing
-----------------------------------------------------------------------
src/charm.py                  118      2     20      1    98%   145, 152
src/consts.py                   3      0      0      0   100%
src/controller.py             259     14     69      8    91%   65, 231, 392, 416, 438, 458, 496, 614, 634-635, 803->814, 807-809, 812-813
src/exceptions.py               4      0      0      0   100%
src/ingress_definition.py     251     26     65      8    89%   35, 51, 147-151, 169, 273-274, 319, 331, 335, 351-352, 367-368, 393, 395, 420-421, 454-455, 462-464, 466
-----------------------------------------------------------------------
TOTAL                         635     42    154     17    92%

Static code analysis report

Run started:2023-10-27 04:03:07.115744

Test results:
    No issues identified.

Code scanned:
    Total lines of code: 2700
    Total lines skipped (#nosec): 2
    Total potential issues skipped due to specifically being disabled (e.g., #nosec BXXX): 0

Run metrics:
    Total issues (by severity):
        Undefined: 0
        Low: 0
        Medium: 0
        High: 0
    Total issues (by confidence):
        Undefined: 0
        Low: 0
        Medium: 0
        High: 0
Files skipped (0):

weiiwang01 commented 1 year ago

All progress moved to the v2 branch, close this pull request.

canonical / nginx-ingress-integrator-operator

Introducing standard ingress relation support #91