[improvement] adding verified clients for validation webhooks

api/v1alpha2/linodecluster_webhook.go

@@ -25,8 +25,8 @@
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
-	"sigs.k8s.io/controller-runtime/pkg/webhook"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	. "github.com/linode/cluster-api-provider-linode/clients"
@@ -35,70 +35,89 @@
 // log is for logging in this package.
 var linodeclusterlog = logf.Log.WithName("linodecluster-resource")
 
+type linodeClusterValidator struct {
+	Client client.Client
+}
+
 // SetupWebhookWithManager will setup the manager to manage the webhooks
 func (r *LinodeCluster) SetupWebhookWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewWebhookManagedBy(mgr).
 		For(r).
+		WithValidator(&linodeClusterValidator{Client: mgr.GetClient()}).
 		Complete()
 }
 
 // TODO(user): change verbs to "verbs=create;update;delete" if you want to enable update and deletion validation.
 // +kubebuilder:webhook:path=/validate-infrastructure-cluster-x-k8s-io-v1alpha2-linodecluster,mutating=false,failurePolicy=fail,sideEffects=None,groups=infrastructure.cluster.x-k8s.io,resources=linodeclusters,verbs=create,versions=v1alpha2,name=validation.linodecluster.infrastructure.cluster.x-k8s.io,admissionReviewVersions=v1
 
-var _ webhook.Validator = &LinodeCluster{}
-
 // ValidateCreate implements webhook.Validator so a webhook will be registered for the type
-func (r *LinodeCluster) ValidateCreate() (admission.Warnings, error) {
-	linodeclusterlog.Info("validate create", "name", r.Name)
+func (r *linodeClusterValidator) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	cluster, ok := obj.(*LinodeCluster)
+	if !ok {
+		return nil, apierrors.NewBadRequest("expected a LinodeCluster Resource")
+	}
+	spec := cluster.Spec
+	linodeclusterlog.Info("validate create", "name", cluster.Name)
 
-	ctx, cancel := context.WithTimeout(context.Background(), defaultWebhookTimeout)
-	defer cancel()
+	var linodeclient LinodeClient = defaultLinodeClient
 
-	return nil, r.validateLinodeCluster(ctx, &defaultLinodeClient)
+	if spec.CredentialsRef != nil {
+		apiToken, err := getCredentialDataFromRef(ctx, r.Client, *spec.CredentialsRef, cluster.GetNamespace())
+		if err != nil {
+			linodeclusterlog.Error(err, "failed getting credentials from secret ref", "name", cluster.Name)
+			return nil, err
+		}
+		linodeclusterlog.Info("creating a verified linode client for create request", "name", cluster.Name)
+		linodeclient.SetToken(string(apiToken))
+	}
+	// TODO: instrument with tracing, might need refactor to preserve readibility
+	var errs field.ErrorList
+
+	if err := r.validateLinodeClusterSpec(ctx, linodeclient, spec); err != nil {
+		errs = slices.Concat(errs, err)
+	}
+
+	if len(errs) == 0 {
+		return nil, nil
+	}
+	return nil, apierrors.NewInvalid(
+		schema.GroupKind{Group: "infrastructure.cluster.x-k8s.io", Kind: "LinodeCluster"},
+		cluster.Name, errs)
 }
 
 // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
