Allow bootstrap configuration to be configured and reentrant by smarterclayton · Pull Request #16571 · openshift/origin

smarterclayton · 2017-09-26T20:47:33Z

Make bootstrapping a real production node possible.

Simplify and streamline the process whereby the bootstrap config is looked up.
1. --bootstrap-config-name can be used to customize which config is looked up (one per node group)
2. Any failure in fetching node config results in termination of the pass - no client side defaulting
3. Handle 0.0.0.0 in dnsIP as a special case which results in the local node IP lookup
Backport the one remaining cert rotation patch and make client and server node side cert rotation mandatory when bootstrapping
1. Fix a number of small issues upstream where node bootstrapping is not reentrant
2. pass cert-dir to the kubelet instead of passing individual key and cert, which was preventing rotation from working
Make openshift start network work podified
1. Allow kubeconfig to be specified on the CLI and override the node-config
2. The proxy healthz was not starting due to missing config - this has been corrected
3. Provide a daemonset example that correctly starts OpenShift SDN in a pod
4. Fix a few minor bugs in openshift-sdn

There is still one bug outstanding upstream that can be fixed separately - the kubelet client rotation can fail due to the cert expiring and be unable to get new certs, so it never exits.

Tested the following scenario extensively (requires a new openshift/node image tagged as v3.7.0-alpha.1):

Update master-config with cert signing on on 10m rotation
create a node-config oc create configmap -n openshift-node node-config --from-file=node-config.yaml=contrib/kubernetes/default-node-config.yaml
start node in bootstrapping mode openshift start node --bootstrap-config-name=node-config --config=/etc/origin/node/node-config.yaml --enable=kubelet --loglevel=3 (which has it run only the kubelet)
run a background oc observe csr -- oc adm certificate approve to approve both csr
run oc create -f contrib/kubernetes/static/network-policy.yaml
run oc create -f contrib/kubernetes/static/network-daemonset.yaml
verify the daemonset starts correctly and that it passes health checks
launch a new pod and verify it has dns oc run --restart=Never --attach -it --image=centos:7 -- /bin/bash and then yum install bind-utils -y && dig +search kubernetes.default.svc
launch two pods in two namespaces and verify multi tenant SDN works

Follow up for the daemonset - openshift-sdn expects to have access to the dockershim.sock which this doesn't bind mount in.

openshift-merge-robot · 2017-09-26T20:47:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: smarterclayton

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

smarterclayton · 2017-09-26T21:06:47Z

@deads2k bootstrapping

enj · 2017-09-27T18:36:05Z

pkg/cmd/server/start/node_args.go

+	// BootstrapConfigName is the name of a config map to read node-config.yaml from.
+	BootstrapConfigName string
+	// BootstrapConfigNamespace is the namespace the config map for bootstrap config is expected to load from.
+	BootstrapConfigNamespace string


Why another configurable namespace? Sounds like another thing we will end up removing in the future.

This is configurable from the node, not from the master. Nodes are allowed to point to different namespaces.

This is configurable from the node, not from the master. Nodes are allowed to point to different namespaces.

I think the "why" question still stands? Why not be prescriptive, at least until someone has a compelling subdivision use-case.

Because we don't as a general rule lock clients into namespaces. I don't see a trend away from clients owning their destiny, and I don't believe in being overly restrictive here.

Because we don't as a general rule lock clients into namespaces. I don't see a trend away from clients owning their destiny, and I don't believe in being overly restrictive here.

summarizing in-person discussion: not configurable in the server, configurable in the client if someone really wants to wire it.

enj · 2017-09-27T18:38:34Z

pkg/cmd/server/bootstrappolicy/constants.go

 	NodeProxierRoleBindingName        = NodeProxierRoleName + "s"
 	NodeAdminRoleBindingName          = NodeAdminRoleName + "s"
 	NodeReaderRoleBindingName         = NodeReaderRoleName + "s"
