Ensure v1 webhook is created and add extra logs (#3736)

operator/utils/k8s/configmap.go

@@ -22,7 +22,7 @@ type ConfigmapCreator struct {
 func NewConfigmapCreator(client kubernetes.Interface, logger logr.Logger, scheme *runtime.Scheme) *ConfigmapCreator {
 	return &ConfigmapCreator{
 		clientset: client,
-		logger:    logger,
+		logger:    logger.WithName("ConfigMapCreator"),
 		scheme:    scheme,
 	}
 }
@@ -43,6 +43,7 @@ func (cc *ConfigmapCreator) CreateConfigmap(ctx context.Context, rawYaml []byte,
 	// add ownership
 	err = ctrl.SetControllerReference(owner, &cm, cc.scheme)
 	if err != nil {
+		cc.logger.Error(err, "Failed to set controller reference on configmap")
 		return err
 	}
 

operator/utils/k8s/crd.go

@@ -24,7 +24,7 @@ func NewCrdCreator(ctx context.Context, apiExtensionsClient apiextensionsclient.
 	return &CrdCreator{
 		apiExtensionsClient: apiExtensionsClient,
 		discoveryClient:     discoveryClient,
-		logger:              logger,
+		logger:              logger.WithName("CRDCreator"),
 		ctx:                 ctx,
 	}
 }
@@ -43,6 +43,7 @@ func (cc *CrdCreator) createCRDV1beta1(rawYaml []byte) (*v1beta1.CustomResourceD
 	crd := v1beta1.CustomResourceDefinition{}
 	err := yaml.Unmarshal(rawYaml, &crd)
 	if err != nil {
+		cc.logger.Error(err, "Failed to unmarshall v1beta1 CRD")
 		return nil, err
 	}
 	client := cc.apiExtensionsClient.ApiextensionsV1beta1().CustomResourceDefinitions()
@@ -53,6 +54,7 @@ func (cc *CrdCreator) createCRDV1(rawYaml []byte) (*extensionsv1.CustomResourceD
 	crd := extensionsv1.CustomResourceDefinition{}
 	err := yaml.Unmarshal(rawYaml, &crd)
 	if err != nil {
+		cc.logger.Error(err, "Failed to unmarshall V1 CRD")
 		return nil, err
 	}
 	client := cc.apiExtensionsClient.ApiextensionsV1().CustomResourceDefinitions()
@@ -68,10 +70,12 @@ func (cc *CrdCreator) findOrCreateCRDV1beta1(rawYaml []byte) (v1.Object, error)
 			cc.logger.Info("CRD v1beta1 not found - trying to create")
 			crd, err = cc.createCRDV1beta1(rawYaml)
 			if err != nil {
+				cc.logger.Error(err, "Failed to create v1beta1 CRD")
 				return nil, err
 			}
 			cc.logger.Info("CRD v1beta1 created")
 		} else {
+			cc.logger.Error(err, "Failed finding v1beta1 crd")
 			return nil, err
 		}
 	} else {
@@ -89,10 +93,12 @@ func (cc *CrdCreator) findOrCreateCRDV1(rawYaml []byte) (*extensionsv1.CustomRes
 			cc.logger.Info("CRD V1 not found - trying to create")
 			crd, err = cc.createCRDV1(rawYaml)
 			if err != nil {
+				cc.logger.Error(err, "Failed to create v1 CRD")
 				return nil, err
 			}
 			cc.logger.Info("CRD V1 created")
 		} else {
+			cc.logger.Error(err, "Failed finding V1 CRD")
 			return nil, err
 		}
 	} else {
@@ -104,14 +110,17 @@ func (cc *CrdCreator) findOrCreateCRDV1(rawYaml []byte) (*extensionsv1.CustomRes
 func (cc *CrdCreator) findOrCreateCRD(rawYamlv1 []byte, rawYamlv1beta1 []byte) (v1.Object, error) {
 	serverVersion, err := GetServerVersion(cc.discoveryClient, cc.logger)
 	if err != nil {
+		cc.logger.Error(err, "Failed to get version from cluster")
 		return nil, err
 	}
 	v, err := semver.NewVersion(serverVersion)
 	if err != nil {
+		cc.logger.Error(err, "Failed to create semver Version")
 		return nil, err
 	}
 	c, err := semver.NewConstraint(">= 1.18.0")
 	if err != nil {
+		cc.logger.Error(err, "Failed applying constraint to check greater than 1.18.0 cluster")
 		return nil, err
 	}
 	check := c.Check(v)

operator/utils/k8s/initializer.go

@@ -48,35 +48,42 @@ func InitializeOperator(ctx context.Context, config *rest.Config, namespace stri
 
 	apiExtensionClient, err := apiextensionsclient.NewForConfig(config)
 	if err != nil {
+		logger.Error(err, "Failed to create apiextensionsClient")
 		return err
 	}
 
 	discoveryClient, err := discovery.NewDiscoveryClientForConfig(config)
 	if err != nil {
+		logger.Error(err, "Failed to create discoveryClient")
 		return err
 	}
 
 	crdCreator := NewCrdCreator(ctx, apiExtensionClient, discoveryClient, logger)
 	bytesV1Beta1, err := LoadBytesFromFile(ResourceFolder, CRDFilenameV1Beta1)
 	if err != nil {
+		logger.Error(err, "Failed to find crd v1beta1", "resourcefolder", ResourceFolder, "filename", CRDFilenameV1Beta1)
 		return err
 	}
 	bytesV1, err := LoadBytesFromFile(ResourceFolder, CRDFilenameV1)
 	if err != nil {
+		logger.Error(err, "Failed to find crd v1", "resourcefolder", ResourceFolder, "filename", CRDFilenameV1)
 		return err
 	}
 	crd, err := crdCreator.findOrCreateCRD(bytesV1, bytesV1Beta1)
 	if err != nil {
+		logger.Error(err, "Failed to create CRD")
 		return err
 	}
 
 	clientset, err := kubernetes.NewForConfig(config)
 	if err != nil {
+		logger.Error(err, "Failed to create clientset")
 		return err
 	}
 
 	dep, err := findMyDeployment(ctx, clientset, namespace)
 	if err != nil {
+		logger.Error(err, "Failed to find deployment")
 		return err
 	}
 
@@ -85,78 +92,87 @@ func InitializeOperator(ctx context.Context, config *rest.Config, namespace stri
 	host2 := fmt.Sprintf("seldon-webhook-service.%s.svc", namespace)
 	certs, err := certSetup([]string{host1, host2})
 	if err != nil {
+		logger.Error(err, "Failed to create certs")
 		return err
 	}
 
 	// Create webhooks
-	wc, err := NewWebhookCreator(clientset, certs, logger, scheme)
-	if err != nil {
-		return err
-	}
-
-	//Delete mutating webhook if existing
-	err = wc.DeleteMutatingWebhook(ctx)
-	if err != nil {
-		return err
-	}
+	wc := NewWebhookCreator(clientset, certs, logger, scheme)
 
 	//Create/Update Validating Webhook
 	bytes, err := LoadBytesFromFile(ResourceFolder, ValidatingWebhookFilename)
 	if err != nil {
+		logger.Error(err, "Failed to find webhook file", "resourcefolder", ResourceFolder, "filename", ValidatingWebhookFilename)
 		return err
 	}
 	err = wc.CreateValidatingWebhookConfigurationFromFile(ctx, bytes, namespace, crd, watchNamespace)
 	if err != nil {
+		logger.Error(err, "Failed to create validating webhook")
 		return err
 	}
 
 	//Create/Update Webhook Service
 	bytes, err = LoadBytesFromFile(ResourceFolder, ServiceFilename)
 	if err != nil {
+		logger.Error(err, "Failed to find webhook service", "resourcefolder", ResourceFolder, "filename", ServiceFilename)
 		return err
 	}
 	err = wc.CreateWebhookServiceFromFile(ctx, bytes, namespace, dep)
 	if err != nil {
+		logger.Error(err, "Failed to create webhook service")
 		return err
 	}
 
 	//Create Configmap
 	cc := NewConfigmapCreator(clientset, logger, scheme)
 	bytes, err = LoadBytesFromFile(ResourceFolder, ConfigMapFilename)
 	if err != nil {
+		logger.Error(err, "Failed to find configmap", "resourcefolder", ResourceFolder, "filename", ConfigMapFilename)
 		return err
 	}
 	err = cc.CreateConfigmap(ctx, bytes, namespace, dep)
 	if err != nil {
+		logger.Error(err, "Failed to create webhook")
 		return err
 	}
 
 	// Create cert files
-	createCertFiles(certs, logger)
+	err = createCertFiles(certs, logger)
+	if err != nil {
+		logger.Error(err, "Failed to create crts files")
+		return err
+	}
 
 	return nil
 }
 
 func createCertFiles(certs *Cert, logger logr.Logger) error {
 	//Save certs to filesystem
-	os.MkdirAll(CertsFolder, os.ModePerm)
+	err := os.MkdirAll(CertsFolder, os.ModePerm)
+	if err != nil {
+		logger.Error(err, "Failed to create folder", "folder", CertsFolder)
+		return err
+	}
 
 	filename := fmt.Sprintf("%s/%s", CertsFolder, CertsTLSCa)
 	logger.Info("Creating ", "filename", filename)
-	err := ioutil.WriteFile(filename, []byte(certs.caPEM), 0600)
+	err = ioutil.WriteFile(filename, []byte(certs.caPEM), 0600)
 	if err != nil {
+		logger.Error(err, "failed to create cert file", "filename", filename)
 		return err
 	}
 	filename = fmt.Sprintf("%s/%s", CertsFolder, CertsTLSKey)
 	logger.Info("Creating ", "filename", filename)
 	err = ioutil.WriteFile(filename, []byte(certs.privKeyPEM), 0600)
 	if err != nil {
+		logger.Error(err, "failed to create cert file", "filename", filename)
 		return err
 	}
 	filename = fmt.Sprintf("%s/%s", CertsFolder, CertsTLSCrt)
 	logger.Info("Creating ", "filename", filename)
 	err = ioutil.WriteFile(filename, []byte(certs.certificatePEM), 0600)
 	if err != nil {
+		logger.Error(err, "failed to create cert file", "filename", filename)
 		return err
 	}
 

operator/utils/k8s/utils.go

@@ -14,6 +14,7 @@ const DefaultMinorVersion = 18
 func GetServerVersion(client discovery.DiscoveryInterface, logger logr.Logger) (string, error) {
 	serverVersion, err := client.ServerVersion()
 	if err != nil {
+		logger.Error(err, "Failed to get server version")
 		return "", err
 	}
 	logger.Info("Server version", "Major", serverVersion.Major, "Minor", serverVersion.Minor)

operator/utils/k8s/webhook.go

@@ -4,7 +4,7 @@ import (
 	"context"
 	"github.com/ghodss/yaml"
 	"github.com/go-logr/logr"
-	"k8s.io/api/admissionregistration/v1beta1"
+	adv1 "k8s.io/api/admissionregistration/v1"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -23,48 +23,35 @@ type WebhookCreator struct {
 	scheme    *runtime.Scheme
 }
 
-func NewWebhookCreator(client kubernetes.Interface, certs *Cert, logger logr.Logger, scheme *runtime.Scheme) (*WebhookCreator, error) {
+func NewWebhookCreator(client kubernetes.Interface, certs *Cert, logger logr.Logger, scheme *runtime.Scheme) *WebhookCreator {
 	return &WebhookCreator{
 		clientset: client,
 		certs:     certs,
-		logger:    logger,
+		logger:    logger.WithName("WebhookCreator"),
 		scheme:    scheme,
-	}, nil
-}
-
-func (wc *WebhookCreator) DeleteMutatingWebhook(ctx context.Context) error {
-	client := wc.clientset.AdmissionregistrationV1beta1().MutatingWebhookConfigurations()
-
-	// Try to delete clusterwide webhook config if available (older versions of seldon core)
-	_, err := client.Get(ctx, MutatingWebhookName, v1.GetOptions{})
-	if err != nil && errors.IsNotFound(err) {
-		wc.logger.Info("existing clusterwide mwc not found", "name", MutatingWebhookName)
-	} else {
-		client.Delete(ctx, MutatingWebhookName, v1.DeleteOptions{})
-		if err != nil {
-			return err
-		}
-		wc.logger.Info("Deleted clusterwide mwc", "name", MutatingWebhookName)
 	}
-
-	return nil
 }
 
 func (wc *WebhookCreator) CreateValidatingWebhookConfigurationFromFile(ctx context.Context, rawYaml []byte, namespace string, owner v1.Object, watchNamespace bool) error {
-	vwc := v1beta1.ValidatingWebhookConfiguration{}
+	vwc := adv1.ValidatingWebhookConfiguration{}
 	err := yaml.Unmarshal(rawYaml, &vwc)
 	if err != nil {
+		wc.logger.Error(err, "Failed to unmarshall validating webhook")
 		return err
 	}
 
 	// create or update via client
-	client := wc.clientset.AdmissionregistrationV1beta1().ValidatingWebhookConfigurations()
+	client := wc.clientset.AdmissionregistrationV1().ValidatingWebhookConfigurations()
 
 	if watchNamespace {
 		// Try to delete clusterwide webhook config if available
 		_, err := client.Get(ctx, vwc.Name, v1.GetOptions{})
-		if err != nil && errors.IsNotFound(err) {
-			wc.logger.Info("existing clusterwide vwc not found", "name", vwc.Name)
+		if err != nil {
+			if errors.IsNotFound(err) {
+				wc.logger.Info("existing clusterwide vwc not found", "name", vwc.Name)
+			} else {
+				wc.logger.Error(err, "Ignoring error trying to get existing validating webhook")
+			}
 		} else {
 			client.Delete(ctx, vwc.Name, v1.DeleteOptions{})
 			if err != nil {
@@ -91,17 +78,24 @@ func (wc *WebhookCreator) CreateValidatingWebhookConfigurationFromFile(ctx conte
 	// add ownership
 	err = ctrl.SetControllerReference(owner, &vwc, wc.scheme)
 	if err != nil {
+		wc.logger.Error(err, "Failed to set owner reference on validating webhook")
 		return err
 	}
 
 	found, err := client.Get(ctx, vwc.Name, v1.GetOptions{})
 	if err != nil && errors.IsNotFound(err) {
 		wc.logger.Info("Creating validating webhook")
 		_, err = client.Create(ctx, &vwc, v1.CreateOptions{})
+		if err != nil {
+			wc.logger.Error(err, "Failed to create validating webhook")
+		}
 	} else if err == nil {
 		wc.logger.Info("Updating validating webhook")
 		found.Webhooks = vwc.Webhooks
 		_, err = client.Update(ctx, found, v1.UpdateOptions{})
+		if err != nil {
+			wc.logger.Error(err, "Failed to update validating webhook")
+		}
 		return err
 	}
 	return err
@@ -135,9 +129,12 @@ func (wc *WebhookCreator) CreateWebhookServiceFromFile(ctx context.Context, rawY
 	if err != nil && errors.IsNotFound(err) {
 		wc.logger.Info("Creating webhook svc")
 		_, err = client.Create(ctx, &svc, v1.CreateOptions{})
+		if err != nil {
+			wc.logger.Error(err, "Failed to update webhook svc")
+		}
 	} else if err == nil {
 		wc.logger.Info("Webhook svc exists won't update - need a patch in future")
-		return err
+		return nil
 	}
 	return err
 }

operator/utils/k8s/webhook_test.go

@@ -110,11 +110,10 @@ func TestValidatingWebhookCreate(t *testing.T) {
 	hosts := []string{"seldon-webhook-service.seldon-system", "seldon-webhook-service.seldon-system.svc"}
 	certs, err := certSetup(hosts)
 	g.Expect(err).To(BeNil())
-	wc, err := NewWebhookCreator(client, certs, ctrl.Log, scheme)
-	g.Expect(err).To(BeNil())
+	wc := NewWebhookCreator(client, certs, ctrl.Log, scheme)
 	err = wc.CreateValidatingWebhookConfigurationFromFile(context.TODO(), bytes, TestNamespace, crd, false)
 	g.Expect(err).To(BeNil())
-	_, err = client.AdmissionregistrationV1beta1().ValidatingWebhookConfigurations().Get(context.TODO(), "seldon-validating-webhook", metav1.GetOptions{})
+	_, err = client.AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(context.TODO(), "seldon-validating-webhook", metav1.GetOptions{})
 	g.Expect(err).To(BeNil())
 }
 
@@ -130,11 +129,10 @@ func TestValidatingWebhookCreateNamespaced(t *testing.T) {
 	hosts := []string{"seldon-webhook-service.seldon-system", "seldon-webhook-service.seldon-system.svc"}
 	certs, err := certSetup(hosts)
 	g.Expect(err).To(BeNil())
-	wc, err := NewWebhookCreator(client, certs, ctrl.Log, scheme)
-	g.Expect(err).To(BeNil())
+	wc := NewWebhookCreator(client, certs, ctrl.Log, scheme)
 	err = wc.CreateValidatingWebhookConfigurationFromFile(context.TODO(), bytes, TestNamespace, crd, true)
 	g.Expect(err).To(BeNil())
-	_, err = client.AdmissionregistrationV1beta1().ValidatingWebhookConfigurations().Get(context.TODO(), "seldon-validating-webhook-"+TestNamespace, metav1.GetOptions{})
+	_, err = client.AdmissionregistrationV1().ValidatingWebhookConfigurations().Get(context.TODO(), "seldon-validating-webhook-"+TestNamespace, metav1.GetOptions{})
 	g.Expect(err).To(BeNil())
 }
 
@@ -149,8 +147,7 @@ func TestWebHookSvcCreate(t *testing.T) {
 	hosts := []string{"seldon-webhook-service.seldon-system", "seldon-webhook-service.seldon-system.svc"}
 	certs, err := certSetup(hosts)
 	g.Expect(err).To(BeNil())
-	wc, err := NewWebhookCreator(client, certs, ctrl.Log, scheme)
-	g.Expect(err).To(BeNil())
+	wc := NewWebhookCreator(client, certs, ctrl.Log, scheme)
 	err = wc.CreateWebhookServiceFromFile(context.TODO(), bytes, TestNamespace, dep)
 	g.Expect(err).To(BeNil())
 }