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.

Sign up for GitHub

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

feat(ingress) Experimental Native Ingress #316

Merged
merged 5 commits into from
Mar 30, 2017
Merged

feat(ingress) Experimental Native Ingress #316

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
badc232
feat(ingress): Experimental Native Ingress
krisnova Mar 2, 2017
9136b81
feat(ingress): Experimental Native Ingress
krisnova Mar 6, 2017
a795d72
chore(ingress): Experimental Native Ingress
krisnova Mar 6, 2017
bece9d5
chore(ingress): Experimental Native Ingress
krisnova Mar 6, 2017
018e469
feature(ingress): Experimental Native Ingress
krisnova Mar 6, 2017
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
2 changes: 1 addition & 1 deletion .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
rootfs/opt/router/sbin/router
rootfs/opt/router/sbin/router.*
vendor/
coverage.txt
coverage.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

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

2 changes: 2 additions & 0 deletions charts/router/templates/router-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
{{- if not .Values.global.experimental_native_ingress }}
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

@mboersma mboersma Mar 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@krancour krancour Apr 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@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. 👍

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry my code smells

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
Expand Down Expand Up @@ -88,3 +89,4 @@ spec:
port: 9090
initialDelaySeconds: 1
timeoutSeconds: 1
{{ end }}{{/* if not .Values.global.experimental_native_ingress */}}
2 changes: 2 additions & 0 deletions charts/router/templates/router-dhparam-secret.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
{{- if not .Values.global.experimental_native_ingress }}
apiVersion: v1
kind: Secret
metadata:
Expand All @@ -7,3 +8,4 @@ metadata:
type: Opaque
data:
dhparam: {{ if not (empty .Values.dhparam) }}{{ .Values.dhparam | b64enc}}{{ else }}LS0tLS1CRUdJTiBESCBQQVJBTUVURVJTLS0tLS0KTUlJQkNBS0NBUUVBNGJPMlpCOTRYVVh3YjlSL0QxM0xFZ2d6V1U2Y0hZSXJzakdseVhUSkZHZmdsaFl5TUtnUQpKRXFsTjd5RWs0UUNVV25iTEtDU2F4anZCTVNqbWlTR3VvajNWRDlYRTVDaGx2SzlGQVp6alFGMFZ2VzdkNDJmCnNkNmlmcUYwSUV0R2lxWDRpTzhtWmhYZVJrc3BYekZXYkhyTklaanBCdmc5VDhmczY0ZCtDU2l2SXBqWWRVQ1UKQWRhNTViRDdLeWJkTWdJMFZza21jL3RpclN5SkIwZDVmUnYraG1vMW9pOG92Rm9vUFp2cS9HWGxPOHlRRzRZTgppWXdWYmU5c29wQ3lYNzEzaUtlM0pBQ29JWnUxZXgrRjVhbU53U3E3Rk9xb2JqK3Z2cURRM0Q2WEJaanczT3NuClFRWmVhTWYyMDVwOERWbFpWLzRqQUtQeEpUQkxyejh6c3dJQkFnPT0KLS0tLS1FTkQgREggUEFSQU1FVEVSUy0tLS0tCg=={{ end }}
{{ end }}{{/* if not .Values.global.experimental_native_ingress */}}
2 changes: 2 additions & 0 deletions charts/router/templates/router-service-account.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
{{- if not .Values.global.experimental_native_ingress }}
apiVersion: v1
kind: ServiceAccount
metadata:
name: deis-router
labels:
heritage: deis
{{ end }}{{/* if not .Values.global.experimental_native_ingress */}}
2 changes: 2 additions & 0 deletions charts/router/templates/router-service.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
{{- if not .Values.global.experimental_native_ingress }}
apiVersion: v1
kind: Service
metadata:
Expand Down Expand Up @@ -27,3 +28,4 @@ spec:
- name: healthz
port: 9090
targetPort: 9090
{{ end }}{{/* if not .Values.global.experimental_native_ingress */}}