From 1d0c71d2b8ee005b0b49ddfd92cae46e4cabf4ae Mon Sep 17 00:00:00 2001 From: Kevin McDermott Date: Thu, 30 Nov 2023 14:12:14 +0000 Subject: [PATCH] Validate CRD Spec fields (#83) * Validate CRD Spec fields This configures the CRD to require that we have either a CAPI Cluster ref, or a Secret ref. GitopsClusters will be rejected if they have neither, or have both. Co-authored-by: Yiannis Triantafyllopoulos <8741709+yiannistri@users.noreply.github.com> --- api/v1alpha1/gitopscluster_types.go | 3 + .../gitops.weave.works_gitopsclusters.yaml | 8 ++ controllers/gitopscluster_controller_test.go | 89 +++++++++++++++++++ 3 files changed, 100 insertions(+) diff --git a/api/v1alpha1/gitopscluster_types.go b/api/v1alpha1/gitopscluster_types.go index 5672e17..25159ab 100644 --- a/api/v1alpha1/gitopscluster_types.go +++ b/api/v1alpha1/gitopscluster_types.go @@ -28,6 +28,8 @@ import ( const GitOpsClusterNoSecretFinalizerAnnotation = "clusters.gitops.weave.works/no-secret-finalizer" // GitopsClusterSpec defines the desired state of GitopsCluster +// +kubebuilder:validation:XValidation:rule="(has(self.secretRef) || has(self.capiClusterRef))",message="must provide a secretRef or capiClusterRef" +// +kubebuilder:validation:XValidation:rule="!(has(self.secretRef) && has(self.capiClusterRef))",message="cannot provide both capiClusterRef and secretRef" type GitopsClusterSpec struct { // SecretRef specifies the Secret containing the kubeconfig for a cluster. // +optional @@ -62,6 +64,7 @@ func (in *GitopsCluster) SetConditions(conditions []metav1.Condition) { // +kubebuilder:printcolumn:name="ClusterConnectivity",type="string",JSONPath=".status.conditions[?(@.type==\"ClusterConnectivity\")].status",description="" // GitopsCluster is the Schema for the gitopsclusters API +// +kubebuilder:validation:XValidation:rule="has(self.spec)",message="must confgure spec" type GitopsCluster struct { metav1.TypeMeta `json:",inline"` metav1.ObjectMeta `json:"metadata,omitempty"` diff --git a/config/crd/bases/gitops.weave.works_gitopsclusters.yaml b/config/crd/bases/gitops.weave.works_gitopsclusters.yaml index 9b49df6..bbf7061 100644 --- a/config/crd/bases/gitops.weave.works_gitopsclusters.yaml +++ b/config/crd/bases/gitops.weave.works_gitopsclusters.yaml @@ -67,6 +67,11 @@ spec: - name type: object type: object + x-kubernetes-validations: + - message: must provide a secretRef or capiClusterRef + rule: (has(self.secretRef) || has(self.capiClusterRef)) + - message: cannot provide both capiClusterRef and secretRef + rule: '!(has(self.secretRef) && has(self.capiClusterRef))' status: description: GitopsClusterStatus defines the observed state of GitopsCluster properties: @@ -141,6 +146,9 @@ spec: type: array type: object type: object + x-kubernetes-validations: + - message: must configure spec + rule: has(self.spec) served: true storage: true subresources: diff --git a/controllers/gitopscluster_controller_test.go b/controllers/gitopscluster_controller_test.go index 7d9e9b6..ee5f476 100644 --- a/controllers/gitopscluster_controller_test.go +++ b/controllers/gitopscluster_controller_test.go @@ -2,6 +2,7 @@ package controllers_test import ( "context" + "path/filepath" "regexp" "testing" "time" @@ -22,6 +23,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/envtest" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -514,6 +516,86 @@ func TestFinalizers(t *testing.T) { } } +func TestGitopsClusterValidation(t *testing.T) { + testEnv := &envtest.Environment{ + CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases")}, + } + testCfg, err := testEnv.Start() + if err != nil { + t.Fatal(err) + } + defer func() { + if err := testEnv.Stop(); err != nil { + t.Fatalf("failed to shutdown testEnv: %s", err) + } + }() + + s := runtime.NewScheme() + if err := gitopsv1alpha1.AddToScheme(s); err != nil { + t.Fatal(err) + } + + cl, err := client.New(testCfg, client.Options{Scheme: s}) + if err != nil { + t.Fatal(err) + } + + t.Run("when neither the secret nor capi cluster are configured", func(t *testing.T) { + testCluster := makeTestCluster(func(c *gitopsv1alpha1.GitopsCluster) { + c.ObjectMeta.Name = "no-config" + c.ObjectMeta.Namespace = "default" + c.Spec.SecretRef = nil + c.Spec.CAPIClusterRef = nil + }) + + err := cl.Create(context.TODO(), testCluster) + assertErrorMatch(t, "must provide a secretRef or capiClusterRef", err) + assertErrorDoesNotMatch(t, "cannot provide both capiClusterRef and secretRef", err) + }) + + t.Run("when both the secret and capi cluster are configured", func(t *testing.T) { + testCluster := makeTestCluster(func(c *gitopsv1alpha1.GitopsCluster) { + c.ObjectMeta.Name = "both-configs" + c.ObjectMeta.Namespace = "default" + c.Spec.SecretRef = &meta.LocalObjectReference{ + Name: "test-secret", + } + c.Spec.CAPIClusterRef = &meta.LocalObjectReference{ + Name: "test-cluster", + } + }) + + err := cl.Create(context.TODO(), testCluster) + assertErrorMatch(t, "cannot provide both capiClusterRef and secretRef", err) + assertErrorDoesNotMatch(t, "must provide a secretRef or capiClusterRef", err) + }) + + t.Run("when the secret is configured", func(t *testing.T) { + testCluster := makeTestCluster(func(c *gitopsv1alpha1.GitopsCluster) { + c.ObjectMeta.Name = "only-secret-configured" + c.ObjectMeta.Namespace = "default" + c.Spec.SecretRef = &meta.LocalObjectReference{ + Name: "test-secret", + } + }) + + assertNoError(t, cl.Create(context.TODO(), testCluster)) + }) + + t.Run("when the capiClusterRef is configured", func(t *testing.T) { + testCluster := makeTestCluster(func(c *gitopsv1alpha1.GitopsCluster) { + c.ObjectMeta.Name = "only-capi-cluster-configured" + c.ObjectMeta.Namespace = "default" + c.Spec.CAPIClusterRef = &meta.LocalObjectReference{ + Name: "test-cluster", + } + }) + + assertNoError(t, cl.Create(context.TODO(), testCluster)) + }) + +} + func makeTestReconciler(t *testing.T, opts controllers.Options, objs ...runtime.Object) *controllers.GitopsClusterReconciler { s, tc := makeTestClientAndScheme(t, opts, objs...) return controllers.NewGitopsClusterReconciler(tc, s, opts) @@ -592,6 +674,13 @@ func assertErrorMatch(t *testing.T, s string, e error) { } } +func assertErrorDoesNotMatch(t *testing.T, s string, e error) { + t.Helper() + if matchErrorString(t, s, e) { + t.Fatalf("error did match, got %s, should not contain %s", e, s) + } +} + func matchErrorString(t *testing.T, s string, e error) bool { t.Helper() if s == "" && e == nil {