WIP: Initial kubeadm api proposal #100

luxas · 2017-01-02T14:33:32Z

This is far from done, but posting progress on my work on the kubeadm API here.

This is kind of a kubeadm-to-beta design doc which we have to discuss in detail.
I'll improve it as we go.

cc @jbeda @mikedanese @dgoodwin @errordeveloper @lukemarsden @dgoodwin @kad @roberthbailey

mikedanese · 2017-01-03T19:05:28Z

docs/design/kubeadm-api.md

+
+First, certificates should be created in a directory.
+There should be:
+ - a CA certificate (`ca.pem`) with its private key (`ca-key.pem`)


nit: we should try to name these ca.key and ca.crt. While they are pem encoded, these are the more common names for this type of file.

Agree, updated the proposal and will update the code as well

mikedanese · 2017-01-03T19:05:40Z

docs/design/kubeadm-api.md

+First, certificates should be created in a directory.
+There should be:
+ - a CA certificate (`ca.pem`) with its private key (`ca-key.pem`)
+   - Is the public key necessary to have here (`ca-pub.pem`)?


ca.crt contains the public key.

Yes, that was what I thought. I just couldn't understand why we had both ca.pem and ca-pub.pem, I'm gonna remove the public key only one because it's unnecessary

mikedanese · 2017-01-03T19:06:33Z

docs/design/kubeadm-api.md

+ - a CA certificate (`ca.pem`) with its private key (`ca-key.pem`)
+   - Is the public key necessary to have here (`ca-pub.pem`)?
+ - an API Server certificate (`apiserver.pem`) signed with `ca.pem` as the CA with its private key (`apiserver-key.pem`)
+ - a Service Account private key that's used for signing Service Accounts' tokens.


We've used the apiserver.key to do this so far. It works fine so I suggest dropping sa.key

What's the difference between having a dedicated private key or reusing the apiserver's?
Is there any difference in security?

mikedanese · 2017-01-03T19:07:12Z

docs/design/kubeadm-api.md

+   - TODO: Is the certificate for the serviceaccount private key necessary?
+
+Also, we should make sure that both RSA and ECDSA private keys work with kubeadm and have tests for both.
+We probably want to expose the private key bitsize also.


How and why?

If we set the bitsize to be large enough, we shouldn't need to expose other options (it's just more confusing/complexity for users). If you really want a different bitsize, you should follow the "bring your own cert" path instead of letting us generate certs, as then you have complete control over the algorithm and the bitsize.

Thanks, I've been trying to find the balance all the time for what should and should not be exposed.
While reading your arguments, it's clear that this falls under the bring-your-own certs thing.
I appreciate that feedback extra much

mikedanese · 2017-01-03T19:08:28Z

docs/design/kubeadm-api.md

+Also, we should make sure that both RSA and ECDSA private keys work with kubeadm and have tests for both.
+We probably want to expose the private key bitsize also.
+
+One more option that could be useful to many would be to let the user specify their own ca.pem and ca-key.pem, 


Hmm, I thought this was already possible. This is absolutely a requirement.

If these exist in the location that kubeadm would create them, then kubeadm should just use them. No need to copy. This should be the general pattern for how we implement phases w.r.t. reentrance

It's not possible right now (like other things that are half-implemented). Updated the statement.

mikedanese · 2017-01-03T19:14:33Z

docs/design/kubeadm-api.md

+`admin.conf` and `kubelet.conf` in `/etc/kubernetes/` by default.
+
+A general question here, do we want the KubernetesDir to be configurable at all or does it only add complexity/confusion if a user ever changed that path?
+I lean towards having that as a constant, since that seems to be the case with a lot of other deployments and the gain is very small.


I also lean towards leaving this unconfigurable. If there is an uproar we can always make it configurable later.

Cool, then we agree 👍

mikedanese · 2017-01-03T19:16:18Z

docs/design/kubeadm-api.md

+
+	// Phases
+	Certificates   Certificates   `json:"certificates"`
+	MakeKubeConfig MakeKubeConfig `json:"makeKubeConfig"`


Drop the Make verb. The naming of the phases should be consistent. Also consider a Phases subobject where these are pointers so we can infer whether they are provided.

fixed, let's discuss the second sentence more generally

roberthbailey · 2017-01-03T19:14:28Z

docs/design/kubeadm-api.md

+## Kubeadm Phases and API
+
+### Refactoring kubeadm
+We all want kubeadm modular and componentized, here’s the proposal for that!


roberthbailey · 2017-01-03T19:14:58Z

docs/design/kubeadm-api.md

+
+### Refactoring kubeadm
+We all want kubeadm modular and componentized, here’s the proposal for that!
+I’m not writing down all reasons for doing this; they are already available in the blog post: https://docs.google.com/document/d/1bpEGhdg8t-2Ae-IL_ezZIPklMHpZLWwnoAPMgLwT3oU


linkify blog post rather than putting the https after the :

It should really soon be up on the site so waiting a little bit before updating that, it was never meant to be the doc, but in absence of the real blog post, I temporarily linked to the doc

roberthbailey · 2017-01-03T19:16:22Z

docs/design/kubeadm-api.md

+
+So far (for kubeadm) I've identified these:
+
+#### Certificates


@cjcullen

We should also create certificates for master -> kubelet connections, which for kube-up we are doing with a second "CA universe".

Where is the code for this? Is using a separate CA required?
I'd very much like to have more information about how you have it setup on GCE regarding that.

roberthbailey · 2017-01-03T19:19:18Z

docs/design/kubeadm-api.md

+   - TODO: Is the certificate for the serviceaccount private key necessary?
+
+Also, we should make sure that both RSA and ECDSA private keys work with kubeadm and have tests for both.
+We probably want to expose the private key bitsize also.


If we set the bitsize to be large enough, we shouldn't need to expose other options (it's just more confusing/complexity for users). If you really want a different bitsize, you should follow the "bring your own cert" path instead of letting us generate certs, as then you have complete control over the algorithm and the bitsize.

roberthbailey · 2017-01-03T19:20:54Z

docs/design/kubeadm-api.md

+that require a specific CA but lets the apiserver and serviceaccount keys/certs be generated by kubeadm.
+
+**Inputs**
+ - `AdvertiseAddresses` is needed for knowing which IPs the certs should be signed for


Why are these inputs separated this way? They are all specifying "names which should be signed" and should be possible to group into a single input (and let the code determine if they are ip addresses or dns names and do the right thing).

This was just the way the arguments are exposed now, my intention was indeed to have just a common field for both DNS and IPs. Suggestions on the name?

roberthbailey · 2017-01-03T19:24:22Z

docs/design/kubeadm-api.md

+I lean towards having that as a constant, since that seems to be the case with a lot of other deployments and the gain is very small.
+
+There also is the question whether this phase should generate both the kubelet and the admin config. Or should those be two different phases/steps?
+How can we make the kubelet have just a little permissions but still use it for bootstrapping the api server? Can start the master ("bootstrap") kubelet with full access, 


Does bootkube do anything here? I agree that we want the kubelet to end up with "dropped permissions" even if it has to start out with root permissions to get things up and running.

Nope, bootkube has only one kubeconfig file for everything as I interpreted the code

luxas · 2017-01-05T14:59:32Z

Thanks @roberthbailey and @mikedanese!

Answered your comments and updated a somehow updated version.
Much still to write down for me however.

I appreciate the comments; let's do this together and come up with a rock-solid API!

pires

I am concerned about the API clarity as while reviewing it, it seemed somewhat confusing.

The document, so far, lgtm.

pires · 2017-01-05T18:59:57Z

docs/design/kubeadm-api.go

+	metav1.TypeMeta `json:",inline"`
+
+	// Data shared between phases. Used for defaulting
+	Master     Master     `json:"master"`


MasterConfiguration.Master is not clear about its intents. Why is it there in the first place?

This is not by far done; I'm trying to see how I can move stuff around to get rid of it

Leftover from the current api

pires · 2017-01-05T19:06:31Z

docs/design/kubeadm-api.go

+	CertificatesDir string `json:"certificatesDir"`
+
+	// Phases
+	Certificates     Certificates     `json:"certificates"`


Certificates is not clear about its intents. If we have certificates then why do we need CertificatesDir? And while I may understand the design, the API itself doesn't seem self-explanatory and I believe will create confusion.

We might group all phases under a Phase(s) struct to make things more clear.
Here, Certificates stands for the first phase whose task is to create certs.
CertificatesDir is a param to the Certificates phase that says where certs should be put

pires · 2017-01-05T19:08:53Z

docs/design/kubeadm-api.go

