From e496aab70135bb5a81a3dc0a6a186ca5e565e0d8 Mon Sep 17 00:00:00 2001 From: Tarik Ghallab Date: Tue, 14 Dec 2021 15:55:58 +0100 Subject: [PATCH] We assume that the default hosts will be always in the same group, this is not true because they can be part of diffirent ingress groups --- .../deployer/kubernetes/ingress.py | 74 +++++++++++-------- .../kubernetes/test_ingress_deploy.py | 25 +++++-- 2 files changed, 60 insertions(+), 39 deletions(-) diff --git a/fiaas_deploy_daemon/deployer/kubernetes/ingress.py b/fiaas_deploy_daemon/deployer/kubernetes/ingress.py index 1334dbc2..4c4568e1 100644 --- a/fiaas_deploy_daemon/deployer/kubernetes/ingress.py +++ b/fiaas_deploy_daemon/deployer/kubernetes/ingress.py @@ -107,22 +107,33 @@ def _get_issuer_type(self, host): return self._tls_issuer_type_default - def _set_ingresses_names(self, ingresses,name): + + + def _fix_ingresses(self, ingresses, default_ingress_items, name): current_name = name new_ingresses = [] + #Search in which ingress group we have default hosts + def _has_default_ingress_item(ingress): + for i in ingress.ingress_items: + if i in default_ingress_items: + return True + return False + for item in ingresses: - new_item =item._replace(name=current_name) + new_item = item._replace(name=current_name) + new_item = new_item._replace(default=_has_default_ingress_item(item)) new_ingresses.append(new_item) - current_name = "{}-{}".format(name,ingresses.index(item)+1) + current_name = "{}-{}".format(name, ingresses.index(item)+1) return new_ingresses - + def _group_ingresses(self, app_spec): ''' Group the ingresses so that those with annotations are individual, those that don't need tls are grouped together, and those using non-default TLS-issuers are separated by TLS-issuer type ''' explicit_host = _has_explicitly_set_host(app_spec.ingresses) ingress_items = [item._replace(host=self._apply_host_rewrite_rules(item.host)) for item in app_spec.ingresses if item.host] - ingress_items += self._expand_default_hosts(app_spec) + default_ingress_items = self._expand_default_hosts(app_spec) + ingress_items += default_ingress_items AnnotatedIngress = namedtuple("AnnotatedIngress", ["name", "ingress_items", "annotations", "explicit_host", "issuer_type", "default"]) @@ -136,45 +147,44 @@ def _group_ingresses(self, app_spec): issuer_type = self._get_issuer_type(ingress_item.host) if ingress_item.annotations: annotated_ingress = AnnotatedIngress(name="", ingress_items=[ingress_item], - annotations=ingress_item.annotations, - explicit_host=True, issuer_type=issuer_type, - default=False) + annotations=ingress_item.annotations, + explicit_host=True, issuer_type=issuer_type, + default=False) annotated_ingresses.append(annotated_ingress) elif self._ingress_tls._should_disable_ingress_tls([ingress_item.host]) is True: - #Group no tls hosts together + # Group no tls hosts together notls_ingress = notls_ingresses.setdefault("no_tls", - AnnotatedIngress(name="", - ingress_items=[], - annotations={}, - explicit_host=explicit_host, - issuer_type=issuer_type, - default=False)) + AnnotatedIngress(name="", + ingress_items=[], + annotations={}, + explicit_host=explicit_host, + issuer_type=issuer_type, + default=False)) notls_ingress.ingress_items.append(ingress_item) elif issuer_type != self._tls_issuer_type_default: - #Group by issuer type + # Group by issuer type annotated_ingress = override_issuer_ingresses.setdefault(issuer_type, - AnnotatedIngress(name="", - ingress_items=[], - annotations={}, - explicit_host=explicit_host, - issuer_type=issuer_type, - default=False)) + AnnotatedIngress(name="", + ingress_items=[], + annotations={}, + explicit_host=explicit_host, + issuer_type=issuer_type, + default=False)) annotated_ingress.ingress_items.append(ingress_item) else: - #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) + # 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)) + AnnotatedIngress(name="", ingress_items=[], annotations={}, + explicit_host=explicit_host, issuer_type=self._tls_issuer_type_default, + default=True)) default_ingress.ingress_items.append(ingress_item) - ingresses.extend(i for i in default_ingresses.values()) ingresses.extend(annotated_ingresses) ingresses.extend(i for i in override_issuer_ingresses.values()) ingresses.extend(i for i in notls_ingresses.values()) - - ingresses = self._set_ingresses_names(ingresses,app_spec.name) + #Set the correct name for each group and set default=True for the groups that have atleast one default host + ingresses = self._fix_ingresses(ingresses, default_ingress_items, app_spec.name) return ingresses @@ -276,7 +286,7 @@ def __init__(self, config): self.enable_deprecated_tls_entry_per_host = config.enable_deprecated_tls_entry_per_host def apply(self, ingress, app_spec, hosts, issuer_type, use_suffixes=True): - if self._should_have_ingress_tls(app_spec,hosts): + if self._should_have_ingress_tls(app_spec, hosts): tls_annotations = {} if self._cert_issuer or app_spec.ingress_tls.certificate_issuer: issuer = app_spec.ingress_tls.certificate_issuer if app_spec.ingress_tls.certificate_issuer else self._cert_issuer @@ -318,11 +328,11 @@ def _should_have_ingress_tls(self, app_spec, hosts): return True elif self._should_disable_ingress_tls(hosts) is True: return False - else: + else: return self._use_ingress_tls == 'default_on' def _should_disable_ingress_tls(self, hosts): - #Check if any ingress host is part of atleast one domain suffix that shoudln't have tls + # Check if any ingress host is part of atleast one domain suffix that shoudln't have tls for suffix in self._tls_certificate_issuer_disable_for_domain_suffixes: for host in hosts: if host == suffix or host.endswith("." + suffix): diff --git a/tests/fiaas_deploy_daemon/deployer/kubernetes/test_ingress_deploy.py b/tests/fiaas_deploy_daemon/deployer/kubernetes/test_ingress_deploy.py index a70ca728..4a114045 100644 --- a/tests/fiaas_deploy_daemon/deployer/kubernetes/test_ingress_deploy.py +++ b/tests/fiaas_deploy_daemon/deployer/kubernetes/test_ingress_deploy.py @@ -703,14 +703,16 @@ def deployer_disable_tls_for_domain_suffixes(self, config, ingress_tls_disable_t #Ingress Deployer #list of ingresses # config - @pytest.mark.parametrize("ingress_tls_disable_tls_for_domain_suffixes, ingresses_spec, expected_host_groups", [ + @pytest.mark.parametrize("ingress_tls_disable_tls_for_domain_suffixes, ingresses_spec, expected_host_groups, hosts,use_suffixes", [ #Disable tls for one default host ({"tls_certificate_issuer_disable_for_domain_suffixes": ["xip.io"]}, [], [ ["testapp.127.0.0.1.xip.io"], #disable tls ["testapp.svc.test.example.com"], #enable tls - ]), + ], + ["testapp.127.0.0.1.xip.io"], + True), #Disable tls for a host with annotations ({"tls_certificate_issuer_disable_for_domain_suffixes": ["foo.example.com"]}, [ @@ -725,7 +727,9 @@ def deployer_disable_tls_for_domain_suffixes(self, config, ingress_tls_disable_t ["ann.foo.example.com"], #annotation disable tls ["ann.sub.example.com"], #annotation enable tls ["testapp.127.0.0.1.xip.io","testapp.svc.test.example.com"], #enable tls - ]), + ], + ["ann.sub.example.com"], + False), #Disable tls for a host with tls issue override ({"tls_certificate_issuer_disable_for_domain_suffixes": ["bar.other.cloud.com"]}, [ @@ -738,7 +742,9 @@ def deployer_disable_tls_for_domain_suffixes(self, config, ingress_tls_disable_t ["bar.other.cloud.com"], #disable tls with issuer override ["foo.other.cloud.com"], #enable tls with issuer override ["testapp.127.0.0.1.xip.io","testapp.svc.test.example.com"], #enable tls - ]), + ], + ["bar.other.cloud.com"], + False), #All use cases ({"tls_certificate_issuer_disable_for_domain_suffixes": ["foo.example.com","xip.io"]}, [ @@ -764,13 +770,14 @@ def deployer_disable_tls_for_domain_suffixes(self, config, ingress_tls_disable_t ["bar.example.com", "foo.bar.example.com","testapp.svc.test.example.com"], #tls enabled ["foo.example.com","sub.foo.example.com","testapp.127.0.0.1.xip.io"], #tls disabled ["other.cloud.com"] #has tls issuer override - ] - ) + ], + ["foo.example.com","sub.foo.example.com","testapp.127.0.0.1.xip.io"], + True) ],indirect=['ingress_tls_disable_tls_for_domain_suffixes'] ) @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): + 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): with mock.patch("k8s.models.ingress.Ingress.get_or_create") as get_or_create: get_or_create.return_value = mock.create_autospec(Ingress, spec_set=True) if len(ingresses_spec): @@ -779,6 +786,10 @@ def test_applies_tls_certificate_issuer_disable_for_domain_suffixes(self, deploy with mock.patch.object(ingress_tls_disable_tls_for_domain_suffixes, "apply",spec_set=True): deployer_disable_tls_for_domain_suffixes.deploy(app_spec, LABELS) host_groups = [sorted(call.args[2]) for call in ingress_tls_disable_tls_for_domain_suffixes.apply.call_args_list] + tls_issuer=DEFAULT_TLS_ISSUER + if hosts[0] == "bar.other.cloud.com": + tls_issuer = "certmanager.k8s.io/issuer" + ingress_tls_disable_tls_for_domain_suffixes.apply.assert_called_with(TypeMatcher(Ingress), app_spec, hosts,tls_issuer, use_suffixes=use_suffixes) assert ingress_tls_disable_tls_for_domain_suffixes.apply.call_count == len(expected_host_groups) assert expected_host_groups == sorted(host_groups)