Add support for disabling use of TLS per domain suffix #162

cha7ri · 2021-11-15T20:07:59Z

Context:

We have clusters with two ingress controllers:

Public ingress controller using the domain example.com
Private ingress controller using the doamin private.example.com

What we want to achieve?

Use letsencrypt to issue certificates for example.com
Disable tls for private.example.com

Solution:

Set use-ingress-tls to default_on to enable tls by default for all ingresses
Add a new config option to FIAAS to disable tls for one or many domains. example tls-certificate-issuer-disable-for-domain-suffixes=private.example.com

What will change:

Group hosts that we want to disable tls for together in one ingress resource (if the user doesn't set annotations).
Do not add kubernetes.io/tls-acme: true or cert-manager.io/cluster-issuer: nameOfClusterIssuer annotation to the ingress resource.
This will tell cert-manager to not create a certificate for those hosts

docs/operator_guide.md

oyvindio

I think the current implementation has behaviour that could be unexpected for some application configurations, see comment in-line.

tests/fiaas_deploy_daemon/test_config.py

docs/operator_guide.md

fiaas_deploy_daemon/deployer/kubernetes/ingress.py

…is is not true because they can be part of diffirent ingress groups

oyvindio

Thanks for improving the tests and even adding e2e tests here. I've taken another look at this and left a few comments and questions in line

oyvindio · 2021-12-16T13:20:53Z

fiaas_deploy_daemon/deployer/kubernetes/ingress.py

+                # This change to fix the issue: when we dont have any default ingress item the ingress is added to the ingresses list anyway. Now it will be added only if we have atleast one default ingress item (host)
+                default_ingress = default_ingresses.setdefault("default",
+                                                               AnnotatedIngress(name="", ingress_items=[], annotations={},
+                                                                                explicit_host=explicit_host, issuer_type=self._tls_issuer_type_default,
+                                                                                default=True))


I'm not sure I understand the background for the change in the grouping logic+how the default ingress is set up. From my understanding of the existing flow it seems like it might not be necessary, and I think it would be good to avoid the additional complexity if possible.

As I understand it, there are already configurations where the default ingress (or any other ingress) could have no ingres items, for example if no ingress suffixes are configured and an application only has a ingress with annotations, like the following:

ingress: - host: example.com annotations: foo: bar

If this happens, the default ingress should be skipped here (and deleted if it already exists):

fiaas-deploy-daemon/fiaas_deploy_daemon/deployer/kubernetes/ingress.py

Lines 71 to 73 in 5f81b65

if len(annotated_ingress.ingress_items) == 0:

LOG.info("No items, skipping: %s", annotated_ingress)

continue

oyvindio · 2022-01-03T13:32:31Z

fiaas_deploy_daemon/deployer/kubernetes/ingress.py

+        elif app_spec.ingress_tls.enabled is True:
+            return True
+        elif self._should_disable_ingress_tls(hosts) is True:
+            return False


I think this could lead to unexpected behavior: If extensions.tls.enabled is set to true in the application's configuration, the "no_tls" ingress would still get a .spec.tls section, because the check on line 333 is never run as the function returned on 332.

As a sidenote, it might be a bit clearer and more efficient to add a boolean field to AnnotatedIngress which is set on the "no_tls" ingress when grouping ingress items, indicating that tls should not be enabled for this ingress.

oyvindio · 2022-01-03T15:48:06Z

tests/fiaas_deploy_daemon/test_e2e.py

    def wait_until_fdd_ready(self, k8s_version, kubernetes, ready):
        wait_until(ready, "web-interface healthy", RuntimeError, patience=PATIENCE)
        if crd_supported(k8s_version):
            wait_until(
                crd_available(kubernetes, timeout=TIMEOUT),
                "CRD available", RuntimeError, patience=PATIENCE
            )
-
-    def prepare_fdd(self, request, kubernetes, k8s_version, use_docker_for_e2e, service_type, service_account=False):
+    def prepare_fdd(self, request, kubernetes, k8s_version, use_docker_for_e2e, service_type, service_account=False, disable_tls_for_doamin_suffixes=False, extra_args=[]):


Looks like there is a typo in the new parameter here; doamin should probably be domain?

Similarly in the fixture above https://github.com/fiaas/fiaas-deploy-daemon/pull/162/files#diff-31f8ae6f0324a07b66e27c40e544839f99e014bb63b03be1cda6e67ee4c53b2aR127

oyvindio · 2022-01-04T11:03:14Z

tests/fiaas_deploy_daemon/e2e_expected/tls_disable_tls_for_one_host1.yml

+      name: v3-data-examples-tls-disable-tls-for-one-host
+  finalizers: []
+spec:
+  tls: []


It seems to me that to fully test the new behavior in this PR, the test should verify that this ingress has a .spec.tls section. Since tls is disabled (default_off) at the fiaas-deploy-daemon level and not enabled at the application config level, only the hostname grouping is tested here. (similarly in tls_disable_tls_for_one_default_host1.yml).

oyvindio · 2022-01-04T13:49:40Z

tests/fiaas_deploy_daemon/test_e2e.py

        try:
-            daemon = subprocess.Popen(args, stdout=sys.stderr, env=merge_dicts(os.environ, {"NAMESPACE": "default"}))
-            time.sleep(1)
-            if daemon.poll() is not None:
-                pytest.fail("fiaas-deploy-daemon has crashed after startup, inspect logs")
-            self.wait_until_fdd_ready(k8s_version, kubernetes_service_account, ready)
+            daemon=self.start_fdd(args,port,ready,k8s_version,kubernetes_per_app_service_account)
            yield "http://localhost:{}/fiaas".format(port)
        finally:
            self._end_popen(daemon)


Collecting the common logic into start_fdd here is a nice improvement. However there is a small change in behavior here which makes tests more difficult to troubleshoot: If start_fdd raises a exception (typically because wait_until_fdd_ready fails because of some issue), the exception is masked by a UnboundLocalError raised in the finally block when trying to reference daemon, since it does not have a value. I think the process may leak if this happens too, as it is not passed to _end_popen.

I only noticed this because tests using this fixture was failing and the UnboundLocalError masked a NameError because kubernetes_per_app_service_account is not defined

oyvindio · 2022-01-04T15:21:11Z

tests/fiaas_deploy_daemon/deployer/kubernetes/test_ingress_deploy.py

+    def ingress_tls_disable_tls_for_domain_suffixes(self, config,request):
+        config.tls_certificate_issuer_disable_for_domain_suffixes = request.param["tls_certificate_issuer_disable_for_domain_suffixes"]
+        config.tls_certificate_issuer_type_overrides = {
+            "other.cloud.com": "certmanager.k8s.io/issuer"


To avoid referring to unknown third party domains (several places in this file), I think it is better to use one of example.{com,org,net} in test data such as here

oyvindio · 2022-01-05T14:38:57Z

tests/fiaas_deploy_daemon/deployer/kubernetes/test_ingress_deploy.py

+    )
+
+    @pytest.mark.usefixtures("delete")
+    def test_applies_tls_certificate_issuer_disable_for_domain_suffixes(self, deployer_disable_tls_for_domain_suffixes, ingress_tls_disable_tls_for_domain_suffixes, app_spec,ingresses_spec,expected_host_groups,hosts,use_suffixes):


I think this test would better verify the changes in behavior in this PR if it verified the complete flow from app_spec/ingress items to the resulting ingress resources, similar to test_ingress_deploy. It seems to me that one of the essential things that should be tested is not only how ingress items are grouped into ingresses, but also which of the ingresses are set up with TLS. As I understand the test as it is now, it doesn't verify this behavior since IngressTls.apply is mocked

oyvindio · 2022-01-06T09:17:22Z

/sem-approve

arealmaas · 2022-06-29T11:51:43Z

Any expected timeline for when this is to be merged? :)