+	NodeConfigReaderRoleBindingName   = NodeConfigReaderRoleName + "s"


No more + "s" please.

Why is it still there for the others?

Why is it still there for the others?

Legacy. We stopped doing it upstream.

Where's the comment in the code saying that?

We should probably separate into section with header and footer to that effect

enj · 2017-09-27T18:39:27Z

pkg/cmd/server/bootstrappolicy/policy.go

+
+func GetBootstrapNodeConfigProvisioningRoleBindings(namespace string) []rbac.RoleBinding {
+	return []rbac.RoleBinding{
+		newOriginRoleBindingForClusterRole(NodeConfigReaderRoleBindingName, NodeConfigReaderRoleName, namespace).


Nothing new should use newOriginRoleBindingForClusterRole

enj · 2017-09-27T18:40:09Z

pkg/cmd/server/bootstrappolicy/policy.go

 	"view",
 )
+
+func GetBootstrapNodeConfigProvisioningRoleBindings(namespace string) []rbac.RoleBinding {


No namespace configuration please.

deads2k · 2017-09-28T14:21:22Z

pkg/cmd/server/bootstrappolicy/policy.go

 	return ret
 }
+
+func GetBootstrapNodeConfigProvisioningRoleBindings(namespace string) []rbac.RoleBinding {


This namespace should be pinnned. @enj did you pull merge yet?

deads2k · 2017-09-28T14:22:15Z

pkg/cmd/server/bootstrappolicy/policy.go

 				// TODO: expose other things like /healthz on the node once we figure out non-resource URL policy across systems
 			},
 		},
+		{


This looks like it should be namespace scoped. #16517 made it easily possible

Lets make this a role since we remove the namespace configuration.

deads2k · 2017-09-28T14:22:44Z

pkg/cmd/server/origin/master.go

 	}

 	// add post-start hooks
+	aggregatedAPIServer.GenericAPIServer.AddPostStartHookOrDie("node.openshift.io-sharednamespace", c.ensureOpenShiftNodeNamespace)


after #16517 this won't be needed.

Because we auto init namespaces in the role bindings?

Switched to the shared mode.

Why is shared resource namespaces still its own hook?

Why is shared resource namespaces still its own hook?

cruft

deads2k · 2017-09-28T14:23:57Z

pkg/cmd/server/start/bootstrap_node.go

 	// try to refresh the latest node-config.yaml
 	o.ConfigFile = filepath.Join(nodeConfigDir, "node-config.yaml")
-	config, err := c.Core().ConfigMaps("kube-system").Get("node-config", metav1.GetOptions{})
+	config, err := c.Core().ConfigMaps(o.NodeArgs.BootstrapConfigNamespace).Get(o.NodeArgs.BootstrapConfigName, metav1.GetOptions{})


doesn't seem like the namespace should be configurable. Name yes, but I don't see why we'd let people change the namespace.

deads2k · 2017-09-28T14:27:08Z

pkg/cmd/server/start/bootstrap_node.go

 	// try to refresh the latest node-config.yaml
 	o.ConfigFile = filepath.Join(nodeConfigDir, "node-config.yaml")
-	config, err := c.Core().ConfigMaps("kube-system").Get("node-config", metav1.GetOptions{})
+	config, err := c.Core().ConfigMaps(o.NodeArgs.BootstrapConfigNamespace).Get(o.NodeArgs.BootstrapConfigName, metav1.GetOptions{})


you're re-using this error down below. Please give it a special name.

you're re-using this error down below. Please give it a special name.

error name still outstanding

deads2k · 2017-09-28T14:29:42Z

pkg/cmd/server/start/bootstrap_node.go


-	return err
+	// if there is no node-config.yaml and no server config map, generate one
+	glog.V(2).Infof("Generating a local configuration since no server config available")


"since no server or local config available"

deads2k · 2017-09-28T14:31:55Z

