Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Cleaning up the setup instructions for Kubernetes 1.7 #1005

Closed
wants to merge 2 commits into from

Conversation

arschles
Copy link
Contributor

@arschles arschles commented Jul 3, 2017

This is a multi-part PR:

  • Moves the aggregated API TLS setup script into the charts/catalog directory
  • Fetches TLS file contents directly from the helm chart to remove the dependency on base64
  • Adds --authentication-skip-lookup=true to work around the lack of the requestheader-client-ca key in the extension-apiserver-authentication ConfigMap in the kube-system namespace
  • Adds a walkthrough doc for Kube 1.7

@arschles arschles self-assigned this Jul 3, 2017
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 3, 2017
Copy link
Contributor

@MHBauer MHBauer left a comment

Choose a reason for hiding this comment

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

haven't tested it yet. have some questions.

@@ -20,3 +20,11 @@ contrib/build/*/tmp/*
.pkg
.kube
.var
charts/catalog/apiserver-key.pem
charts/catalog/apiserver.csr
charts/catalog/apiserver.pem
Copy link
Contributor

Choose a reason for hiding this comment

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

can we put these in a subdir to isolate them?

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'm not sure, I know this works at the top level chart directory though, and I'd rather make progress this way for now

tls.crt: {{ .Values.apiserver.tls.cert }}
tls.key: {{ .Values.apiserver.tls.key }}
tls.crt: {{ (.Files.Get .Values.apiserver.tls.certFileName) | b64enc }}
tls.key: {{ (.Files.Get .Values.apiserver.tls.keyFileName) | b64enc }}
Copy link
Contributor

Choose a reason for hiding this comment

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

is b64enc a helm thing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it uses sprig for template functions. relevant docs: https://masterminds.github.io/sprig/encoding.html

@@ -13,6 +13,10 @@ spec:
service:
namespace: {{ .Release.Namespace }}
name: {{ template "fullname" . }}-apiserver
caBundle: {{ .Values.apiserver.tls.ca }}
caBundle: {{ (.Files.Get .Values.apiserver.tls.caFileName) | b64enc }}
priority: 100
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 this needs to be in the else of the below stuff

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MHBauer seems like it would need to be under an if for v1alpha1.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's fine too.

keyFileName:
# The name of the file that contains the CA to authenticate connections from API server
# proxies. This field is not required. If it is not set, the service-catalog API server
# will be started with the --authentication-skip-lookup flag, which will disable the auth checks
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure setting CA should tie to setting this other flag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they seem to be one or the other

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @MHBauer

@@ -0,0 +1,489 @@
# Service Catalog Demonstration Walkthrough for Kubernetes v1.7.0 and Above
Copy link
Contributor

Choose a reason for hiding this comment

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

I know we wanted a separate walkthrough, but I figured it would be for the older versions. Is it expected to look at this one or the existing one when first starting?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would depend on the target Kubernetes cluster. We need to do a better job in directing users to the right doc in the README, but I'd like to do that in a follow-up

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with sorting out directing users in a follow-up.


```console
helm delete --purge catalog
k delete apiservice v1alpha1.servicecatalog.k8s.io
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure we can expect everyone to have kubectl aliased to k. Can we explicitly right out kubectl?

Copy link
Contributor

@luxas luxas left a comment

Choose a reason for hiding this comment

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

Quick pass. I really think we should proceed with #936 and #981 first, and then this will be easier. The longer PRs just sit and wait, the more skew like this will happen.

instructions here for enabling cluster DNS for all Kubernetes cluster
installations, but here are a few notes:

* If you are using Google Container Engine or minikube, you likely have cluster
Copy link
Contributor

Choose a reason for hiding this comment

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

or kubeadm

server is pluggable, and we currently support the following implementations:

1. Etcd 3
2. Third Party Resources (also, known as TPRs) - this is an _alpha_ feature right now. It has known issues
Copy link
Contributor

Choose a reason for hiding this comment

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

will be migrated to CRD + link to issue


For example, on a mac,
```console
curl -LO https://storage.googleapis.com/kubernetes-release/release/v1.7.0/bin/darwin/amd64/kubectl
Copy link
Contributor

Choose a reason for hiding this comment

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

dl.k8s.io points to storage.googleapis.com/kubernetes-release, can use that instead...

Create the new `Broker` resource with the following command:

```console
kubectl create -f contrib/examples/walkthrough/ups-broker.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Soon this will be in the helm chart: #980

@@ -59,6 +59,8 @@ spec:
- "{{ .Values.apiserver.verbosity }}"
{{- if .Values.apiserver.tls.requestHeaderCA }}
- --requestheader-client-ca-file=/var/run/kubernetes-service-catalog/requestheader-ca.crt
{{ else }}
- --authentication-skip-lookup=true
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I think it's possible to use delegated authn/authz without passing the requestheader-ca when the correct RBAC bindings are in place. See #936 and #981

# This field is required if apiserver.auth.enabled is true.
caFileName:
# The name of the file that contains the x509 certificate for use in the aforementioned
# APIService object. This field is required if apiserver.auth.enabled is true.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think so. RBAC and delegated authn/authz can happen while still serving with self-signed certs

keyFileName:
# The name of the file that contains the CA to authenticate connections from API server
# proxies. This field is not required. If it is not set, the service-catalog API server
# will be started with the --authentication-skip-lookup flag, which will disable the auth checks
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @MHBauer

--set apiserver.tls.ca=$(base64 --wrap 0 ${SC_SERVING_CA}) \
--set apiserver.tls.cert=$(base64 --wrap 0 ${SC_SERVING_CERT}) \
--set apiserver.tls.key=$(base64 --wrap 0 ${SC_SERVING_KEY})
--set useAggregator=true \
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 we should just switch this to true by default, see #981

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 it would be fine for #981 to do that.

@@ -179,12 +178,14 @@ keys we just generated inline.

```
helm install charts/catalog \
--name ${HELM_NAME} --namespace ${SVCCAT_NAMESPACE} \
--name ${HELM_NAME} \
--namespace ${SVCCAT_NAMESPACE} \
--set apiserver.auth.enabled=true \
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 we should just switch this to true by default, see #981

--set apiserver.tls.cert=$(base64 --wrap 0 ${SC_SERVING_CERT}) \
--set apiserver.tls.key=$(base64 --wrap 0 ${SC_SERVING_KEY})
--set useAggregator=true \
--set apiserver.insecure=false \
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 we should just switch this to false by default, see #981

{{ else if .Capabilities.APIVersions.Has "apiregistration.k8s.io/v1beta1" -}}
groupPriorityMinimum: 10000
versionPriority: 20
{{- end }}
Copy link
Contributor

Choose a reason for hiding this comment

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

@arschles can we pull this out into a separate PR (without the filesget -> base64 change) ? I think it's worth having while we figure out the details on everything else.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MHBauer I will not be able to split this out today. Please feel free to pull this into another PR, and I'll rebase as necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks @MHBauer

Particularly, making it easier and more straightforward to install service-catalog
for use behind the aggregator
@arschles
Copy link
Contributor Author

@luxas @MHBauer There are tweaks requested and also a request to wait until #936 and #981 are in. I'd like to get this PR in now, and address the tweaks and follow-ups in those PRs.

@luxas
Copy link
Contributor

luxas commented Jul 11, 2017

I'd like the tweaks/comments to be adressed and see the CI passing, but other than that it's fine by me to merge if follow-ups to enhance this are promised

Copy link
Contributor

@pmorie pmorie left a comment

Choose a reason for hiding this comment

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

LGTM

--set apiserver.tls.ca=$(base64 --wrap 0 ${SC_SERVING_CA}) \
--set apiserver.tls.cert=$(base64 --wrap 0 ${SC_SERVING_CERT}) \
--set apiserver.tls.key=$(base64 --wrap 0 ${SC_SERVING_KEY})
--set useAggregator=true \
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 it would be fine for #981 to do that.

@@ -0,0 +1,489 @@
# Service Catalog Demonstration Walkthrough for Kubernetes v1.7.0 and Above
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with sorting out directing users in a follow-up.

@pmorie pmorie added this to the 0.0.13 milestone Jul 11, 2017
@pmorie pmorie added the LGTM1 label Jul 11, 2017
@duglin duglin modified the milestones: 0.0.13, 0.0.14 Jul 13, 2017
@pmorie
Copy link
Contributor

pmorie commented Jul 13, 2017

@arschles jenkins looks like a legit error:

``` 1. Executing helm install /var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/charts/catalog --name catalog --namespace catalog --set debug=true,insecure=true,controllerManager.image=gcr.io/service-catalog-jenkins-jobs/catalog/controller-manager:01cdcb1,apiserver.image=gcr.io/service-catalog-jenkins-jobs/catalog/apiserver:01cdcb1,apiserver.service.type=LoadBalancer Error: render error in "catalog/templates/apiserver-cert-secret.yaml": template: catalog/templates/apiserver-cert-secret.yaml:12:33: executing "catalog/templates/apiserver-cert-secret.yaml" at <.Values.apiserver.tl...>: wrong type for value; expected string; got interface {} Command failed. Will retry in 5 seconds. 2. Executing helm install /var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/charts/catalog --name catalog --namespace catalog --set debug=true,insecure=true,controllerManager.image=gcr.io/service-catalog-jenkins-jobs/catalog/controller-manager:01cdcb1,apiserver.image=gcr.io/service-catalog-jenkins-jobs/catalog/apiserver:01cdcb1,apiserver.service.type=LoadBalancer Error: render error in "catalog/templates/apiserver-cert-secret.yaml": template: catalog/templates/apiserver-cert-secret.yaml:12:33: executing "catalog/templates/apiserver-cert-secret.yaml" at <.Values.apiserver.tl...>: wrong type for value; expected string; got interface {} Command failed. Will retry in 7 seconds. 3. Executing helm install /var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/charts/catalog --name catalog --namespace catalog --set debug=true,insecure=true,controllerManager.image=gcr.io/service-catalog-jenkins-jobs/catalog/controller-manager:01cdcb1,apiserver.image=gcr.io/service-catalog-jenkins-jobs/catalog/apiserver:01cdcb1,apiserver.service.type=LoadBalancer Error: render error in "catalog/templates/apiserver-cert-secret.yaml": template: catalog/templates/apiserver-cert-secret.yaml:12:33: executing "catalog/templates/apiserver-cert-secret.yaml" at <.Values.apiserver.tl...>: wrong type for value; expected string; got interface {} Command failed. Will retry in 11 seconds. 4. Executing helm install /var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/charts/catalog --name catalog --namespace catalog --set debug=true,insecure=true,controllerManager.image=gcr.io/service-catalog-jenkins-jobs/catalog/controller-manager:01cdcb1,apiserver.image=gcr.io/service-catalog-jenkins-jobs/catalog/apiserver:01cdcb1,apiserver.service.type=LoadBalancer Error: render error in "catalog/templates/apiserver-cert-secret.yaml": template: catalog/templates/apiserver-cert-secret.yaml:12:33: executing "catalog/templates/apiserver-cert-secret.yaml" at <.Values.apiserver.tl...>: wrong type for value; expected string; got interface {} Command failed. Will retry in 16 seconds. 5. Executing helm install /var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/charts/catalog --name catalog --namespace catalog --set debug=true,insecure=true,controllerManager.image=gcr.io/service-catalog-jenkins-jobs/catalog/controller-manager:01cdcb1,apiserver.image=gcr.io/service-catalog-jenkins-jobs/catalog/apiserver:01cdcb1,apiserver.service.type=LoadBalancer Error: render error in "catalog/templates/apiserver-cert-secret.yaml": template: catalog/templates/apiserver-cert-secret.yaml:12:33: executing "catalog/templates/apiserver-cert-secret.yaml" at <.Values.apiserver.tl...>: wrong type for value; expected string; got interface {} Command failed. Will retry in 25 seconds. 6. Executing helm install /var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/charts/catalog --name catalog --namespace catalog --set debug=true,insecure=true,controllerManager.image=gcr.io/service-catalog-jenkins-jobs/catalog/controller-manager:01cdcb1,apiserver.image=gcr.io/service-catalog-jenkins-jobs/catalog/apiserver:01cdcb1,apiserver.service.type=LoadBalancer Error: render error in "catalog/templates/apiserver-cert-secret.yaml": template: catalog/templates/apiserver-cert-secret.yaml:12:33: executing "catalog/templates/apiserver-cert-secret.yaml" at <.Values.apiserver.tl...>: wrong type for value; expected string; got interface {} Command failed. Will retry in 37 seconds. 7. Executing helm install /var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/charts/catalog --name catalog --namespace catalog --set debug=true,insecure=true,controllerManager.image=gcr.io/service-catalog-jenkins-jobs/catalog/controller-manager:01cdcb1,apiserver.image=gcr.io/service-catalog-jenkins-jobs/catalog/apiserver:01cdcb1,apiserver.service.type=LoadBalancer Error: render error in "catalog/templates/apiserver-cert-secret.yaml": template: catalog/templates/apiserver-cert-secret.yaml:12:33: executing "catalog/templates/apiserver-cert-secret.yaml" at <.Values.apiserver.tl...>: wrong type for value; expected string; got interface {} Command failed. Will retry in 56 seconds. 8. Executing helm install /var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/charts/catalog --name catalog --namespace catalog --set debug=true,insecure=true,controllerManager.image=gcr.io/service-catalog-jenkins-jobs/catalog/controller-manager:01cdcb1,apiserver.image=gcr.io/service-catalog-jenkins-jobs/catalog/apiserver:01cdcb1,apiserver.service.type=LoadBalancer Error: render error in "catalog/templates/apiserver-cert-secret.yaml": template: catalog/templates/apiserver-cert-secret.yaml:12:33: executing "catalog/templates/apiserver-cert-secret.yaml" at <.Values.apiserver.tl...>: wrong type for value; expected string; got interface {} Command failed. Will retry in 60 seconds. 9. Executing helm install /var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/charts/catalog --name catalog --namespace catalog --set debug=true,insecure=true,controllerManager.image=gcr.io/service-catalog-jenkins-jobs/catalog/controller-manager:01cdcb1,apiserver.image=gcr.io/service-catalog-jenkins-jobs/catalog/apiserver:01cdcb1,apiserver.service.type=LoadBalancer Error: render error in "catalog/templates/apiserver-cert-secret.yaml": template: catalog/templates/apiserver-cert-secret.yaml:12:33: executing "catalog/templates/apiserver-cert-secret.yaml" at <.Values.apiserver.tl...>: wrong type for value; expected string; got interface {} Command failed. Will retry in 60 seconds. 10. Executing helm install /var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/charts/catalog --name catalog --namespace catalog --set debug=true,insecure=true,controllerManager.image=gcr.io/service-catalog-jenkins-jobs/catalog/controller-manager:01cdcb1,apiserver.image=gcr.io/service-catalog-jenkins-jobs/catalog/apiserver:01cdcb1,apiserver.service.type=LoadBalancer Error: render error in "catalog/templates/apiserver-cert-secret.yaml": template: catalog/templates/apiserver-cert-secret.yaml:12:33: executing "catalog/templates/apiserver-cert-secret.yaml" at <.Values.apiserver.tl...>: wrong type for value; expected string; got interface {} Command 'helm install /var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/charts/catalog --name catalog --namespace catalog --set debug=true,insecure=true,controllerManager.image=gcr.io/service-catalog-jenkins-jobs/catalog/controller-manager:01cdcb1,apiserver.image=gcr.io/service-catalog-jenkins-jobs/catalog/apiserver:01cdcb1,apiserver.service.type=LoadBalancer' failed 10 times, aborting. /var/lib/jenkins/workspace/service-catalog-PR-testing2/src/github.com/kubernetes-incubator/service-catalog/contrib/hack/test_walkthrough.sh: line 147: Error deploying service catalog to cluster. (exit 1) ```

@duglin duglin modified the milestones: 0.0.14, 0.0.15 Jul 27, 2017
@duglin duglin modified the milestones: 0.0.15, 0.0.16 Aug 10, 2017
@duglin duglin modified the milestones: 0.0.16, 0.0.17 Aug 17, 2017
@kibbles-n-bytes kibbles-n-bytes added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2017
@arschles
Copy link
Contributor Author

Closing in favor of #1163

@arschles arschles closed this Aug 28, 2017
@arschles arschles deleted the agg-base64 branch August 28, 2017 21:31
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants