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.

Sign up for GitHub

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

Jump to bottom

kubeadm: Make a separate util package for kubeconfig logic #41603

Merged
merged 1 commit into from
Feb 19, 2017
Merged

kubeadm: Make a separate util package for kubeconfig logic #41603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion cmd/kubeadm/app/cmd/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ go_library(
"//cmd/kubeadm/app/phases/token:go_default_library",
"//cmd/kubeadm/app/preflight:go_default_library",
"//cmd/kubeadm/app/util:go_default_library",
"//cmd/kubeadm/app/util/kubeconfig:go_default_library",
"//cmd/kubeadm/app/util/token:go_default_library",
"//pkg/bootstrap/api:go_default_library",
"//pkg/kubectl:go_default_library",
Expand Down Expand Up @@ -64,7 +65,7 @@ go_test(
library = ":go_default_library",
tags = ["automanaged"],
deps = [
"//cmd/kubeadm/app/phases/kubeconfig:go_default_library",
"//cmd/kubeadm/app/constants:go_default_library",
"//cmd/kubeadm/app/preflight:go_default_library",
],
)
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/cmd/init.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ func (i *Init) Run(out io.Writer) error {
return err
}

client, err := kubemaster.CreateClientAndWaitForAPI(path.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, kubeconfigphase.AdminKubeConfigFileName))
client, err := kubemaster.CreateClientAndWaitForAPI(path.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, kubeadmconstants.AdminKubeConfigFileName))
if err != nil {
return err
}
Expand Down
6 changes: 3 additions & 3 deletions cmd/kubeadm/app/cmd/join.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ import (
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
"k8s.io/kubernetes/cmd/kubeadm/app/discovery"
kubenode "k8s.io/kubernetes/cmd/kubeadm/app/node"
kubeconfigphase "k8s.io/kubernetes/cmd/kubeadm/app/phases/kubeconfig"
"k8s.io/kubernetes/cmd/kubeadm/app/preflight"
kubeadmutil "k8s.io/kubernetes/cmd/kubeadm/app/util"
kubeconfigutil "k8s.io/kubernetes/cmd/kubeadm/app/util/kubeconfig"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think you should have a pkgs alias here. kubeconfig doesn't seem to collide with anything.

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 actually think it's much better to throw util at the end so one knows for sure that it's question about a generic helpers lib.

If the name was kubeconfig, I'd think it the question was about the kubeconfig phase for example, or a k8s-generic factory or something

Copy link
Contributor

Choose a reason for hiding this comment

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

I disagree, and here is why: https://blog.golang.org/package-names

Copy link
Contributor

Choose a reason for hiding this comment

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

@pires, @jbeda, @mikedanese what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Don't steal good names from the user. Avoid giving a package a name that is commonly used in client code. For example, the buffered I/O package is called bufio, not buf, since buf is a good variable name for a buffer.

I don't think this should be kubeconfig for that reason for instance.
Further, that doc said there should be no package named util and I'm trying to move things away from kubeadmutil (the "generic" one) as much as possible.

What's your suggestion here other than kubeconfig?

)

var (
Expand Down Expand Up @@ -139,8 +139,8 @@ func (j *Join) Run(out io.Writer) error {
return err
}

kubeconfigFile := filepath.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, kubeconfigphase.KubeletKubeConfigFileName)
if err := kubeconfigphase.WriteKubeconfigToDisk(kubeconfigFile, cfg); err != nil {
kubeconfigFile := filepath.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, kubeadmconstants.KubeletKubeConfigFileName)
if err := kubeconfigutil.WriteToDisk(kubeconfigFile, cfg); err != nil {
return err
}