-func (r *LinodeCluster) ValidateUpdate(old runtime.Object) (admission.Warnings, error) {
-	linodeclusterlog.Info("validate update", "name", r.Name)
+func (r *linodeClusterValidator) ValidateUpdate(ctx context.Context, oldObj, newObj runtime.Object) (admission.Warnings, error) {
+	old, ok := oldObj.(*LinodeCluster)
+	if !ok {
+		return nil, apierrors.NewBadRequest("expected a LinodeCluster Resource")
+	}
+	linodeclusterlog.Info("validate update", "name", old.Name)
 
 	// TODO(user): fill in your validation logic upon object update.
 	return nil, nil
 }
 
 // ValidateDelete implements webhook.Validator so a webhook will be registered for the type
-func (r *LinodeCluster) ValidateDelete() (admission.Warnings, error) {
-	linodeclusterlog.Info("validate delete", "name", r.Name)
+func (r *linodeClusterValidator) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) {
+	c, ok := obj.(*LinodeCluster)
+	if !ok {
+		return nil, apierrors.NewBadRequest("expected a LinodeCluster Resource")
+	}
+	linodeclusterlog.Info("validate delete", "name", c.Name)
 
 	// TODO(user): fill in your validation logic upon object deletion.
 	return nil, nil
 }
 
-func (r *LinodeCluster) validateLinodeCluster(ctx context.Context, client LinodeClient) error {
-	// TODO: instrument with tracing, might need refactor to preserve readibility
-	var errs field.ErrorList
-
-	if err := r.validateLinodeClusterSpec(ctx, client); err != nil {
-		errs = slices.Concat(errs, err)
-	}
-
-	if len(errs) == 0 {
-		return nil
-	}
-	return apierrors.NewInvalid(
-		schema.GroupKind{Group: "infrastructure.cluster.x-k8s.io", Kind: "LinodeCluster"},
-		r.Name, errs)
-}
-
-func (r *LinodeCluster) validateLinodeClusterSpec(ctx context.Context, client LinodeClient) field.ErrorList {
-	// TODO: instrument with tracing, might need refactor to preserve readibility
+func (r *linodeClusterValidator) validateLinodeClusterSpec(ctx context.Context, linodeclient LinodeClient, spec LinodeClusterSpec) field.ErrorList {
 	var errs field.ErrorList
 
-	if err := validateRegion(ctx, client, r.Spec.Region, field.NewPath("spec").Child("region")); err != nil {
+	if err := validateRegion(ctx, linodeclient, spec.Region, field.NewPath("spec").Child("region")); err != nil {
 		errs = append(errs, err)
 	}
 
-	if r.Spec.Network.LoadBalancerType == "dns" {
-		if r.Spec.Network.DNSRootDomain == "" {
+	if spec.Network.LoadBalancerType == "dns" {
+		if spec.Network.DNSRootDomain == "" {
 			errs = append(errs, &field.Error{
 				Field: "dnsRootDomain needs to be set when LoadBalancer Type is DNS",
 				Type:  field.ErrorTypeRequired,

api/v1alpha2/linodecluster_webhook_test.go

@@ -24,8 +24,11 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.uber.org/mock/gomock"
+	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/utils/ptr"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/linode/cluster-api-provider-linode/mock"
 
@@ -54,6 +57,8 @@ func TestValidateLinodeCluster(t *testing.T) {
 				},
 			},
 		}
+		validator              = &linodeClusterValidator{}
+		expectedErrorSubString = "spec.region: Not found: \"example\""
 	)
 
 	NewSuite(t, mock.MockLinodeClient{}).Run(
@@ -63,7 +68,8 @@ func TestValidateLinodeCluster(t *testing.T) {
 					mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
 				}),
 				Result("success", func(ctx context.Context, mck Mock) {
-					assert.NoError(t, cluster.validateLinodeCluster(ctx, mck.LinodeClient))
+					errs := validator.validateLinodeClusterSpec(ctx, mck.LinodeClient, cluster.Spec)
+					require.Empty(t, errs)
 				}),
 			),
 		),
@@ -73,13 +79,20 @@ func TestValidateLinodeCluster(t *testing.T) {
 			})),
 		),
 		Result("error", func(ctx context.Context, mck Mock) {
-			assert.Error(t, cluster.validateLinodeCluster(ctx, mck.LinodeClient))
+			errs := validator.validateLinodeClusterSpec(ctx, mck.LinodeClient, cluster.Spec)
+			for _, err := range errs {
+				assert.ErrorContains(t, err, expectedErrorSubString)
+			}
 		}),
 	)
 }
 
 func TestValidateCreate(t *testing.T) {
 	t.Parallel()
+	ctrl := gomock.NewController(t)
+	defer ctrl.Finish()
+
+	mockK8sClient := mock.NewMockK8sClient(ctrl)
 
 	var (
 		cluster = LinodeCluster{
@@ -94,18 +107,60 @@ func TestValidateCreate(t *testing.T) {
 				},
 			},
 		}
+		expectedErrorSubString = "\"example\" is invalid: spec.region: Not found:"
+		credentialsRefCluster  = LinodeCluster{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "example",
+				Namespace: "example",
+			},
+			Spec: LinodeClusterSpec{
+				CredentialsRef: &corev1.SecretReference{
+					Name: "cluster-credentials",
+				},
+				Region: "us-ord",
+				Network: NetworkSpec{
+					LoadBalancerType: "NodeBalancer",
+				},
+			},
+		}
+		validator = &linodeClusterValidator{}
 	)
 
 	NewSuite(t, mock.MockLinodeClient{}).Run(
 		OneOf(
 			Path(
-				Call("invalid region", func(ctx context.Context, mck Mock) {
-					mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, errors.New("invalid region")).AnyTimes()
+				Call("invalid request", func(ctx context.Context, mck Mock) {
+
 				}),
 				Result("error", func(ctx context.Context, mck Mock) {
-					//nolint:contextcheck // no context passed
-					_, err := cluster.ValidateCreate()
-					assert.Error(t, err)
+					_, err := validator.ValidateCreate(ctx, &cluster)
+					assert.ErrorContains(t, err, expectedErrorSubString)
+				}),
+			),
+		),
+		OneOf(
+			Path(
+				Call("verfied linodeClient", func(ctx context.Context, mck Mock) {
+					mockK8sClient.EXPECT().Get(ctx, gomock.Any(), gomock.Any()).
+						DoAndReturn(func(ctx context.Context, key types.NamespacedName, obj *corev1.Secret, opts ...client.GetOption) error {
+							cred := corev1.Secret{
+								ObjectMeta: metav1.ObjectMeta{
+									Name:      "cluster-credentials",
+									Namespace: "example",
+								},
+								Data: map[string][]byte{
+									"apiToken": []byte("token"),
+								},
+							}
+							*obj = cred
+
+							return nil
+						}).AnyTimes()
+				}),
+				Result("valid", func(ctx context.Context, mck Mock) {
+					str, err := getCredentialDataFromRef(ctx, mockK8sClient, *credentialsRefCluster.Spec.CredentialsRef, cluster.GetNamespace())
+					require.NoError(t, err)
+					assert.Equal(t, []byte("token"), str)
 				}),
 			),
 		),
@@ -142,6 +197,7 @@ func TestValidateDNSLinodeCluster(t *testing.T) {
 				},
 			},
 		}
