Support deploying multiple ingresses #98
Conversation
|
This will need fiaas/k8s#76 |
| def _delete_unused(self, labels): | ||
| filter_labels = { | ||
| "app": Equality(labels["app"]), | ||
| "fiaas/deployment_id": Inequality(labels["fiaas/deployment_id"]) |
There was a problem hiding this comment.
I think this need another filter: "fiaas/deployment_id": Exists()
Otherwise, it will match all ingresses from the app that does not have a fiaas/deployment_id (e.g. custom ingresses for that app created off-band). This feature should make these extra ingresses not necessary, but still seems like better fall on the safe side?
There was a problem hiding this comment.
Ah, true. But passing a map with duplicate keys isn't possible :/
There was a problem hiding this comment.
Maybe like this? Not sure if it's the tidiest API: fiaas/k8s#80
There was a problem hiding this comment.
I think that solution is good. I thought about a map where the values may be lists, but I think the list of tuples better represents what the k8s API is.
gregjones
force-pushed
the
ingresseseseses
branch
from
9532889 to
bab164a
Compare
gregjones
force-pushed
the
ingresseseseses
branch
2 times, most recently
from
8ed4bf1 to
4498364
Compare
This adds support for deploying multiple ingresses with FIAAS, by separating the ingresses specified in the config by their distinct requirements for annotations. This is useful as it means for ingress controllers that use annotations for configuration - this can be determining which controller is used (public/private network, or configuration that affects behaviour (whitelisting, HTTP-auth) per host/path.
gregjones
force-pushed
the
ingresseseseses
branch
from
4498364 to
7040674
Compare
| unannotated_ingress = AnnotatedIngress(name=app_spec.name, ingress_items=[], annotations={}) | ||
| ingresses_by_annotations = [unannotated_ingress] | ||
| for ingress_item in app_spec.ingresses: | ||
| LOG.info(ingress_item) |
There was a problem hiding this comment.
It was your intention to only log the item here?
There was a problem hiding this comment.
Ah, no. Left over from debugging: removed in 98bd437
There was a problem hiding this comment.
Maybe link to the initial issue as there were some design notes that may be interesting to keep?
Other than that, kudos for making the change backwards compatible and easy to understand. I really thought at the beginning this would be a major change, but it's not.
🏆
Add documentation for new feature
|
I added some docs too for this, PTAL |
| 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"]) |
There was a problem hiding this comment.
Maybe extracting the grouping logic to a separate function would make this section read a little better?
| # Group app_spec.ingresses to separate those with annotations | ||
| AnnotatedIngress = namedtuple("AnnotatedIngress", ["name", "ingress_items", "annotations"]) | ||
| unannotated_ingress = AnnotatedIngress(name=app_spec.name, ingress_items=[], annotations={}) | ||
| ingresses_by_annotations = [unannotated_ingress] |
There was a problem hiding this comment.
nit :The naming here confused me a bit; maybe something like all_ingresses or just ingresses would read better?
| - path: /foo | ||
| annotations: | ||
| some/annotation: bar | ||
| ``` |
There was a problem hiding this comment.
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?
This adds support for deploying multiple ingresses with FIAAS, by
separating the ingresses specified in the config by their distinct
requirements for annotations. This is useful as it means for ingress
controllers that use annotations for configuration - this can be
determining which controller is used (public/private network,
or configuration that affects behaviour (whitelisting, HTTP-auth)
per host/path.
This closes #89