Skip to content

Refactor master vs node kube-env and salt auth#8009

Merged
zmerlynn merged 2 commits intokubernetes:masterfrom
mbforbes:refactorEnv
May 12, 2015
Merged

Refactor master vs node kube-env and salt auth#8009
zmerlynn merged 2 commits intokubernetes:masterfrom
mbforbes:refactorEnv

Conversation

@mbforbes
Copy link
Copy Markdown
Contributor

@mbforbes mbforbes commented May 8, 2015

Done for #7938 (as part of work on #6088)

+cc @roberthbailey

@mbforbes mbforbes added the sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. label May 8, 2015
@mbforbes
Copy link
Copy Markdown
Contributor Author

mbforbes commented May 8, 2015

Running e2es, so don't merge yet.

roberthbailey
roberthbailey reviewed May 9, 2015
View reviewed changes
cluster/gce/configure-vm.sh Outdated
Copy link
Copy Markdown
Contributor

@roberthbailey roberthbailey May 9, 2015

Choose a reason for hiding this comment

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

For reboots, you should put a guard around this like we do for the master auth, e.g. if [[ ! -e "${kubelet_auth_file}" ]]; then ... fi

Copy link
Copy Markdown
Contributor Author

@mbforbes mbforbes May 12, 2015

Choose a reason for hiding this comment

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

Done. I figured I'd leave it because it seems idempotent, but I guess no reason to do it twice.

Actually, maybe that's a good question: do we actually want to re-do it every time, if it would be idempotent if it's the same, but allow it to be changed if the config does change?

Also, used single [ and ] to match what's been done in the master function, but happy to change them all to double if you'd prefer.

@roberthbailey
Copy link
Copy Markdown
Contributor

roberthbailey commented May 9, 2015

A few nits, otherwise LGTM.

@roberthbailey
Copy link
Copy Markdown
Contributor

roberthbailey commented May 12, 2015

@mbforbes I have a few commits in flight that are touching this same bit of code (creating auth bits for the master and nodes). I think this one should go in first, so can you address comments and re-ping when it's ready for review?

@mbforbes
Copy link
Copy Markdown
Contributor Author

mbforbes commented May 12, 2015

@roberthbailey should be ready for review again. Sorry for the delay—I was working on other stuff today as e2es were busted so I couldn't test this :-/

@mbforbes
Copy link
Copy Markdown
Contributor Author

mbforbes commented May 12, 2015

With that said, still haven't gotten to run e2es; I can do this tomorrow morning.

@roberthbailey
Copy link
Copy Markdown
Contributor

roberthbailey commented May 12, 2015

LGTM assuming e2es pass.

@vmarmol
Copy link
Copy Markdown
Contributor

vmarmol commented May 12, 2015

/cc @dchen1107 @bakins we probably need to replicate this in the CoreOS setup

@mbforbes
Copy link
Copy Markdown
Contributor Author

mbforbes commented May 12, 2015

Found bugs and changed an if—ran e2es and they pass. Ping @zmerlynn

@bakins
Copy link
Copy Markdown

bakins commented May 12, 2015

This should be fine on CoreOS on GCE. Running through tests to confirm.

bakins
bakins reviewed May 12, 2015
View reviewed changes
cluster/gce/coreos/helper.sh
Copy link
Copy Markdown

@bakins bakins May 12, 2015

Choose a reason for hiding this comment

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

I think we need to change https://github.com/GoogleCloudPlatform/kubernetes/blob/2f2816368f667bb81e6f59f910b0353604a6ccda/cluster/gce/coreos/node.yaml#L20 to use KUBELET_TOKEN. testing.

@bakins
Copy link
Copy Markdown

bakins commented May 12, 2015

Confirmed that we need to change to using KUBLET_TOKEN in node.xml. Tests run with CoreOS as minion with this:

diff --git a/cluster/gce/coreos/node.yaml b/cluster/gce/coreos/node.yaml
index 884074f..97daf77 100644
--- a/cluster/gce/coreos/node.yaml
+++ b/cluster/gce/coreos/node.yaml
@@ -17,7 +17,7 @@ write_files:
       source /etc/kube-env

       /usr/bin/mkdir -p /var/lib/kubelet
-      /bin/echo  {\"BearerToken\": \"${KUBE_BEARER_TOKEN}\", \"Insecure\": true } > /var/lib/kubelet/kubernetes_auth
+      /bin/echo  {\"BearerToken\": \"${KUBELET_TOKEN}\", \"Insecure\": true } > /var/lib/kubelet/kubernetes_auth
   - path: /run/config-kube-proxy.sh
     permissions: "0755"
     content: |

@mbforbes
Copy link
Copy Markdown
Contributor Author

mbforbes commented May 12, 2015

@bakins huge thanks for checking this and giving the fix! I've made the change you wrote to gce/coreos/node.yaml and squashed into my first commit. I assume by "tests run" you mean "tests pass?"

@roberthbailey I think this is good

@bakins
Copy link
Copy Markdown

bakins commented May 12, 2015

@mbforbes Yes, tests pass. Sorry for being unclear :) Thanks!

@zmerlynn
Copy link
Copy Markdown
Member

zmerlynn commented May 12, 2015

LGTM

zmerlynn added a commit that referenced this pull request May 12, 2015
@zmerlynn
Merge pull request #8009 from mbforbes/refactorEnv
0b0bace 
Refactor master vs node kube-env and salt auth
@zmerlynn zmerlynn merged commit 0b0bace into kubernetes:master May 12, 2015
@roberthbailey roberthbailey mentioned this pull request May 15, 2015
Closed
@mbforbes mbforbes mentioned this pull request May 19, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@zmerlynn zmerlynn

Labels

sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@mbforbes @roberthbailey @vmarmol @bakins @zmerlynn @googlebot