-
Notifications
You must be signed in to change notification settings - Fork 385
api-aggregation setup instructions #895
api-aggregation setup instructions #895
Conversation
+1 we need to get to a point where this is the default and that our e2es verify all content is removed from the namespace when deleted by virtue of this integration. |
Step by step directions and script here: https://github.com/openshift/origin/pull/13229/files |
@deads2k what is |
|
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.
The content mostly looks ok (a few comments inline). Your formatting is a bit off though, but you can probably clean that up at the end.
docs/api-aggregation-setup.md
Outdated
Need to figure out all of the permissions we need, and group them into a role. | ||
|
||
# not sure if I still need to create this (incorrectly spelled) namespace | ||
kubectl create namespace catlog |
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.
not entirely sure what this is here for...
docs/api-aggregation-setup.md
Outdated
You can see we use the generated cert and key, along with the cores CA | ||
certificate. (TODO: Is this good or bad practice? Is there an | ||
alternative delegated kind of CA we can use if we don't have access to | ||
kube itself on a cloud provider.) |
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.
so, it doesn't matter what CA you use here, since we explicitly tell the aggregator which CA to use (the caBundle
field). Using the core CA is "easy" because it's already there in most setups.
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 think that may be more 'obvious' to someone who does this kind of cert management.
I think we should also have a basic workflow beyond using an existing CA of "create a new CA" and proceed from there.
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.
We can probably just point to the relevant section in auth.md
.
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.
Oh, brilliant. Did a copy or version of auth.md
ever make it to somewhere more visible that we should link to it instead now?
Which section were you thinking, specifically?
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.
There's a copy in the service catalog docs, and a slight more generic copy here: https://github.com/kubernetes-incubator/apiserver-builder/blob/master/docs/concepts/auth.md
Either works for me. I was thinking this section: https://github.com/kubernetes-incubator/apiserver-builder/blob/master/docs/concepts/auth.md#generating-certificates
Something like "follow the instructions here to generate the CA and equivalent serving certificates. Set PURPOSE
to ..."
docs/api-aggregation-setup.md
Outdated
# this won't show up if you are talking to the insecure port. the insecure port is not behind the aggregator. | ||
|
||
# Get the appropriate tls ca/cert/keys from kube | ||
# TODO how do I know which ones to get? Are they ones given to specific flags on the apiserver, etc etc? |
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.
You don't need to use the main CA, but if you want to, it's the cert passed with the --root-ca-file
flag to the controller-manager.
export NAME_SPACE=catalog | ||
export SERVICE_NAME=catalog-catalog-apiserver | ||
export ALT_NAMES="\"${SERVICE_NAME}.${NAME_SPACE}\",\"${SERVICE_NAME}.${NAME_SPACE}.svc"\" | ||
echo '{"CN":"'${SERVICE_NAME}'","hosts":['${ALT_NAMES}'],"key":{"algo":"rsa","size":2048}}' | cfssl gencert -ca=${SERVINGCA_CERT} -ca-key=${SERVINGCA_KEY} -config=server-ca-config.json - | cfssljson -bare apiserver |
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.
@DirectXMan12 do -ca=
and -ca-key=
have to belong to the same CA set?
Is this where I would switch out one or both of them to change the CA in the bundle?
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.
yeah, they should be each other's public/private cert/key. Switch out both to sign the serving certs with a different CA.
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.
make sense, just checking. I wish there was a sort of 'bundled form' but that's probably bad practice somehow because you want to expose one and not the other. Better that they are different files.
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.
yeah, it's like public and private SSH keys -- you (generally) wouldn't want them bundled together, because one shouldn't be passed around ;-)
failure is probably the missing header for my 'script' Okay, I was under the misconception that the CA had to be the core CA, and so now the instructions have two paths.
I scriptified the first flow, but you need cfssl tools to run it. Not bulletproof. The 'user-configurable' parts are the variables |
@MHBauer travis failure is legit:
|
@@ -1,5 +1,5 @@ | |||
{{- if .Values.useAggregator }} | |||
apiVersion: apiregistration.k8s.io/v1alpha1 | |||
apiVersion: apiregistration.k8s.io/v1beta1 |
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.
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.
@MHBauer I don't know of a trick, but it would have to but we can ask the user to specify it in another helm value.
Also, you could ask in the Kubernetes Slack #helm-dev channel to see if there's a trick
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.
At this point, I am most interested in the 1.7 use-case. I would rather just keep this PR focused on 1.7
contrib/api-setup.sh
Outdated
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. |
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.
@pmorie what's wrong with the boilerplate now?
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.
your shebang has no bang, it looks like?
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.
whoops.
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.
Looks like a promising start - I'd prefer to merge only once it has complete sentences. Left a couple suggestions
contrib/api-setup.sh
Outdated
# distributed under the License is distributed on an "AS IS" BASIS, | ||
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
# See the License for the specific language governing permissions and | ||
# limitations under the License. |
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.
your shebang has no bang, it looks like?
contrib/api-setup.sh
Outdated
# See the License for the specific language governing permissions and | ||
# limitations under the License. | ||
|
||
export HELM_NAME=catalog |
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.
We should probably make a note that catalog
is currently expected as the namespace.
Related: #926
docs/api-aggregation-setup.md
Outdated
@@ -0,0 +1,198 @@ | |||
|
|||
# generic aggregator information here |
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.
'Setting up the Kubernetes API aggregator'
docs/api-aggregation-setup.md
Outdated
For background on why we're messing with certificates and CA's at all, | ||
please check [the auth doc](auth.md). | ||
|
||
# steps |
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.
Capitalize headers consistently, plz :)
docs/api-aggregation-setup.md
Outdated
# steps | ||
|
||
## Prerequisites | ||
Need cfssl tools, install with `go get -u github.com/cloudflare/cfssl/cmd/...`. |
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.
You need
docs/api-aggregation-setup.md
Outdated
cp /var/run/kubernetes/${SERVINGCA_CERT} . | ||
``` | ||
|
||
### TODO explain what we're setting up |
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.
Are you planning to address these TODOs in this PR?
I took another pass through the doc. Should be closer to ready. Removed the RBAC info to cover it in a separate document. |
docs/api-aggregation-setup.md
Outdated
|
||
# Setting up Service Catalog for API Aggregation in Kubernetes | ||
|
||
The aggregator is a server that sits in front of everything. |
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.
Might be nice to mention that what you mean is the aggregator sits in front of the apiserver
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.
Also would be helpful if you had 1-2 sentences that motivate the need for an aggregator. What problem does having an aggregator solve?
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.
Made some doc suggestions that might help folks to understand why an aggregator would benefit them. Great job!
docs/api-aggregation-setup.md
Outdated
`apiregistration.k8s.io/v1alpha1` if running in kubernetes v1.6) | ||
|
||
This won't show up if you are talking to the insecure port. The | ||
insecure port is not behind the aggregator. |
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'd prefer to make this a little more concrete since it could imply that we chose to not put it behind the aggregator... how about As of now the aggregator only supports routing of requests sent to the secure port
or something like that. Make it clear that its not our choice/config-option - its the design of the aggregator.
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.
It's as concrete as is possible. It's a statement of fact.
docs/api-aggregation-setup.md
Outdated
echo '{"CN":"'${SERVICE_NAME}'","hosts":['${ALT_NAMES}'],"key":{"algo":"rsa","size":2048}}' | cfssl gencert -ca=${SERVINGCA_CERT} -ca-key=${SERVINGCA_KEY} -config=server-ca-config.json - | cfssljson -bare apiserver | ||
``` | ||
|
||
### put the output we need for running our apiserver behind the aggregator |
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.
s/put/Put/ for consistency
docs/api-aggregation-setup.md
Outdated
## Install the Service Catalog with Helm Charts | ||
|
||
|
||
Use helm to install the Service Catalog into the configured name and |
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.
"into the configured name" sounds odd- almost like "name" is a resource. Did you mean "Use helm to install the Service Catalog, associating it with the configured name ${HELM_NAME}, and into the specified namespace." ?
docs/api-aggregation-setup.md
Outdated
|
||
If it doesn't show up the kubectl discovery cache is stale and needs | ||
to be deleted. It may be located in the `.kube` directory, | ||
approximately `.kube/cache/discovery/`. |
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.
perhaps add "$HOME" before ".kube/..." ?
docs/api-aggregation-setup.md
Outdated
to be deleted. It may be located in the `.kube` directory, | ||
approximately `.kube/cache/discovery/`. | ||
|
||
Now e2e should work with the same kubeconfig set for both the core |
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.
s/e2e/service catalogs's e2e/
docs/api-aggregation-setup.md
Outdated
helm install /root/go/src/github.com/kubernetes-incubator/service-catalog/charts/catalog \ | ||
--name ${HELM_NAME} --namespace ${SVCCAT_NAMESPACE} \ | ||
--set apiserver.auth.enabled=true \ | ||
--set useAggregator=true \ |
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.
todo: check if this is only needed for rbac or otherwise what it does.
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.
Overall LGTM; I went through these instructions myself and got things working. Left a couple nits/suggestions.
@@ -1,5 +1,5 @@ | |||
{{- if .Values.useAggregator }} | |||
apiVersion: apiregistration.k8s.io/v1alpha1 | |||
apiVersion: apiregistration.k8s.io/v1beta1 |
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.
At this point, I am most interested in the 1.7 use-case. I would rather just keep this PR focused on 1.7
contrib/api-setup.sh
Outdated
@@ -0,0 +1,61 @@ | |||
#!/bin/sh -- |
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.
extremely minor nit: can we call this script apiserver-ssl-setup.sh
?
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.
Also, I don't think this script is referenced from your write-up. Is that intentional?
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 can call it whatever you want.
I pulled the script out at the last minute. Probably ought to mention it.
|
||
### Create our own new CA and generate keys | ||
|
||
This is an example. It is written with zero understanding of the best |
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 would actually like to leave this comment in, and I think @MHBauer is right to advertise that this might not be good-practice-compliant.
various files may be named differently and in different locations. | ||
|
||
|
||
``` |
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 running through these instructions now.
I think it would be good to clarify what directory it's expected that these commands should be executed in.
``` | ||
|
||
### Use the existing keys plus the config file to generate the new signing certificate and key | ||
|
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 would note here that you must install the catalog into the right namespace for the certificate to be signed correctly for the service's DNS name.
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.
Yea, I think I wrote that down earlier, but it doesn't seem super clear that all of these things have to match and why.
export SC_SERVING_CA=${SVCCAT_CA_CERT} | ||
export SC_SERVING_CERT=apiserver.pem | ||
export SC_SERVING_KEY=apiserver-key.pem | ||
``` |
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.
Nit: I think if you go this route the APIService resource needs to set InsecureSkipTLSVerify
since the CA will necessarily not be in the aggregator's trust store.
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 don't understand what you want me to do. I haven't encountered any problems with the script or setup as it is, but I do want to cover as wide of cases as possible.
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 think that for now, what you have is actually fine. I have a specific thing that I realized is more for when we do this in the walkthrough
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.
Created kubernetes/kubectl#31 for an issue I found when I messed up the namespace running through these instruction
|
||
### Create our own new CA and generate keys | ||
|
||
This is an example. It is written with zero understanding of the best |
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.
That should not block this PR being merged. I have no clue about best practices for SSL certs and this moves things in the right direction.
Updated and squashed. |
@@ -1,5 +1,9 @@ | |||
{{- if .Values.useAggregator }} | |||
{{- if .Capabilities.APIVersions.Has "apiregistration.k8s.io/v1beta1" }} | |||
apiVersion: apiregistration.k8s.io/v1beta1 | |||
{{- else if .Capabilities.APIVersions.Has "apiregistration.k8s.io/v1alpha1" }} |
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.
This is ugly but it works.
docs/api-aggregation-setup.md
Outdated
and provides the keys we just generated inline. | ||
|
||
``` | ||
helm install /root/go/src/github.com/kubernetes-incubator/service-catalog/charts/catalog \ |
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.
let's use a relative path here, assuming the user is executing from the root of the repository (./charts/catalog
)
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.
Sure. Last vestiges of this being a script that I ran myself on my machine.
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.
@arschles any thoughts on using ${GOPATH} as reference? Probably best to still just pretend we're in the project root.
@kibbles-n-bytes tiller didn't come up. How do I restart jenkins? |
--name ${HELM_NAME} --namespace ${SVCCAT_NAMESPACE} \ | ||
--set apiserver.auth.enabled=true \ | ||
--set useAggregator=true \ | ||
--set apiserver.tls.ca=$(base64 --wrap 0 ${SC_SERVING_CA}) \ |
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.
@MHBauer there is no ca
field in the values.yaml
file under apiserver.tls
. Can you add one with an empty value and a comment to describe what it is?
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.
@DirectXMan12 any suggestions here? We use it in charts/catalog/templates/apiregistration.yaml
I'm thinking
# Base64-encoded certificate authority for the APIService object to register
# the Service Catalog API with the apiregistration
@arschles Not sure what to use as a value. Should I generate a new set of matching stuff and replace everything tls related? I'm not sure what's in there now and don't want to break everything.
@@ -0,0 +1,61 @@ | |||
#!/bin/sh -- |
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'd prefer to remove the --
since it'll cause people to wonder what the heck we're up to when it serves no purpose what so ever. Unless we want to just mess with people's heads, then I'm all for it! ;-)
`source`ed to define all of the variables it contains in the current | ||
shell process. | ||
|
||
The aggregator is a new feature of kubernetes and is an alpha API |
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.
missing server
after API
. I'd move this paragraph up to the top where brad made his comments.
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.
it's an api not a server.
- A script to setup the certificates needed for API Aggregations - Change to the apiregistration as it is now a beta API - An informational file to describe what we are doing and why I wish to thank these people - @DirectXMan12 for the auth doc, and walking me through cert generation - @liggitt for help with troubleshooting RBAC and a way to disable RBAC - @ivan4th for Mirantis/kubeadm-dind-cluster
Lost some content from an earlier rebase. Only documentation changes. Ought to still be green. |
@@ -9,6 +9,9 @@ apiserver: | |||
# are some outstanding problems with the TLS-secured endpoint | |||
insecure: true | |||
tls: | |||
# Base64-encoded certificate authority for the APIService object to register | |||
# the Service Catalog API with the apiregistration | |||
ca: "" |
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.
👍
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 haven't had the chance to thoroughly test this, but it's a great start and I would prefer to get it merged now and for us to build on this foundation.
Also, I'd like to reiterate how excited and happy I am for us to fully utilize the aggregator. Thanks for getting us started @MHBauer
LGTM
#952 to follow up on the additional CI work needed. |
mad help from @DirectXMan12 and @liggitt. could not have been anywhere close without them. shoutout to @ivan4th for Mirantis/kubeadm-dind-cluster which has been wicket helpful.
These instructions allow me to set both kubeconfigs of e2e to the same file and successfully pass.
lots of todos embedded in the short doc
need to look at feasibility of I, myself doing the integration in jenkins or leaving for a follow up
apiregistration api is beta in 1.7, alpha in 1.6, so if we intend to support this on 1.6 we need to both show how to set up the aggregator (or at least link to it) and have a switch for the flag based on the k8s version.
RBAC was whack a mole for a while. I need to turn it on in local-up-cluster and finish figuring out all the permissions, or otherwise list them all out by reviewing the code with someone.
Additionally to figuring out our process, we need to do a general guide for other apiservers that may exist in the future. I think it sounds related to the apiserver starter kit. @pwittrock WDYT?