pkg/cmd/server/start/start_kube_controller_manager.go

 	}
 	if _, ok := cmdLineArgs["experimental-cluster-signing-duration"]; !ok {
-		cmdLineArgs["experimental-cluster-signing-duration"] = []string{"0s"}
+		cmdLineArgs["experimental-cluster-signing-duration"] = []string{"720h"}


I'd prefer much shorter to flush out refreshing errors. o(12 hours)

It doesn't work in 3.7 for client certs so we're going to do it on the node level or backport the required changes. Also, the goal is to get to bootstrapping first, then tighten rotation, so I don't want to block on that.

It doesn't work in 3.7 for client certs so we're going to do it on the node level or backport the required changes. Also, the goal is to get to bootstrapping first, then tighten rotation, so I don't want to block on that.

This looks like a time bomb. Are you saying you're going to be manually refereshing certificates or something?

Uh so what happens after a month?

deads2k · 2017-09-28T14:35:26Z

pkg/cmd/server/start/bootstrap_node.go

-			return err
-		}
-		return nil
+	switch {


if you are suppose to bootstrap and you can't, I think you should fail, not use a local config instead.

Fair. Although should probably be based on whether you asked for a config or not.

I'm torn on this. An empty config map should result in a default. A missing config map or typod map seems like an error. But at the same time a 403 or other error (especially for the default config) seems borderline arbitrary. A node having a config you didn't expect is bad, a node being down because of a correlated auth mixup also seems bad.

simo5 · 2017-09-28T15:29:06Z

This PR does a lot more than "correcting small issues", for example it permits to configure stuff that some people believe should be hard coded (eg namespaces).

Can you please give a better explanation of what is the aim of this PR @smarterclayton ?

tiran · 2017-09-28T15:40:02Z

@smarterclayton Could you please explain why you are proposing --bootstrap-config-name and --bootstrap-config-namespace? The PR message doesn't contain an explanation. @openshift/sig-security doesn't understand why the new options are necessary and which value are added by them.

@enj is also not keen on configurable config namespace.

smarterclayton · 2017-09-28T23:14:38Z

Nodes configure themselves against masters. Nodes need to be able to bulk configured to a class of config (name). We don't hardcode namespaces for config lookup in general principle. Admins are free to point configuration elsewhere. There is nothing special about the config source location - especially for nodes that may be owned by separate security partitions. Bootstrapping is the replacement for static config and will be the default mode for online 3.7+ and future openshift deployments. The option to statically deliver config is preserved for bare metal environments.

smarterclayton · 2017-09-29T00:50:59Z

Seeing multiple rebinds using the new way for namespace mappings: #16611

smarterclayton · 2017-09-29T01:16:29Z

Note that I in general agree with the "don't make things configurable that you don't want to be configurable". On the server side, the namespaces we create in bootstrap and our core setup definitely fall under that. But compiling in our binaries that act as clients of the system to say "I will not accept results that aren't from a precanned namespace" crosses the line a bit into being too forceful. If someone wants to bind their node-config with their kubelet config and set it in kube-system because that's the only place the new kubelet will look, I'm ok with that being an option for a client.

…

On Thu, Sep 28, 2017 at 8:39 PM, OpenShift CI Robot < ***@***.***> wrote: @smarterclayton <https://github.com/smarterclayton>: The following tests *failed*, say /retest to rerun them all: Test name Commit Details Rerun command ci/openshift-jenkins/extended_conformance_gce 9a6fa55 <9a6fa55> link <https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/16571/test_pull_request_origin_extended_conformance_gce/8768/> /test extended_conformance_gce ci/openshift-jenkins/integration 8b7da17 <8b7da17> link <https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/16571/test_pull_request_origin_integration/8603/> /test integration Full PR test history <https://openshift-gce-devel.appspot.com/pr/16571>. Your PR dashboard <https://openshift-gce-devel.appspot.com/pr/smarterclayton>. Please help us cut down on flakes by linking to <https://github.com/kubernetes/community/blob/master/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests> an open issue <https://github.com/openshift/origin/issues?q=is:issue+is:open> when you hit one in your PR. Instructions for interacting with me using PR comments are available here <https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md>. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:> repository. I understand the commands that are listed here <https://github.com/kubernetes/test-infra/blob/master/commands.md>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16571 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p5PiRustT6_5O5CCsFnvsKxHuMaDks5snDwkgaJpZM4Pk3C7> .

deads2k · 2017-10-10T13:32:18Z

contrib/kubernetes/default-node-config.yaml

+  - RotateKubeletClientCertificate=true,RotateKubeletServerCertificate=true
+masterClientConnectionOverrides:
+  acceptContentTypes: application/vnd.kubernetes.protobuf,application/json
+  burst: 40


qps looks ok, but burst looks small

Actually our default.

deads2k · 2017-10-10T13:48:54Z

contrib/kubernetes/static/network-daemonset.yaml

+      hostPID: true
+      containers:
+      - name: network
+        image: openshift/node:v3.7.0-alpha.1


not a parameterized version/tag?

Just an example, wanted to have the larger discussion about how static config flows from openshift/origin -> ansible first. Wanted to have something checked in that should be reproducible for a bit, then will be made formal and moved out of here.

deads2k · 2017-10-10T14:06:14Z

pkg/cmd/server/kubernetes/network/network.go

+	// Start up a metrics server if requested
+	if len(c.ProxyConfig.MetricsBindAddress) > 0 {
+		mux := http.NewServeMux()
+		mux.HandleFunc("/proxyMode", func(w http.ResponseWriter, r *http.Request) {


name it alpha or something? this is like config-lite.

This is consistent with upstream, so we can't rename it.

deads2k · 2017-10-10T14:16:47Z

pkg/cmd/server/start/node_args.go

 	// VolumeDir is the volume storage directory.
 	VolumeDir string
+	// VolumeDirProvided is set to true if the user has specified this flag.
+	VolumeDirProvided bool


Hold your nose a month or so longer and most of this code dies and goes away and we're all happy.

deads2k · 2017-10-11T12:54:58Z

I still think you should remove --bootstrap. I don't think it's adding value. Making them choose a particular config seems like the right thing to do.

lgtm otherwise.

smarterclayton · 2017-10-12T02:55:48Z

/test all

Updated to remove the --bootstrap flag.

The cert may have expired.

When we mount /var/run/openshift-sdn into the container, we need to be able to clear its contents but the directory itself cannot be removed as it is a mount point. Also clarify one error.

This makes running in a separate process for networks able to have a health check and for metrics to be reported.

CFSSL throws an opaque error, and bootstrapping requires user intervention to configure anyway.

This allows the kubelet to be configured to load default configuration out of a known namespace. By default it is openshift-node/node-config. Correct an error in bootstrapping where errors weren't logged, and properly ignore forbidden errors when trying to load the config map. Add a better description of bootstrapping to openshift start node. Ensure the volume directory is correctly loaded from node-config during bootstrapping instead of being overwritten into the config directory. Enable client and server rotation on the node automatically when bootstrapping, and only do a client certificate creation (server bootstrapping done by kubelet only). This unfortunately requires setting a fake value in the node config that will be cleared later - as we are moving towards a future where node-config does not exist this entire section will likely go away. Relax validation on node-config to allow cert-dir to be provided instead of explicit certificates. bootstrap

Other config variants will be stored in this location. The new namespace ensures clean security isolation.

Need to be able to take node-config from bootstrap node. For openshift start network the --kubeconfig flag from the CLI overrides the value of masterKubeConfig in the provided node config. If the value is empty (like it is by default) the in-cluster-config is used. Reorganize the node startup slightly so there is even less overlap between kubelet and network. A future change will completely separate these two initialization paths.

openshift-ci-robot · 2017-10-12T15:12:43Z

@smarterclayton: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/origin/verify	`4e4e5ce`	link	`/test origin-verify`
ci/openshift-jenkins/origin/unit	`4e4e5ce`	link	`/test origin-ut`
ci/openshift-jenkins/experimental/integration	`d6a7486`	link	`/test origin-it`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

smarterclayton · 2017-10-12T16:04:19Z

/retest

smarterclayton · 2017-10-12T19:00:01Z

Applying label

openshift-merge-robot · 2017-10-13T10:24:39Z

Automatic merge from submit-queue.

openshift-merge-robot assigned juanvallejo and enj Sep 26, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 26, 2017

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 26, 2017

smarterclayton assigned deads2k and unassigned juanvallejo Sep 26, 2017

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2017

smarterclayton force-pushed the bootstrap_config branch from b76d21a to 6704770 Compare September 27, 2017 18:50

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2017

enj suggested changes Sep 27, 2017

View reviewed changes

smarterclayton force-pushed the bootstrap_config branch from 6704770 to 9a6fa55 Compare September 27, 2017 19:47

openshift-merge-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2017

deads2k reviewed Sep 28, 2017

View reviewed changes

smarterclayton force-pushed the bootstrap_config branch from 9a6fa55 to 8b7da17 Compare September 29, 2017 00:05

smarterclayton force-pushed the bootstrap_config branch from 8b7da17 to 51410f3 Compare September 29, 2017 03:47

openshift-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 29, 2017

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2017

smarterclayton force-pushed the bootstrap_config branch from d5b2d8e to d6a7486 Compare October 9, 2017 20:54

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2017

deads2k reviewed Oct 10, 2017

View reviewed changes

smarterclayton force-pushed the bootstrap_config branch from d6a7486 to d349f9b Compare October 12, 2017 00:04

smarterclayton added 11 commits October 12, 2017 10:35

UPSTREAM: 49899: Update the client cert used by the kubelet on expiry

3b88b4a

UPSTREAM: 53037: Verify client cert before reusing existing bootstrap

e7de2fd

The cert may have expired.

Allow resync in oc observe without --names

79750e9

Tolerate being unable to remove /var/run/openshift-sdn

1b2f999

When we mount /var/run/openshift-sdn into the container, we need to be able to clear its contents but the directory itself cannot be removed as it is a mount point. Also clarify one error.

The proxy health server should be on, it does not leak info

ae01595

This makes running in a separate process for networks able to have a health check and for metrics to be reported.

Allow network to cross compile on master

13ef9b9

Set a default certificate duration for bootstrapping

737d65b

CFSSL throws an opaque error, and bootstrapping requires user intervention to configure anyway.

Auto-create openshift-node and given nodes read on node-config

c9db154

Other config variants will be stored in this location. The new namespace ensures clean security isolation.

Add a prototypical network-daemonset

c3d5830

smarterclayton force-pushed the bootstrap_config branch from d349f9b to ae05ccd Compare October 12, 2017 14:38

smarterclayton added the lgtm Indicates that a PR is ready to be merged. label Oct 12, 2017

openshift-merge-robot merged commit 259bd33 into openshift:master Oct 13, 2017

This was referenced Nov 15, 2017

oc cluster join fail with 3.7.0-rc0 #17331

Closed

oc cluster join -- how to use? #13336

Closed

Conversation

smarterclayton commented Sep 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-merge-robot commented Sep 26, 2017

Uh oh!

smarterclayton commented Sep 26, 2017

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!

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!

simo5 commented Sep 28, 2017

Uh oh!

tiran commented Sep 28, 2017

Uh oh!

smarterclayton commented Sep 28, 2017 via email

Uh oh!

smarterclayton commented Sep 29, 2017

Uh oh!

smarterclayton commented Sep 26, 2017 •

edited

Loading

openshift-ci-robot commented Oct 12, 2017 •

edited

Loading