documentation for running event-gateway on k8s #498

sebito91 · 2018-08-06T21:45:26Z

What did you implement:

Update instructions on deploying to kubernetes, both local and remote clusters.

How did you implement it:

Helm charts, custom resource definitions and kubernetes

How can we verify it:

Todos:

Write tests
Write documentation
Fix linting errors
Make sure code coverage hasn't dropped
Provide verification commands / resources
Enable "Allow edits from maintainers" for this PR
Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

codecov · 2018-08-06T21:58:07Z

Codecov Report

Merging #498 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #498   +/-   ##
=======================================
  Coverage   71.01%   71.01%           
=======================================
  Files          37       37           
  Lines        2201     2201           
=======================================
  Hits         1563     1563           
  Misses        575      575           
  Partials       63       63

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 8b31577...1b8a70f. Read the comment docs.

mthenw · 2018-08-13T07:32:35Z

README.md

@@ -92,6 +92,15 @@ Then run the binary in development mode with:
 $ event-gateway -dev
 ```

+### Kubernetes
+
+We've added `helm` charts to the repo for a quick deploy to an existing cluster. Please note, the default


Maybe instead of writing "we've added" we could change it to something like "There are"

mthenw · 2018-08-13T07:34:21Z

contrib/helm/event-gateway/values.yaml

What's the reason for this change?

I prefer the GNU CLI flags and changed these accordingly. In golang now there isn't really any difference between the single - and the double -- when processing long flags. IMHO, short flags are single - and long flags are --.

https://www.gnu.org/prep/standards/html_node/Command_002dLine-Interfaces.html

If you're not happy with this change I'm happy to revert.

Would be great to be consistent in the docs. Here https://github.com/serverless/event-gateway/blob/cd14c4004bb4aa3c3082526235fff0d8a357736d/docs/running-locally.md e.g. we use single -. I'm fine with both options.

Also a good call here, will update to keep the double -- for consistency.

mthenw · 2018-08-13T07:37:46Z

contrib/README.md

+
+With your environment set up, you can now jump to the [examples](#examples) section to put your `event-gateway` to use!
+
+## Minikube or local clusters


I think we should simply link to this page https://kubernetes.io/docs/tasks/tools/install-minikube/

Also lates version of Docker includes simply way to run K8s locally https://docs.docker.com/docker-for-mac/kubernetes/#override-the-default-orchestrator. I think we should also include that in this guide. Tho it can be done as a separate PR (task).

I would disagree with your first comment, as the k8s documentation is notoriously difficult to read. I was debating where to include the raw fedora instructions but opted to do it ourselves because each environment is different. Still pending instructions on the MacOS side, but will update that shortly.

ACK on the second comment, will investigate this option soon.

Regarding installation guide, so maybe better options is to extract it to separate md file? Probably most of the people interested in k8s will have minikube already installed. I think it will be better this guide will be straight-forward.

Good call on that, I'll move the instructions to a supplemental file and link to it here. Will ping you when that update is ready.

alexdebrie

@sebito91 This looks great. Very clean scripts and the README is mostly pretty clear.

The biggest areas I got hung up on are:

Minikube and what areas that affects.
Duplicated helm portions in the quickstart?
Environment variable vs. IP address for EG.

Happy to discuss further.

alexdebrie · 2018-08-15T21:58:09Z

contrib/README.md

+instructions [here](https://docs.helm.sh/using_helm/#quickstart) if you have not set this up previously.
+
+**NOTE:** This portion of the config expects you to have a pre-existing kubernetes cluster (not minikube). For 
+local development please check the [minikube](MINIKUBE.md) information below.


@sebito91 I'm confused about this note. Couple questions:

Is this different than the GKE vs. minikube note in the first paragraph of this section?

Does it mean I don't run the helm commands below?

It says to check the minikube information "below", but does that refer to the MINIKUBE.md doc? Don't see anything below.

Ah, good call. @mthenw had comments earlier to move the minikube instructions from this doc to a standalone, the MINIKUBE.md you see now. I'll update this to remove the below from the statement.

Other than that, the instructions should be the exact same pushing to minikube locally or to GKE. I have to admit I have not tested the latter.

alexdebrie · 2018-08-15T22:01:59Z

contrib/README.md

+
+With your environment set up, you can now jump to the [examples](#examples) section to put your `event-gateway` to use!
+
+### Using helm


Is this Using helm section a duplicate of the section above? Should I be running both the commands above and the commands in this Using helm section?

They are indeed largely the same (if not exact now). Initially the requirements were different for minikube, specifically since minikube doesn't support LoadBalancers like GKE would. We replaced the LB with Ingress across the board, so the instructions are largely the same.

I think in honesty we should just remove each of the helm and CRD sections here, they are confusing.

Agree that this section is confusing and seems redundant. Can we simply remove this and rename Quickstart as "Quickstart using helm". We can add a CRD Quickstart later

alexdebrie · 2018-08-15T22:05:46Z

contrib/README.md

+routing will ensure the request goes to the proper service managed by the cluster. 
+
+**DOUBLENOTE:** if you did not want to use an environment variable for connecting to the `event-gateway`, you can use
+the IP address of your Ingress. Please check the [Quickstart](#quickstart) for reference. 


I'm a little confused on the env variable vs. IP address split here.

That does say the same thing :/

It should reference the /etc/hosts instructions.

alexdebrie · 2018-08-15T22:07:12Z

contrib/README.md

+curl --request POST \
+  --url http://eventgateway.minikube/v1/spaces/default/eventtypes \
+  --header 'content-type: application/json' \
+  --data '{ "name": "eventgateway.function.invoked" }'


It might be good to do the default http.request Event Type. Then, after setting up a subscription, show the user how to emit an http.request event and get a response.

On the other hand, it could require a working function example, which would add length to the tutorial. 🤔 Open to thoughts here.

These are loosely arranged in functions as a reference, not necessarily in any particular sequence. I think if anything we might want a separate tutorial on how to use an actual EG example from https://github.com/serverless/event-gateway/tree/master/examples

Yep good point. Not worth including in this resource ATM.

ganeshvlrk

@sebito91 Looks good overall. Left some comments for your review though

ganeshvlrk · 2018-08-15T22:33:16Z

README.md

+
+The repo contains `helm` charts for a quick deploy to an existing cluster. Please note, the default
+installation requires proper `loadbalancer` support out of the box. If you don't have this enabled in your
+cluster you can check the `minikube` instructions for more guidance.


@sebito91 When we say "requires proper loadbalancer support OOTB" what do we mean? Didn't we switch to using an ingress?

This needs to be removed, great catch.

ganeshvlrk · 2018-08-15T22:34:04Z

README.md

+cluster you can check the `minikube` instructions for more guidance.
+
+[Helm Chart](./contrib/helm/README.md)
+[Minikube](./contrib/helm/README.md)


Are both of these links correct?
Should Minikube link out to Minikube readme ?

ganeshvlrk · 2018-08-15T23:00:16Z

contrib/MINIKUBE.md

+# Event Gateway on Kubernetes (minikube)
+
+To develop and deploy the `event-gateway` and all related elements locally, the easiest method includes using 
+the [minikube](https://github.com/kubernetes/minikube) toolset. To get started, set up your cluster with the 


would recommend calling it "...set up your local cluster with..."

ganeshvlrk · 2018-08-15T23:01:41Z

contrib/README.md

+# Event Gateway on Kubernetes 
+
+This chart deploys the Event Gateway with etcd onto a Kubernetes cluster. Please note, the default instructions expect
+an existing kubernetes cluster that supports ingress, such as GKE. If your environment doesn't have ingress support


Are there any instructions that users can use to test if their cluster supports ingress?

Also, here we say ingress, but in the top level Readme, we say load balancer which should be fixed.

FIxed the top-level README but I'm not quite sure if there are examples to check for ingress enablement. From a minikube perspective we need to active enable the addons (e.g. minikube addons enable ingress), but from from hosted version of k8s I'm not sure.

I can dig into this and amend as necessary.

ganeshvlrk · 2018-08-15T23:12:02Z

contrib/README.md

+
+**etcd-operator**
+```bash
+helm install stable/etcd-operator --name ego [--namespace <namespace>]


