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

Fix kubeadm for v1alpha1 configs #64708

Merged
merged 1 commit into from
Jun 5, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 10 additions & 0 deletions cmd/kubeadm/app/apis/kubeadm/v1alpha1/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -91,3 +91,13 @@ go_test(
embed = [":go_default_library"],
deps = ["//vendor/k8s.io/apimachinery/pkg/apis/meta/v1/unstructured:go_default_library"],
)

go_test(
name = "go_default_xtest",
srcs = ["conversion_test.go"],
deps = [
":go_default_library",
"//cmd/kubeadm/app/apis/kubeadm:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
],
)
8 changes: 3 additions & 5 deletions cmd/kubeadm/app/apis/kubeadm/v1alpha1/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -145,13 +145,11 @@ func UpgradeNodeRegistrationOptionsForMaster(in *MasterConfiguration, out *kubea
}
}

// UpgradeBootstrapTokens should create at least one empty bootstrap token in the out config.
func UpgradeBootstrapTokens(in *MasterConfiguration, out *kubeadm.MasterConfiguration) error {
if len(in.Token) == 0 {
return nil
}

bts, err := kubeadm.NewBootstrapTokenString(in.Token)
if err != nil {
// Ignore the error if the incoming token was empty.
if err != nil && in.Token != "" {
return fmt.Errorf("can't parse .Token, and hence can't convert v1alpha1 API to a newer version: %v", err)
}

Expand Down
103 changes: 103 additions & 0 deletions cmd/kubeadm/app/apis/kubeadm/v1alpha1/conversion_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/*
Copyright 2018 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package v1alpha1_test
Copy link
Member

Choose a reason for hiding this comment

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

?
Why not just v1alpha1?

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 do this to enforce that the tests only test the public interfaces.

Copy link
Member

Choose a reason for hiding this comment

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

Why do we have to restrict ourselves to only test the public interfaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing the interface allows the implementation to change. If we test the implementation we are restricted to that particular implementation (unless you also want to refactor the tests at the same time).

Here is an article that sums up the argument https://testing.googleblog.com/2013/08/testing-on-toilet-test-behavior-not.html.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To answer your question directly, we restrict ourselves so that our intention in only testing the public interfaces, unless there is good reason, is very clear.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I think we agree here but talk past each other. However, I don't have time to argue more on this nit, this is fine by me ;)


import (
"reflect"
"testing"
"time"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/v1alpha1"
)

func TestUpgradeBootstrapTokens(t *testing.T) {
testcases := []struct {
name string
in *v1alpha1.MasterConfiguration
expectedOut *kubeadm.MasterConfiguration
expectError bool
}{
{
name: "empty configs should create at least one token",
in: &v1alpha1.MasterConfiguration{},
expectedOut: &kubeadm.MasterConfiguration{
BootstrapTokens: []kubeadm.BootstrapToken{
{
Token: nil,
},
},
},
expectError: false,
},
{
Copy link
Member

Choose a reason for hiding this comment

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

also add the case that in.Token is something valid

Copy link
Member

Choose a reason for hiding this comment

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

Also add a test that makes sure TTL, Groups, Usages, etc. are preserved here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done ✅

name: "fail at parsing incoming token",
in: &v1alpha1.MasterConfiguration{
Token: "some fake token",
},
expectError: true,
},
{
name: "input has values",
in: &v1alpha1.MasterConfiguration{
Token: "abcdef.abcdefghijklmnop",
TokenTTL: &metav1.Duration{
Duration: time.Duration(10 * time.Hour),
},
TokenUsages: []string{"action"},
TokenGroups: []string{"group", "group2"},
},
expectedOut: &kubeadm.MasterConfiguration{
BootstrapTokens: []kubeadm.BootstrapToken{
{
Token: &kubeadm.BootstrapTokenString{
ID: "abcdef",
Secret: "abcdefghijklmnop",
},
TTL: &metav1.Duration{
Duration: time.Duration(10 * time.Hour),
},
Usages: []string{"action"},
Groups: []string{"group", "group2"},
},
},
},
expectError: false,
},
}

for _, tc := range testcases {
t.Run(tc.name, func(t *testing.T) {
out := &kubeadm.MasterConfiguration{}
err := v1alpha1.UpgradeBootstrapTokens(tc.in, out)

if tc.expectError {
if err == nil {
t.Fatal("expected an error but did not get one.")
}
// do not continue if we got an expected error
Copy link
Member

Choose a reason for hiding this comment

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

I think t.Fatalf does this automatically?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but if if the err was an error I don't want to run the rest of the tests, I want to return because probably the next few tests will then throw nil pointer errors.

return
}

if !reflect.DeepEqual(out.BootstrapTokens, tc.expectedOut.BootstrapTokens) {
t.Fatalf("\nexpected: %v\ngot: %v", tc.expectedOut.BootstrapTokens, out.BootstrapTokens)
}
})
}

}
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/cmd/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -482,8 +482,8 @@ func (i *Init) Run(out io.Writer) error {
}

// PHASE 7: Make the control plane self-hosted if feature gate is enabled
glog.V(1).Infof("[init] feature gate is enabled. Making control plane self-hosted")
if features.Enabled(i.cfg.FeatureGates, features.SelfHosting) {
glog.V(1).Infof("[init] feature gate is enabled. Making control plane self-hosted")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This snuck in but I think it's a safe enough change.

The intent was that this only prints if the feature is enabled, but it was always printing.

Copy link
Member

Choose a reason for hiding this comment

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

That's ok by me

// Temporary control plane is up, now we create our self hosted control
// plane components and remove the static manifests:
glog.Infoln("[self-hosted] creating self-hosted control plane")
Expand Down
1 change: 0 additions & 1 deletion hack/.golint_failures
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ cmd/kube-controller-manager/app
cmd/kube-proxy/app
cmd/kube-scheduler/app
cmd/kubeadm/app
cmd/kubeadm/app/apis/kubeadm/v1alpha1
Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised the tests are passing, IIRC we had to add this for the autogenerated funcs, but that's apprarently not the case anymore 👍

cmd/kubeadm/app/apis/kubeadm/v1alpha2
cmd/kubeadm/app/util/config
cmd/kubelet/app
Expand Down