chore: remove some commented code and simplify some (openkruise#983)

Signed-off-by: Zhizhen He <hezhizhen.yi@gmail.com>
Signed-off-by: Liu Zhenwei <zwliu@thoughtworks.com>

cmd/daemon/main.go

@@ -23,17 +23,17 @@ import (
 	_ "net/http/pprof"
 	"time"
 
-	"github.com/openkruise/kruise/pkg/daemon"
-	"sigs.k8s.io/controller-runtime/pkg/manager/signals"
-
-	"github.com/openkruise/kruise/pkg/client"
-	"github.com/openkruise/kruise/pkg/features"
-	utilfeature "github.com/openkruise/kruise/pkg/util/feature"
 	"github.com/spf13/pflag"
 	"k8s.io/klog/v2"
 	"k8s.io/klog/v2/klogr"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client/config"
+	"sigs.k8s.io/controller-runtime/pkg/manager/signals"
+
+	"github.com/openkruise/kruise/pkg/client"
+	"github.com/openkruise/kruise/pkg/daemon"
+	"github.com/openkruise/kruise/pkg/features"
+	utilfeature "github.com/openkruise/kruise/pkg/util/feature"
 )
 
 var (

pkg/webhook/cloneset/mutating/cloneset_create_update_handler.go

@@ -22,25 +22,20 @@ import (
 	"net/http"
 	"reflect"
 
+	admissionv1 "k8s.io/api/admission/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/klog/v2"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+
 	"github.com/openkruise/kruise/apis/apps/defaults"
 	appsv1alpha1 "github.com/openkruise/kruise/apis/apps/v1alpha1"
 	"github.com/openkruise/kruise/pkg/features"
 	"github.com/openkruise/kruise/pkg/util"
 	utilfeature "github.com/openkruise/kruise/pkg/util/feature"
-	admissionv1 "k8s.io/api/admission/v1"
-	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/klog/v2"
-	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 )
 
 // CloneSetCreateUpdateHandler handles CloneSet
 type CloneSetCreateUpdateHandler struct {
-	// To use the client, you need to do the following:
-	// - uncomment it
-	// - import sigs.k8s.io/controller-runtime/pkg/client
-	// - uncomment the InjectClient method at the bottom of this file.
-	// Client  client.Client
-
 	// Decoder decodes objects
 	Decoder *admission.Decoder
 }
@@ -87,14 +82,6 @@ func (h *CloneSetCreateUpdateHandler) Handle(ctx context.Context, req admission.
 	return resp
 }
 
-//var _ inject.Client = &CloneSetCreateUpdateHandler{}
-//
-//// InjectClient injects the client into the CloneSetCreateUpdateHandler
-//func (h *CloneSetCreateUpdateHandler) InjectClient(c client.Client) error {
-//	h.Client = c
-//	return nil
-//}
-
 var _ admission.DecoderInjector = &CloneSetCreateUpdateHandler{}
 
 // InjectDecoder injects the decoder into the CloneSetCreateUpdateHandler

pkg/webhook/server.go

@@ -22,16 +22,17 @@ import (
 	"net/http"
 	"time"
 
-	webhookutil "github.com/openkruise/kruise/pkg/webhook/util"
-	webhookcontroller "github.com/openkruise/kruise/pkg/webhook/util/controller"
-	"github.com/openkruise/kruise/pkg/webhook/util/health"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/rest"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/conversion"
+
+	webhookutil "github.com/openkruise/kruise/pkg/webhook/util"
+	webhookcontroller "github.com/openkruise/kruise/pkg/webhook/util/controller"
+	"github.com/openkruise/kruise/pkg/webhook/util/health"
 )
 
 type GateFunc func() (enabled bool)

pkg/webhook/util/configuration/configuration.go

@@ -23,13 +23,13 @@ import (
 	"net/url"
 	"reflect"
 
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-
-	webhookutil "github.com/openkruise/kruise/pkg/webhook/util"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	clientset "k8s.io/client-go/kubernetes"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+
+	webhookutil "github.com/openkruise/kruise/pkg/webhook/util"
 )
 
 const (
@@ -73,10 +73,12 @@ func Ensure(kubeClient clientset.Interface, handlers map[string]admission.Handle
 		if wh.ClientConfig.Service != nil {
 			wh.ClientConfig.Service.Namespace = webhookutil.GetNamespace()
 			wh.ClientConfig.Service.Name = webhookutil.GetServiceName()
+
+			if host := webhookutil.GetHost(); len(host) > 0 {
+				convertClientConfig(&wh.ClientConfig, host, webhookutil.GetPort())
+			}
 		}
-		if host := webhookutil.GetHost(); len(host) > 0 && wh.ClientConfig.Service != nil {
-			convertClientConfig(&wh.ClientConfig, host, webhookutil.GetPort())
-		}
+
 		mutatingWHs = append(mutatingWHs, *wh)
 	}
 	mutatingConfig.Webhooks = mutatingWHs
@@ -96,10 +98,12 @@ func Ensure(kubeClient clientset.Interface, handlers map[string]admission.Handle
 		if wh.ClientConfig.Service != nil {
 			wh.ClientConfig.Service.Namespace = webhookutil.GetNamespace()
 			wh.ClientConfig.Service.Name = webhookutil.GetServiceName()
+
+			if host := webhookutil.GetHost(); len(host) > 0 {
+				convertClientConfig(&wh.ClientConfig, host, webhookutil.GetPort())
+			}
 		}
-		if host := webhookutil.GetHost(); len(host) > 0 && wh.ClientConfig.Service != nil {
-			convertClientConfig(&wh.ClientConfig, host, webhookutil.GetPort())
-		}
+
 		validatingWHs = append(validatingWHs, *wh)
 	}
 	validatingConfig.Webhooks = validatingWHs
@@ -122,7 +126,8 @@ func Ensure(kubeClient clientset.Interface, handlers map[string]admission.Handle
 func getPath(clientConfig *admissionregistrationv1.WebhookClientConfig) (string, error) {
 	if clientConfig.Service != nil {
 		return *clientConfig.Service.Path, nil
-	} else if clientConfig.URL != nil {
+	}
+	if clientConfig.URL != nil {
 		u, err := url.Parse(*clientConfig.URL)
 		if err != nil {
 			return "", err

pkg/webhook/util/controller/webhook_controller.go

@@ -22,12 +22,6 @@ import (
 	"sync"
 	"time"
 
-	extclient "github.com/openkruise/kruise/pkg/client"
-	webhookutil "github.com/openkruise/kruise/pkg/webhook/util"
-	"github.com/openkruise/kruise/pkg/webhook/util/configuration"
-	"github.com/openkruise/kruise/pkg/webhook/util/crd"
-	"github.com/openkruise/kruise/pkg/webhook/util/generator"
-	"github.com/openkruise/kruise/pkg/webhook/util/writer"
 	admissionregistrationv1 "k8s.io/api/admissionregistration/v1"
 	v1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
@@ -46,6 +40,13 @@ import (
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog/v2"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+
+	extclient "github.com/openkruise/kruise/pkg/client"
+	webhookutil "github.com/openkruise/kruise/pkg/webhook/util"
+	"github.com/openkruise/kruise/pkg/webhook/util/configuration"
+	"github.com/openkruise/kruise/pkg/webhook/util/crd"
+	"github.com/openkruise/kruise/pkg/webhook/util/generator"
+	"github.com/openkruise/kruise/pkg/webhook/util/writer"
 )
 
 const (
@@ -72,13 +73,10 @@ type Controller struct {
 	handlers   map[string]admission.Handler
 
 	informerFactory informers.SharedInformerFactory
-	//secretLister       corelisters.SecretNamespaceLister
-	//mutatingWCLister   admissionregistrationlisters.MutatingWebhookConfigurationLister
-	//validatingWCLister admissionregistrationlisters.ValidatingWebhookConfigurationLister
-	crdClient   apiextensionsclientset.Interface
-	crdInformer cache.SharedIndexInformer
-	crdLister   apiextensionslisters.CustomResourceDefinitionLister
-	synced      []cache.InformerSynced
+	crdClient       apiextensionsclientset.Interface
+	crdInformer     cache.SharedIndexInformer
+	crdLister       apiextensionslisters.CustomResourceDefinitionLister
+	synced          []cache.InformerSynced
 
 	queue workqueue.RateLimitingInterface
 }
@@ -94,9 +92,6 @@ func New(cfg *rest.Config, handlers map[string]admission.Handler) (*Controller,
 
 	secretInformer := coreinformers.New(c.informerFactory, namespace, nil).Secrets()
 	admissionRegistrationInformer := admissionregistrationinformers.New(c.informerFactory, v1.NamespaceAll, nil)
-	//c.secretLister = secretInformer.Lister().Secrets(namespace)
-	//c.mutatingWCLister = admissionRegistrationInformer.MutatingWebhookConfigurations().Lister()
-	//c.validatingWCLister = admissionRegistrationInformer.ValidatingWebhookConfigurations().Lister()
 
 	secretInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
 		AddFunc: func(obj interface{}) {
@@ -229,14 +224,14 @@ func (c *Controller) sync() error {
 		klog.Infof("Finished to sync webhook certs and configurations")
 	}()
 
-	var dnsName string
-	var certWriter writer.CertWriter
-	var err error
-
-	if dnsName = webhookutil.GetHost(); len(dnsName) == 0 {
+	dnsName := webhookutil.GetHost()
+	if len(dnsName) == 0 {
 		dnsName = generator.ServiceToCommonName(webhookutil.GetNamespace(), webhookutil.GetServiceName())
 	}
 
+	var certWriter writer.CertWriter
+	var err error
+
 	certWriterType := webhookutil.GetCertWriter()
 	if certWriterType == writer.FsCertWriter || (len(certWriterType) == 0 && len(webhookutil.GetHost()) != 0) {
 		certWriter, err = writer.NewFSCertWriter(writer.FSCertWriterOptions{

pkg/webhook/util/crd/crd.go

@@ -21,15 +21,16 @@ import (
 	"fmt"
 	"reflect"
 
-	"github.com/openkruise/kruise/apis"
-	webhookutil "github.com/openkruise/kruise/pkg/webhook/util"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apiextensionsclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
 	apiextensionslisters "k8s.io/apiextensions-apiserver/pkg/client/listers/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+
+	"github.com/openkruise/kruise/apis"
+	webhookutil "github.com/openkruise/kruise/pkg/webhook/util"
 )
 
 var (

pkg/webhook/util/generator/selfsigned.go

@@ -65,12 +65,9 @@ func (cp *SelfSignedCertGenerator) SetCA(caKey, caCert []byte) {
 // client to verify the server authentication chain.
 // The cert will be valid for 365 days.
 func (cp *SelfSignedCertGenerator) Generate(commonName string) (*Artifacts, error) {
-	var signingKey *rsa.PrivateKey
-	var signingCert *x509.Certificate
-	var valid bool
 	var err error
 
-	valid, signingKey, signingCert = cp.validCACert()
+	valid, signingKey, signingCert := cp.validCACert()
 	if !valid {
 		signingKey, err = NewPrivateKey()
 		if err != nil {
@@ -115,12 +112,10 @@ func (cp *SelfSignedCertGenerator) Generate(commonName string) (*Artifacts, erro
 }
 
 func (cp *SelfSignedCertGenerator) validCACert() (bool, *rsa.PrivateKey, *x509.Certificate) {
-	if !ValidCACert(cp.caKey, cp.caCert, cp.caCert, "",
-		time.Now().AddDate(1, 0, 0)) {
+	if !ValidCACert(cp.caKey, cp.caCert, cp.caCert, "", time.Now().AddDate(1, 0, 0)) {
 		return false, nil, nil
 	}
 
-	var ok bool
 	key, err := keyutil.ParsePrivateKeyPEM(cp.caKey)
 	if err != nil {
 		return false, nil, nil

pkg/webhook/util/generator/util.go

@@ -24,7 +24,7 @@ import (
 	"time"
 )
 
-// ValidCACert think cert and key are valid if they meet the following requirements:
+// ValidCACert treats cert and key are valid if they meet the following requirements:
 // - key and cert are valid pair
 // - caCert is the root ca of cert
 // - cert is for dnsName

pkg/webhook/util/health/checker.go

@@ -132,8 +132,5 @@ func Checker(_ *http.Request) error {
 	lock.Lock()
 	defer lock.Unlock()
 	_, err = client.Do(req)
-	if err != nil {
-		return err
-	}
-	return nil
+	return err
 }

pkg/webhook/util/writer/certwriter.go

@@ -18,14 +18,12 @@ limitations under the License.
 package writer
 
 import (
-	"crypto/tls"
-	"crypto/x509"
-	"encoding/pem"
 	"errors"
 	"time"
 
-	"github.com/openkruise/kruise/pkg/webhook/util/generator"
 	"k8s.io/klog/v2"
+
+	"github.com/openkruise/kruise/pkg/webhook/util/generator"
 )
 
 const (
@@ -65,7 +63,7 @@ func handleCommon(dnsName string, ch certReadWriter) (*generator.Artifacts, bool
 	// Recreate the cert if it's invalid.
 	valid := validCert(certs, dnsName)
 	if !valid {
-		klog.Info("cert is invalid or expiring, regenerating a new one")
+		klog.Info("cert is invalid or expired, regenerating a new one")
 		certs, err = ch.overwrite(certs.ResourceVersion)
 		if err != nil {
 			return nil, false, err
@@ -81,57 +79,29 @@ func createIfNotExists(ch certReadWriter) (*generator.Artifacts, bool, error) {
 	if isNotFound(err) {
 		// Create if not exists
 		certs, err = ch.write()
-		switch {
 		// This may happen if there is another racer.
-		case isAlreadyExists(err):
+		if isAlreadyExists(err) {
 			certs, err = ch.read()
-			return certs, true, err
-		default:
-			return certs, true, err
 		}
+		return certs, true, err
 	}
 	return certs, false, err
 }
 
 // certReadWriter provides methods for reading and writing certificates.
 type certReadWriter interface {
-	// read reads a webhook name and returns the certs for it.
+	// read a webhook name and returns the certs for it.
 	read() (*generator.Artifacts, error)
-	// write writes the certs and return the certs it wrote.
+	// write the certs and return the certs it wrote.
 	write() (*generator.Artifacts, error)
-	// overwrite overwrites the existing certs and return the certs it wrote.
+	// overwrite the existing certs and return the certs it wrote.
 	overwrite(resourceVersion string) (*generator.Artifacts, error)
 }
 
 func validCert(certs *generator.Artifacts, dnsName string) bool {
-	if certs == nil || certs.Cert == nil || certs.Key == nil || certs.CACert == nil {
-		return false
-	}
-
-	// Verify key and cert are valid pair
-	_, err := tls.X509KeyPair(certs.Cert, certs.Key)
-	if err != nil {
-		return false
-	}
-
-	// Verify cert is good for desired DNS name and signed by CA and will be valid for desired period of time.
-	pool := x509.NewCertPool()
-	if !pool.AppendCertsFromPEM(certs.CACert) {
-		return false
-	}
-	block, _ := pem.Decode(certs.Cert)
-	if block == nil {
+	if certs == nil {
 		return false
 	}
-	cert, err := x509.ParseCertificate(block.Bytes)
-	if err != nil {
-		return false
-	}
-	ops := x509.VerifyOptions{
-		DNSName:     dnsName,
-		Roots:       pool,
-		CurrentTime: time.Now().AddDate(0, 6, 0),
-	}
-	_, err = cert.Verify(ops)
-	return err == nil
+	expired := time.Now().AddDate(0, 6, 0)
+	return generator.ValidCACert(certs.Key, certs.Cert, certs.CACert, dnsName, expired)
 }