+		validator = &linodeClusterValidator{}
 	)
 
 	NewSuite(t, mock.MockLinodeClient{}).Run(
@@ -151,7 +207,8 @@ func TestValidateDNSLinodeCluster(t *testing.T) {
 					mck.LinodeClient.EXPECT().GetRegion(gomock.Any(), gomock.Any()).Return(nil, nil).AnyTimes()
 				}),
 				Result("success", func(ctx context.Context, mck Mock) {
-					assert.NoError(t, validCluster.validateLinodeCluster(ctx, mck.LinodeClient))
+					errs := validator.validateLinodeClusterSpec(ctx, mck.LinodeClient, validCluster.Spec)
+					require.Empty(t, errs)
 				}),
 			),
 		),
@@ -161,7 +218,10 @@ func TestValidateDNSLinodeCluster(t *testing.T) {
 			})),
 		),
 		Result("error", func(ctx context.Context, mck Mock) {
-			require.ErrorContains(t, inValidCluster.validateLinodeCluster(ctx, mck.LinodeClient), "dnsRootDomain")
+			errs := validator.validateLinodeClusterSpec(ctx, mck.LinodeClient, inValidCluster.Spec)
+			for _, err := range errs {
+				require.Contains(t, err.Error(), "dnsRootDomain")
+			}
 		}),
 	)
 }