feat(ingress) Experimental Native Ingress #732

krisnova · 2017-02-09T16:20:41Z

Work for experimental native ingress. Run the documentation to see how to use the new feature.

Requires deis/controller#1243
Requires deis/builder#495
Requires deis/router#316

mboersma · 2017-02-15T21:53:37Z

.gitignore

@@ -3,6 +3,7 @@
 # Packages
 *.egg
 *.egg-info
+.idea*


I'm a broken record, sorry, but ideally this would be in a separate PR.

mboersma · 2017-02-15T21:56:11Z

charts/workflow/values.yaml

+  # The public resolvable hostname to build your cluster with.
+  #
+  # This will be the hostname that is used to build endpoints such as "deis.$HOSTNAME"
+  hostname: ""


What happens if experimental_native_ingress: true, but the hostname field here wasn't updated? Ideally that would lead to a hard-stop error before helm actually installs the chart, but I don't know off-hand how to accomplish that.

This is analogous to the router chart's platform_domain setting, yes?

Can I propose that platform_domain is a better name than hostname? And to both 1. avoid confusion with the setting of the same name under router and 2. move this configuration closer to the point of use, this belongs under controller settings instead of global (because, afaik, the controller chart, which will have to honor this new setting won't be able to "look up" to the "parent" chart's global settings).

@mboersma As it stands we will get a regex error before we install the chart.

kris-nova:workflow kris$ helm install . --namespace deis --set global.experimental_native_ingress=true Error: release elegant-cardinal failed: Ingress.extensions "controller-api-server-ingress" is invalid: spec.rules[0].host: Invalid value: "deis.": must match the regex [a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)* (e.g. 'example.com') kris-nova:workflow kris$

@krancour thanks for the suggestions, I changed hostname to platform_domain and pulled it into the controller and out of global.

krisnova · 2017-03-06T16:44:16Z

Example command for reference:

helm install . --namespace deis --set global.experimental_native_ingress=true,controller.platform_domain=fabulous.af

Adding documentation changes for Kubernetes ingress support. Non breaking change, as users must opt-in to the feature.

Remove extra spaces to clean up deploy-an-app.md Non breaking change.

A few semantic changes, and supporting enabled/disable in requirements.yaml Non breaking change.

We changed the helm charts for this, just updating docs to match Non breaking change.

mboersma · 2017-03-09T16:27:17Z

charts/workflow/requirements.yaml

@@ -45,6 +45,7 @@ dependencies:
    version: <registry-token-refresher-tag>
    repository: https://charts.deis.com/registry-token-refresher
  - name: router
+    condition: !global.experimental_native_engress


Typo: "engress"

mboersma · 2017-03-09T16:36:59Z

charts/workflow/values.yaml

@@ -51,6 +51,12 @@ global:
  host_port: 5555
  # Prefix for the imagepull secret created when using private registry
  secret_prefix: "private-registry"
+  # Experimental feature to toggle using kubernetes ingress instead of the Deis router.


Although we always used to, we should avoid using "Deis" as shorthand for "Deis Workflow" these days. Maybe reword this section like this?

# Experimental feature to use Kubernetes ingress instead of Workflow's deis-router. # # Valid values are: # - true: deis-router will not be deployed. Workflow will not be usable until a Kubernetes ingress controller is installed. # - false: deis-router will be deployed (default).

indeed, it confuses people

mboersma · 2017-03-09T16:49:03Z

charts/workflow/values.yaml

@@ -107,6 +113,10 @@ controller:
  # disabled - turns off open registration
  # admin_only - allows for registration by an admin only.
  registration_mode: "admin_only"
+  # The public resolvable hostname to build your cluster with.


I would say "publicly resolvable here" or "resolvable public hostname" here.

Even with the following line clarifying it, I somehow still feel thrown off by the phrase "to build your cluster with."

Can we maybe replace this and the following sentence with something like:

A resolvable domain or subdomain name. All applications created by the controller will, by default, be subdomains of this. When experimental native ingress is enabled, these subdomains are used in defining host-based routing rules. If, for instance, platform_domain were "example.com", given a application "foobar", the controller will create a rule to route foo.example.com to the appropriate backend.

^ And that's totally imperfect. Feel free to iterate on that if you can improve it further.

Changed to "publicly resolvable" to keep it simple

If we want to elaborate more on this later we can.

mboersma · 2017-03-09T16:56:48Z

src/installing-workflow/index.md

@@ -33,6 +33,10 @@ More rigorous installations would benefit from using outside sources for the fol
 * [Redis](../managing-workflow/platform-logging.md#configuring-off-cluster-redis) - Such as AWS Elasticache
 * [InfluxDB](../managing-workflow/platform-monitoring.md#configuring-off-cluster-influxdb) and [Grafana](../managing-workflow/platform-monitoring.md#off-cluster-grafana)

+#### (Experimental) Kubernetes Native Ingress
+
+Workflow now offers [experimental native ingress](experimental-native-ingress.md) that will allow users to take advantage of native Kubernetes ingress with their cluster. Users will be able to use and define any compatible Kubernetes ingress controller. Feel free to start following along with the [experimental native ingress](experimental-native-ingress.md) guide now.


Maybe make this paragraph a bit more direct?

Workflow now offers experimental native ingress to take advantage of native Kubernetes routing. Any compatible Kubernetes ingress controller can be used in place of Workflow's nginx-based deis-router. Follow this guide to enable experimental native ingress.

mboersma · 2017-03-09T16:57:28Z

src/quickstart/deploy-an-app.md

@@ -1,26 +1,44 @@
-## Register an Admin User
+## Determine your host and hostname values


For consistency, I think we capitalize all the words here except for "and."

mboersma · 2017-03-09T17:08:10Z

src/installing-workflow/experimental-native-ingress.md

+$ helm install deis/workflow --namespace deis --set global.experimental_native_ingress=true,controller.platform_domain=deis.com
+```
+
+Where `global.hostname` is a **required** parameter that is traditionally not required for Workflow. In this example we are using `deis.com` for `$hostname`.


Are we using global.hostname or controller.platform_domain? The example disagrees with this sentence.

controller.platform_domain. It was changed elsewhere but not here.

mboersma · 2017-03-09T17:08:49Z

src/installing-workflow/experimental-native-ingress.md

+Wait for the pods that Helm launched to be ready. Monitor their status by running:
+
+```
+$ kubectl --namespace=deis get pods


Kubernetes will have the pod information stored very quickly, and I think adding a -w adds an implicit requirement on noting ^C and that sounds like adding a lot for a small gain.

mboersma · 2017-03-09T17:09:18Z

src/installing-workflow/experimental-native-ingress.md

+$ kubectl --namespace=deis get pods
+```
+
+You should also notice that a Kubernetes ingress has been installed on your cluster. You can view it by running:


"a Kubernetes ingress controller has been installed"? Or is just "ingress" the right terminology here?

"ingress" is the proper terminology here, as @kris-nova is referring to ingress resources that are part of the chart and define routing rules for the controller, grafana, etc.

That being said, "a Kubernetes ingress," might need to change because I do think there's more than one installed as part of the platform itself.

mboersma · 2017-03-09T17:09:56Z

src/installing-workflow/experimental-native-ingress.md

+
+## Install a Kubernetes Ingress Controller
+
+Now that Workflow has been deployed with the `global.exerpimental_native_ingress` flag set to `true`, we will need a Kubernetes ingress controller in place to begin routing traffic.


Typo: "exerpimental"

mboersma · 2017-03-09T17:11:35Z

src/installing-workflow/experimental-native-ingress.md

+
+Once all of the pods are in the `READY` state, and `deis.$host` resolves to the external IP found above Workflow is up an running!
+
+After installing Workflow, [register a user and deploy an application](../quickstart/deploy-an-app.md).


Since it's experimental, should we provide a link in the docs to github issue reporting? Or encourage users to share their stories of success with different ingress controllers by submitting docs PRs as HOWTOs?

Not necessary, just thinking...

s/engress/ingress Breaking change (as the feature wasn't working before the typo fix)

rimusz · 2017-03-09T17:39:04Z

What is missing I see is the ingress rules annotation e.g. kubernetes.io/ingress.class: "traefik" (or whatever ingress controller is used there) setup option in controller/apps ingress rules. Without it the traffic will not pass ingress controller but each rule will get the loadbalancer created instead.

The case above does not apply for AWS, but for GKE/GCP is needed, as otherwise the GKE/GCP controller will fight the custom controller for the Ingress.

krancour · 2017-03-10T00:02:15Z

@rimusz @kris-nova

What is missing I see is the ingress rules annotation e.g. kubernetes.io/ingress.class: "traefik" setup option in controller/apps ingress rules.

Well... as much as I love it. Let's not lock ourselves into Traefik. More on that in a second.

Without it the traffic will not pass ingress controller but each rule will get the loadbalancer created instead.

Yes... because that's how the L7 ingress controller (which is a default in GKE) behaves. The fact that it works by creating load balancers isn't wrong. It's merely an implementation detail, although it might not be one you care for. (I don't.) As you've alluded to, various annotations are typically used as signals to different ingress controllers that they either should or should not act on any given ingress. The precise annotations used seem to vary from one ingress controller to the next.

@kris-nova buried somewhere in the Google doc I'd written up about this, this detail was discussed. I probably should have called more attention to it. What we will need to do is allow, within this chart, for users to specify annotations as k/v pairs in their values.yaml. Those will have to be passed to the controller either through an env var or configmap and the controller will have to ensure the indicated annotations are added to each ingress it creates.

(If that's already present, I apologize. I am just now starting to review in earnest, and I am taking a guess that @rimusz wouldn't have overlooked this if it were already there.)

The case above does not apply for AWS, but for GKE/GCP is needed, as otherwise the GKE/GCP controller will fight the custom controller for the Ingress.

Right. Because no solution (that I am familiar with) for provisioning k8s on AWS includes a default ingress controller of any kind. So in practical terms, this only (currently) is a problem in GKE, but it can become a problem anywhere if someone has (for whatever reasons) installed more than one ingress controller.

krancour · 2017-03-10T00:05:35Z

charts/workflow/requirements.yaml

@@ -45,6 +45,7 @@ dependencies:
    version: <registry-token-refresher-tag>
    repository: https://charts.deis.com/registry-token-refresher
  - name: router
+    condition: !global.experimental_native_ingress


Does the ! operator actually work here? I was hoping it would, but I wasn't sure and hadn't had a chance to verify yet.

But if it works... this is exactly what I had in mind for how the router chart, in its entirety, is conditionally included or excluded. I ❤️ this.

So I decided to take this functionality out for this PR.. seems out of scope, and honestly if it's critical that we move our abstraction out to another abstracted place of abstraction we can PR that later.

Also this is blocked on a helm issue: helm/helm#2111

So the plan is to include the router even if the user opts in to native ingress?

No, we wouldn't include deis-router in that case. The debate is whether to use Helm 2.2.x's tags & conditions feature as the mechanism to disable deis-router, or whether to use our convention of big {{ if }} blocks which is more backward-compatible and consistent with other components.

I want to argue that we turn off deis-router "the old way," then make a separate effort to refactor all sub-charts to use the new Helm functionality. But it may not be that simple; see #771.

@mboersma I don't like the "big {{ if }}" approach because it requires changes to the router chart that endow that chart with an awareness that some parent chart might include it but wish for it to do nothing. That doesn't seem very clean to me.

It's not very clean, agreed. The advantages are:

works with older versions of helm

matches the (ugly) way we do things in other charts

But I was off-base in suggesting we could fix everything in a unified way: it may not be feasible to convert other sub-charts to use tags & conditions in a perfectly backward-compatible fashion. So this should be an isolated decision. I know you feel strongly about it @krancour and my main concern at this point is that @kris-nova doesn't get whiplash if we want to revert the deis-router changes. 😄

krancour · 2017-03-10T00:28:29Z

src/installing-workflow/experimental-native-ingress.md

@@ -0,0 +1,80 @@
+# Experimental Native Ingress


I'm wondering if this page repeats too much documentation from the "normal" doc for installing without this experimental feature. Do we want to consider a lesser page that just highlights what's different when you install with this feature flag set? I really don't know what's best here. I'm just asking the question.

So I already considered hooking into the existing page, I think it makes sense to have a clear change in flow for an entire section, than to try to add a bunch of conditional logic to an existing flow. I visualize the flows like this:

Separate and clear

---- ----- < > ------ ----

Together and conditional

---- <>-<>-<>-<> ----

Does "separate and clear" become a maintenance burden if / as additional flags for major / experimental features get added. I'm afraid of committing to the notion of a page per permutation.

I think the wise thing to do here is to let problems present themselves instead of trying to be too defensive and predict the future. If it becomes a problem, we can always change. The concern seems trivial to be honest, and this an experimental feature after all, shouldn't we be experimenting to see what works best?

I don't think it's a trivial concern. We generally accept the practice of avoiding DRY exceptions in code. imo, docs shouldn't be any different. In either case, a DRY exception represents an on-going maintenance burden.

Though I don't think it's a trivial concern, I'm not going to belabor it either. You're doing the legwork on this, so I defer to your judgement.

krancour · 2017-03-10T00:29:59Z

src/installing-workflow/experimental-native-ingress.md

+
+The experimental ingress feature requires a user to set up a hostname, and assumes the `deis.$host` convention.
+
+We need to point the `deis.$host` record to the public IP address of your ingress controller. You can get the public IP using the following command.


Shouldn't it actually be *.$host that resolves to their ingress controller?

Go back to working if statements, until we can resolve helm/helm#2111 Non breaking change

Changing documentation per the code review, mostly typos and wordsmithing. Non breaking change

Fixing typo in `experimental` Non breaking change

bacongobbler · 2017-03-16T16:09:51Z

src/installing-workflow/experimental-native-ingress.md

+$ helm install deis/workflow --namespace deis --set global.experimental_native_ingress=true,controller.platform_domain=deis.com
+```
+
+Where `controller.platform_domain` is a **required** parameter that is traditionally not required for Workflow. In this example we are using `deis.com` for `$hostname`.


Should we make a prelude here saying that this is documented further below in "Configuring DNS"?

bacongobbler · 2017-03-16T16:10:18Z

src/installing-workflow/experimental-native-ingress.md

+
+Where `controller.platform_domain` is a **required** parameter that is traditionally not required for Workflow. In this example we are using `deis.com` for `$hostname`.
+
+


there's a few extra unneeded newlines here inbetween paragraphs. Mind removing them?

bacongobbler · 2017-03-16T16:11:00Z

src/installing-workflow/experimental-native-ingress.md

+```
+$ kubectl get ingress --namespace deis
+```
+


extra newline here

bacongobbler · 2017-03-16T16:11:26Z

src/installing-workflow/experimental-native-ingress.md

+installation: if a component's dependencies are not yet available, that component will exit and Kubernetes will
+automatically restart it.
+
+Here, it can be seen that the controller, builder and registry all took a few loops before they were able to start:


took a few loops waiting for minio*

extra context is good :)

bacongobbler · 2017-03-16T16:15:49Z

src/installing-workflow/index.md

@@ -33,6 +33,9 @@ More rigorous installations would benefit from using outside sources for the fol
 * [Redis](../managing-workflow/platform-logging.md#configuring-off-cluster-redis) - Such as AWS Elasticache
 * [InfluxDB](../managing-workflow/platform-monitoring.md#configuring-off-cluster-influxdb) and [Grafana](../managing-workflow/platform-monitoring.md#off-cluster-grafana)

+#### (Experimental) Kubernetes Native Ingress
+
+Workflow now offers [experimental native ingress](experimental-native-ingress.md) to take advantage of native Kubernetes routing. Any compatible Kubernetes ingress controller can be used in place of Workflow's nginx-based deis-router. Follow [this guide](experimental-native-ingress.md) to enable experimental native ingress.
 ## Add the Deis Chart Repository


Can you add an extra newline here between the paragraph and the new heading? Just a small pet peeve of mine :)

Fixing extraneous new lines, and wordsmithing. Non breaking change

krisnova · 2017-03-16T19:42:05Z

@bacongobbler thanks for the review, your concerns have been addressed.

bacongobbler · 2017-03-16T19:44:10Z

As far as I'm concerned this all looks good to me.

krisnova · 2017-03-21T18:30:03Z

Wooo! Thanks @mboersma and @bacongobbler for the approval here!

I think we should wait for the other PRs to fall into alignment before merging. Particularly deis/controller#1243

Adding ingress support to controller Adding changes to chart to support feature Adding ingress rules for controller API, and SSH on TCP 2222 Adding ingress support in general Requires deis/workflow#732 Requires deis/builder#495 Requires deis/router#316 Technically a non breaking change, as the user must opt-in to the feature

mboersma · 2017-03-30T20:51:27Z

Replaced by #782.

deis-admin added the in progress label Feb 9, 2017

krisnova mentioned this pull request Feb 9, 2017

feat(ingress) Experimental Native Ingress deis/controller#1243

Closed

krisnova changed the title ~~[WIP] - Custom Ingress Controller~~ [WIP] - Experimental Native Ingress Feb 14, 2017

krisnova mentioned this pull request Feb 14, 2017

feat(ingress) Experimental Native Ingress deis/router#316

Merged

mboersma reviewed Feb 15, 2017

View reviewed changes

krisnova force-pushed the byo-ingress branch from 33d28a4 to e2f3c55 Compare March 2, 2017 21:20

krisnova changed the title ~~[WIP] - Experimental Native Ingress~~ Experimental Native Ingress Mar 2, 2017

krisnova changed the title ~~Experimental Native Ingress~~ feat(ingress): Experimental Native Ingress Mar 8, 2017

krisnova changed the title ~~feat(ingress): Experimental Native Ingress~~ feat(ingress) Experimental Native Ingress Mar 8, 2017

krisnova added 3 commits March 8, 2017 07:54

feat(ingress): Experimental Native Ingress

430b391

Adding documentation changes for Kubernetes ingress support. Non breaking change, as users must opt-in to the feature.

chore(ingress): Remove extra spaces

81c9f2c

Remove extra spaces to clean up deploy-an-app.md Non breaking change.

feat(ingress): Support enable/disable conditions in Workflow chart

9ed34ea

A few semantic changes, and supporting enabled/disable in requirements.yaml Non breaking change.

krisnova force-pushed the byo-ingress branch from dd1ce52 to 9ed34ea Compare March 8, 2017 14:55

krisnova mentioned this pull request Mar 8, 2017

Proposal: Allow opting out of the router (user brings their own ingress controller instead) #584

Closed

chore(ingress): Fix example usage in docs to match new YAML

62d8684

We changed the helm charts for this, just updating docs to match Non breaking change.

rimusz mentioned this pull request Mar 8, 2017

Native ingress/router hybrid mode option #762

Open

mboersma suggested changes Mar 9, 2017

View reviewed changes

chore(ingress): Fix typo

2f585c1

s/engress/ingress Breaking change (as the feature wasn't working before the typo fix)

mboersma added this to the v2.13 milestone Mar 9, 2017

mboersma added awaiting review enhancement and removed in progress labels Mar 9, 2017

mboersma assigned krisnova Mar 9, 2017

krancour reviewed Mar 10, 2017

View reviewed changes

krisnova added 3 commits March 14, 2017 10:07

chore(ingress): Revert to using if statements in router

d8b6f8c

Go back to working if statements, until we can resolve helm/helm#2111 Non breaking change

chore(ingress): Documentation updates from code review

8f3b769

Changing documentation per the code review, mostly typos and wordsmithing. Non breaking change

chore(ingress): Typo in experimental

e8c4692

Fixing typo in `experimental` Non breaking change

krisnova mentioned this pull request Mar 14, 2017

feat(ingress) Experimental Native Ingress deis/builder#495

Merged

bacongobbler reviewed Mar 16, 2017

View reviewed changes

fix(ingress): Documentation touch ups

69c5dad

Fixing extraneous new lines, and wordsmithing. Non breaking change

bacongobbler approved these changes Mar 16, 2017

View reviewed changes

bacongobbler added the LGTM1 label Mar 16, 2017

mboersma added LGTM2 and removed awaiting review labels Mar 21, 2017

mboersma approved these changes Mar 21, 2017

View reviewed changes

bacongobbler mentioned this pull request Mar 28, 2017

Proposal: make Let's Encrypt a first-class entity in Workflow #708

Open

mboersma mentioned this pull request Mar 30, 2017

feat(ingress): experimental native ingress #782

Merged

mboersma mentioned this pull request Mar 30, 2017

feat(ingress): experimental native ingress deis/controller#1274

Merged

mboersma closed this Mar 30, 2017

mariusmarais mentioned this pull request Jun 1, 2017

Experimental Native Ingress not supporting deis domain #816

Closed

		@@ -1,26 +1,44 @@
		## Register an Admin User
		## Determine your host and hostname values


		## Install a Kubernetes Ingress Controller

		Now that Workflow has been deployed with the `global.exerpimental_native_ingress` flag set to `true`, we will need a Kubernetes ingress controller in place to begin routing traffic.


		Once all of the pods are in the `READY` state, and `deis.$host` resolves to the external IP found above Workflow is up an running!

		After installing Workflow, [register a user and deploy an application](../quickstart/deploy-an-app.md).


		The experimental ingress feature requires a user to set up a hostname, and assumes the `deis.$host` convention.

		We need to point the `deis.$host` record to the public IP address of your ingress controller. You can get the public IP using the following command.


		Where `controller.platform_domain` is a required parameter that is traditionally not required for Workflow. In this example we are using `deis.com` for `$hostname`.

feat(ingress) Experimental Native Ingress #732

feat(ingress) Experimental Native Ingress #732

Conversation

krisnova commented Feb 9, 2017 • edited Loading

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

krisnova commented Mar 6, 2017 • edited Loading

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

krancour Mar 10, 2017 • edited Loading

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

mboersma Mar 9, 2017 • edited Loading

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

rimusz commented Mar 9, 2017 • edited Loading

krancour commented Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

krancour Mar 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mboersma Mar 16, 2017 • edited Loading

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

krisnova Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Separate and clear

Together and conditional

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

bacongobbler Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krisnova commented Mar 16, 2017

bacongobbler commented Mar 16, 2017 • edited Loading

krisnova commented Mar 21, 2017

mboersma commented Mar 30, 2017 • edited Loading

krisnova commented Feb 9, 2017 •

edited

Loading

krisnova commented Mar 6, 2017 •

edited

Loading

krancour Mar 10, 2017 •

edited

Loading

mboersma Mar 9, 2017 •

edited

Loading

rimusz commented Mar 9, 2017 •

edited

Loading

krancour commented Mar 10, 2017 •

edited

Loading

krancour Mar 10, 2017 •

edited

Loading

mboersma Mar 16, 2017 •

edited

Loading

krisnova Mar 14, 2017 •

edited

Loading

bacongobbler Mar 16, 2017 •

edited

Loading

bacongobbler commented Mar 16, 2017 •

edited

Loading

mboersma commented Mar 30, 2017 •

edited

Loading