+
+// Shared
+// TODO: This field should be HA friendly
+type Master struct {


MasterParameters or similar?

Yes, this should change indeed.

MasterConfiguration is the convention we use for other componentconfig structs.

yes, that stays the same.

This Master struct that's here was named API before, but should not exist at all I think

luxas · 2017-01-05T19:34:03Z

The main problem here @pires is that we want to avoid duplication in the API while we still want to allow different phases. A simple example:

Phase A needs input foo and bar. If we just run phase A we give the PhaseA struct to kubeadm and it will run that phase, example:

kubeadm phase phasea --config a.yaml

a.yaml

phaseA:
   foo: "abc"
   bar: "def"

This is pretty straightforward, right? (When there are real values of course)

Then imagine Phase B, that takes bar and baz as inputs. Oops, the param bar is required by both phases. Anyway, it doesn't matter when running only phase B:

kubeadm phase phaseb --config b.yaml

b.yaml

phaseB:
   bar: "def"
   baz: "ghi"

This is yet ok. What's problematic though, is when two or more phases share input args and kubeadm init is run, that does everything.

Then we'll have something like this:

kubeadm init --config init.yaml

init.yaml

masterConfiguration:
  phaseA:
    foo: "abc"
    **bar: "def"**
  phaseB:
    **bar: "def"**
    baz: "ghi"

Now things get messy. As an user, I shouldn't have to make sure that the bar values are consistent in both places, I don't care. I just want to specify it once. But code-wise, we have to get that value from somewhere.

We could do like this, and let the phase-individual bar params take input from a shared, top-level bar param, but that's very confusing (now we have three properties!):
init.yaml

masterConfiguration:
  bar: "def"
  phaseA:
    foo: "abc"
    <bar is unset, but the field exists here and defaults to the top-level value>
  phaseB:
    <bar is unset, but the field exists here and defaults to the top-level value>
    baz: "ghi"

Do anyone have a suggestion how we should design so we don't face that problem?
@mikedanese ^?

…exist) and only four files instead of 8 previously. Basically implements what has been discussed so far in kubernetes/kubeadm#100

@mikedanese

Automatic merge from submit-queue (batch tested with PRs 39199, 37273, 29183, 39638, 40199) Refactor/improve the kubeadm generation of certificates **What this PR does / why we need it**: Continues to refactor/improve kubeadm towards beta. **Special notes for your reviewer**: Modify the certs that are generated; generate on demand (if not exist) and only four files instead of eight previously. Basically implements what has been discussed so far in kubernetes/kubeadm#100 **Release note**: ```release-note NONE ``` cc @mikedanese @pires @lukemarsden @errordeveloper @dgoodwin @roberthbailey

mikedanese · 2017-01-30T16:40:38Z

docs/design/kubeadm-api.go

+	Addons           Addons           `json:"addons"`
+}
+
+type Phase struct {


PhaseMetadata?

luxas · 2017-02-21T19:19:09Z

Closing in favor for: #156

WIP: Initial kubeadm api proposal

acc77f5

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

luxas mentioned this pull request Jan 3, 2017

Refactor the certificate and kubeconfig code in the kubeadm binary into two phases kubernetes/kubernetes#39280

Merged

mikedanese self-assigned this Jan 3, 2017

mikedanese reviewed Jan 3, 2017

View reviewed changes

roberthbailey reviewed Jan 3, 2017

View reviewed changes

wip

bbbbfd2

pires reviewed Jan 5, 2017

View reviewed changes

luxas mentioned this pull request Jan 9, 2017

Refactor/improve the kubeadm generation of certificates kubernetes/kubernetes#39638

Merged

luxas mentioned this pull request Jan 13, 2017

Add authorization mode to kubeadm kubernetes/kubernetes#39846

Merged

This was referenced Jan 16, 2017

Kubeadm should support running master as unschedulable #115

Closed

Completely redesign the API configuration group #117

Closed

wip: update the proposal a little

1c95a16

wip

e5ebbd3

mikedanese reviewed Jan 30, 2017

View reviewed changes

docs/design/kubeadm-api.go

Addons Addons `json:"addons"`

}

type Phase struct {

Copy link

Member

mikedanese Jan 30, 2017

Choose a reason for hiding this comment

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

PhaseMetadata?

wip2

2b616a4

luxas mentioned this pull request Feb 7, 2017

WIP: kubeadm: First stab at the kubeadm beta API: add a Certificates Phase object kubernetes/kubernetes#41098

Closed

luxas mentioned this pull request Feb 8, 2017

Make /etc/kubernetes/ a constant #147

Closed

luxas closed this Feb 21, 2017

yuvipanda mentioned this pull request Jun 14, 2017

api-server tries to talk to a random IP address instead of itself #305

Closed

NeilW mentioned this pull request May 7, 2020

document the usage of --service-cidr and improve validation #2132

Closed


		So far (for kubeadm) I've identified these:

		#### Certificates

WIP: Initial kubeadm api proposal #100

WIP: Initial kubeadm api proposal #100

Conversation

luxas commented Jan 2, 2017

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

mikedanese Jan 3, 2017 • 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

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

mikedanese Jan 3, 2017 • 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

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

luxas commented Jan 5, 2017

pires 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

luxas commented Jan 5, 2017 • edited Loading

Choose a reason for hiding this comment

luxas commented Feb 21, 2017

mikedanese Jan 3, 2017 •

edited

Loading

mikedanese Jan 3, 2017 •

edited

Loading

luxas commented Jan 5, 2017 •

edited

Loading