Merge pull request #8 from navidshariaty/main

enhance error messages and fix tests
snapp-incubator · Nov 19, 2023 · 1f0acc2 · 1f0acc2
2 parents 7fc1a2e + 3204919
commit 1f0acc2
Show file tree

Hide file tree

Showing 4 changed files with 83 additions and 72 deletions.
diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml
@@ -18,25 +18,25 @@ jobs:
         with:
           version: latest
           args: --timeout 5m
-  # test:
-  #   name: test
-  #   runs-on: ubuntu-latest
-  #   steps:
-  #     - uses: actions/checkout@v2
-  #     - uses: actions/setup-go@v2
-  #       with:
-  #         go-version: '^1.16'
-  #     - uses: RyanSiu1995/kubebuilder-action@v1.2.1
-  #     - run: make test
-  #     - uses: codecov/codecov-action@v1
-  #       with:
-  #         files: coverage.out
+  test:
+    name: test
+    runs-on: ubuntu-latest
+    steps:
+      - uses: actions/checkout@v2
+      - uses: actions/setup-go@v2
+        with:
+          go-version: '^1.16'
+      - uses: RyanSiu1995/kubebuilder-action@v1.2.1
+      - run: make test
+      - uses: codecov/codecov-action@v1
+        with:
+          files: coverage.out
   docker:
     name: docker
     runs-on: ubuntu-latest
     needs:
       - lint
-     # - test
+      - test
     steps:
       - uses: actions/checkout@v2
       - uses: docker/setup-qemu-action@v1

