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

Support deploying multiple ingresses #98

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 32 additions & 7 deletions docs/v3_spec.md
Original file line number Diff line number Diff line change
Expand Up @@ -152,21 +152,22 @@ ingress:
paths:
- path: /
port: http
annotations: {}
```

All applications will get a set of default hosts, if the cluster operator has defined ingress suffixes.
All applications will get a set of default hosts, if the cluster operator has defined ingress suffixes.
If you do not specify a host in your `ingress` configuration, these default hosts will be used.
For example :
For example :
1. `your-app.example1.com`
2. `your-app.example2.com`

When you expose a path on a host you get that one as well. For example :
When you expose a path on a host you get that one as well. For example :
```yaml
ingress:
- host: example.com
paths:
- path: /my-path
```
```

If you want to customize paths for default hosts as well, you can do it as :
```yaml
Expand All @@ -179,8 +180,8 @@ ingress:

```

This will make `/some-other-path` available on default hosts, but not on the host you provided in ingress.
Remember, default hosts will also contain the paths from the ingress.
This will make `/some-other-path` available on default hosts, but not on the host you provided in ingress.
Remember, default hosts will also contain the paths from the ingress.

### host

Expand All @@ -203,7 +204,7 @@ ingress:
- host: your-app.example.com
```

If the operator of your cluster has configured host-rewrite rules they will be applied to the hostname given in this
If the operator of your cluster has configured host-rewrite rules they will be applied to the hostname given in this
field. See [the operator guide](operator_guide.md#host-rewrite-rules) for details about how this feature works.

In typical clusters, this value should be the host used by your application in production, and host-rewrite rules should
Expand Down Expand Up @@ -234,6 +235,30 @@ your application. Requests to `your-app.example.com/metrics` will go to the port
be defined under the `ports` configuration structure. It is also possible to use a port number, but named ports are
strongly recommended.

### annotations

| **Type** | **Required** |
|----------|--------------|
| object | no |

A map of annotations to add to the ingress.

Each entry in the ingress list that contains a non-empty `annotations` value will cause a separate Ingress object to
be created during the deployment. All items that have an empty value will have their hosts/paths merged into a single
Ingress.

Example:
```yaml
ingress:
- host: your-app.example.com
paths:
- path: /foo
- host: other.example.com
paths:
- path: /foo
annotations:
some/annotation: bar
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design document has some nice examples of how this feature works; do you think it makes sense to add some more of these examples here too?


## healthchecks

Expand Down
90 changes: 68 additions & 22 deletions fiaas_deploy_daemon/deployer/kubernetes/ingress.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,14 @@
from itertools import chain

from k8s.client import NotFound
from k8s.base import Equality, Inequality, Exists
from k8s.models.common import ObjectMeta
from k8s.models.ingress import Ingress, IngressSpec, IngressRule, HTTPIngressRuleValue, HTTPIngressPath, IngressBackend, \
IngressTLS

from fiaas_deploy_daemon.retry import retry_on_upsert_conflict
from fiaas_deploy_daemon.tools import merge_dicts
from collections import namedtuple

LOG = logging.getLogger(__name__)

Expand All @@ -43,48 +45,88 @@ def deploy(self, app_spec, labels):
if self._should_have_ingress(app_spec):
self._create(app_spec, labels)
else:
self.delete(app_spec)
self._delete_unused(app_spec, labels)

def delete(self, app_spec):
LOG.info("Deleting ingress for %s", app_spec.name)
LOG.info("Deleting ingresses for %s", app_spec.name)
try:
Ingress.delete(app_spec.name, app_spec.namespace)
Ingress.delete_list(namespace=app_spec.namespace, labels={"app": Equality(app_spec.name), "fiaas/deployment_id": Exists()})
except NotFound:
pass