Is the release name (--name) required? I guess its autogenerated if not specified

Yes, that's correct. It's better to name it than to go searching for the service later.

[sborza@icebox]:~/src/github.com/serverless/event-gateway (k8s):$ helm install -h This command installs a chart archive. ... Usage: helm install [CHART] [flags] Flags: --ca-file string verify certificates of HTTPS-enabled servers using this CA bundle --cert-file string identify HTTPS client using this SSL certificate file --dep-up run helm dependency update before installing the chart --devel use development versions, too. Equivalent to version '>0.0.0-0'. If --version is set, this is ignored. --dry-run simulate an install --key-file string identify HTTPS client using this SSL key file --keyring string location of public keys used for verification (default "/home/sborza/.gnupg/pubring.gpg") -n, --name string release name. If unspecified, it will autogenerate one for you ...

ganeshvlrk · 2018-08-15T23:12:11Z

contrib/README.md

+
+**event-gateway**
+```bash
+helm install event-gateway --name eg [--namespace <namespace>]


Same comment as above

ganeshvlrk · 2018-08-15T23:22:11Z

contrib/README.md

+
+With your environment set up, you can now jump to the [examples](#examples) section to put your `event-gateway` to use!
+
+### Using helm


Agree that this section is confusing and seems redundant. Can we simply remove this and rename Quickstart as "Quickstart using helm". We can add a CRD Quickstart later

ganeshvlrk · 2018-08-15T23:25:44Z

contrib/README.md

+}
+```
+
+#### Register an event