diff --git a/api/v1alpha1/team_webhook.go b/api/v1alpha1/team_webhook.go
@@ -51,25 +51,26 @@ var _ webhook.Validator = &Team{}
 var teamns corev1.Namespace
 
 func (t *Team) ValidateCreate() error {
-	teamlog.Info("validate create", "name", t.Name)
+	teamlog.Info("validating team create", "name", t.GetName())
 	clientSet, err := getClient()
 	if err != nil {
+		teamlog.Error(err, "error happened while validating create", "namespace", t.GetNamespace(), "name", t.GetName())
 		return errors.New("could not create client, failed to update team object")
 	}
 	for _, ns := range t.Spec.Namespaces {
-		//check if namespace does not exist or has been deleted
-		teamns, err = nsExists(ns, clientSet)
+		// Check if namespace does not exist or has been deleted
+		teamns, err = nsExists(clientSet, t.Name, ns)
 		if err != nil {
 			return err
 		}
 
-		//check if namespace already has been added to another team
+		// Check if namespace already has been added to another team
 		err = nsHasTeam(t, &teamns)
 		if err != nil {
 			return err
 		}
 
-		//Check If user has access to this namespace
+		// Check If user has access to this namespace
 		err = teamAdminAccess(t, ns, clientSet)
 		if err != nil {
 			return err
@@ -79,15 +80,16 @@ func (t *Team) ValidateCreate() error {
 }
 
 func (t *Team) ValidateUpdate(old runtime.Object) error {
-	teamlog.Info("validate update", "name", t.Name)
+	teamlog.Info("validating team update", "name", t.GetName())
 
 	clientSet, err := getClient()
 	if err != nil {
+		teamlog.Error(err, "error happened while validating update", "namespace", t.GetNamespace(), "name", t.GetName())
 		return errors.New("fail to get client, failed to update team object")
 	}
 	for _, ns := range t.Spec.Namespaces {
 		//check if namespace does not exist or has been deleted
-		teamns, err = nsExists(ns, clientSet)
+		teamns, err = nsExists(clientSet, t.Name, ns)
 		if err != nil {
 			return err
 		}
@@ -126,7 +128,7 @@ func (t *Team) ValidateUpdate(old runtime.Object) error {
 			}
 		}
 		if !exists {
-			errMessage := fmt.Sprintf("namespace %s has team label but does not exist in %s team", ni.Name, t.Name)
+			errMessage := fmt.Sprintf("namespace \"%s\" has team label but does not exist in \"%s\" team", ni.Name, t.Name)
 			return errors.New(errMessage)
 		}
 	}
@@ -153,11 +155,11 @@ func getClient() (c kubernetes.Clientset, err error) {
 	return *clientSet, nil
 }
 
-func nsExists(ns string, c kubernetes.Clientset) (tns corev1.Namespace, err error) {
+func nsExists(c kubernetes.Clientset, team, ns string) (tns corev1.Namespace, err error) {
 	teamNS, errNSGet := c.CoreV1().Namespaces().Get(context.TODO(), ns, metav1.GetOptions{})
 
 	if errNSGet != nil {
-		errorResp := fmt.Sprintf("Error while getting namespace %s: %s", ns, errNSGet.Error())
+		errorResp := fmt.Sprintf("Error while getting namespace \"%s\" for team \"%s\". error: %s", ns, team, errNSGet.Error())
 		return *teamNS, errors.New(errorResp)
 	}
 	return *teamNS, nil
@@ -166,7 +168,7 @@ func nsExists(ns string, c kubernetes.Clientset) (tns corev1.Namespace, err erro
 func nsHasTeam(r *Team, tns *corev1.Namespace) (err error) {
 	if val, ok := tns.Labels["snappcloud.io/team"]; ok {
 		if tns.Labels["snappcloud.io/team"] != r.Name {
-			errorResp := fmt.Sprintf("namespace %s already has the team label %s, please ask in cloud-support if you need to detach the namespace from previous team", tns.Name, val)
+			errorResp := fmt.Sprintf("namespace \"%s\" inside the Namespaces of team \"%s\" already has the team label \"%s\", please ask in cloud-support if you need to detach the namespace from previous team", tns.Name, r.Name, val)
 			return errors.New(errorResp)
 		}
 	}
@@ -194,10 +196,10 @@ func teamAdminAccess(r *Team, ns string, c kubernetes.Clientset) (err error) {
 		Create(context.TODO(), &check, metav1.CreateOptions{})
 
 	if errAuth != nil {
-		teamlog.Error(errAuth, "team owner does not have permission to create role binding")
+		teamlog.Error(errAuth, "error happened while checking team owner permission")
 	}
 	if !resp.Status.Allowed {
-		errMessage := fmt.Sprintf("team owner is not allowed to add namespace %s, please add %s as admin of the project with the followig command: oc policy add-role-to-user admin %s -n %s", ns, r.Spec.TeamAdmin, r.Spec.TeamAdmin, ns)
+		errMessage := fmt.Sprintf("team owner \"%s\" is not allowed to add namespace \"%s\" to team \"%s\", please add \"%s\" as admin of the project with the followig command: oc policy add-role-to-user admin %s -n %s", r.Spec.TeamAdmin, ns, r.Name, r.Spec.TeamAdmin, r.Spec.TeamAdmin, ns)
 		return errors.New(errMessage)
 	}
 	return nil

diff --git a/api/v1alpha1/team_webhook_test.go b/api/v1alpha1/team_webhook_test.go
@@ -3,14 +3,19 @@ package v1alpha1
 import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 var _ = Describe("", func() {
 	var (
-		fooTeamName      = "foo-team"
-		fooTeamAdminName = "foo-admin"
+		fooTeamName       = "foo-team"
+		fooTeamAdminName  = "foo-admin"
+		fooTeamNamespaces = []string{"default"}
 	)
 	var (
 		err error
@@ -54,36 +59,45 @@ var _ = Describe("", func() {
 			Expect(err).NotTo(BeNil())
 		})
 
-		//It("should fail if TeamAdmin does not exist", func() {
-		//	fooTeamTmp := &Team{
-		//		TypeMeta:   fooTeam.TypeMeta,
-		//		ObjectMeta: fooTeam.ObjectMeta,
-		//		Spec: TeamSpec{
-		//			TeamAdmin:  "not-existing-team-admin",
-		//			Namespaces: fooTeamNamespaces,
-		//		},
-		//	}
-		//	err = k8sClient.Create(ctx, fooTeamTmp)
-		//	Expect(err).NotTo(BeNil())
-		//})
-		//
-		//It("should fail if at least one namespace has team label from another team", func() {
-		//	ns := &corev1.Namespace{}
-		//	err = k8sClient.Get(ctx, types.NamespacedName{Name: fooTeamNamespaces[0]}, ns)
-		//	Expect(err).To(BeNil())
-		//	patch := []byte(`{"metadata":{"labels":{"snappcloud.io/team": "non-existing-team"}}}`)
-		//	err = k8sClient.Patch(ctx, ns, client.RawPatch(types.StrategicMergePatchType, patch))
-		//	Expect(err).To(BeNil())
-		//	fooTeamTmp := &Team{
-		//		TypeMeta:   fooTeam.TypeMeta,
-		//		ObjectMeta: fooTeam.ObjectMeta,
-		//		Spec: TeamSpec{
-		//			TeamAdmin:  fooTeamAdminName,
-		//			Namespaces: fooTeamNamespaces,
-		//		},
-		//	}
-		//	err = k8sClient.Create(ctx, fooTeamTmp)
-		//	Expect(err).NotTo(BeNil())
-		//})
+		It("should fail if TeamAdmin does not exist", func() {
+			ns := &v1.Namespace{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "foo-namespace",
+				},
+			}
+			err = k8sClient.Create(ctx, ns)
+			Expect(err).To(BeNil())
+			fooTeamTmp := &Team{
+				TypeMeta:   fooTeam.TypeMeta,
+				ObjectMeta: fooTeam.ObjectMeta,
+				Spec: TeamSpec{
+					TeamAdmin: "not-existing-team-admin",
+					Namespaces: []string{
+						"foo-namespace",
+					},
+				},
+			}
+			err = k8sClient.Create(ctx, fooTeamTmp)
+			Expect(err).NotTo(BeNil())
+		})
+
+		It("should fail if at least one namespace has team label from another team", func() {
+			ns := &corev1.Namespace{}
+			err = k8sClient.Get(ctx, types.NamespacedName{Name: fooTeamNamespaces[0]}, ns)
+			Expect(err).To(BeNil())
+			patch := []byte(`{"metadata":{"labels":{"snappcloud.io/team": "non-existing-team"}}}`)
+			err = k8sClient.Patch(ctx, ns, client.RawPatch(types.StrategicMergePatchType, patch))
+			Expect(err).To(BeNil())
+			fooTeamTmp := &Team{
+				TypeMeta:   fooTeam.TypeMeta,
+				ObjectMeta: fooTeam.ObjectMeta,
+				Spec: TeamSpec{
+					TeamAdmin:  fooTeamAdminName,
+					Namespaces: fooTeamNamespaces,
+				},
+			}
+			err = k8sClient.Create(ctx, fooTeamTmp)
+			Expect(err).NotTo(BeNil())
+		})
 	})
 })
diff --git a/api/v1alpha1/webhook_suite_test.go b/api/v1alpha1/webhook_suite_test.go
@@ -28,9 +28,7 @@ import (
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 
-	admissionv1beta1 "k8s.io/api/admission/v1beta1"
-	//+kubebuilder:scaffold:imports
-	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -69,28 +67,25 @@ var _ = BeforeSuite(func() {
 	}
 
 	var err error
+
+	err = SchemeBuilder.AddToScheme(scheme.Scheme)
+	Expect(err).To(BeNil())
+
 	// cfg is defined in this file globally.
 	cfg, err = testEnv.Start()
 	Expect(err).NotTo(HaveOccurred())
 	Expect(cfg).NotTo(BeNil())
 
-	scheme := runtime.NewScheme()
-	err = AddToScheme(scheme)
-	Expect(err).NotTo(HaveOccurred())
-
-	err = admissionv1beta1.AddToScheme(scheme)
-	Expect(err).NotTo(HaveOccurred())
-
 	//+kubebuilder:scaffold:scheme
 
-	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme})
+	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
 	Expect(err).NotTo(HaveOccurred())
 	Expect(k8sClient).NotTo(BeNil())
 
 	// start webhook server using Manager
 	webhookInstallOptions := &testEnv.WebhookInstallOptions
 	mgr, err := ctrl.NewManager(cfg, ctrl.Options{
-		Scheme:             scheme,
+		Scheme:             scheme.Scheme,
 		Host:               webhookInstallOptions.LocalServingHost,
 		Port:               webhookInstallOptions.LocalServingPort,
 		CertDir:            webhookInstallOptions.LocalServingCertDir,