cha7ri requested a review from a team as a code owner November 15, 2021 20:08

cha7ri force-pushed the disable_tld_per_domain branch from d2f48c5 to 34e3b80 Compare November 16, 2021 07:56

birgirst reviewed Nov 16, 2021

View reviewed changes

docs/operator_guide.md Outdated Show resolved Hide resolved

oyvindio requested changes Nov 18, 2021

View reviewed changes

tests/fiaas_deploy_daemon/test_config.py Outdated Show resolved Hide resolved

docs/operator_guide.md Outdated Show resolved Hide resolved

fiaas_deploy_daemon/deployer/kubernetes/ingress.py Outdated Show resolved Hide resolved

disable tls for a specific daomain suffix

9be2b76

cha7ri force-pushed the disable_tld_per_domain branch from 6a307d7 to 9be2b76 Compare November 30, 2021 09:37

Tarik Ghallab and others added 6 commits December 1, 2021 17:11

fix tests

41743a8

clean up

ee4e239

fix unit test

a51245b

add more unti tests

43df80a

Merge branch 'master' into disable_tld_per_domain

fb97b2f

merge fix

9d3ae9f

birgirst changed the title ~~disable TLS per domain suffix~~ Add support for disabling use of TLS per domain suffix Dec 13, 2021

We assume that the default hosts will be always in the same group, th…

19604ef

…is is not true because they can be part of diffirent ingress groups

cha7ri force-pushed the disable_tld_per_domain branch from e496aab to 19604ef Compare December 14, 2021 15:25

cha7ri requested a review from oyvindio January 3, 2022 09:03

oyvindio requested changes Jan 6, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for disabling use of TLS per domain suffix #162

Add support for disabling use of TLS per domain suffix #162

cha7ri commented Nov 15, 2021 •

edited

Loading

oyvindio left a comment

oyvindio left a comment

oyvindio Dec 16, 2021

oyvindio Jan 3, 2022

oyvindio Jan 3, 2022

oyvindio Jan 4, 2022

oyvindio Jan 4, 2022

oyvindio Jan 4, 2022

oyvindio Jan 4, 2022

oyvindio Jan 5, 2022

oyvindio commented Jan 6, 2022

arealmaas commented Jun 29, 2022

	if len(annotated_ingress.ingress_items) == 0:
	LOG.info("No items, skipping: %s", annotated_ingress)
	continue

Add support for disabling use of TLS per domain suffix #162

Are you sure you want to change the base?

Add support for disabling use of TLS per domain suffix #162

Conversation

cha7ri commented Nov 15, 2021 • edited Loading

Context:

What we want to achieve?

Solution:

What will change:

oyvindio left a comment

Choose a reason for hiding this comment

oyvindio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oyvindio commented Jan 6, 2022

arealmaas commented Jun 29, 2022

cha7ri commented Nov 15, 2021 •

edited

Loading