We are registering an 'Event Type' as against an 'Event'. Would be good to update references in this section to reflect the same

ganeshvlrk · 2018-08-15T23:29:59Z

contrib/README.md

+1. [Quickstart](#quickstart)
+    1. [Using helm](#using-helm)
+    1. [Using custom resources](#using-custom-resources)
+1. [Examples](#examples)


This section should ideally take the user through an end to end EG example that it links out to. Just providing Curl examples for the API is not incredibly satisfying as an outcome for what EG enables developers to do. Can be done in a follow up PR. @alexdebrie @sebito91 Please review.

alexdebrie

A few more notes

alexdebrie · 2018-08-16T00:58:54Z

contrib/MINIKUBE.md

+
+To develop and deploy the `event-gateway` and all related elements locally, the easiest method includes using 
+the [minikube](https://github.com/kubernetes/minikube) toolset. To get started, set up your cluster with the 
+following instructions...


Remove "..."

alexdebrie · 2018-08-16T01:02:05Z

contrib/README.md

+instructions [here](https://docs.helm.sh/using_helm/#quickstart) if you have not set this up previously.
+
+**NOTE:** This portion of the config expects you to have a pre-existing kubernetes cluster (not minikube). For 
+local development please check the [minikube](MINIKUBE.md) information.


Is this note still needed? As I understand it, the helm instructions work with both minikube and something like GKE?

alexdebrie · 2018-08-16T01:11:49Z

contrib/README.md

+Next we'll need to collect the Event Gateway IP to use on the CLI. We have a couple of options available to reference the
+internal services of the kubernetes cluster exposed via Ingress:
+
+1. add Ingress IP to /etc/hosts (recommended)


The switching between eventgateway.minikube and EVENT_GATEWAY_URL environment variable is confusing to me. I'd suggest consolidating on one or the other just to make it easier. The more forks in a tutorial, the more confusing it gets.

My guess would be to use the environment variable as some people may be averse to messing with their /etc/hosts file but will defer to you.

I was hoping that showing both methods in various functions would be helpful, given that we need to specify a header in one version but not the other.

Do you think this would be better suited for the TUTORIAL.md we discussed offline?

alexdebrie · 2018-08-16T01:12:36Z

contrib/README.md

+curl --request POST \
+  --url http://eventgateway.minikube/v1/spaces/default/eventtypes \
+  --header 'content-type: application/json' \
+  --data '{ "name": "eventgateway.function.invoked" }'


Yep good point. Not worth including in this resource ATM.

mthenw · 2018-08-16T07:22:30Z

contrib/README.md

@@ -0,0 +1,341 @@
+# Event Gateway on Kubernetes 


This is readme for the whole contrib directory. It also contains other stuff (Terraform module) so please rename this README and create another one pointing also to https://github.com/serverless/event-gateway/blob/master/contrib/terraform/README.md

OK, I can move this down a level

mthenw · 2018-08-16T13:23:06Z

contrib/helm/MINIKUBE.md

@@ -0,0 +1,66 @@
+# Event Gateway on Kubernetes (minikube)


This doc is about setting up minikube. I think we should rename the title.

mthenw · 2018-08-16T13:23:54Z

README.md

 ```

+### Kubernetes


We do not point to the long readme file here. Only to MINIKUBE guide. I think it's worth to link "helm chart" to the /contrib/helm dir.

mthenw · 2018-08-16T13:26:36Z

examples/aws-golang-simple-http-endpoint/package.json

@@ -8,7 +8,6 @@
    "@serverless/serverless-event-gateway-plugin": "^0.7.3"


Can we remove those files from this PR? :)

alexdebrie

LGTM

sebito91 added the WIP label Aug 6, 2018

sebito91 self-assigned this Aug 6, 2018

sebito91 requested a review from alexdebrie August 6, 2018 21:45

sebito91 force-pushed the k8s branch from 22f8b23 to b3ceeee Compare August 6, 2018 21:46

sebito91 changed the title ~~K8s~~ documentation for running event-gateway on k8s Aug 6, 2018

mthenw suggested changes Aug 13, 2018

View reviewed changes

sebito91 added 15 commits August 14, 2018 21:17

adding more changes to README

2ad08d1

interim commit for more README updates

2f0c593

more updates to README

ebb4f53

adding in fedora section

0eb794c

moving README one level higher

dc2f511

adding in ports description

fa79548

breaking up lines a bit

f7a1d7d

fixing the event url

ee5abf9

adding in dev flag for running

08570dd

adding example function register

aacd0c9

updating data

4d696d3

updating for typo

6f3842e

cleaning up k8s content

4bb0f4e

updating based on feedback

29c5b8f

interim commit for k8s

c2df1e0

sebito91 force-pushed the k8s branch from b0acd83 to c2df1e0 Compare August 15, 2018 02:21

sebito91 added 4 commits August 14, 2018 21:24

cleaning up helm files

f9e143d

moving to template variables

8cec6b0

initial commit minikube readme

e26e418

updating README

22d2ed2

sebito91 removed the WIP label Aug 15, 2018

sebito91 added 2 commits August 15, 2018 16:39

updating number

ee9f732

updating formatting

54dc823

more formatting

347c8b2

alexdebrie reviewed Aug 15, 2018

View reviewed changes

sebito91 added 3 commits August 15, 2018 17:33

updating based upon feedback

c1e699d

updating with trigger

2040648

updating the TOC

03b1bbc

ganeshvlrk reviewed Aug 15, 2018

View reviewed changes

alexdebrie reviewed Aug 16, 2018

View reviewed changes

updating based upon further feedback

ed8c510

serverless deleted a comment from sebito91 Aug 16, 2018

mthenw suggested changes Aug 16, 2018

View reviewed changes

sebito91 added 2 commits August 16, 2018 08:12

updating with more feedback

c46f730

updating to query all event types

16765e8

mthenw suggested changes Aug 16, 2018

View reviewed changes

mthenw reviewed Aug 16, 2018

View reviewed changes

updating the README for the contrib level folder

25a3e38

sebito91 force-pushed the k8s branch from 509b9cd to 25a3e38 Compare August 16, 2018 14:24

sebito91 added 2 commits August 16, 2018 09:24

updating to builleted items

554216f

removing extraneous reference

0ba45ab

mthenw approved these changes Aug 16, 2018

View reviewed changes

sebito91 added 2 commits August 16, 2018 10:39

updating README with feedback

2153ab0

cleaning up doublenote

1b8a70f

alexdebrie approved these changes Aug 16, 2018

View reviewed changes

sebito91 merged commit 02e8f5f into serverless:master Aug 16, 2018

sebito91 deleted the k8s branch August 16, 2018 19:11


		With your environment set up, you can now jump to the [examples](#examples) section to put your `event-gateway` to use!

		## Minikube or local clusters


		With your environment set up, you can now jump to the [examples](#examples) section to put your `event-gateway` to use!

		### Using helm

		@@ -8,7 +8,6 @@
		"@serverless/serverless-event-gateway-plugin": "^0.7.3"

documentation for running event-gateway on k8s #498

documentation for running event-gateway on k8s #498

Conversation

sebito91 commented Aug 6, 2018 • edited Loading

What did you implement:

How did you implement it:

How can we verify it:

Todos:

codecov bot commented Aug 6, 2018 • edited Loading

Codecov Report

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

mthenw Aug 13, 2018 • 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

alexdebrie left a 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

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

ganeshvlrk left a 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

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

alexdebrie left a 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

mthenw Aug 16, 2018 • 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

alexdebrie left a comment

Choose a reason for hiding this comment

sebito91 commented Aug 6, 2018 •

edited

Loading

codecov bot commented Aug 6, 2018 •

edited

Loading

mthenw Aug 13, 2018 •

edited

Loading

mthenw Aug 16, 2018 •

edited

Loading