-
Notifications
You must be signed in to change notification settings - Fork 113
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
Initial implementation of Guestbook example #11
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
/bin/ | ||
/node_modules/ |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
name: guestbook | ||
runtime: nodejs | ||
description: Kubernetes Guestbook example based on https://kubernetes.io/docs/tutorials/stateless-application/guestbook/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,153 @@ | ||
// Copyright 2016-2018, Pulumi Corporation. All rights reserved. | ||
|
||
import * as pulumi from "pulumi"; | ||
import * as kubernetes from "@pulumi/kubernetes"; | ||
|
||
// REDIS MASTER | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of a single file, is it worth breaking this apart more similar to https://github.com/kubernetes/kubernetes/tree/master/examples/guestbook? Feels like this will make it a little easier to consume in addition to being more familiar to those who have seen that example code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW, there's a Ksonnet example here https://github.com/ksonnet/ksonnet/blob/master/examples/guestbook.jsonnet which is a single file. May be worth looking at that for inspiration for what "abstractions", if any, might make sense to introduce in the name of shrinking the example even further. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's only separate files in the default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree. I wrote this before looking at the Ksonnet example and I definitely like the idea of using abstractions to simplify to the point where it obviously belongs in one file 😄 |
||
|
||
let redisMasterLabels = { app: "redis", tier: "backend", role: "master"}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, variable reuse 😀 |
||
let redisMasterService = new kubernetes.Service("redis-master", { | ||
metadata: [{ | ||
name: "redis-master", | ||
labels: [redisMasterLabels], | ||
}], | ||
spec: [{ | ||
port: [{ port: 6379, targetPort: 6379 }], | ||
selector: [redisMasterLabels], | ||
}], | ||
}); | ||
let redisMasterDeployment = new kubernetes.Deployment("redis-master", { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding Ksonnet, it's interesting that they got the creation of services down to a single line, and it's done by the service borrowing fields from the deployment:
I assume that's just a simple helper library? Is there any way to get this cheaply "inline" in this file? |
||
metadata: [{ | ||
name: "redis-master", | ||
}], | ||
spec: [{ | ||
selector: [redisMasterLabels], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't see the selector in the YAML/Ksonnet example. Is this correct? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah sorry guess I totally missed that! |
||
replicas: 1, | ||
strategy: [], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this be omitted? (It isn't in the original YAML/Ksonnet examples.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes - it can - dropped. |
||
template: [{ | ||
metadata: [{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think the Kubernetes format actually uses an array for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes - I think I'm just going to go ahead and fix that and take the breaking change pain now. It has a particularly big impact on the Kubernetes provider - and I wouldn't want anyone to use this until we've fixed that (whereas it's rather rarely an issue in the AWS provider). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
labels: [redisMasterLabels], | ||
}], | ||
spec: [{ | ||
container: [{ | ||
name: "master", | ||
image: "k8s.gcr.io/redis:e2e", | ||
resources: [{ | ||
requests: [{ | ||
cpu: "100m", | ||
memory: "100Mi", | ||
}] | ||
}], | ||
port: [{ | ||
containerPort: 6379, | ||
}], | ||
}], | ||
}], | ||
}], | ||
}], | ||
}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Believe it or not, this is one area where YAML looks nicer, more concise! (Cascading Perhaps gets better with Python. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed - this is fairly ugly relative to YAML. It does have the benefit of type checking, intellisense, etc. in TypeScript - which is a slight win. I think we really need to find ways to (a) introduce some named types for common JSON sections and encourage reuse (b) create abstractions for common patterns. |
||
|
||
// REDIS SLAVE | ||
let redisSlaveLabels = { app: "redis", tier: "backend", role: "slave" }; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it worth making this an array? I.e., There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Once we fix pulumi/pulumi-terraform#37 the arrays will go away - that's why I did it this way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
let redisSlaveService = new kubernetes.Service("redis-slave", { | ||
metadata: [{ | ||
name: "redis-slave", | ||
labels: [redisSlaveLabels], | ||
}], | ||
spec: [{ | ||
port: [{ port: 6379, targetPort: 6379 }], | ||
selector: [redisSlaveLabels], | ||
}], | ||
}); | ||
let redisSlaveDeployment = new kubernetes.Deployment("redis-slave", { | ||
metadata: [{ | ||
name: "redis-slave", | ||
}], | ||
spec: [{ | ||
selector: [redisSlaveLabels], | ||
replicas: 1, | ||
strategy: [], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we omit? |
||
template: [{ | ||
metadata: [{ | ||
labels: [redisSlaveLabels], | ||
}], | ||
spec: [{ | ||
container: [{ | ||
name: "slave", | ||
image: "gcr.io/google_samples/gb-redisslave:v1", | ||
resources: [{ | ||
requests: [{ | ||
cpu: "100m", | ||
memory: "100Mi", | ||
}] | ||
}], | ||
env: [{ | ||
name: "GET_HOSTS_FROM", | ||
value: "dns", | ||
// If your cluster config does not include a dns service, then to instead access an environment | ||
// variable to find the master service's host, comment out the 'value: dns' line above, and | ||
// uncomment the line below: | ||
// value: env | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit, I think this should be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed. |
||
}], | ||
port: [{ | ||
containerPort: 6379, | ||
}], | ||
}], | ||
}], | ||
}], | ||
}], | ||
}); | ||
|
||
// FRONTEND | ||
let frontendLabels = { app: "guestbook", tier: "frontend" }; | ||
let frontendService = new kubernetes.Service("frontend", { | ||
metadata: [{ | ||
name: "frontend", | ||
labels: [frontendLabels], | ||
}], | ||
spec: [{ | ||
type: "NodePort", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems different from the example at https://github.com/kubernetes/kubernetes/blob/master/examples/guestbook/frontend-service.yaml, which says: spec:
# if your cluster supports it, uncomment the following to automatically create
# an external load-balanced IP for the frontend service.
# type: LoadBalancer
ports:
- port: 80
selector:
app: guestbook
tier: frontend Intentional? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The copy of this in the actual walkthrough uses Unfortunately, neither appears to work correctly on Docker for Mac Kubernetes install currently. There's lots of mentions of problems with this on the web - including this issue explicitly asking for documentation of what is actually supported: docker/docs#5690. I do want this example to ultimately be able to provide an output property with a URL you can open in the browser to try the app. It seems that may not be possible with deployments to Docker for Mac today. And cannot in general be done in a a single consistent way across providers with Kubernetes. 😢 |
||
port: [{ port: 80 }], | ||
selector: [frontendLabels], | ||
}], | ||
}); | ||
let frontendDeployment = new kubernetes.Deployment("frontend", { | ||
metadata: [{ | ||
name: "frontend", | ||
}], | ||
spec: [{ | ||
selector: [frontendLabels], | ||
replicas: 3, | ||
strategy: [], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we omit? |
||
template: [{ | ||
metadata: [{ | ||
labels: [frontendLabels], | ||
}], | ||
spec: [{ | ||
container: [{ | ||
name: "php-redis", | ||
image: "gcr.io/google-samples/gb-frontend:v4", | ||
resources: [{ | ||
requests: [{ | ||
cpu: "100m", | ||
memory: "100Mi", | ||
}] | ||
}], | ||
env: [{ | ||
name: "GET_HOSTS_FROM", | ||
value: "dns", | ||
// If your cluster config does not include a dns service, then to instead access an environment | ||
// variable to find the master service's host, comment out the 'value: dns' line above, and | ||
// uncomment the line below: | ||
// value: env | ||
}], | ||
port: [{ | ||
containerPort: 80, | ||
}], | ||
}], | ||
}], | ||
}], | ||
}], | ||
}); | ||
|
||
export let frontendPort: pulumi.Output<number> = frontendService.spec.apply(spec => spec[0].port![0].nodePort); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious...why is the Also, is the |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
{ | ||
"name": "guestbook", | ||
"version": "0.1", | ||
"main": "bin/index.js", | ||
"typings": "bin/index.d.ts", | ||
"scripts": { | ||
"build": "tsc" | ||
}, | ||
"devDependencies": { | ||
"@types/node": "^9.3.0", | ||
"typescript": "^2.5.3" | ||
}, | ||
"peerDependencies": { | ||
"@pulumi/kubernetes": "*", | ||
"pulumi": "*" | ||
}, | ||
"license": "MIT" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
{ | ||
"compilerOptions": { | ||
"outDir": "bin", | ||
"target": "es6", | ||
"module": "commonjs", | ||
"moduleResolution": "node", | ||
"declaration": true, | ||
"sourceMap": true, | ||
"stripInternal": true, | ||
"experimentalDecorators": true, | ||
"pretty": true, | ||
"noFallthroughCasesInSwitch": true, | ||
"noImplicitAny": true, | ||
"noImplicitReturns": true, | ||
"forceConsistentCasingInFileNames": true, | ||
"strictNullChecks": true | ||
}, | ||
"files": [ | ||
"index.ts" | ||
] | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | ||
# yarn lockfile v1 | ||
|
||
|
||
"@types/node@^9.3.0": | ||
version "9.4.0" | ||
resolved "https://registry.yarnpkg.com/@types/node/-/node-9.4.0.tgz#b85a0bcf1e1cc84eb4901b7e96966aedc6f078d1" | ||
|
||
typescript@^2.5.3: | ||
version "2.7.1" | ||
resolved "https://registry.yarnpkg.com/typescript/-/typescript-2.7.1.tgz#bb3682c2c791ac90e7c6210b26478a8da085c359" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm slightly nervous about depending on an unofficial fork. If we do this, let's make sure to file a bug to track. Do you know of the plans to merge this in, and/or the overall roadmap for Kubernetes support? Is this something we're going to have to take into our own hands?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Kubernetes is not in practice particularly usable in the state it's in in the mainline TF provider. Seems that almost everyone using it in practice is using some fork. There are lots though, and no one that is clearly on track to get merged to mainline.
We will need to work with either TF folks or with one of the fork maintainers to have a robust plan here.
But I don't think there is any world where we can have a valuable Kubernetes story without supporting these resources - so it's just a questions of if/when we depend on them via the mainline or via some fork that community + ourselves commit to making sure is in good state.
hashicorp/terraform-provider-kubernetes#3
hashicorp/terraform-provider-kubernetes#70
hashicorp/terraform-provider-kubernetes#100
hashicorp/terraform-provider-kubernetes#101
I suggest we create our own fork, depend on that, and set it for now to match the one I'm using here. We can then reach our to the maintainer of that, ask that he open a PR against mainline, and help advocate for that PR getting merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, surprising that the community is in this state, but maybe it's good news in that it's an area we can help with and help the community.
As to our own fork, I'm happy with that approach, that's consistent with our other providers.