Splitting up the Walkthrough for 1.6 and 1.7 instructions by arschles · Pull Request #1163 · kubernetes-retired/service-catalog

arschles · 2017-08-28T21:31:10Z

As discussed, we will provide installation and walkthrough documents for both 1.6 (and prior) and 1.7 (and later) clusters. However, we will deprecate the 1.6 documents. I've done that here.

I've also split the walkthrough documents apart. For each Kubernetes version, I've provided an installation document and a walkthrough document.

Finally, I've moved the docs/api-aggregation-setup.md file into the installation guide for 1.7, and re-structured it to match the structure of the installation guide for 1.6.

There is still work to follow-up on from this PR. This is a step in the right direction, and will improve on the current state of the world. @jboyd01 and I intend to move forward with follow-ups to further improve our install & walkthrough docs.

This patch replaces #1005 (which is now closed). I'd like to complete the 1.7 instructions (which are currently a TODO in this patch) in a follow-up.

cc/ @jboyd01, who is also interested in working on 1.7 install/walkthrough instructions.

kibbles-n-bytes · 2017-08-28T22:48:48Z


 This directory contains API resources for use with the [demo
-walkthrough](../../../docs/walkthrough.md).
+walkthrough](../../../docs/introduction.md).


I think linking to the introduction doc here is a bit misleading; the new structure of the docs has the core of the walkthrough separated to its own doc(s), with the intro being much larger in scope. I would instead link to the 1.7 walkthrough doc (since the 1.6 one is deprecated, it's unambiguous).

@kibbles-n-bytes I updated the language around the link. I want people to go to the intro doc so they can choose whether they want to install or do the demo.

kibbles-n-bytes · 2017-08-28T22:49:46Z

 ## Demo walkthrough

-Check out the [walk-through](walkthrough.md) for a detailed guide of an example
+Check out the [introduction](./introduction.md) for a detailed guide of an example


Similar to the comment from the walkthrough/README.md; I would link to the 1.7 walkthrough doc here, and then have a separate little sub-section above here for the intro.

As with #1163 (comment), I updated the language around this link because I want to funnel people through the intro doc.

kibbles-n-bytes · 2017-08-28T22:50:16Z

@@ -0,0 +1,164 @@
+# Installing Service Catalog on Clusters Running Kubernetes 1.7 and Above


Kubernetes 1.7 and Above -> Kubernetes 1.6

kibbles-n-bytes · 2017-08-28T22:51:43Z

 ```

-## Get the appropriate tls ca/cert/key from kube
+## Get the Appropriate TLA CA, Certificate and Key from Kubernetes


TLA->TLS

kibbles-n-bytes · 2017-08-28T22:52:40Z

+maintained and we do not recommend using them to run a production installation
+of Service Catalog.
+
+## Kubernetes 1.6 and Below (Deprecated)


Since this is deprecated, I'd put it below the 1.7 section.

kibbles-n-bytes · 2017-08-28T22:54:46Z

+This document assumes that you've installed Service Catalog onto your cluster.
+If you haven't, please see [install-1.7.md](./install-1.7.md).
+
+TODO


I'd write here that they should be looking at the 1.6 walkthrough doc, and link to it.

The 1.6 doc won't work for them, so they shouldn't look there.

Does the walkthrough not work at all? Or is it just that the service catalog is behind the aggregator, so there is no "service-catalog" k8s context. If the latter, it seems misleading to basically send the reader to a dead end.

Correct, there is no service-catalog k8s context. It doesn't work at all.

@jboyd01 and I are going to follow-up immediately after this PR to fill in this section.

kibbles-n-bytes · 2017-08-28T22:56:08Z

+maintained and we do not recommend using them to run a production installation
+of Service Catalog.
+
+## Kubernetes 1.6 and Below (Deprecated)


Same comment about "and Below"

kibbles-n-bytes · 2017-08-28T22:56:30Z

@@ -0,0 +1,164 @@
+# Installing Service Catalog on Clusters Running Kubernetes 1.7 and Above


Is the "1.6 and Below" aspect true? Does the service catalog currently work on versions below 1.6? I would call a hard line at 1.6 just to avoid any issues.

I changed to just 1.6 - you're right that 1.5 is a no-go.

jboyd01

This looks like the right direction to head in, we need to make additional passes in the walkthrough(s) to fix up some naming (Binding vs ServiceInstanceCredentials etc). I've got notes and will make a pass through at that once this is merged.

jboyd01 · 2017-08-29T13:51:54Z

@@ -0,0 +1,165 @@
+# Installing Service Catalog on Clusters Running Kubernetes 1.6


I know the next paragraph states 1.6 is deprecated, I'd suggest adding (DEPRECATED) or something to the title above

jboyd01 · 2017-08-29T13:56:04Z

+and go inside the cluster, and register themselves on demand to augment the 
+externally facing API that kubernetes offers.
+
+Instead of running multiple API servers on different ports, API aggregation 


"Instead of running multiple API servers on different ports" - - we still run multiple API servers on different ports, but aggregation enables a single point of entry for administration, right? ie might reword to indicate benefit is to admins with only needing to interact with the primary kube api server.

jboyd01 · 2017-08-29T14:19:33Z

-please check [the auth doc](auth.md).
+```console
+mkdir certs
+```


do we want to change dir to there now? The original steps below don't cd and copy certs to the current directory (root of the SC source for me)

Probably, but I haven't fully tested these instructions (I forked them and made changes there). I'd just like to get them into the right file for now and then improve in follow-ups. Cool with you?

I ended up addressing this after all

jboyd01 · 2017-08-29T14:33:56Z

 ```shell
-source /contrib/svc-cat-apiserver-aggregation-tls-setup.sh
+source ./contrib/svc-cat-apiserver-aggregation-tls-setup.sh
 ```


We don't really want to run this script as it would overwrite any custom certs the user copied from their cloud environment as detailed in the steps above. I believe all the steps above around the certs and TLS are executed with defaults by the svc-cat-apiserver-aggregation-tls-setup.sh script. I realize this content was not added by his PR, but at some point this summary should be removed or replaced with an accurate summary.

I agree. I'd just like to get this content into the right place for now and then improve in follow-ups. Cool with you?

You're in the docs, why not fix them now?

I will fix them, I just want to keep this PR's scope down as much as possible (it's already big).

arschles · 2017-08-29T16:51:42Z

Thanks @jboyd01. Is it cool with you if I address your changes in an immediate follow-up?

Follow-up: I ended up addressing these comments in this PR after all

For 1.6 and 1.7 instructions

MHBauer

Reading the commit log makes this all seem disconnected, but all the links I checked seem to work and I think I understand the flow of a user encounter.

MHBauer · 2017-08-29T16:57:05Z

    application configuration primitives in Kubernetes: Services, Secrets, and
    ConfigMaps.

+For more introduction, installation and self-guided demo instructions, please


remove 'more'. this is the first mention of instructions here.

But this line is under the "Introduction" section of the readme.

ah okay, "For more introduction, including installation and self-guided demo instructions, ..."

MHBauer · 2017-08-29T16:59:29Z

+`KUBE_ENABLE_CLUSTER_DNS` environment variable is set as follows:
+
+```console
+hack/local-up-cluster.sh -O


What does -O do? Is it going to print something? Can we get example output included?

I have no idea; I barely ever use local-up. I copied this from the 1.6 walkthrough

Example 2: hack/local-up-cluster.sh -O (auto-guess the bin path for your platform). The alternative being to specify the path with -o or no args and have it rebuild. I doubt you want to include example output, it's quite long.

Then that example makes even less sense.

How about :
KUBE_ENABLE_CLUSTER_DNS=true hack/local-up-cluster.sh -O

Is DNS enabled by default now? Maybe we can just remove the mention of it in the text above the console output, at which point we don't need that either.

I'll add the env var into the command. Can we address the rest of this in a follow-up please?

MHBauer · 2017-08-29T17:03:18Z

-instructions here for enabling cluster DNS for all Kubernetes cluster
-installations, but here are a few notes:
+This document assumes that you've installed Service Catalog onto your cluster.
+If you haven't, please see [install-1.6.md](./install-1.6.md).


clean up the link text here and above to be something like "the installation instructions for v1.X"

MHBauer · 2017-08-29T17:05:17Z

+The service catalog is packaged as a Helm chart located in the
+[charts/catalog](../charts/catalog) directory in this repository, and supports a
+wide variety of customizations which are detailed in that directory's
+[README.md](https://github.com/kubernetes-incubator/service-catalog/blob/master/charts/catalog/README.md).


relative link vs whole link here? Should go to almost the same place as the link two lines above, right?

yep. I'll change it to a relative link.

MHBauer · 2017-08-29T17:05:46Z

+
+1. Etcd 3
+2. Third Party Resources (also, known as TPRs) - this is an _alpha_ feature 
+right now. It has known issues and may be removed at any time


Add period at end of sentence.

MHBauer · 2017-08-29T17:19:31Z

+Certificate, and the Serving Private Key. 

-This is an example. It is written with zero understanding of the best
-practices of secure certificate generation.


@duglin will like this.

MHBauer · 2017-08-29T17:21:15Z

 ```

-## Get the appropriate tls ca/cert/key from kube
+## Get the Appropriate TLS CA, Certificate and Key from Kubernetes


The heading levels might need to be adjusted, this was supposed to be a peer with the "generate the Cert Bundle" part, as it is an alternative and both should not be done at the same time.

I put the two options under the same heading

MHBauer · 2017-08-29T17:24:38Z

@@ -1,17 +1,28 @@
+# Installing Service Catalog on Clusters Running Kubernetes 1.7 and Above


Overall this feels more like the api-aggregation doc than a 1.7 install doc. Maybe we can just have the final "helm command" and link to a separate place where the cert stuff is described.

script run + helm command seems simplest to me.

MHBauer · 2017-08-29T17:26:29Z

 ```shell
-source /contrib/svc-cat-apiserver-aggregation-tls-setup.sh
+source ./contrib/svc-cat-apiserver-aggregation-tls-setup.sh
 ```


You're in the docs, why not fix them now?

MHBauer · 2017-08-29T17:28:30Z

+## Kubernetes 1.7 and Above
+
+- [Installation instructions](./install-1.7.md)
+- [Demo instructions](./walkthrough-1.7.md)


Leave this link off as the document is a dead end. Or have that document link back here.

I left a note that this doc is a WIP

The install instructions say that you should create this directory, so let’s ignore it

…nstall docs

arschles · 2017-08-29T19:54:42Z

@MHBauer I've addressed your comments. Can you PTAL?

vaikas

There's a few things that I'd consider nits, I like the breaking of the documents, it's an improvement and we can make smaller fixes after we merge this more easily.

vaikas · 2017-08-29T20:10:42Z

+externally facing API that kubernetes offers.
+
+Instead of requiring the end-user to access multiple API servers, the API 
+aggregation system allows many servers to run inside the cluster, and combines


nit:
s/many servers/many API servers/

MHBauer

the hack local up thing, otherwise looking good.

MHBauer · 2017-08-29T21:40:54Z

    application configuration primitives in Kubernetes: Services, Secrets, and
    ConfigMaps.

+For more introduction, installation and self-guided demo instructions, please


ah okay, "For more introduction, including installation and self-guided demo instructions, ..."

MHBauer · 2017-08-29T21:43:00Z

+`KUBE_ENABLE_CLUSTER_DNS` environment variable is set as follows:
+
+```console
+hack/local-up-cluster.sh -O


Then that example makes even less sense.

How about :
KUBE_ENABLE_CLUSTER_DNS=true hack/local-up-cluster.sh -O

Is DNS enabled by default now? Maybe we can just remove the mention of it in the text above the console output, at which point we don't need that either.

MHBauer · 2017-08-29T21:43:51Z

+## Helm
+
+You *must* use [Helm](http://helm.sh/) v2 or newer in the installation steps
+below.


Anything in the minor releases of helm we need? I thought there was something in 2.3 or so that was needed. Cannot remember what.

I recall that, yes. I'll try to find out and address it in a follow-up

MHBauer · 2017-08-29T21:44:50Z

@@ -1,17 +1,28 @@
+# Installing Service Catalog on Clusters Running Kubernetes 1.7 and Above


MHBauer · 2017-08-29T21:45:14Z

+## Kubernetes 1.7 and Above
+
+- [Installation instructions](./install-1.7.md)
+- [Demo instructions](./walkthrough-1.7.md) (this is a work in progress)


I'm okay with this.

MHBauer

LGTM

pmorie

Big medicine. I have a few suggestions/nits. The only big thing is that the 1.7 walkthrough needs to either have all the walkthrough content tailored for 1.7, or somehow give the correct commands to run.

pmorie · 2017-08-29T22:02:15Z

 .pkg
 .kube
 .var
+docs/certs


Path looks weird, but whatever

yea, the install directions for 1.7 in this PR are tailored toward operating the install out of this directory. can change it in a follow-up if needed

pmorie · 2017-08-29T22:04:04Z


-This is an example. It is written with zero understanding of the best
-practices of secure certificate generation.
+The certificate bundle is made up of Certificate Authority, a Serving


of a Certificate Authority

done

update: I made this change after the PR was merged. Addressed in #1169

pmorie · 2017-08-29T22:05:20Z

 through a short demo.

-## Step 0 - Prerequisites
+This document contains instructions for a self-guided demo of the Service 


Might want to put a bold NOTE here or something.

done

update: I made this change after the PR was merged. Addressed in #1169

pmorie · 2017-08-29T22:06:37Z

 ```

-## Step 9 - ServiceInstanceCredential to the ServiceInstance
+# Step 5 - ServiceInstanceCredential to the ServiceInstance


This reads awkwardly - how about 'Requesting a ServiceInstanceCredential to use the ServiceInstance`? As-is, doesn't read quite right to me.

Good catch - thanks @pmorie. Will do in a follow-up

Done in #1169

pmorie · 2017-08-29T22:08:11Z

 Notice that a new `Secret` named `ups-instance-credential` has been created.

-## Step 10 - Unbinding from the ServiceInstance
+# Step 6 - Unbinding from the ServiceInstance


We should probably not call this unbinding anymore. How about 'delete the ServiceInstanceCredential', and update the language to be something like 'Imagine we're done using this service instance. We need to delete the credentials we requested earlier.'

done

update: I made this change after the PR was merged. Addressed in #1169

pmorie · 2017-08-29T22:10:00Z

+This document assumes that you've installed Service Catalog onto your cluster.
+If you haven't, please see [install-1.7.md](./install-1.7.md).
+
+This document is a work in progress. Instructions for the self-guided demo


I think we should either:

duplicate the walkthrough content here, and only maintain both until the beta

expand significantly on the commands to use

I already wrote in the 1.6 walkthrough that it's deprecated and may be removed at any time. In a follow-up, I am going to duplicate the existing walkthrough and expand on it. Does that address your concern @pmorie?

Yeah, that does, thanks.

pmorie · 2017-08-29T22:12:09Z

I see that this already has LGTM2, and I won't block it merging, but I would like to have my comments addressed ASAP in a follow-up.

@MHBauer

This is a follow-up to kubernetes-retired#1163 (comment) sion_r135920291 Cc/ @MHBauer

nilebox · 2017-08-30T01:19:35Z

+minikube start --extra-config=apiserver.Authorization.Mode=RBAC
+```
+
+AssumingBy default, `helm init` installs the Tiller pod into the `kube-system`


@arschles "AssumingBy" typo?

@MHBauer

* Specifying that you need Helm v2.5.0 for installation This is a follow-up to #1163 (comment) sion_r135920291 Cc/ @MHBauer * Reducing Helm version redundancy

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 28, 2017

arschles requested a review from MHBauer August 28, 2017 21:31

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 28, 2017

arschles mentioned this pull request Aug 28, 2017

Cleaning up the setup instructions for Kubernetes 1.7 #1005

Closed

arschles requested review from duglin, pmorie and vaikas August 28, 2017 21:34

kibbles-n-bytes reviewed Aug 28, 2017

View reviewed changes

jboyd01 approved these changes Aug 29, 2017

View reviewed changes

arschles force-pushed the walkthrough-note branch from cbb0726 to 933ea73 Compare August 29, 2017 17:25

Splitting the walkthrough up

7b6b771

For 1.6 and 1.7 instructions

arschles force-pushed the walkthrough-note branch from 933ea73 to 7b6b771 Compare August 29, 2017 17:26

MHBauer reviewed Aug 29, 2017

View reviewed changes

arschles added 14 commits August 29, 2017 11:26

Making the readme link relative

c1e2665

Linking to the right final cleanup page

b7afcc4

Adding period at end of sentence

79fc07f

Using curly fracked env var syntax

08f9621

Fixing NodePort language

df2970f

Cd-ing into certs dir

8552397

Removing file names, replacing with more descriptive text

0ad2a9a

Adding more description to tooling

fdc2f55

Removing DNS entries note

a9f9037

Kube core => core Kubernetes API server

ca93148

Specifying that there are 2 options for getting a CA and keys

7707e94

Noting WIP for the 1.7 walkthrough

46ab2fa

Ignoring the certs directory

c12c1a5

The install instructions say that you should create this directory, so let’s ignore it

Splitting the aggregated API install docs w/ script from the manual i…

9a862e3

…nstall docs

Adding cfssl install instructions

09b29e3

Adding a prerequisites section to the 1.7 install doc

bcdbc68

vaikas approved these changes Aug 29, 2017

View reviewed changes

vaikas added the LGTM1 label Aug 29, 2017

arschles added 2 commits August 29, 2017 13:17

s/many servers/many API servers

063b5bd

Adding deprecated note to the 1.6 walkthrough doc

cddc005

arschles mentioned this pull request Aug 29, 2017

Service Catalog walkthrough is broken for minikube #1069

Closed

Removing the link to "final cleanup"

b553a5a

MHBauer suggested changes Aug 29, 2017

View reviewed changes

arschles added 2 commits August 29, 2017 14:49

Clarify the link to the intro doc

c1e97fe

Adding the env var to the local-up command

67f8ad5

MHBauer approved these changes Aug 29, 2017

View reviewed changes

MHBauer added the LGTM2 label Aug 29, 2017

pmorie reviewed Aug 29, 2017

View reviewed changes

pmorie merged commit 43f7cfb into kubernetes-retired:master Aug 29, 2017

arschles mentioned this pull request Aug 29, 2017

Adding more small fixes to the walkthrough & install docs #1169

Merged

arschles added a commit to arschles/kubernetes-service-catalog that referenced this pull request Aug 29, 2017

Specifying that you need Helm v2.5.0 for installation

c21f560

This is a follow-up to kubernetes-retired#1163 (comment) sion_r135920291 Cc/ @MHBauer

This was referenced Aug 29, 2017

Specifying that you need Helm v2.5.0 for installation #1170

Merged

Adding walkthrough instructions for 1.7 #1171

Merged

nilebox reviewed Aug 30, 2017

View reviewed changes

		@@ -0,0 +1,164 @@
		# Installing Service Catalog on Clusters Running Kubernetes 1.7 and Above

		@@ -0,0 +1,165 @@
		# Installing Service Catalog on Clusters Running Kubernetes 1.6

		@@ -1,17 +1,28 @@
		# Installing Service Catalog on Clusters Running Kubernetes 1.7 and Above

Conversation

arschles commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jboyd01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arschles commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MHBauer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

arschles commented Aug 28, 2017 •

edited

Loading

arschles commented Aug 29, 2017 •

edited

Loading