@retry_on_upsert_conflict
def _create(self, app_spec, labels):
LOG.info("Creating/updating ingress for %s", app_spec.name)
annotations = {
u"fiaas/expose": u"true" if _has_explicitly_set_host(app_spec) else u"false"
LOG.info("Creating/updating ingresses for %s", app_spec.name)
custom_labels = merge_dicts(app_spec.labels.ingress, labels)

# Group app_spec.ingresses to separate those with annotations
AnnotatedIngress = namedtuple("AnnotatedIngress", ["name", "ingress_items", "annotations"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe extracting the grouping logic to a separate function would make this section read a little better?

unannotated_ingress = AnnotatedIngress(name=app_spec.name, ingress_items=[], annotations={})
ingresses_by_annotations = [unannotated_ingress]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit :The naming here confused me a bit; maybe something like all_ingresses or just ingresses would read better?

for ingress_item in app_spec.ingresses:
if ingress_item.annotations:
next_name = "{}-{}".format(app_spec.name, len(ingresses_by_annotations))
annotated_ingresses = AnnotatedIngress(name=next_name, ingress_items=[ingress_item], annotations=ingress_item.annotations)
ingresses_by_annotations.append(annotated_ingresses)
else:
unannotated_ingress.ingress_items.append(ingress_item)

LOG.info("Will create %s ingresses", len(ingresses_by_annotations))
for annotated_ingress in ingresses_by_annotations:
if len(annotated_ingress.ingress_items) == 0:
LOG.info("No items, skipping: %s", annotated_ingress)
continue

self._create_ingress(app_spec, annotated_ingress, custom_labels)

self._delete_unused(app_spec, custom_labels)

@retry_on_upsert_conflict
def _create_ingress(self, app_spec, annotated_ingress, labels):
default_annotations = {
u"fiaas/expose": u"true" if _has_explicitly_set_host(annotated_ingress.ingress_items) else u"false"
}
annotations = merge_dicts(annotated_ingress.annotations, app_spec.annotations.ingress, default_annotations)

custom_labels = merge_dicts(app_spec.labels.ingress, labels)
custom_annotations = merge_dicts(app_spec.annotations.ingress, annotations)
metadata = ObjectMeta(name=app_spec.name, namespace=app_spec.namespace, labels=custom_labels,
annotations=custom_annotations)
metadata = ObjectMeta(name=annotated_ingress.name, namespace=app_spec.namespace, labels=labels,
annotations=annotations)

per_host_ingress_rules = [
IngressRule(host=self._apply_host_rewrite_rules(ingress_item.host),
http=self._make_http_ingress_rule_value(app_spec, ingress_item.pathmappings))
for ingress_item in app_spec.ingresses
for ingress_item in annotated_ingress.ingress_items
if ingress_item.host is not None
]
default_host_ingress_rules = self._create_default_host_ingress_rules(app_spec)
if annotated_ingress.annotations:
use_suffixes = False
host_ingress_rules = per_host_ingress_rules
else:
use_suffixes = True
host_ingress_rules = per_host_ingress_rules + self._create_default_host_ingress_rules(app_spec)

ingress_spec = IngressSpec(rules=per_host_ingress_rules + default_host_ingress_rules)
ingress_spec = IngressSpec(rules=host_ingress_rules)

ingress = Ingress.get_or_create(metadata=metadata, spec=ingress_spec)
self._ingress_tls.apply(ingress, app_spec, self._get_hosts(app_spec))

hosts_for_tls = [rule.host for rule in host_ingress_rules]
self._ingress_tls.apply(ingress, app_spec, hosts_for_tls, use_suffixes=use_suffixes)
self._owner_references.apply(ingress, app_spec)
ingress.save()

def _delete_unused(self, app_spec, labels):
filter_labels = [
("app", Equality(labels["app"])),
("fiaas/deployment_id", Exists()),
("fiaas/deployment_id", Inequality(labels["fiaas/deployment_id"]))
]
Ingress.delete_list(namespace=app_spec.namespace, labels=filter_labels)

def _generate_default_hosts(self, name):
for suffix in self._ingress_suffixes:
yield u"{}.{}".format(name, suffix)

def _create_default_host_ingress_rules(self, app_spec):
all_pathmappings = chain.from_iterable(ingress_item.pathmappings for ingress_item in app_spec.ingresses)
all_pathmappings = chain.from_iterable(ingress_item.pathmappings
for ingress_item in app_spec.ingresses if not ingress_item.annotations)
http_ingress_rule_value = self._make_http_ingress_rule_value(app_spec, all_pathmappings)
return [IngressRule(host=host, http=http_ingress_rule_value)
for host in self._generate_default_hosts(app_spec.name)]
Expand All @@ -99,7 +141,7 @@ def _should_have_ingress(self, app_spec):
return self._can_generate_host(app_spec) and _has_ingress(app_spec) and _has_http_port(app_spec)

def _can_generate_host(self, app_spec):
return len(self._ingress_suffixes) > 0 or _has_explicitly_set_host(app_spec)
return len(self._ingress_suffixes) > 0 or _has_explicitly_set_host(app_spec.ingresses)

@staticmethod
def _make_http_ingress_rule_value(app_spec, pathmappings):
Expand All @@ -115,8 +157,8 @@ def _get_hosts(self, app_spec):
for ingress_item in app_spec.ingresses if ingress_item.host is not None]


def _has_explicitly_set_host(app_spec):
return any(ingress.host is not None for ingress in app_spec.ingresses)
def _has_explicitly_set_host(ingress_items):
return any(ingress_item.host is not None for ingress_item in ingress_items)


def _has_http_port(app_spec):
Expand All @@ -142,7 +184,7 @@ def __init__(self, config):
self._shortest_suffix = sorted(config.ingress_suffixes, key=len)[0] if config.ingress_suffixes else None
self.enable_deprecated_tls_entry_per_host = config.enable_deprecated_tls_entry_per_host

def apply(self, ingress, app_spec, hosts):
def apply(self, ingress, app_spec, hosts, use_suffixes=True):
if self._should_have_ingress_tls(app_spec):
tls_annotations = {}
if self._cert_issuer or app_spec.ingress_tls.certificate_issuer:
Expand All @@ -162,8 +204,12 @@ def apply(self, ingress, app_spec, hosts):
else:
ingress.spec.tls = []

collapsed = self._collapse_hosts(app_spec, hosts)
ingress.spec.tls.append(IngressTLS(hosts=collapsed, secretName="{}-ingress-tls".format(app_spec.name)))
if use_suffixes:
# adding app-name to suffixes could result in a host too long to be the common-name of a cert, and
# as the user doesn't control it we should generate a host we know will fit
hosts = self._collapse_hosts(app_spec, hosts)

ingress.spec.tls.append(IngressTLS(hosts=hosts, secretName="{}-ingress-tls".format(ingress.metadata.name)))

def _collapse_hosts(self, app_spec, hosts):
"""The first hostname in the list will be used as Common Name in the certificate"""
Expand Down
1 change: 1 addition & 0 deletions fiaas_deploy_daemon/specs/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ def version(self):
IngressItemSpec = namedtuple("IngressItemSpec", [
"host",
"pathmappings",
"annotations"
])

IngressPathMappingSpec = namedtuple("IngressPathMappingSpec", [
Expand Down
1 change: 1 addition & 0 deletions fiaas_deploy_daemon/specs/v3/defaults.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ ingress: # Generate ingress rules for access from outside cluster. To disable, s
paths: # List of paths exposed to which application port
- path: / # Path the application answers on
port: http # Name of the port path is served on
annotations: {}
healthchecks: # Healthchecks defined for your application. If omitted and a single port is defined, liveness will default to http or tcp depending on port type, and readiness will be a copy of liveness. If no ports or multiple ports are defined, healthchecks are not provided and should be defined explicitly
liveness:
# Valid configuration requires exactly one of execute|http|tcp
Expand Down
6 changes: 3 additions & 3 deletions fiaas_deploy_daemon/specs/v3/factory.py
Original file line number Diff line number Diff line change
Expand Up @@ -207,15 +207,15 @@ def resolve_port_number(port):
else:
raise InvalidConfiguration("{} is not a valid port name or port number".format(port))

def ingress_item(host, paths):
def ingress_item(host, paths, annotations):
ingress_path_mapping_specs = [
IngressPathMappingSpec(path=pathmapping["path"], port=resolve_port_number(pathmapping["port"]))
for pathmapping in paths
]
return IngressItemSpec(host=host, pathmappings=ingress_path_mapping_specs)
return IngressItemSpec(host=host, pathmappings=ingress_path_mapping_specs, annotations=annotations)

if len(http_ports.items()) > 0:
return [ingress_item(host_path_mapping["host"], host_path_mapping["paths"])
return [ingress_item(host_path_mapping["host"], host_path_mapping["paths"], host_path_mapping["annotations"])
for host_path_mapping in ingress_lookup]
else:
return []
Expand Down
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ def read(filename):
"pinject == 0.14.1",
"six == 1.12.0",
"dnspython == 1.16.0",
"k8s == 0.14.0",
"k8s == 0.15.0",
"monotonic == 1.5",
"appdirs == 1.4.3",
"requests-toolbelt == 0.9.1",
Expand Down
4 changes: 2 additions & 2 deletions tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ def prometheus_registry():


@pytest.helpers.register
def assert_any_call(mockk, first, *args):
def assert_any_call(mockk, first, *args, **kwargs):
__tracebackhide__ = True

def _assertion():
mockk.assert_any_call(first, *args)
mockk.assert_any_call(first, *args, **kwargs)

_add_useful_error_message(_assertion, mockk, first, args)

Expand Down
2 changes: 1 addition & 1 deletion tests/fiaas_deploy_daemon/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ def app_spec():
deployment_id="test_app_deployment_id",
labels=LabelAndAnnotationSpec({}, {}, {}, {}, {}, {}),
annotations=LabelAndAnnotationSpec({}, {}, {}, {}, {}, {}),
ingresses=[IngressItemSpec(host=None, pathmappings=[IngressPathMappingSpec(path="/", port=80)])],
ingresses=[IngressItemSpec(host=None, pathmappings=[IngressPathMappingSpec(path="/", port=80)], annotations={})],
strongbox=StrongboxSpec(enabled=False, iam_role=None, aws_region="eu-west-1", groups=None),
singleton=False,
ingress_tls=IngressTlsSpec(enabled=False, certificate_issuer=None),
Expand Down
Loading