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: do not panic if etcd local alpha phase is called when an external etcd config is used #69420

Merged
merged 1 commit into from
Oct 5, 2018
Merged

kubeadm: do not panic if etcd local alpha phase is called when an external etcd config is used #69420

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: 3 additions & 0 deletions cmd/kubeadm/app/phases/etcd/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ const (

// CreateLocalEtcdStaticPodManifestFile will write local etcd static pod manifest file.
func CreateLocalEtcdStaticPodManifestFile(manifestDir string, cfg *kubeadmapi.InitConfiguration) error {
if cfg.ClusterConfiguration.Etcd.External != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Small nit
Please use fmt.Errorf. it is the standard in kubeadm codebase

Copy link
Member

Choose a reason for hiding this comment

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

but errors.New() makes good sense too if the are no arguments.
it's much cheaper on the CPU compared to fmt.Errorf().

in fact someone made a big PR to fix this across k8s, but he got tired of rebasing and closed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have an opinion here because I am missing context on how the project wants to make error reporting homogeneous.

Copy link
Member

Choose a reason for hiding this comment

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

@neolit123
I'm in favour of errors too, but if we have to switch we should do this across all the kubeadm codebase.
In the meantime fmt.Errorf is the standard to use

return fmt.Errorf("etcd static pod manifest cannot be generated for cluster using external etcd")
}
glog.V(1).Infoln("creating local etcd static pod manifest file")
// gets etcd StaticPodSpec, actualized for the current InitConfiguration
spec := GetEtcdPodSpec(cfg)
Expand Down
66 changes: 48 additions & 18 deletions cmd/kubeadm/app/phases/etcd/local_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,34 +54,64 @@ func TestGetEtcdPodSpec(t *testing.T) {
}

func TestCreateLocalEtcdStaticPodManifestFile(t *testing.T) {

// Create temp folder for the test case
tmpdir := testutil.SetupTempDir(t)
defer os.RemoveAll(tmpdir)

// Creates a Master Configuration
cfg := &kubeadmapi.InitConfiguration{
ClusterConfiguration: kubeadmapi.ClusterConfiguration{
KubernetesVersion: "v1.7.0",
Etcd: kubeadmapi.Etcd{
Local: &kubeadmapi.LocalEtcd{
DataDir: "/var/lib/etcd",
Image: "k8s.gcr.io/etcd",
var tests = []struct {
Copy link
Member

Choose a reason for hiding this comment

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

Another nit for consistency with the rest of kubeadm codebase

Typically we are storing only input/output variables of the test cases in this variable, not the test execution logic.

In this case in the tests input is cfg, while the test output is expectedError.

The run function should go into the for loop,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated! :)

cfg *kubeadmapi.InitConfiguration
expectedError bool
}{
{
cfg: &kubeadmapi.InitConfiguration{
ClusterConfiguration: kubeadmapi.ClusterConfiguration{
KubernetesVersion: "v1.7.0",
Etcd: kubeadmapi.Etcd{
Local: &kubeadmapi.LocalEtcd{
DataDir: "/var/lib/etcd",
Image: "k8s.gcr.io/etcd",
},
},
},
},
expectedError: false,
},
{
cfg: &kubeadmapi.InitConfiguration{
ClusterConfiguration: kubeadmapi.ClusterConfiguration{
KubernetesVersion: "v1.7.0",
Etcd: kubeadmapi.Etcd{
External: &kubeadmapi.ExternalEtcd{
Endpoints: []string{
"https://etcd-instance:2379",
},
CAFile: "/etc/kubernetes/pki/etcd/ca.crt",
CertFile: "/etc/kubernetes/pki/etcd/apiserver-etcd-client.crt",
KeyFile: "/etc/kubernetes/pki/etcd/apiserver-etcd-client.key",
},
},
},
},
expectedError: true,
},
}

// Execute createStaticPodFunction
manifestPath := filepath.Join(tmpdir, kubeadmconstants.ManifestsSubDirName)
err := CreateLocalEtcdStaticPodManifestFile(manifestPath, cfg)
if err != nil {
t.Errorf("Error executing CreateEtcdStaticPodManifestFile: %v", err)
for _, test := range tests {
// Execute createStaticPodFunction
manifestPath := filepath.Join(tmpdir, kubeadmconstants.ManifestsSubDirName)
err := CreateLocalEtcdStaticPodManifestFile(manifestPath, test.cfg)

if !test.expectedError {
if err != nil {
t.Errorf("CreateLocalEtcdStaticPodManifestFile failed when not expected: %v", err)
}
// Assert expected files are there
testutil.AssertFilesCount(t, manifestPath, 1)
testutil.AssertFileExists(t, manifestPath, kubeadmconstants.Etcd+".yaml")
} else {
testutil.AssertError(t, err, "etcd static pod manifest cannot be generated for cluster using external etcd")
}
}

// Assert expected files are there
testutil.AssertFilesCount(t, manifestPath, 1)
testutil.AssertFileExists(t, manifestPath, kubeadmconstants.Etcd+".yaml")
}

func TestGetEtcdCommand(t *testing.T) {
Expand Down
11 changes: 11 additions & 0 deletions cmd/kubeadm/test/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,17 @@ func AssertFileExists(t *testing.T, dirName string, fileNames ...string) {
}
}

// AssertError checks that the provided error matches the expected output
func AssertError(t *testing.T, err error, expected string) {
if err == nil {
t.Errorf("no error was found, but '%s' was expected", expected)
return
}
if err.Error() != expected {
t.Errorf("error '%s' does not match expected error: '%s'", err.Error(), expected)
}
}

// GetDefaultInternalConfig returns a defaulted kubeadmapi.InitConfiguration
func GetDefaultInternalConfig(t *testing.T) *kubeadmapi.InitConfiguration {
internalcfg, err := configutil.ConfigFileAndDefaultsToInternalConfig("", &kubeadmapiv1alpha3.InitConfiguration{})
Expand Down