Skip to content

Commit 83fc13e

Browse files
authored
Merge pull request kubernetes#74511 from rojkov/kubeadm-refactor-enforceRequirements
kubeadm: move duplicated code into enforceRequirements()
2 parents 3678704 + 226843f commit 83fc13e

File tree

6 files changed

+168
-92
lines changed

6 files changed

+168
-92
lines changed

cmd/kubeadm/app/cmd/upgrade/apply.go

Lines changed: 1 addition & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,6 @@ import (
2727
clientset "k8s.io/client-go/kubernetes"
2828
"k8s.io/klog"
2929
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
30-
"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/validation"
3130
"k8s.io/kubernetes/cmd/kubeadm/app/cmd/options"
3231
cmdutil "k8s.io/kubernetes/cmd/kubeadm/app/cmd/util"
3332
"k8s.io/kubernetes/cmd/kubeadm/app/constants"
@@ -76,39 +75,7 @@ func NewCmdApply(apf *applyPlanFlags) *cobra.Command {
7675
DisableFlagsInUseLine: true,
7776
Short: "Upgrade your Kubernetes cluster to the specified version.",
7877
Run: func(cmd *cobra.Command, args []string) {
79-
var userVersion string
80-
var err error
81-
flags.ignorePreflightErrorsSet, err = validation.ValidateIgnorePreflightErrors(flags.ignorePreflightErrors)
82-
kubeadmutil.CheckErr(err)
83-
84-
// Ensure the user is root
85-
klog.V(1).Infof("running preflight checks")
86-
err = runPreflightChecks(flags.ignorePreflightErrorsSet)
87-
kubeadmutil.CheckErr(err)
88-
89-
// If the version is specified in config file, pick up that value.
90-
if flags.cfgPath != "" {
91-
// Note that cfg isn't preserved here, it's just an one-off to populate userVersion based on --config
92-
cfg, err := configutil.LoadInitConfigurationFromFile(flags.cfgPath)
93-
kubeadmutil.CheckErr(err)
94-
95-
if cfg.KubernetesVersion != "" {
96-
userVersion = cfg.KubernetesVersion
97-
}
98-
}
99-
100-
// If the new version is already specified in config file, version arg is optional.
101-
if userVersion == "" {
102-
err = cmdutil.ValidateExactArgNumber(args, []string{"version"})
103-
kubeadmutil.CheckErr(err)
104-
}
105-
106-
// If option was specified in both args and config file, args will overwrite the config file.
107-
if len(args) == 1 {
108-
userVersion = args[0]
109-
}
110-
111-
err = assertVersionStringIsEmpty(userVersion)
78+
userVersion, err := getK8sVersionFromUserInput(flags.applyPlanFlags, args, true)
11279
kubeadmutil.CheckErr(err)
11380

11481
err = runApply(flags, userVersion)
@@ -227,14 +194,6 @@ func runApply(flags *applyFlags, userVersion string) error {
227194
return nil
228195
}
229196

230-
func assertVersionStringIsEmpty(version string) error {
231-
if len(version) == 0 {
232-
return errors.New("version string can't be empty")
233-
}
234-
235-
return nil
236-
}
237-
238197
// EnforceVersionPolicies makes sure that the version the user specified is valid to upgrade to
239198
// There are both fatal and skippable (with --force) errors
240199
func EnforceVersionPolicies(newK8sVersionStr string, newK8sVersion *version.Version, flags *applyFlags, versionGetter upgrade.VersionGetter) error {

cmd/kubeadm/app/cmd/upgrade/apply_test.go

Lines changed: 0 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -25,30 +25,6 @@ import (
2525
"k8s.io/kubernetes/cmd/kubeadm/app/constants"
2626
)
2727

28-
func TestAssertVersionStringIsEmpty(t *testing.T) {
29-
var tcases = []struct {
30-
name string
31-
version string
32-
expectedErr bool
33-
}{
34-
{
35-
name: "Version string is not empty",
36-
version: "some string",
37-
},
38-
{
39-
name: "Version string is empty",
40-
expectedErr: true,
41-
},
42-
}
43-
for _, tt := range tcases {
44-
t.Run(tt.name, func(t *testing.T) {
45-
if assertVersionStringIsEmpty(tt.version) == nil && tt.expectedErr {
46-
t.Errorf("No error triggered for string '%s'", tt.version)
47-
}
48-
})
49-
}
50-
}
51-
5228
func TestSessionIsInteractive(t *testing.T) {
5329
var tcases = []struct {
5430
name string

cmd/kubeadm/app/cmd/upgrade/common.go

Lines changed: 43 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,10 @@ import (
3131
"k8s.io/apimachinery/pkg/util/sets"
3232
fakediscovery "k8s.io/client-go/discovery/fake"
3333
clientset "k8s.io/client-go/kubernetes"
34+
"k8s.io/klog"
3435
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
36+
"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/validation"
37+
cmdutil "k8s.io/kubernetes/cmd/kubeadm/app/cmd/util"
3538
"k8s.io/kubernetes/cmd/kubeadm/app/constants"
3639
"k8s.io/kubernetes/cmd/kubeadm/app/features"
3740
"k8s.io/kubernetes/cmd/kubeadm/app/phases/upgrade"
@@ -42,8 +45,47 @@ import (
4245
kubeconfigutil "k8s.io/kubernetes/cmd/kubeadm/app/util/kubeconfig"
4346
)
4447

48+
func getK8sVersionFromUserInput(flags *applyPlanFlags, args []string, versionIsMandatory bool) (string, error) {
49+
var userVersion string
50+
51+
// If the version is specified in config file, pick up that value.
52+
if flags.cfgPath != "" {
53+
// Note that cfg isn't preserved here, it's just an one-off to populate userVersion based on --config
54+
cfg, err := configutil.LoadInitConfigurationFromFile(flags.cfgPath)
55+
if err != nil {
56+
return "", err
57+
}
58+
59+
userVersion = cfg.KubernetesVersion
60+
}
61+
62+
// the version arg is mandatory unless version is specified in the config file
63+
if versionIsMandatory && userVersion == "" {
64+
if err := cmdutil.ValidateExactArgNumber(args, []string{"version"}); err != nil {
65+
return "", err
66+
}
67+
}
68+
69+
// If option was specified in both args and config file, args will overwrite the config file.
70+
if len(args) == 1 {
71+
userVersion = args[0]
72+
}
73+
74+
return userVersion, nil
75+
}
76+
4577
// enforceRequirements verifies that it's okay to upgrade and then returns the variables needed for the rest of the procedure
4678
func enforceRequirements(flags *applyPlanFlags, dryRun bool, newK8sVersion string) (clientset.Interface, upgrade.VersionGetter, *kubeadmapi.InitConfiguration, error) {
79+
ignorePreflightErrorsSet, err := validation.ValidateIgnorePreflightErrors(flags.ignorePreflightErrors)
80+
if err != nil {
81+
return nil, nil, nil, err
82+
}
83+
84+
// Ensure the user is root
85+
klog.V(1).Info("running preflight checks")
86+
if err := runPreflightChecks(ignorePreflightErrorsSet); err != nil {
87+
return nil, nil, nil, err
88+
}
4789

4890
client, err := getClient(flags.kubeConfigPath, dryRun)
4991
if err != nil {
@@ -56,7 +98,7 @@ func enforceRequirements(flags *applyPlanFlags, dryRun bool, newK8sVersion strin
5698
}
5799

58100
// Run healthchecks against the cluster
59-
if err := upgrade.CheckClusterHealth(client, flags.ignorePreflightErrorsSet); err != nil {
101+
if err := upgrade.CheckClusterHealth(client, ignorePreflightErrorsSet); err != nil {
60102
return nil, nil, nil, errors.Wrap(err, "[upgrade/health] FATAL")
61103
}
62104

cmd/kubeadm/app/cmd/upgrade/common_test.go

Lines changed: 123 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,134 @@ package upgrade
1818

1919
import (
2020
"bytes"
21+
"fmt"
22+
"io/ioutil"
23+
"os"
2124
"testing"
25+
"time"
2226

2327
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
2428
)
2529

30+
const (
31+
validConfig = `apiVersion: kubeadm.k8s.io/v1beta1
32+
kind: ClusterConfiguration
33+
kubernetesVersion: 1.13.0
34+
`
35+
)
36+
37+
func TestGetK8sVersionFromUserInput(t *testing.T) {
38+
var tcases = []struct {
39+
name string
40+
isVersionMandatory bool
41+
clusterConfig string
42+
args []string
43+
expectedErr bool
44+
expectedVersion string
45+
}{
46+
{
47+
name: "No config and version as an argument",
48+
isVersionMandatory: true,
49+
args: []string{"v1.13.1"},
50+
expectedVersion: "v1.13.1",
51+
},
52+
{
53+
name: "Neither config nor version specified",
54+
isVersionMandatory: true,
55+
expectedErr: true,
56+
},
57+
{
58+
name: "No config and empty version as an argument",
59+
isVersionMandatory: true,
60+
args: []string{""},
61+
expectedErr: true,
62+
},
63+
{
64+
name: "Valid config, but no version specified",
65+
isVersionMandatory: true,
66+
clusterConfig: validConfig,
67+
expectedVersion: "v1.13.0",
68+
},
69+
{
70+
name: "Valid config and different version specified",
71+
isVersionMandatory: true,
72+
clusterConfig: validConfig,
73+
args: []string{"v1.13.1"},
74+
expectedVersion: "v1.13.1",
75+
},
76+
{
77+
name: "Version is optional",
78+
},
79+
}
80+
for tnum, tt := range tcases {
81+
t.Run(tt.name, func(t *testing.T) {
82+
flags := &applyPlanFlags{}
83+
if len(tt.clusterConfig) > 0 {
84+
tmpfile := fmt.Sprintf("/tmp/kubeadm-upgrade-common-test-%d-%d.yaml", tnum, time.Now().Unix())
85+
if err := ioutil.WriteFile(tmpfile, []byte(tt.clusterConfig), 0666); err != nil {
86+
t.Fatalf("Failed to create test config file: %+v", err)
87+
}
88+
defer os.Remove(tmpfile)
89+
90+
flags.cfgPath = tmpfile
91+
}
92+
93+
userVersion, err := getK8sVersionFromUserInput(flags, tt.args, tt.isVersionMandatory)
94+
95+
if err == nil && tt.expectedErr {
96+
t.Error("Expected error, but got success")
97+
}
98+
if err != nil && !tt.expectedErr {
99+
t.Errorf("Unexpected error: %+v", err)
100+
}
101+
if userVersion != tt.expectedVersion {
102+
t.Errorf("Expected '%s', but got '%s'", tt.expectedVersion, userVersion)
103+
}
104+
})
105+
}
106+
}
107+
108+
func TestEnforceRequirements(t *testing.T) {
109+
tcases := []struct {
110+
name string
111+
newK8sVersion string
112+
dryRun bool
113+
flags applyPlanFlags
114+
expectedErr bool
115+
}{
116+
{
117+
name: "Fail pre-flight check",
118+
expectedErr: true,
119+
},
120+
{
121+
name: "Bogus preflight check disabled when also 'all' is specified",
122+
flags: applyPlanFlags{
123+
ignorePreflightErrors: []string{"bogusvalue", "all"},
124+
},
125+
expectedErr: true,
126+
},
127+
{
128+
name: "Fail to create client",
129+
flags: applyPlanFlags{
130+
ignorePreflightErrors: []string{"all"},
131+
},
132+
expectedErr: true,
133+
},
134+
}
135+
for _, tt := range tcases {
136+
t.Run(tt.name, func(t *testing.T) {
137+
_, _, _, err := enforceRequirements(&tt.flags, tt.dryRun, tt.newK8sVersion)
138+
139+
if err == nil && tt.expectedErr {
140+
t.Error("Expected error, but got success")
141+
}
142+
if err != nil && !tt.expectedErr {
143+
t.Errorf("Unexpected error: %+v", err)
144+
}
145+
})
146+
}
147+
}
148+
26149
func TestPrintConfiguration(t *testing.T) {
27150
var tests = []struct {
28151
name string

cmd/kubeadm/app/cmd/upgrade/plan.go

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,8 @@ import (
2929
"k8s.io/apimachinery/pkg/util/version"
3030
"k8s.io/klog"
3131
kubeadmapi "k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm"
32-
"k8s.io/kubernetes/cmd/kubeadm/app/apis/kubeadm/validation"
3332
"k8s.io/kubernetes/cmd/kubeadm/app/phases/upgrade"
3433
kubeadmutil "k8s.io/kubernetes/cmd/kubeadm/app/util"
35-
configutil "k8s.io/kubernetes/cmd/kubeadm/app/util/config"
3634
etcdutil "k8s.io/kubernetes/cmd/kubeadm/app/util/etcd"
3735
)
3836

@@ -50,27 +48,8 @@ func NewCmdPlan(apf *applyPlanFlags) *cobra.Command {
5048
Use: "plan [version] [flags]",
5149
Short: "Check which versions are available to upgrade to and validate whether your current cluster is upgradeable. To skip the internet check, pass in the optional [version] parameter.",
5250
Run: func(_ *cobra.Command, args []string) {
53-
var userVersion string
54-
var err error
55-
flags.ignorePreflightErrorsSet, err = validation.ValidateIgnorePreflightErrors(flags.ignorePreflightErrors)
51+
userVersion, err := getK8sVersionFromUserInput(flags.applyPlanFlags, args, false)
5652
kubeadmutil.CheckErr(err)
57-
// Ensure the user is root
58-
err = runPreflightChecks(flags.ignorePreflightErrorsSet)
59-
kubeadmutil.CheckErr(err)
60-
61-
// If the version is specified in config file, pick up that value.
62-
if flags.cfgPath != "" {
63-
cfg, err := configutil.LoadInitConfigurationFromFile(flags.cfgPath)
64-
kubeadmutil.CheckErr(err)
65-
66-
if cfg.KubernetesVersion != "" {
67-
userVersion = cfg.KubernetesVersion
68-
}
69-
}
70-
// If option was specified in both args and config file, args will overwrite the config file.
71-
if len(args) == 1 {
72-
userVersion = args[0]
73-
}
7453

7554
err = runPlan(flags, userVersion)
7655
kubeadmutil.CheckErr(err)

cmd/kubeadm/app/cmd/upgrade/upgrade.go

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ import (
2323
"github.com/spf13/cobra"
2424
"github.com/spf13/pflag"
2525

26-
"k8s.io/apimachinery/pkg/util/sets"
2726
"k8s.io/kubernetes/cmd/kubeadm/app/cmd/options"
2827
cmdutil "k8s.io/kubernetes/cmd/kubeadm/app/cmd/util"
2928
kubeadmconstants "k8s.io/kubernetes/cmd/kubeadm/app/constants"
@@ -39,7 +38,6 @@ type applyPlanFlags struct {
3938
allowRCUpgrades bool
4039
printConfig bool
4140
ignorePreflightErrors []string
42-
ignorePreflightErrorsSet sets.String
4341
out io.Writer
4442
}
4543

@@ -52,7 +50,6 @@ func NewCmdUpgrade(out io.Writer) *cobra.Command {
5250
allowExperimentalUpgrades: false,
5351
allowRCUpgrades: false,
5452
printConfig: false,
55-
ignorePreflightErrorsSet: sets.NewString(),
5653
out: out,
5754
}
5855

0 commit comments

Comments
 (0)