Expand Down
8 changes: 4 additions & 4 deletions cmd/kubeadm/app/cmd/reset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import (
"github.com/spf13/cobra"

kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
"k8s.io/kubernetes/cmd/kubeadm/app/phases/kubeconfig"
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
Copy link
Contributor

@dmmcquay dmmcquay Feb 16, 2017 •
edited

Choose a reason for hiding this comment

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

Similar to my last comment. I really dislike how many package alias we use. Why can't we just use constants here? For me, what that shows, is that our package names are not well thought out.

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 agree that some of the package aliases are bad, but I think this one makes sense as well.
This says explicitely that we're talking about kubeadm constants, no one else's constants.

"k8s.io/kubernetes/cmd/kubeadm/app/preflight"
kubeadmutil "k8s.io/kubernetes/cmd/kubeadm/app/util"
"k8s.io/kubernetes/pkg/util/initsystem"
Expand Down Expand Up @@ -150,7 +150,7 @@ func drainAndRemoveNode(removeNode bool) error {
hostname = strings.ToLower(hostname)

// TODO: Use the "native" k8s client for this once we're confident the versioned is working
kubeConfigPath := filepath.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, kubeconfig.KubeletKubeConfigFileName)
kubeConfigPath := filepath.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, kubeadmconstants.KubeletKubeConfigFileName)

getNodesCmd := fmt.Sprintf("kubectl --kubeconfig %s get nodes | grep %s", kubeConfigPath, hostname)
output, err := exec.Command("sh", "-c", getNodesCmd).Output()
Expand Down Expand Up @@ -221,8 +221,8 @@ func resetConfigDir(configPathDir, pkiPathDir string) {
}

