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

Code clean up for kubeadm #117619

Merged
merged 1 commit into from Apr 26, 2023
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
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/apis/bootstraptoken/v1/utils.go
Expand Up @@ -102,7 +102,7 @@ func BootstrapTokenToSecret(bt *BootstrapToken) *v1.Secret {
Name: bootstraputil.BootstrapTokenSecretName(bt.Token.ID),
Namespace: metav1.NamespaceSystem,
},
Type: v1.SecretType(bootstrapapi.SecretTypeBootstrapToken),
Type: bootstrapapi.SecretTypeBootstrapToken,
Data: encodeTokenSecretData(bt, time.Now()),
}
}
Expand Down
5 changes: 3 additions & 2 deletions cmd/kubeadm/app/apis/bootstraptoken/v1/utils_test.go
Expand Up @@ -26,6 +26,7 @@ import (

v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
bootstrapapi "k8s.io/cluster-bootstrap/token/api"
)

func TestMarshalJSON(t *testing.T) {
Expand Down Expand Up @@ -275,7 +276,7 @@ func TestBootstrapTokenToSecret(t *testing.T) {
Name: "bootstrap-token-abcdef",
Namespace: "kube-system",
},
Type: v1.SecretType("bootstrap.kubernetes.io/token"),
Type: bootstrapapi.SecretTypeBootstrapToken,
Data: map[string][]byte{
"token-id": []byte("abcdef"),
"token-secret": []byte("abcdef0123456789"),
Expand Down Expand Up @@ -648,7 +649,7 @@ func TestBootstrapTokenFromSecret(t *testing.T) {
Name: rt.name,
Namespace: "kube-system",
},
Type: v1.SecretType("bootstrap.kubernetes.io/token"),
Type: bootstrapapi.SecretTypeBootstrapToken,
Data: rt.data,
})
if (err != nil) != rt.expectedError {
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/cmd/config.go
Expand Up @@ -173,7 +173,7 @@ var legacyKindToGroupMap = map[string]string{
func getSupportedComponentConfigKinds() []string {
objects := []string{}
for componentType := range legacyKindToGroupMap {
objects = append(objects, string(componentType))
objects = append(objects, componentType)
}
sort.Strings(objects)
return objects
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/cmd/phases/init/addons.go
Expand Up @@ -42,7 +42,7 @@ var (
Install the kube-proxy addon components via the API server.
`)

printManifest bool = false
printManifest = false
)

// NewAddonPhase returns the addon Cobra command
Expand Down
10 changes: 5 additions & 5 deletions cmd/kubeadm/app/cmd/phases/join/data_test.go
Expand Up @@ -40,8 +40,8 @@ func (j *testJoinData) Client() (clientset.Interface, error) { return
func (j *testJoinData) IgnorePreflightErrors() sets.Set[string] { return nil }
func (j *testJoinData) OutputWriter() io.Writer { return nil }
func (j *testJoinData) PatchesDir() string { return "" }
func (t *testJoinData) DryRun() bool { return false }
func (t *testJoinData) KubeConfigDir() string { return "" }
func (t *testJoinData) KubeletDir() string { return "" }
func (t *testJoinData) ManifestDir() string { return "" }
func (t *testJoinData) CertificateWriteDir() string { return "" }
func (j *testJoinData) DryRun() bool { return false }
func (j *testJoinData) KubeConfigDir() string { return "" }
func (j *testJoinData) KubeletDir() string { return "" }
func (j *testJoinData) ManifestDir() string { return "" }
func (j *testJoinData) CertificateWriteDir() string { return "" }
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/cmd/upgrade/diff_test.go
Expand Up @@ -33,7 +33,7 @@ func createTestRunDiffFile(contents []byte) (string, error) {
if err != nil {
return "", errors.Wrap(err, "failed to create temporary test file")
}
if _, err := file.Write([]byte(contents)); err != nil {
if _, err := file.Write(contents); err != nil {
return "", errors.Wrap(err, "failed to write to temporary test file")
}
if err := file.Close(); err != nil {
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/cmd/upgrade/node.go
Expand Up @@ -206,7 +206,7 @@ func (d *nodeData) IgnorePreflightErrors() sets.Set[string] {
return d.ignorePreflightErrors
}

// KubeconfigPath returns the path to the user kubeconfig file.
// KubeConfigPath returns the path to the user kubeconfig file.
Copy link
Member

Choose a reason for hiding this comment

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

strange, are we not running a linter on these packages - i.e. to be able to catch the godoc problems?

Copy link
Member Author

@SataQiu SataQiu Apr 26, 2023

Choose a reason for hiding this comment

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

It seems that the staticcheck's behavior is inconsistent with golint :(

https://github.com/golang/lint

Copy link
Member Author

Choose a reason for hiding this comment

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

$ cd cmd/kubeadm/app/constants
$ golint
constants.go:425:2: comment on exported const KubeControllerManagerUserName should be of the form "KubeControllerManagerUserName ..."
constants.go:427:2: comment on exported const KubeAPIServerUserName should be of the form "KubeAPIServerUserName ..."
constants.go:429:2: comment on exported const KubeSchedulerUserName should be of the form "KubeSchedulerUserName ..."
constants.go:431:2: comment on exported const EtcdUserName should be of the form "EtcdUserName ..."
constants.go:433:2: comment on exported const ServiceAccountKeyReadersGroupName should be of the form "ServiceAccountKeyReadersGroupName ..."

$ staticcheck
<no output>

Copy link
Member

Choose a reason for hiding this comment

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

hm, i was under the impression we are running a suite of linters on k/k, which covers the godoc exports:
https://github.com/kubernetes/kubernetes/blob/master/hack/verify-golangci-lint.sh

cc @pohly @dims

Copy link
Contributor

Choose a reason for hiding this comment

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

We run golangci-lint with a pretty restricted set of enabled checks: https://github.com/kubernetes/kubernetes/blob/master/hack/golangci.yaml

See https://groups.google.com/a/kubernetes.io/g/dev/c/myGiml72IbM/m/aQTb6KToAwAJ for an effort to enable additional linters.

However, enforcing this particular godoc convention is not planned. It's too contentious and often not necessary.

Copy link
Member

Choose a reason for hiding this comment

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

thanks for the info, i guess we are going to have to run the godoc checks out of band / accept PRs that try to fix these.

Copy link
Contributor

Choose a reason for hiding this comment

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

So you want "godocs" enabled for cmd/kubeadm?

Once golangci/golangci-lint#3617 is merged, it should be possible to enable additional, stricter checks for specific directories that are not enabled elsewhere. Right now that is possible, but very awkward.

Copy link
Member

Choose a reason for hiding this comment

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

yes, we have to skip the kubeadm api packages, but it would be nice to enable it for the rest.

func (d *nodeData) KubeConfigPath() string {
return d.kubeConfigPath
}
Expand Down
10 changes: 5 additions & 5 deletions cmd/kubeadm/app/constants/constants.go
Expand Up @@ -422,15 +422,15 @@ const (
// CgroupDriverSystemd holds the systemd driver type
CgroupDriverSystemd = "systemd"

// The username of the user that kube-controller-manager runs as.
// KubeControllerManagerUserName is the username of the user that kube-controller-manager runs as.
KubeControllerManagerUserName string = "kubeadm-kcm"
// The username of the user that kube-apiserver runs as.
// KubeAPIServerUserName is the username of the user that kube-apiserver runs as.
KubeAPIServerUserName string = "kubeadm-kas"
// The username of the user that kube-scheduler runs as.
// KubeSchedulerUserName is the username of the user that kube-scheduler runs as.
KubeSchedulerUserName string = "kubeadm-ks"
// The username of the user that etcd runs as.
// EtcdUserName is the username of the user that etcd runs as.
EtcdUserName string = "kubeadm-etcd"
// The group of users that are allowed to read the service account private key.
// ServiceAccountKeyReadersGroupName is the group of users that are allowed to read the service account private key.
ServiceAccountKeyReadersGroupName string = "kubeadm-sa-key-readers"
)

Expand Down
8 changes: 4 additions & 4 deletions cmd/kubeadm/app/features/features.go
Expand Up @@ -78,17 +78,17 @@ func ValidateVersion(allFeatures FeatureList, requestedFeatures map[string]bool,

// Enabled indicates whether a feature name has been enabled
func Enabled(featureList map[string]bool, featureName string) bool {
if enabled, ok := featureList[string(featureName)]; ok {
if enabled, ok := featureList[featureName]; ok {
return enabled
}
return InitFeatureGates[string(featureName)].Default
return InitFeatureGates[featureName].Default
}

// Supports indicates whether a feature name is supported on the given
// feature set
func Supports(featureList FeatureList, featureName string) bool {
for k, v := range featureList {
if featureName == string(k) {
if featureName == k {
return v.PreRelease != featuregate.Deprecated
}
}
Expand All @@ -99,7 +99,7 @@ func Supports(featureList FeatureList, featureName string) bool {
func Keys(featureList FeatureList) []string {
var list []string
for k := range featureList {
list = append(list, string(k))
list = append(list, k)
}
return list
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/phases/controlplane/volumes_test.go
Expand Up @@ -631,7 +631,7 @@ func TestAddExtraHostPathMounts(t *testing.T) {
if _, ok := mounts.volumeMounts["component"][volumeName]; !ok {
t.Errorf("Expected to find volume mount %q", volumeName)
}
if *vol.HostPath.Type != v1.HostPathType(hostMount.PathType) {
if *vol.HostPath.Type != hostMount.PathType {
t.Errorf("Expected to host path type %q", hostMount.PathType)
}
volMount := mounts.volumeMounts["component"][volumeName]
Expand Down
4 changes: 2 additions & 2 deletions cmd/kubeadm/app/phases/upgrade/health.go
Expand Up @@ -207,9 +207,9 @@ func deleteHealthCheckJob(client clientset.Interface, ns, jobName string) error

// controlPlaneNodesReady checks whether all control-plane Nodes in the cluster are in the Running state
func controlPlaneNodesReady(client clientset.Interface, _ *kubeadmapi.ClusterConfiguration) error {
selectorControlPlane := labels.SelectorFromSet(labels.Set(map[string]string{
selectorControlPlane := labels.SelectorFromSet(map[string]string{
constants.LabelNodeRoleControlPlane: "",
}))
})
nodes, err := client.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{
LabelSelector: selectorControlPlane.String(),
})
Expand Down
22 changes: 11 additions & 11 deletions cmd/kubeadm/app/preflight/checks.go
Expand Up @@ -58,9 +58,9 @@ const (
bridgenf6 = "/proc/sys/net/bridge/bridge-nf-call-ip6tables"
ipv4Forward = "/proc/sys/net/ipv4/ip_forward"
ipv6DefaultForwarding = "/proc/sys/net/ipv6/conf/default/forwarding"
externalEtcdRequestTimeout = time.Duration(10 * time.Second)
externalEtcdRequestTimeout = 10 * time.Second
externalEtcdRequestRetries = 3
externalEtcdRequestInterval = time.Duration(5 * time.Second)
externalEtcdRequestInterval = 5 * time.Second
)

var (
Expand Down Expand Up @@ -384,7 +384,7 @@ func (ipc InPathCheck) Check() (warnings, errs []error) {
return nil, nil
}

// HostnameCheck checks if hostname match dns sub domain regex.
// HostnameCheck checks if hostname match dns subdomain regex.
// If hostname doesn't match this regex, kubelet will not launch static pods like kube-apiserver/kube-controller-manager and so on.
type HostnameCheck struct {
nodeName string
Expand All @@ -395,7 +395,7 @@ func (HostnameCheck) Name() string {
return "Hostname"
}

// Check validates if hostname match dns sub domain regex.
// Check validates if hostname match dns subdomain regex.
// Check hostname length and format
func (hc HostnameCheck) Check() (warnings, errorList []error) {
klog.V(1).Infoln("checking whether the given node name is valid and reachable using net.LookupHost")
Expand Down Expand Up @@ -578,8 +578,8 @@ func (kubever KubernetesVersionCheck) Check() (warnings, errorList []error) {

// Checks if k8sVersion greater or equal than the first unsupported versions by current version of kubeadm,
// that is major.minor+1 (all patch and pre-releases versions included)
// NB. in semver patches number is a numeric, while prerelease is a string where numeric identifiers always have lower precedence than non-numeric identifiers.
// thus setting the value to x.y.0-0 we are defining the very first patch - prereleases within x.y minor release.
// NB. in semver patches number is a numeric, while pre-release is a string where numeric identifiers always have lower precedence than non-numeric identifiers.
// thus setting the value to x.y.0-0 we are defining the very first patch - pre-releases within x.y minor release.
firstUnsupportedVersion := versionutil.MustParseSemantic(fmt.Sprintf("%d.%d.%s", kadmVersion.Major(), kadmVersion.Minor()+1, "0-0"))
if k8sVersion.AtLeast(firstUnsupportedVersion) {
return []error{errors.Errorf("Kubernetes version is greater than kubeadm version. Please consider to upgrade kubeadm. Kubernetes version: %s. Kubeadm version: %d.%d.x", k8sVersion, kadmVersion.Components()[0], kadmVersion.Components()[1])}, nil
Expand Down Expand Up @@ -920,10 +920,10 @@ func InitNodeChecks(execer utilsexec.Interface, cfg *kubeadmapi.InitConfiguratio
}

// File content check for IPV4 and IPV6 are needed if it is:
// (dual stack) `--service-cidr` or `--pod-network-cidr` is set with an IPV4 and IPV6 CIDR, `--apiserver-advertise-address` is optional as it can be auto detected.
// (dual stack) `--service-cidr` or `--pod-network-cidr` is set with an IPV4 and IPV6 CIDR, `--apiserver-advertise-address` is optional as it can be auto-detected.
// (single stack) which is decided by the `--apiserver-advertise-address`.
// Note that for the case of dual stack, user might only give IPV6 CIDR for `--service-cidr` and leave the `--apiserver-advertise-address` a default value which will be
// auto detected and properly bound to an IPV4 address, this will make the cluster non-functional eventually. The case like this should be avoided by the validation instead,
// auto-detected and properly bound to an IPV4 address, this will make the cluster non-functional eventually. The case like this should be avoided by the validation instead,
// i.e. We don't care whether the input values for those parameters are set correctly here but if it's an IPV4 scoped CIDR or address we will add the file content check for IPV4,
// as does the IPV6.
IPV4Check := false
Expand Down Expand Up @@ -1105,7 +1105,7 @@ func RunPullImagesCheck(execer utilsexec.Interface, cfg *kubeadmapi.InitConfigur
return RunChecks(checks, os.Stderr, ignorePreflightErrors)
}

// RunChecks runs each check, displays it's warnings/errors, and once all
// RunChecks runs each check, displays its warnings/errors, and once all
// are processed will exit if any errors occurred.
func RunChecks(checks []Checker, ww io.Writer, ignorePreflightErrors sets.Set[string]) error {
var errsBuffer bytes.Buffer
Expand Down Expand Up @@ -1133,7 +1133,7 @@ func RunChecks(checks []Checker, ww io.Writer, ignorePreflightErrors sets.Set[st
return nil
}

// setHasItemOrAll is helper function that return true if item is present in the set (case insensitive) or special key 'all' is present
// setHasItemOrAll is helper function that return true if item is present in the set (case-insensitive) or special key 'all' is present
func setHasItemOrAll(s sets.Set[string], item string) bool {
if s.Has("all") || s.Has(strings.ToLower(item)) {
return true
Expand All @@ -1142,7 +1142,7 @@ func setHasItemOrAll(s sets.Set[string], item string) bool {
}

// normalizeURLString returns the normalized string, or an error if it can't be parsed into an URL object.
// It takes an URL string as input.
// It takes a URL string as input.
func normalizeURLString(s string) (string, error) {
u, err := url.Parse(s)
if err != nil {
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/util/initsystem/initsystem.go
Expand Up @@ -18,7 +18,7 @@ package initsystem

// InitSystem is the interface that describe behaviors of an init system
type InitSystem interface {
// return a string describing how to enable a service
// EnableCommand returns a string describing how to enable a service
EnableCommand(service string) string

// ServiceStart tries to start a specific service
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/util/pkiutil/testing/testing.go
Expand Up @@ -39,7 +39,7 @@ func RunWithPrivateKeyFixtureDirectory(m *testing.M) {
os.Exit(m.Run())
}

// Reset() indicates a new test is starting and previously returned private key fixtures may be reused.
// Reset indicates a new test is starting and previously returned private key fixtures may be reused.
func Reset() {
lock.Lock()
defer lock.Unlock()
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/util/staticpod/utils.go
Expand Up @@ -90,7 +90,7 @@ func ComponentPod(container v1.Container, volumes map[string]v1.Volume, annotati
func ComponentResources(cpu string) v1.ResourceRequirements {
return v1.ResourceRequirements{
Requests: v1.ResourceList{
v1.ResourceName(v1.ResourceCPU): resource.MustParse(cpu),
v1.ResourceCPU: resource.MustParse(cpu),
},
}
}
Expand Down
2 changes: 1 addition & 1 deletion cmd/kubeadm/app/util/version.go
Expand Up @@ -35,7 +35,7 @@ import (
)

const (
getReleaseVersionTimeout = time.Duration(10 * time.Second)
getReleaseVersionTimeout = 10 * time.Second
)

var (
Expand Down