feat(ingress) Experimental Native Ingress #316

krisnova · 2017-02-14T21:58:44Z

Preliminary work for disabling the router chart when using EXPERIMENTAL_NATIVE_INGRESS

Note: this will probably be refactored once we figure out #771

Requires deis/builder#495
Requires deis/workflow#732
Requires deis/controller#1243

codecov-io · 2017-02-14T22:20:29Z

Codecov Report

Merging #316 into master will increase coverage by 3.78%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #316      +/-   ##
==========================================
+ Coverage   55.69%   59.48%   +3.78%     
==========================================
  Files           6        6              
  Lines         386      543     +157     
==========================================
+ Hits          215      323     +108     
- Misses        151      189      +38     
- Partials       20       31      +11

Impacted Files	Coverage Δ
model/model.go	`54.16% <0%> (+7.82%)`	✅
nginx/config.go	`64.93% <0%> (+8.07%)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d6f130...018e469. Read the comment docs.

krancour · 2017-02-15T21:27:24Z

@kris-nova, I think I see a problem here...

.Values.global.experimental_native_ingress looks like a value from the "parent" Workflow chart, which merely depends upon this chart. I don't think this chart will ever see that value (i.e., it can't look "up"), which can certainly complicate the notion of conditionally installing or not installing the resources defined in this chart based on some config option in the parent chart.

Thinking through this more, I suspect what we really need to do is not touch the router chart at all, but conditionally include it or not include it in the Workflow chart. I'm wondering if that can actually be done... going to consult with the Helm team a bit.

mboersma · 2017-02-15T21:35:55Z

.gitignore

@@ -4,3 +4,4 @@ rootfs/opt/router/sbin/router
 rootfs/opt/router/sbin/router.*
 vendor/
 coverage.txt
+.idea*


Hiding files under .idea isn't strictly related to the ingress changes, so ideally this would be in a separate PR.

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

charts/router/templates/router-deployment.yaml

@@ -76,3 +77,4 @@ spec:
            port: 9090
          initialDelaySeconds: 1
          timeoutSeconds: 1
+{{ end }}


These files should end with a newline to satisfy some tools.

Would it improve readability to finish with a comment like:

{{ end }}{{/* if not .Values.global.experimental_native_ingress */}}

I'm just asking--not sure myself.

Added, also added a newline back to the end.. dang emacs 😝

Adding logic to disable router Non breaking change, as users must opt-in to the feature.

Add newline and comment for huge if statmenet Non breaking change, just semantic

krancour · 2017-03-06T19:53:21Z

.gitignore

@@ -1,4 +1,4 @@
 rootfs/opt/router/sbin/router
 rootfs/opt/router/sbin/router.*
 vendor/
-coverage.txt
+coverage.txt


Looks like this removes the trailing newline in this file. Can we leave that be?

Remove trailing newline Non breaking change, just semantic

krancour · 2017-03-06T19:56:46Z

charts/router/templates/router-deployment.yaml

@@ -1,3 +1,4 @@
+{{- if not .Values.global.experimental_native_ingress }}


I think I may have said something to this effect once before, but I don't think this works... .Values.global looks like a reference to the "top level" configuration in the Workflow chart. This chart can't "look up" into there because... well... imagine the scenario where this chart is installed standalone (i.e. without the rest of Workflow)... there wouldn't even be anything to look up to.

I think this repo probably does not require any changes to support the native ingress initiative.

I added the global.experimental_native_engress value to the values.yaml for the router chart.. this should support a standalone installation of the router

Ah. I am sorry for a bit of mis-understanding here. I'd forgotten that global was a special scope within Helm that allowed charts to "look up." I've now realized that...

I understand what you're trying to do here. Regardless of whether someone installs the router chart directly or installs the Workflow chart, which includes the router chart, a value for global.experimental_native_ingress should be available.

Although I understand what you're trying to do and agree it would work, I think there are a couple smells here... and I think there's a much cleaner alternative.

The smells:

The router chart now has the ability, when installed, depending on the value of global.experimental_native_ingress to actually not install any of its k8s resources. i.e. It is possible to effect a "no-op" install of the router. (Install this chart-- but dont!) This doesn't make sense unless considered in the broader context of re-use by the Workflow chart. That means the router chart has been designed with that specific re-use by the Workflow chart in mind. And that's a smell. Because, what really, does "experimental native ingress" mean to the router itself? Nothing, in fact.

As I've said before, I think the router requires no changes to support this experiment. (Neither Go nor chart changes are in order.) All that needs to be done is for the Workflow chart to conditionally include or not include the router chart. Until re-reading the Helm documentation this evening, I'd hoped, but hadn't known for sure that was possible... it is...

This section of the documentation shows two techniques that could be used to conditionally include or exclude the router chart in its entirety from a Workflow installation:

https://github.com/kubernetes/helm/blob/master/docs/charts.md#tags-and-condition-fields-in-requirementsyaml

Point: @knova I think I misunderstood the context earlier. I see @krancour's point that this makes a change to the router's own chart that allows it to be disabled completely, which seems wrong in the context of the router chart standing alone. We do intend the individual component charts to be useful separate from Workflow itself if at all possible.

Counterpoint: changing to use the new tags and conditions in Helm requires Workflow to specify helm v2.2.1 or later, is my understanding. And other components such as minio are disabled in a similarly inelegant way already. It could be argued that using tags + conditions isn't in scope for this overall feature, and that we should instead make a coordinated effort to refactor all conditional components at once.

@krancour would you be amenable to allowing this smelly-but-consistent change into router if we follow up with a set of PRs that embrace Helm tags & conditions that remove the currently enclosing {{ if blocks from all relevant Workflow charts?

@krancour would you be amenable to allowing this smelly-but-consistent change into router if we follow up with...

Sorry, @mboersma.., late to the game on this, but yes. 👍

Thanks @krancour, no worries. We do intend to revert this change at some point in the future in favor of the cleaner tags & conditionals approach.

Sorry my code smells

Adding global.experimental_native_ingress flag to values.yaml so this can be ran standalone Non breaking change, just semantic

krisnova · 2017-03-21T17:17:17Z

@mboersma @bacongobbler does anyone else see anything that needs attention on this one?

bacongobbler · 2017-03-21T17:21:41Z

Nope! Looks like all of the templated objects from this chart is covered.

mboersma · 2017-03-21T18:11:45Z

With the caveat that we intend to refactor this eventually (see deis/workflow#771), I'm ok with this change.

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

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

This was referenced Feb 14, 2017

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

Closed

feat(ingress) Experimental Native Ingress deis/workflow#732

Closed

mboersma reviewed Feb 15, 2017

View reviewed changes

feat(ingress): Experimental Native Ingress

badc232

Adding logic to disable router Non breaking change, as users must opt-in to the feature.

krisnova force-pushed the byo-ingress branch from 84bcb72 to badc232 Compare March 2, 2017 21:25

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

krisnova added 2 commits March 6, 2017 10:30

feat(ingress): Experimental Native Ingress

9136b81

Add newline and comment for huge if statmenet Non breaking change, just semantic

chore(ingress): Experimental Native Ingress

a795d72

Add newline and comment for huge if statmenet Non breaking change, just semantic

krancour reviewed Mar 6, 2017

View reviewed changes

chore(ingress): Experimental Native Ingress

bece9d5

Remove trailing newline Non breaking change, just semantic

krancour reviewed Mar 6, 2017

View reviewed changes

feature(ingress): Experimental Native Ingress

018e469

Adding global.experimental_native_ingress flag to values.yaml so this can be ran standalone Non breaking change, just semantic

krisnova closed this Mar 8, 2017

deis-admin removed the in progress label Mar 8, 2017

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

krisnova reopened this Mar 14, 2017

deis-admin added the in progress label Mar 14, 2017

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

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

mboersma assigned krisnova Mar 15, 2017

mboersma added the enhancement label Mar 15, 2017

mboersma mentioned this pull request Mar 15, 2017

Refactor charts to use Helm Tags and Condition fields deis/workflow#771

Open

krisnova mentioned this pull request Mar 21, 2017

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

Merged

bacongobbler approved these changes Mar 21, 2017

View reviewed changes

bacongobbler added the LGTM1 label Mar 21, 2017

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

mboersma approved these changes Mar 21, 2017

View reviewed changes

mboersma mentioned this pull request Mar 30, 2017

feat(ingress): experimental native ingress deis/workflow#782

Merged

mboersma mentioned this pull request Mar 30, 2017

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

Merged

mboersma merged commit b8e207e into deis:master Mar 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ingress) Experimental Native Ingress #316

feat(ingress) Experimental Native Ingress #316

krisnova commented Feb 14, 2017 •

edited

Loading

codecov-io commented Feb 14, 2017 •

edited

Loading

krancour commented Feb 15, 2017

mboersma Feb 15, 2017

mboersma Feb 15, 2017

krisnova Mar 6, 2017

krancour Mar 6, 2017

krancour Mar 6, 2017

krisnova Mar 6, 2017

krancour Mar 7, 2017

mboersma Mar 15, 2017 •

edited

Loading

krancour Apr 3, 2017 •

edited

Loading

mboersma Apr 3, 2017

krisnova Apr 3, 2017

krisnova commented Mar 21, 2017

bacongobbler commented Mar 21, 2017

mboersma commented Mar 21, 2017

		@@ -1,3 +1,4 @@
		{{- if not .Values.global.experimental_native_ingress }}

feat(ingress) Experimental Native Ingress #316

feat(ingress) Experimental Native Ingress #316

Conversation

krisnova commented Feb 14, 2017 • edited Loading

codecov-io commented Feb 14, 2017 • edited Loading

Codecov Report

krancour commented Feb 15, 2017

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 15, 2017 • edited Loading

Choose a reason for hiding this comment

krancour Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krisnova commented Mar 21, 2017

bacongobbler commented Mar 21, 2017

mboersma commented Mar 21, 2017

krisnova commented Feb 14, 2017 •

edited

Loading

codecov-io commented Feb 14, 2017 •

edited

Loading

mboersma Mar 15, 2017 •

edited

Loading

krancour Apr 3, 2017 •

edited

Loading