filesToClean := []string{
filepath.Join(configPathDir, kubeconfig.AdminKubeConfigFileName),
filepath.Join(configPathDir, kubeconfig.KubeletKubeConfigFileName),
filepath.Join(configPathDir, kubeadmconstants.AdminKubeConfigFileName),
filepath.Join(configPathDir, kubeadmconstants.KubeletKubeConfigFileName),
}
fmt.Printf("[reset] Deleting files: %v\n", filesToClean)
for _, path := range filesToClean {
Expand Down
20 changes: 10 additions & 10 deletions cmd/kubeadm/app/cmd/reset_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (
"path/filepath"
"testing"

"k8s.io/kubernetes/cmd/kubeadm/app/phases/kubeconfig"
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
"k8s.io/kubernetes/cmd/kubeadm/app/preflight"
)

Expand Down Expand Up @@ -64,8 +64,8 @@ func TestConfigDirCleaner(t *testing.T) {
"manifests/etcd.yaml",
"manifests/kube-apiserver.yaml",
"pki/ca.pem",
kubeconfig.AdminKubeConfigFileName,
kubeconfig.KubeletKubeConfigFileName,
kubeadmconstants.AdminKubeConfigFileName,
kubeadmconstants.KubeletKubeConfigFileName,
},
verifyExists: []string{
"manifests",
Expand All @@ -78,7 +78,7 @@ func TestConfigDirCleaner(t *testing.T) {
},
setupFiles: []string{
"pki/ca.pem",
kubeconfig.KubeletKubeConfigFileName,
kubeadmconstants.KubeletKubeConfigFileName,
},
verifyExists: []string{
"pki",
Expand All @@ -96,8 +96,8 @@ func TestConfigDirCleaner(t *testing.T) {
"manifests/etcd.yaml",
"manifests/kube-apiserver.yaml",
"pki/ca.pem",
kubeconfig.AdminKubeConfigFileName,
kubeconfig.KubeletKubeConfigFileName,
kubeadmconstants.AdminKubeConfigFileName,
kubeadmconstants.KubeletKubeConfigFileName,
"cloud-config",
},
verifyExists: []string{
Expand All @@ -116,8 +116,8 @@ func TestConfigDirCleaner(t *testing.T) {
"manifests/etcd.yaml",
"manifests/kube-apiserver.yaml",
"pki/ca.pem",
kubeconfig.AdminKubeConfigFileName,
kubeconfig.KubeletKubeConfigFileName,
kubeadmconstants.AdminKubeConfigFileName,
kubeadmconstants.KubeletKubeConfigFileName,
".cloud-config",
".mydir/.myfile",
},
Expand Down Expand Up @@ -167,8 +167,8 @@ func TestConfigDirCleaner(t *testing.T) {

// Verify the files we cleanup implicitly in every test:
assertExists(t, tmpDir)
assertNotExists(t, filepath.Join(tmpDir, kubeconfig.AdminKubeConfigFileName))
assertNotExists(t, filepath.Join(tmpDir, kubeconfig.KubeletKubeConfigFileName))
assertNotExists(t, filepath.Join(tmpDir, kubeadmconstants.AdminKubeConfigFileName))
assertNotExists(t, filepath.Join(tmpDir, kubeadmconstants.KubeletKubeConfigFileName))
assertDirEmpty(t, filepath.Join(tmpDir, "manifests"))
assertDirEmpty(t, filepath.Join(tmpDir, "pki"))

Expand Down
9 changes: 4 additions & 5 deletions cmd/kubeadm/app/cmd/token.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,9 @@ import (
"k8s.io/client-go/pkg/api"
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
kubemaster "k8s.io/kubernetes/cmd/kubeadm/app/master"
"k8s.io/kubernetes/cmd/kubeadm/app/phases/kubeconfig"
tokenphase "k8s.io/kubernetes/cmd/kubeadm/app/phases/token"
kubeadmutil "k8s.io/kubernetes/cmd/kubeadm/app/util"
kubeconfigutil "k8s.io/kubernetes/cmd/kubeadm/app/util/kubeconfig"
tokenutil "k8s.io/kubernetes/cmd/kubeadm/app/util/token"
bootstrapapi "k8s.io/kubernetes/pkg/bootstrap/api"
"k8s.io/kubernetes/pkg/kubectl"
Expand Down Expand Up @@ -131,7 +130,7 @@ func NewCmdTokenGenerate(out io.Writer) *cobra.Command {

// RunCreateToken generates a new bootstrap token and stores it as a secret on the server.
func RunCreateToken(out io.Writer, cmd *cobra.Command, tokenDuration time.Duration, token string) error {
client, err := kubemaster.CreateClientFromFile(path.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, kubeconfig.AdminKubeConfigFileName))
client, err := kubeconfigutil.ClientSetFromFile(path.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, kubeadmconstants.AdminKubeConfigFileName))
if err != nil {
return err
}
Expand Down Expand Up @@ -164,7 +163,7 @@ func RunGenerateToken(out io.Writer) error {

// RunListTokens lists details on all existing bootstrap tokens on the server.
func RunListTokens(out io.Writer, errW io.Writer, cmd *cobra.Command) error {
client, err := kubemaster.CreateClientFromFile(path.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, kubeconfig.AdminKubeConfigFileName))
client, err := kubeconfigutil.ClientSetFromFile(path.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, kubeadmconstants.AdminKubeConfigFileName))
if err != nil {
return err
}
Expand Down Expand Up @@ -223,7 +222,7 @@ func RunDeleteToken(out io.Writer, cmd *cobra.Command, tokenId string) error {
return err
}

client, err := kubemaster.CreateClientFromFile(path.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, kubeconfig.AdminKubeConfigFileName))
client, err := kubeconfigutil.ClientSetFromFile(path.Join(kubeadmapi.GlobalEnvParams.KubernetesDir, kubeadmconstants.AdminKubeConfigFileName))
if err != nil {
return err
}
Expand Down
3 changes: 3 additions & 0 deletions cmd/kubeadm/app/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,9 @@ const (
FrontProxyClientCertName = "front-proxy-client.crt"
FrontProxyClientKeyName = "front-proxy-client.key"

AdminKubeConfigFileName = "admin.conf"
KubeletKubeConfigFileName = "kubelet.conf"

// TODO: These constants should actually come from pkg/kubeapiserver/authorizer, but we can't vendor that package in now
// because of all the other sub-packages that would get vendored. To fix this, a pkg/kubeapiserver/authorizer/modes package
// or similar should exist that only has these constants; then we can vendor it.
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/master/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ go_library(
"//cmd/kubeadm/app/constants:go_default_library",
"//cmd/kubeadm/app/images:go_default_library",
"//cmd/kubeadm/app/util:go_default_library",
"//cmd/kubeadm/app/util/kubeconfig:go_default_library",
"//pkg/kubectl/cmd/util:go_default_library",
"//vendor:github.com/ghodss/yaml",
"//vendor:k8s.io/apimachinery/pkg/api/errors",
Expand All @@ -35,7 +36,6 @@ go_library(
"//vendor:k8s.io/client-go/pkg/api",
"//vendor:k8s.io/client-go/pkg/api/v1",
"//vendor:k8s.io/client-go/pkg/apis/extensions/v1beta1",
"//vendor:k8s.io/client-go/tools/clientcmd",
"//vendor:k8s.io/client-go/util/cert",
],
)
Expand Down
21 changes: 2 additions & 19 deletions cmd/kubeadm/app/master/apiclient.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,31 +29,14 @@ import (
"k8s.io/client-go/pkg/api"
"k8s.io/client-go/pkg/api/v1"
extensions "k8s.io/client-go/pkg/apis/extensions/v1beta1"
"k8s.io/client-go/tools/clientcmd"
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
kubeadmutil "k8s.io/kubernetes/cmd/kubeadm/app/util"
kubeconfigutil "k8s.io/kubernetes/cmd/kubeadm/app/util/kubeconfig"
)

func CreateClientFromFile(path string) (*clientset.Clientset, error) {
adminKubeconfig, err := clientcmd.LoadFromFile(path)
if err != nil {
return nil, fmt.Errorf("failed to load admin kubeconfig [%v]", err)
}
adminClientConfig, err := clientcmd.NewDefaultClientConfig(*adminKubeconfig, &clientcmd.ConfigOverrides{}).ClientConfig()
if err != nil {
return nil, fmt.Errorf("failed to create API client configuration [%v]", err)
}

client, err := clientset.NewForConfig(adminClientConfig)
if err != nil {
return nil, fmt.Errorf("failed to create API client [%v]", err)
}
return client, nil
}

func CreateClientAndWaitForAPI(file string) (*clientset.Clientset, error) {
client, err := CreateClientFromFile(file)
client, err := kubeconfigutil.ClientSetFromFile(file)
if err != nil {
return nil, err
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/node/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ go_library(
tags = ["automanaged"],
deps = [
"//cmd/kubeadm/app/apis/kubeadm:go_default_library",
"//cmd/kubeadm/app/phases/kubeconfig:go_default_library",
"//cmd/kubeadm/app/util/kubeconfig:go_default_library",
"//cmd/kubeadm/app/util/token:go_default_library",
"//pkg/kubelet/util/csr:go_default_library",
"//vendor:github.com/square/go-jose",
Expand Down
4 changes: 2 additions & 2 deletions cmd/kubeadm/app/node/bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import (
"k8s.io/client-go/tools/clientcmd"
clientcmdapi "k8s.io/client-go/tools/clientcmd/api"
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
kubeconfigphase "k8s.io/kubernetes/cmd/kubeadm/app/phases/kubeconfig"
kubeconfigutil "k8s.io/kubernetes/cmd/kubeadm/app/util/kubeconfig"
tokenutil "k8s.io/kubernetes/cmd/kubeadm/app/util/token"
)

Expand Down Expand Up @@ -98,7 +98,7 @@ func EstablishMasterConnection(c *kubeadmapi.TokenDiscovery, clusterInfo *kubead

// creates a set of clients for this endpoint
func createClients(caCert []byte, endpoint, token string, nodeName types.NodeName) (*apiClient, error) {
clientConfig := kubeconfigphase.MakeClientConfigWithToken(
clientConfig := kubeconfigutil.CreateWithToken(
endpoint,
"kubernetes",
fmt.Sprintf("kubelet-%s", nodeName),
Expand Down
10 changes: 1 addition & 9 deletions cmd/kubeadm/app/phases/kubeconfig/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,6 @@ licenses(["notice"])
load(
"@io_bazel_rules_go//go:def.bzl",
"go_library",
"go_test",
)

go_test(
name = "go_default_test",
srcs = ["kubeconfig_test.go"],
library = ":go_default_library",
tags = ["automanaged"],
deps = ["//cmd/kubeadm/app/apis/kubeadm:go_default_library"],
)

go_library(
Expand All @@ -26,6 +17,7 @@ go_library(
deps = [
"//cmd/kubeadm/app/constants:go_default_library",
"//cmd/kubeadm/app/phases/certs/pkiutil:go_default_library",
"//cmd/kubeadm/app/util/kubeconfig:go_default_library",
"//vendor:k8s.io/client-go/tools/clientcmd",
"//vendor:k8s.io/client-go/tools/clientcmd/api",
"//vendor:k8s.io/client-go/util/cert",
Expand Down