Modify nodes to register directly with the master. #6949

roberthbailey · 2015-04-17T01:28:31Z

roberthbailey · 2015-04-17T01:28:40Z

roberthbailey · 2015-04-17T01:29:51Z

pkg/tools/etcd_helper.go

@@ -165,12 +165,14 @@ func (h *EtcdHelper) ExtractObjToList(key string, listObj runtime.Object) error
 	}

 	nodes := make([]*etcd.Node, 0)
-	nodes = append(nodes, response.Node)
+	if !IsEtcdNotFound(err) {


@smarterclayton I found that if there is an etcd not found error here (type 100) then response will be null so response.Node or response.EtcdIndex (below) will cause this goroutine to panic.

Derek just fixed this in another pull.

#6938 covers it

On Apr 16, 2015, at 9:30 PM, Robert Bailey notifications@github.com wrote:

In pkg/tools/etcd_helper.go:

@@ -165,12 +165,14 @@ func (h *EtcdHelper) ExtractObjToList(key string, listObj runtime.Object) error
}

nodes := make([]*etcd.Node, 0)

nodes = append(nodes, response.Node)

if !IsEtcdNotFound(err) {
@smarterclayton I found that if there is an etcd not found error here (type 100) then response will be null so response.Node or response.EtcdIndex (below) will cause this goroutine to panic.

—
Reply to this email directly or view it on GitHub.

I've rebased on top of that PR.

roberthbailey · 2015-04-17T01:32:41Z

@derekwaynecarr @pires @justinsb I haven't been able to test this on other cloud providers -- can you take a look and see if you think it will break anything?

smarterclayton · 2015-04-17T01:43:53Z

What are the security implications of this? How do I control as an admin clients interfering with other nodes? Does this require the cloud provider on the kubelet to have security credentials to the cluster (very scary)? Can I still disable the cloud provider on the kubelet?

roberthbailey · 2015-04-17T01:50:07Z

Security implications: A node with credentials can add itself to the cluster. Right now, nodes can do this because they have bearer tokens.

Right now we don't have any authz in the apiserver. Once we do, we should add policies that only allow nodes to update their own data (and not that of other nodes). Any node today has free reign to update anything in the APIserver because it has full read/write access through the secured port with the node bearer token.

As a first cut I just moved the cloud provider bits from the nodecontroller to the kubelet. But I think we should be able to remove most/all of them. For GCE (and likely AWS) we may still want make a call into the cloud provider to hit the local metadata server and get the external IP address of a node. But calling into the cloud provider to determine system resources seems silly.

Yes, you can still disable the cloud provider on the kubelet. When you run ./hack/test-cmd.sh it runs the kubelet without a cloud provider (https://github.com/GoogleCloudPlatform/kubernetes/blob/master/hack/test-cmd.sh#L72-L83) and all tests still pass.

smarterclayton · 2015-04-17T02:19:49Z

On Apr 16, 2015, at 9:50 PM, Robert Bailey notifications@github.com wrote:

Security implications: A node with credentials can add itself to the cluster. Right now, nodes can do this because they have bearer tokens.

Right now we don't have any authz in the apiserver.

The policy model @deads2k has created for OpenShift (which post 1.0 we'll evaluate bringing into kube) should be able to handle this. We also have the pattern to properly give kubelets unique identity, although we do not yet have the ideal policies.
Once we do, we should add policies that only allow nodes to update their own data (and not that of other nodes). Any node today has free reign to update anything in the APIserver because it has full read/write access through the secured port with the node bearer token.

I just wanted to make sure this didn't change the detente. Thanks.
As a first cut I just moved the cloud provider bits from the nodecontroller to the kubelet. But I think we should be able to remove most/all of them. For GCE (and likely AWS) we may still want make a call into the cloud provider to hit the local metadata server and get the external IP address of a node. But calling into the cloud provider to determine system resources seems silly.

Yes, you can still disable the cloud provider on the kubelet. When you run ./hack/test-cmd.sh it runs the kubelet without a cloud provider (https://github.com/GoogleCloudPlatform/kubernetes/blob/master/hack/test-cmd.sh#L72-L83) and all tests still pass.

—
Reply to this email directly or view it on GitHub.

smarterclayton · 2015-04-17T17:52:02Z

pkg/kubelet/kubelet.go

 // syncNodeStatus periodically synchronizes node status to master.
 func (kl *Kubelet) syncNodeStatus() {
 	if kl.kubeClient == nil {
 		return
 	}
+	if err := kl.registerNode(); err != nil {
+		glog.Errorf("Failed to register node: %s. Giving up.", err)


We should use util.HandleError here

I didn't realize that existed -- cool. I've passed it a new error so that we keep the custom text on top of the error text returned from registerNode.

pires · 2015-04-17T18:47:58Z

@roberthbailey right now I can only test on GCE since I no longer use AWS.

Now, from what I understand from your changes, you're moving cloud-provider stuff from controller-manager (poll) to kubelet (push). If my assumption is correct, I think this is great because it can replace what I and others (cc @AntonioMeireles @wkharold) already use with kube-register.

After reading @smarterclayton I too believe there are risks, but right now, as a sysadmin, I'm OK with it.

roberthbailey · 2015-04-17T19:05:17Z

@pires I'm trying to move the node creation from being done in a non-obvious way (a control loop that talks to the cloud provider and uses a regular expression to decide which nodes should be part of the cluster) to an explicit call from a node to join a specific master.

Right now this call "just works" because the kubelet already has credentials to talk to the master, but in the future (with dynamic clustering) the node will first ask to join by sending a CSR to the master to get itself credentials to POST and create a node entry. This will give sysadmins the ability to set policies on whether nodes can join automatically or whether they need to be manually approved to join a cluster. But that's post-1.0. For now, if a node has credentials, it will automatically join the cluster.

Some of the fields for the node may still make sense to fill in from the node controller (which is why I cc'd @bgrant0607 as I thought he'd have an opinion here) but I think that most of the ones we are setting are better known by the kubelet itself (e.g. node resources, hostname, ip addresses).

pires · 2015-04-17T20:01:24Z

@roberthbailey just to be sure, this would remove the need for tools such as kube-register, right?

roberthbailey · 2015-04-17T20:31:22Z

@pires I think so. Would it be possible for you to test this CL on CoreOS and find out?

AntonioMeireles · 2015-04-17T20:35:22Z

@roberthbailey given the nature of this @kelseyhightower and his pals at CoreOS are the most abilitated to postulate about this. anyway i may be able to test this CL early next week...

pires · 2015-04-17T20:42:26Z

@roberthbailey sure will you be available on IRC for debugging?

roberthbailey · 2015-04-17T20:55:11Z

I'll hop on right now.

pires · 2015-04-17T21:42:11Z

Removed kube-register from master node, replaced kubelet with @roberthbailey version and... it works!

$ kubectl get minions
NAME           LABELS        STATUS
172.17.8.102   Schedulable   <none>    Ready
172.17.8.103   Schedulable   <none>    Ready

bgrant0607 · 2015-04-17T23:16:38Z

cc @gmarek @davidopp

bgrant0607 · 2015-04-17T23:35:06Z

Nodes shouldn't need to call the cloud provider.

We have discussed making it possible to contact the local metadata server for clouds that provide that, but I'd like to avoid that, since it's not universally available.

We need to make some changes to the Node object. The node name is currently overloaded for at least 2 purposes: object name and cloud provider ("external") ID. It's often set to the hostname or address, which adds to the confusion about the name requirements and usage.

The only information currently required to create a node is the externalID, which is automatically defaulted to the node name if not provided -- that part I don't mind. However, I'd like the externalID to be used to lookup the node with the cloud provider, rather than looking up the external ID using the node name:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/cloudprovider/nodecontroller/nodecontroller.go#L640

Addresses are also looked up via the cloud provider. It's true that the node knows at least a subset of its addresses, though not necessarily external ones. The node controller could fill in additional addresses after creation (late initialization).

We don't really have a good way of knowing which address is preferred for master-to-node communication (/validate, log access, exec, port forwarding, proxy, redirect), and we don't know which name/address is specified in the node cert, to the extent that's relevant -- #6985.

Some master component will allocate the PodCIDR via late initialization.

Capacity can be populated after creation, also.

bgrant0607 · 2015-04-17T23:40:00Z

This should also allow us to get rid of the --machines flag, which @eparis should also like.

kube-register allows users to specify fleet node labels to select which nodes to add to Kubernetes. That's still useful, so I don't think this obsoletes kube-register. I wouldn't mind adding fleet/kube-register as a cloudprovider -- #2890 @kelseyhightower .

bgrant0607 · 2015-04-17T23:41:41Z

So maybe the one cloudprovider bit the node needs is the ability to get its externalID, which it should get from a local metadata server, or maybe just use the hostname if no cloudprovider.

bgrant0607 · 2015-04-17T23:43:04Z

pkg/kubelet/kubelet.go

+		ObjectMeta: api.ObjectMeta{Name: kl.hostname},
+		Status: api.NodeStatus{
+			Capacity: api.ResourceList{
+				api.ResourceCPU:    *resource.NewMilliQuantity(1000, resource.DecimalSI),


Kubelet already has code for populating this with real values.
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/kubelet/kubelet.go#L1768

Why ask the cloud provider to determine the node resources when the node can just self report them? How will that work for deployments without a cloud provider?

Also, see https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/cloudprovider/gce/gce.go#L512

No good reason -- legacy cruft. We must get the values from the node.

I think this code was in NodeController so there would be some capacity in NodeStatus between the time the node is created in the master, and the first time the node reports its status.

Fine with leaving it this way in this PR, but we should clean up at some point. Please leave a TODO if you don't think you can get to it soon.

roberthbailey · 2015-04-17T23:53:56Z

kube-register allows users to specify fleet node labels to select which nodes to add to Kubernetes.

Could this be done instead by using fleet node labels to choose which --api_server argument we pass to the kubelet on each node? It doesn't make sense for a kubelet to connect to an apiserver and watch for pods to run (and send events) if it isn't actually part of the cluster.

bgrant0607 · 2015-04-17T23:56:25Z

pkg/cloudprovider/nodecontroller/nodecontroller.go

 	"time"

 	"github.com/GoogleCloudPlatform/kubernetes/pkg/api"
-	apierrors "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/client/record"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/cloudprovider"


You should be able to delete the cloudprovider here.

Now that I re-added the code to handle deleting nodes that no longer exist, we are still using the cloud provider.

bgrant0607 · 2015-04-18T00:02:55Z

Actually, I'm liking this more. I think we should eliminate cloudprovider stuff from the nodecontroller and move it out of the cloudprovider subtree, as discussed in #4851.

The node should get any info it needs from the metadata server, if present, or from a local config file, fleet, whatever -- the mechanism should be pluggable. Calling the existing cloudprovider API could be a stopgap.

We'll likely need to resurrect a cloudprovider-oriented nodecontroller in the future in order to automatically repair and provision nodes (cluster auto-scaling), but that's ok.

roberthbailey · 2015-05-19T04:37:35Z

Just finished running e2e tests:

Ran 43 of 49 Specs in 1678.866 seconds
SUCCESS! -- 43 Passed | 0 Failed | 1 Pending | 5 Skipped PASS

mbforbes · 2015-05-19T16:46:30Z

(Needs rebase)

- Delete nodes when they are no longer ready and don't exist in the cloud provider. - Label each node with it's hostname. - Add flag to skip node registration. - Add a test for registering an existing node.

roberthbailey · 2015-05-19T16:55:32Z

Rebased again.

erictune · 2015-05-19T17:01:03Z

reviewing now

roberthbailey · 2015-05-19T17:05:28Z

Thanks @erictune

erictune · 2015-05-19T18:29:40Z

LGTM

Modify nodes to register directly with the master.

mbforbes · 2015-05-19T23:32:24Z

/xref should fix #8315 (linking for tracking)

lavalamp · 2015-05-20T19:40:25Z

pkg/cloudprovider/nodecontroller/nodecontroller.go

-		availableCIDRs.Delete(node.Spec.PodCIDR)
+// reconcilePodCIDRs looks at each node and assigns it a valid CIDR
+// if it doesn't currently have one.
+func (nc *NodeController) reconcilePodCIDRs(nodes *api.NodeList) {


Should it be called reconcile_Node_CIDRs?

Yes, probably should have renamed it while I was in here...

googlebot added the cla: yes label Apr 17, 2015

roberthbailey reviewed Apr 17, 2015
View reviewed changes

roberthbailey mentioned this pull request Apr 17, 2015

Fix nil pointer in etcd tools #6938

Merged

roberthbailey force-pushed the node-register branch from fcae414 to d5da62c Compare April 17, 2015 17:43

smarterclayton reviewed Apr 17, 2015
View reviewed changes

bgrant0607 assigned erictune Apr 17, 2015

roberthbailey force-pushed the node-register branch from d5da62c to 75c6d7a Compare April 17, 2015 19:10

roberthbailey mentioned this pull request Apr 17, 2015

Generator for json definition of nodes #7002

Closed

bgrant0607 reviewed Apr 17, 2015
View reviewed changes

roberthbailey mentioned this pull request May 19, 2015

Setup master as a node too #8444

Closed

roberthbailey added 2 commits May 19, 2015 09:55

Modify nodes to register directly with the master.

01467e0

- Delete nodes when they are no longer ready and don't exist in the cloud provider. - Label each node with it's hostname. - Add flag to skip node registration. - Add a test for registering an existing node.

Configure the cloud provider for the kubelet.

8e356f8

roberthbailey force-pushed the node-register branch from 394c419 to 8e356f8 Compare May 19, 2015 16:55

roberthbailey mentioned this pull request May 19, 2015

Fire an event when CIDRs can't be assigned to nodes #8506

Closed

erictune added a commit that referenced this pull request May 19, 2015

Merge pull request #6949 from roberthbailey/node-register

1f4172d

Modify nodes to register directly with the master.

erictune merged commit 1f4172d into kubernetes:master May 19, 2015

mbforbes mentioned this pull request May 19, 2015

Wrong certificates in k8s cluster #8315

Closed

ixdy mentioned this pull request May 19, 2015

Merge Freeze: Stabilize end to end tests on May 19 2015 #8534

Closed

This was referenced May 20, 2015

Node status test should mock out ip lookup #8549

Closed

cannot create node via kubectl #8520

Closed

lavalamp reviewed May 20, 2015
View reviewed changes

dchen1107 mentioned this pull request May 20, 2015

kubelet tries to start pods before cbr0 is set #8581

Closed

ghost mentioned this pull request May 20, 2015

Revert "Modify nodes to register directly with the master." #8588

Merged

cjcullen mentioned this pull request May 20, 2015

Route creation reconciler loop. #8164

Merged

This was referenced May 21, 2015

General system instability - large numbers of e2e tests failing as of 09:50 on 05/21 #8637

Closed

Prevent the kubelet on the master from registering itself into the cluster on GCE. #8640

Closed

bgrant0607 mentioned this pull request May 28, 2015

New minions cannot be added using kubectl #8468

Closed

This was referenced May 31, 2015

Auto-populate node labels with node information from cloud provider #9044

Closed

Clarify strange kubernetes behaviour (bugs or features) #8661

Closed

roberthbailey deleted the node-register branch August 20, 2015 21:36

Modify nodes to register directly with the master. #6949

Modify nodes to register directly with the master. #6949

Conversation

roberthbailey commented Apr 17, 2015

roberthbailey commented Apr 17, 2015

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

roberthbailey commented Apr 17, 2015

smarterclayton commented Apr 17, 2015

roberthbailey commented Apr 17, 2015

smarterclayton commented Apr 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pires commented Apr 17, 2015

roberthbailey commented Apr 17, 2015

pires commented Apr 17, 2015

roberthbailey commented Apr 17, 2015

AntonioMeireles commented Apr 17, 2015

pires commented Apr 17, 2015

roberthbailey commented Apr 17, 2015

pires commented Apr 17, 2015

bgrant0607 commented Apr 17, 2015

bgrant0607 commented Apr 17, 2015

bgrant0607 commented Apr 17, 2015

bgrant0607 commented Apr 17, 2015

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

roberthbailey commented Apr 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgrant0607 commented Apr 18, 2015

roberthbailey commented May 19, 2015

mbforbes commented May 19, 2015

roberthbailey commented May 19, 2015

erictune commented May 19, 2015

roberthbailey commented May 19, 2015

erictune commented May 19, 2015

mbforbes commented May 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment