Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GCE kube-up] Don't provision kubeconfig file for kube-proxy service account #52183

Merged
merged 2 commits into from
Oct 14, 2017

Conversation

MrHohn
Copy link
Member

@MrHohn MrHohn commented Sep 8, 2017

What this PR does / why we need it:

Offloading the burden of provisioning kubeconfig file for kube-proxy service account from GCE startup scripts. This also helps us decoupling kube-proxy daemonset upgrade from node upgrade.

Previous attempt on #51172, using InClusterConfig for kube-proxy based on discussions on kubernetes/client-go#281.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #NONE

Special notes for your reviewer:
/assign @bowei @thockin
cc @luxas @murali-reddy

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 8, 2017
@k8s-github-robot k8s-github-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Sep 8, 2017
@MrHohn
Copy link
Member Author

MrHohn commented Sep 8, 2017

Forgot to note, this doesn't change how kube-proxy static pods are deployed.

@MrHohn
Copy link
Member Author

MrHohn commented Sep 8, 2017

Ref #23225.

return nil, nil, err
// When only master URL is set, use service account but override the host.
if len(config.KubeConfigFile) == 0 && len(masterOverride) != 0 {
glog.Info("Only --master was specified. Using service account with overrided host.")
Copy link
Member

Choose a reason for hiding this comment

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

If we want to keep backwards-compat, we can't do this.
--master only currently means "I can run anywhere, and I will connect insecurely (via 8080 port) to the API server"
I guess we need to add yet an other --use-service-account-credentials or similar flag :/

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we need to add yet an other --use-service-account-credentials or similar flag :/

Thanks, I think you are right...

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a flag --service-account-master-url. PTAL, thanks!

@luxas
Copy link
Member

luxas commented Sep 8, 2017

This isn't targeted for v1.8, right?

@thockin
Copy link
Member

thockin commented Sep 8, 2017

Assign back to me once it is LGTM'ed and ready for approval.

@thockin thockin removed their assignment Sep 8, 2017
@MrHohn
Copy link
Member Author

MrHohn commented Sep 8, 2017

This isn't targeted for v1.8, right?

Nope, let's do it for v1.9 :)

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2017
@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 26, 2017
@MrHohn MrHohn force-pushed the kube-proxy-incluster-host branch 2 times, most recently from ba84b11 to 3d151bf Compare September 26, 2017 20:23
@MrHohn
Copy link
Member Author

MrHohn commented Sep 26, 2017

/retest

1 similar comment
@MrHohn
Copy link
Member Author

MrHohn commented Sep 26, 2017

/retest

@MrHohn
Copy link
Member Author

MrHohn commented Sep 27, 2017

Tests passed, code is ready for review.

cc @ncdc for kube-proxy config API changes
cc @mikedanese for kubeconfig changes in GCE kube-up

@ncdc
Copy link
Member

ncdc commented Sep 27, 2017

Would you be willing to do this without adding a new command line flag to kube-proxy? I'd really prefer to avoid adding new flags since we have a config file now.

@MrHohn
Copy link
Member Author

MrHohn commented Sep 27, 2017

Would you be willing to do this without adding a new command line flag to kube-proxy? I'd really prefer to avoid adding new flags since we have a config file now.

I would like to do so. What is the timeline of componentconfig API (enabled by default already)? I'm a bit lost after reading kubernetes/enhancements#115. Would be great to have some guidelines for converting flags to config file.

@ncdc
Copy link
Member

ncdc commented Sep 27, 2017

The kube-proxy config is functional right now (you can do kube-proxy --config FILE), but the structs/fields are still alpha and subject to change. One example of a change I'd like to make is part of #52198 where I've adjusted some of the json field names.

Ultimately we need to move the kube-proxy config structs into their own api group, and componentconfig needs to go away.

I would recommend converting to a config file soon, but one concern I do have is about upgrades: if we roll out a cluster with version n, then later we adjust the config file format, how do we handle upgrades to n+1 as well as downgrades back to n?

cc @mikedanese @mtaufen

@MrHohn
Copy link
Member Author

MrHohn commented Sep 27, 2017

I would recommend converting to a config file soon, but one concern I do have is about upgrades: if we roll out a cluster with version n, then later we adjust the config file format, how do we handle upgrades to n+1 as well as downgrades back to n?

Is populating config file content via configmap (like what kubeadm alread did for kubeconfig) and making sure configmap is updated during upgrade/downgrade be feasible?

@ncdc
Copy link
Member

ncdc commented Sep 27, 2017

As long as you can get a file on disk in a path that kube-proxy has access to, it can come from anywhere.

@mtaufen
Copy link
Contributor

mtaufen commented Sep 27, 2017

I would recommend converting to a config file soon, but one concern I do have is about upgrades: if we roll out a cluster with version n, then later we adjust the config file format, how do we handle upgrades to n+1 as well as downgrades back to n?

What is blocking stabilizing the config structure, other than moving it out of componentconfig and making your desired changes? Why not just do that now and move it out of alpha, so that you have API stability across upgrades?

@mtaufen
Copy link
Contributor

mtaufen commented Sep 27, 2017

With the Kubelet, we won't convert to using the config file until the kubeletconfig API group is out of alpha, and the loading-from-a-file mechanism will remain behind an alpha gate until then.

@ncdc
Copy link
Member

ncdc commented Sep 28, 2017

What is blocking stabilizing the config structure, other than moving it out of componentconfig and making your desired changes? Why not just do that now and move it out of alpha, so that you have API stability across upgrades?

Nothing, other than time 😄

@liggitt liggitt added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 5, 2017
@MrHohn
Copy link
Member Author

MrHohn commented Oct 5, 2017

@liggitt Thanks for the comment.

That would keep InClusterConfig() behavior intact and avoid an odd mix of flag+envvar client building

Good point. No I don't have a compelling reason now. I was thinking by making it a flag/config we could make kube-proxy binary self-contained, though that isn't really true as by using InClusterConfig() we already assume kube-proxy runs as a pod.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2017
@MrHohn
Copy link
Member Author

MrHohn commented Oct 5, 2017

Revised PR a bit to keep InClusterConfig() behavior intact.

Pushed as separate commits and hopefully keeping review simple.

  • Changes to kube-proxy are on 9d1be35.
  • Changes to GCE kube-up are on 8317a4f.

The last commit tweaks KUBE_PROXY_DAEMONSET to prove this is working. Will remove later.

Sorry for the last minute changes @mikedanese @ncdc

@MrHohn
Copy link
Member Author

MrHohn commented Oct 5, 2017

/retest

@liggitt
Copy link
Member

liggitt commented Oct 5, 2017

thanks, the client building looks more coherent now
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 5, 2017
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2017
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2017
@MrHohn
Copy link
Member Author

MrHohn commented Oct 10, 2017

Rebased. @mikedanese @ncdc any chance to take another look? Thanks!

Copy link
Member

@ncdc ncdc left a comment

Choose a reason for hiding this comment

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

The go code changes lgtm, although I have a question if we could do this without adding a flag.

&clientcmd.ConfigOverrides{ClusterInfo: clientcmdapi.Cluster{Server: masterOverride}}).ClientConfig()
if err != nil {
return nil, nil, err
if config.UseServiceAccount {
Copy link
Member

Choose a reason for hiding this comment

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

@liggitt @deads2k what if we just did this instead?

loader := clientcmd.NewDefaultClientConfigLoadingRules()
loader.ExplicitPath = config.KubeConfigFile
kubeConfig, err := clientcmd.BuildConfigFromKubeconfigGetter(masterOverride, loader.Load)

I believe this would allow us to skip adding a "use service account" flag:

  1. Use the kubeconfig file, if it's specified in the kube-proxy config
  2. Support the master url override
  3. Use the in-cluster config if the kubeconfig file is not specified

WDYT? If this is too big or risky of a change, then we can certainly keep what's here in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

In fact, seems like BuildConfigFromFlags() has supported this use case?

// BuildConfigFromFlags is a helper function that builds configs from a master
// url or a kubeconfig filepath. These are passed in as command line flags for cluster
// components. Warnings should reflect this usage. If neither masterUrl or kubeconfigPath
// are passed in we fallback to inClusterConfig. If inClusterConfig fails, we fallback
// to the default config.
func BuildConfigFromFlags(masterUrl, kubeconfigPath string) (*restclient.Config, error) {
if kubeconfigPath == "" && masterUrl == "" {
glog.Warningf("Neither --kubeconfig nor --master was specified. Using the inClusterConfig. This might not work.")
kubeconfig, err := restclient.InClusterConfig()
if err == nil {
return kubeconfig, nil
}
glog.Warning("error creating inClusterConfig, falling back to default config: ", err)
}
return NewNonInteractiveDeferredLoadingClientConfig(
&ClientConfigLoadingRules{ExplicitPath: kubeconfigPath},
&ConfigOverrides{ClusterInfo: clientcmdapi.Cluster{Server: masterUrl}}).ClientConfig()
}

If using InClusterConfig() when neither kubeconfig nor masterUrl is specified for kube-proxy is not considered breaking backwards-compat, I'd happy to do it this way. The current behavior is using default API client. cc @luxas

Copy link
Member Author

Choose a reason for hiding this comment

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

Took another look, using BuildConfigFromFlags() seems valid here. As when none of masterUrl nor kubeconfigPath is defined, it will call InClusterConfig(). Then when InClusterConfig() fails, it then falls back to default config. This seems to match the current behavior.

Copy link
Member

Choose a reason for hiding this comment

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

loader := clientcmd.NewDefaultClientConfigLoadingRules()

That would make kube-proxy honor $KUBECONFIG and $HOME/.kube/config, which seems problematic. Traditionally, we've had server components use explicit flags to specify connection config files, rather than picking up env-based config like $KUBECONFIG.

InClusterConfig() falls somewhere between an explicit --kubeconfig and an implicit $KUBECONFIG or homedir behavior, since it is using paths and envs only intended to be set in a containerized environment.

Copy link
Member

Choose a reason for hiding this comment

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

The kubeconfig is going to be specific in the config file via --config. We won't support --kubeconfig any more.

Maybe if the kubeconfig in the config file is blank, we fall back to in cluster?

Copy link
Member

Choose a reason for hiding this comment

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

specific file falling back to InClusterConfig() if unspecified seems ok. Just make sure we don't pull in $KUBECONFIG or $HOME/.kube/config

Copy link
Member

Choose a reason for hiding this comment

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

K

Copy link
Member

Choose a reason for hiding this comment

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

@MrHohn can you make the change such that if config.KubeConfigFile is empty, we use the in-cluster config?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ncdc Sounds good, will ping again when ready.

@k8s-github-robot k8s-github-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Oct 13, 2017
@MrHohn
Copy link
Member Author

MrHohn commented Oct 13, 2017

@MrHohn can you make the change such that if config.KubeConfigFile is empty, we use the in-cluster config?

Tests passed, PR is ready for review. @ncdc PTAL thanks!

if len(config.KubeConfigFile) == 0 && len(masterOverride) == 0 {
glog.Warningf("Neither --kubeconfig nor --master was specified. Using default API client. This might not work.")
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to log that it's using in-cluster config?

Copy link
Member

Choose a reason for hiding this comment

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

+1 on logging why it is falling back to in cluster config. LGTM otherwise

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. Added a log line for this.

@ncdc
Copy link
Member

ncdc commented Oct 13, 2017

LGTM. @liggitt please check the in-cluster setup.

@MrHohn
Copy link
Member Author

MrHohn commented Oct 13, 2017

Thanks for reviewing! Removed the dummy commit.

@mikedanese
Copy link
Member

cluster changes still looks good.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, mikedanese, MrHohn

Associated issue: 281

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

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

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

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 52883, 52183, 53915, 53848). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 1c17d98 into kubernetes:master Oct 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.