Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(): remove validation for existing project ns #229

Merged
merged 2 commits into from
Nov 12, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions config/events/controller.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,18 @@ events:
type: Warning
reportingController: controller
message: Namespace creation failed.
- name: NamespaceUpdated
reason: NamespaceUpdated
action: UpdateNamespace
type: Normal
reportingController: controller
message: Namespace updated.
- name: NamespaceUpdateFailed
reason: NamespaceUpdateFailed
action: UpdateNamespace
type: Warning
reportingController: controller
message: Namespace update failed.
- name: NamespaceDeleted
reason: NamespaceDeleted
action: DeleteNamespace
Expand Down
2 changes: 2 additions & 0 deletions config/events/events_config_map.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ data:
- SecretDeletionFailed
- NamespaceCreated
- NamespaceCreationFailed
- NamespaceUpdated
- NamespaceUpdateFailed
- NamespaceDeleted
- NamespaceDeletionFailed
- WorkerClusterRoleCreated
Expand Down
18 changes: 18 additions & 0 deletions events/events_generated.go
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,22 @@ var EventsMap = map[events.EventName]*events.EventSchema{
ReportingController: "controller",
Message: "Namespace creation failed.",
},
"NamespaceUpdated": {
Name: "NamespaceUpdated",
Reason: "NamespaceUpdated",
Action: "UpdateNamespace",
Type: events.EventTypeNormal,
ReportingController: "controller",
Message: "Namespace updated.",
},
"NamespaceUpdateFailed": {
Name: "NamespaceUpdateFailed",
Reason: "NamespaceUpdateFailed",
Action: "UpdateNamespace",
Type: events.EventTypeWarning,
ReportingController: "controller",
Message: "Namespace update failed.",
},
"NamespaceDeleted": {
Name: "NamespaceDeleted",
Reason: "NamespaceDeleted",
Expand Down Expand Up @@ -743,6 +759,8 @@ var (
EventSecretDeletionFailed events.EventName = "SecretDeletionFailed"
EventNamespaceCreated events.EventName = "NamespaceCreated"
EventNamespaceCreationFailed events.EventName = "NamespaceCreationFailed"
EventNamespaceUpdated events.EventName = "NamespaceUpdated"
EventNamespaceUpdateFailed events.EventName = "NamespaceUpdateFailed"
EventNamespaceDeleted events.EventName = "NamespaceDeleted"
EventNamespaceDeletionFailed events.EventName = "NamespaceDeletionFailed"
EventWorkerClusterRoleCreated events.EventName = "WorkerClusterRoleCreated"
Expand Down
29 changes: 29 additions & 0 deletions service/namespace_service.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package service
import (
"context"
"fmt"

"github.com/kubeslice/kubeslice-controller/metrics"

"github.com/kubeslice/kubeslice-controller/events"
Expand Down Expand Up @@ -84,6 +85,34 @@ func (n *NamespaceService) ReconcileProjectNamespace(ctx context.Context, namesp
"object_kind": metricKindNamespace,
},
)
} else {
// check if the namespace has the correct labels
if !util.CompareLabels(nsResource.Labels, n.getResourceLabel(namespace, owner)) {
nsResource.Labels = n.getResourceLabel(namespace, owner)
priyank-upadhyay marked this conversation as resolved.
Show resolved Hide resolved
err := util.UpdateResource(ctx, nsResource)
nsResource.Namespace = ControllerNamespace
priyank-upadhyay marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
util.RecordEvent(ctx, eventRecorder, nsResource, nil, events.EventNamespaceUpdateFailed)
n.mf.RecordCounterMetric(metrics.KubeSliceEventsCounter,
map[string]string{
"action": "update_failed",
"event": string(events.EventNamespaceUpdateFailed),
"object_name": nsResource.Name,
"object_kind": metricKindNamespace,
},
)
return ctrl.Result{}, err
}
util.RecordEvent(ctx, eventRecorder, nsResource, nil, events.EventNamespaceUpdated)
n.mf.RecordCounterMetric(metrics.KubeSliceEventsCounter,
map[string]string{
"action": "updated",
"event": string(events.EventNamespaceUpdated),
"object_name": nsResource.Name,
"object_kind": metricKindNamespace,
},
)
}
}
return ctrl.Result{}, nil
}
Expand Down
61 changes: 31 additions & 30 deletions service/namespace_service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,10 @@ package service

import (
"context"
"testing"

"github.com/kubeslice/kubeslice-controller/metrics"
metricMock "github.com/kubeslice/kubeslice-controller/metrics/mocks"
"testing"

ossEvents "github.com/kubeslice/kubeslice-controller/events"
"github.com/kubeslice/kubeslice-monitoring/pkg/events"
Expand Down Expand Up @@ -52,9 +53,9 @@ func TestNamespaceSuite(t *testing.T) {

var NamespaceTestbed = map[string]func(*testing.T){
"TestReconcileProjectNamespace_NamespaceGetsCreatedWithOwnerLabelAndReturnsReconciliationComplete_Happypath": TestReconcileProjectNamespace_NamespaceGetsCreatedWithOwnerLabelAndReturnsReconciliationComplete_Happypath,
"TestReconcileProjectNamespace_DoesNothingIfNamespaceExistAlready": TestReconcileProjectNamespace_DoesNothingIfNamespaceExistAlready,
"TestDeleteNamespace_DeletesObjectWithReconciliationComplete": TestDeleteNamespace_DeletesObjectWithReconciliationComplete,
"TestDeleteNamespace_DoesNothingIfNamespaceDoNotExist": TestDeleteNamespace_DoesNothingIfNamespaceDoNotExist,
// "TestReconcileProjectNamespace_DoesNothingIfNamespaceExistAlready": TestReconcileProjectNamespace_DoesNothingIfNamespaceExistAlready,
"TestDeleteNamespace_DeletesObjectWithReconciliationComplete": TestDeleteNamespace_DeletesObjectWithReconciliationComplete,
"TestDeleteNamespace_DoesNothingIfNamespaceDoNotExist": TestDeleteNamespace_DoesNothingIfNamespaceDoNotExist,
}

func TestReconcileProjectNamespace_NamespaceGetsCreatedWithOwnerLabelAndReturnsReconciliationComplete_Happypath(t *testing.T) {
Expand Down Expand Up @@ -96,32 +97,32 @@ func TestReconcileProjectNamespace_NamespaceGetsCreatedWithOwnerLabelAndReturnsR
mMock.AssertExpectations(t)
}

func TestReconcileProjectNamespace_DoesNothingIfNamespaceExistAlready(t *testing.T) {
namespaceName := "cisco"
mMock := &metricMock.IMetricRecorder{}
namespaceService := NamespaceService{
mf: mMock,
}

namespace := &corev1.Namespace{}
namespaceObject := client.ObjectKey{
Name: namespaceName,
}
clientMock := &utilMock.Client{}
scheme := runtime.NewScheme()
controllerv1alpha1.AddToScheme(scheme)
ctx := prepareNamespaceTestContext(context.Background(), clientMock, scheme)
mMock.On("WithProject", mock.AnythingOfType("string")).Return(&metrics.MetricRecorder{}).Once()
clientMock.On("Get", ctx, namespaceObject, namespace).Return(nil)
project := &controllerv1alpha1.Project{}
project.ObjectMeta.Labels = map[string]string{"testLabel": "testValue"}
result, err := namespaceService.ReconcileProjectNamespace(ctx, namespaceName, project)
expectedResult := ctrl.Result{}
require.Equal(t, result, expectedResult)
require.Nil(t, err)
clientMock.AssertExpectations(t)
mMock.AssertExpectations(t)
}
// func TestReconcileProjectNamespace_DoesNothingIfNamespaceExistAlready(t *testing.T) {
// namespaceName := "cisco"
// mMock := &metricMock.IMetricRecorder{}
// namespaceService := NamespaceService{
// mf: mMock,
// }

// namespace := &corev1.Namespace{}
// namespaceObject := client.ObjectKey{
// Name: namespaceName,
// }
// clientMock := &utilMock.Client{}
// scheme := runtime.NewScheme()
// controllerv1alpha1.AddToScheme(scheme)
// ctx := prepareNamespaceTestContext(context.Background(), clientMock, scheme)
// mMock.On("WithProject", mock.AnythingOfType("string")).Return(&metrics.MetricRecorder{}).Once()
// clientMock.On("Get", ctx, namespaceObject, namespace).Return(nil)
// project := &controllerv1alpha1.Project{}
// project.ObjectMeta.Labels = map[string]string{"testLabel": "testValue"}
// result, err := namespaceService.ReconcileProjectNamespace(ctx, namespaceName, project)
// expectedResult := ctrl.Result{}
// require.Equal(t, result, expectedResult)
// require.Nil(t, err)
// clientMock.AssertExpectations(t)
// mMock.AssertExpectations(t)
// }

func TestDeleteNamespace_DeletesObjectWithReconciliationComplete(t *testing.T) {
namespaceName := "cisco"
Expand Down
29 changes: 16 additions & 13 deletions service/project_webhook_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,12 @@ package service
import (
"context"
"fmt"
apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime/schema"
"os"
"strings"

apierrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/runtime/schema"

controllerv1alpha1 "github.com/kubeslice/kubeslice-controller/apis/controller/v1alpha1"
"github.com/kubeslice/kubeslice-controller/util"
corev1 "k8s.io/api/core/v1"
Expand All @@ -40,9 +41,11 @@ func ValidateProjectCreate(ctx context.Context, project *controllerv1alpha1.Proj
if err := validateProjectName(project.Name); err != nil {
return apierrors.NewInvalid(schema.GroupKind{Group: apiGroupKubeSliceControllers, Kind: "Project"}, project.Name, field.ErrorList{err})
}
if err := validateProjectNamespaceIfAlreadyExists(ctx, project.Name); err != nil {
return apierrors.NewInvalid(schema.GroupKind{Group: apiGroupKubeSliceControllers, Kind: "Project"}, project.Name, field.ErrorList{err})
}
// FIXME: Remove the comment after testing.
// Validation for existing project namespace is not required. User may want to use an existing namespace.
// if err := validateProjectNamespaceIfAlreadyExists(ctx, project.Name); err != nil {
// return apierrors.NewInvalid(schema.GroupKind{Group: apiGroupKubeSliceControllers, Kind: "Project"}, project.Name, field.ErrorList{err})
// }
if err := validateDNSCompliantSANames(ctx, project); err != nil {
return apierrors.NewInvalid(schema.GroupKind{Group: apiGroupKubeSliceControllers, Kind: "Project"}, project.Name, field.ErrorList{err})
}
Expand Down Expand Up @@ -99,14 +102,14 @@ func validateAppliedInControllerNamespace(ctx context.Context, project *controll
return nil
}

// validateProjectNamespaceIfAlreadyExists is a function validate the whether the project namespace already exists or not
func validateProjectNamespaceIfAlreadyExists(ctx context.Context, projectName string) *field.Error {
projectNamespace := fmt.Sprintf(ProjectNamespacePrefix, projectName)
if exist, _ := util.GetResourceIfExist(ctx, client.ObjectKey{Name: projectNamespace}, &corev1.Namespace{}); exist {
return field.Invalid(field.NewPath("project namespace"), projectNamespace, "already exists")
}
return nil
}
// // validateProjectNamespaceIfAlreadyExists is a function validate the whether the project namespace already exists or not
// func validateProjectNamespaceIfAlreadyExists(ctx context.Context, projectName string) *field.Error {
// projectNamespace := fmt.Sprintf(ProjectNamespacePrefix, projectName)
// if exist, _ := util.GetResourceIfExist(ctx, client.ObjectKey{Name: projectNamespace}, &corev1.Namespace{}); exist {
// return field.Invalid(field.NewPath("project namespace"), projectNamespace, "already exists")
// }
// return nil
// }

// validateDNSCompliantSANames is a function to validate the service account name whether it is DNS compliant
func validateDNSCompliantSANames(ctx context.Context, project *controllerv1alpha1.Project) *field.Error {
Expand Down
46 changes: 23 additions & 23 deletions service/project_webhook_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ func TestProjectWebhookSuite(t *testing.T) {
}

var ProjectWebhookTestbed = map[string]func(*testing.T){
"TestValidateProjectCreate_Applied_Namespace_Error": TestValidateProjectCreate_Applied_Namespace_Error,
"TestValidateProjectCreate_FailsIfNamespaceAlreadyExists": TestValidateProjectCreate_FailsIfNamespaceAlreadyExists,
"TestValidateProjectCreate_Applied_Namespace_Error": TestValidateProjectCreate_Applied_Namespace_Error,
// "TestValidateProjectCreate_FailsIfNamespaceAlreadyExists": TestValidateProjectCreate_FailsIfNamespaceAlreadyExists,
"TestValidateProjectCreate_FailsIf_Sa_Name_Not_DNS_Compliant": TestValidateProjectCreate_FailsIf_Sa_Name_Not_DNS_Compliant,
"TestValidateProjectCreate_FailsIfNameContainsDot": TestValidateProjectCreate_FailsIfNameContainsDot,
"TestValidateProjectCreate_FailsIfNameContainsGreaterThan30Characters": TestValidateProjectCreate_FailsIfNameContainsGreaterThan30Characters,
Expand Down Expand Up @@ -101,21 +101,21 @@ func TestValidateProjectCreate_FailsIfNameContainsGreaterThan30Characters(t *tes
clientMock.AssertExpectations(t)
}

func TestValidateProjectCreate_FailsIfNamespaceAlreadyExists(t *testing.T) { //todo
project := &controllerv1alpha1.Project{}
clientMock := &utilMock.Client{}
name := "testProject"
project.ObjectMeta.Name = name
namespace := "avesha-controller"
project.ObjectMeta.Namespace = namespace
os.Setenv("KUBESLICE_CONTROLLER_MANAGER_NAMESPACE", namespace)
ctx := prepareProjectWebhookTestContext(context.Background(), clientMock, nil)
clientMock.On("Get", ctx, mock.Anything, mock.Anything).Return(nil).Once()
err := ValidateProjectCreate(ctx, project)
require.Error(t, err)
require.Contains(t, err.Error(), "already exists", namespace)
clientMock.AssertExpectations(t)
}
// func TestValidateProjectCreate_FailsIfNamespaceAlreadyExists(t *testing.T) { //todo
// project := &controllerv1alpha1.Project{}
// clientMock := &utilMock.Client{}
// name := "testProject"
// project.ObjectMeta.Name = name
// namespace := "avesha-controller"
// project.ObjectMeta.Namespace = namespace
// os.Setenv("KUBESLICE_CONTROLLER_MANAGER_NAMESPACE", namespace)
// ctx := prepareProjectWebhookTestContext(context.Background(), clientMock, nil)
// clientMock.On("Get", ctx, mock.Anything, mock.Anything).Return(nil).Once()
// err := ValidateProjectCreate(ctx, project)
// require.Error(t, err)
// require.Contains(t, err.Error(), "already exists", namespace)
// clientMock.AssertExpectations(t)
// }

func TestValidateProjectCreate_FailsIf_Sa_Name_Not_DNS_Compliant(t *testing.T) { //todo
project := &controllerv1alpha1.Project{}
Expand All @@ -131,9 +131,9 @@ func TestValidateProjectCreate_FailsIf_Sa_Name_Not_DNS_Compliant(t *testing.T) {
project.Spec.ServiceAccount.ReadOnly = []string{invalidName1, invalidName2}
project.Spec.ServiceAccount.ReadWrite = []string{invalidNameRw1, invalidNameRw2}
os.Setenv("KUBESLICE_CONTROLLER_MANAGER_NAMESPACE", namespace)
notFoundError := k8sError.NewNotFound(util.Resource("projecttest"), "isnotFound")
// notFoundError := k8sError.NewNotFound(util.Resource("projecttest"), "isnotFound")
ctx := prepareProjectWebhookTestContext(context.Background(), clientMock, nil)
clientMock.On("Get", ctx, mock.Anything, mock.Anything).Return(notFoundError).Once()
// clientMock.On("Get", ctx, mock.Anything, mock.Anything).Return(notFoundError).Once()
err := ValidateProjectCreate(ctx, project)
require.Error(t, err)
require.Contains(t, err.Error(), "Invalid value", invalidName1, invalidName2)
Expand All @@ -155,9 +155,9 @@ func TestValidateProjectCreate_HappyPath(t *testing.T) { //todo
project.Spec.ServiceAccount.ReadOnly = []string{validName1, validName2}
project.Spec.ServiceAccount.ReadWrite = []string{validNameRw1, validNameRw2}
os.Setenv("KUBESLICE_CONTROLLER_MANAGER_NAMESPACE", namespace)
notFoundError := k8sError.NewNotFound(util.Resource("projecttest"), "isnotFound")
// notFoundError := k8sError.NewNotFound(util.Resource("projecttest"), "isnotFound")
ctx := prepareProjectWebhookTestContext(context.Background(), clientMock, nil)
clientMock.On("Get", ctx, mock.Anything, mock.Anything).Return(notFoundError).Once()
// clientMock.On("Get", ctx, mock.Anything, mock.Anything).Return(notFoundError).Once()
err := ValidateProjectCreate(ctx, project)
require.Nil(t, err)
clientMock.AssertExpectations(t)
Expand Down Expand Up @@ -220,8 +220,8 @@ func Test_ValidateProjectUpdate_ThrowsErrorIf_SA_DNS_Invalid_throws_error(t *tes
project.Spec.ServiceAccount.ReadWrite = []string{invalidNameRw1, invalidNameRw2}
os.Setenv("KUBESLICE_CONTROLLER_MANAGER_NAMESPACE", namespace)
ctx := prepareProjectWebhookTestContext(context.Background(), clientMock, nil)
notFoundError := k8sError.NewNotFound(util.Resource("projecttest"), "isnotFound")
clientMock.On("Get", ctx, mock.Anything, mock.Anything).Return(notFoundError).Times(4)
// notFoundError := k8sError.NewNotFound(util.Resource("projecttest"), "isnotFound")
// clientMock.On("Get", ctx, mock.Anything, mock.Anything).Return(notFoundError).Times(4)
//calls rolebinding next
clientMock.On("Get", ctx, mock.Anything, mock.Anything).Return(nil).Times(1)
err := ValidateProjectUpdate(ctx, project)
Expand Down
18 changes: 16 additions & 2 deletions util/reconciliation_utility.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,12 @@ import (
"encoding/base64"
"encoding/json"
"fmt"
"github.com/kubeslice/kubeslice-monitoring/pkg/events"
corev1 "k8s.io/api/core/v1"
"reflect"
"strings"

"github.com/kubeslice/kubeslice-monitoring/pkg/events"
corev1 "k8s.io/api/core/v1"

"k8s.io/apimachinery/pkg/runtime"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
Expand Down Expand Up @@ -268,6 +269,19 @@ func CheckForProjectNamespace(namespace *corev1.Namespace) bool {
return namespace.Labels[LabelName] == fmt.Sprintf(LabelValue, "Project", namespace.Name)
}

// CompareLabels is a function to compare the labels
func CompareLabels(label1 map[string]string, label2 map[string]string) bool {
if len(label1) != len(label2) {
return false
}
for key, value := range label1 {
if label2[key] != value {
return false
}
}
return true
}

// GetProjectName is function to get the project name from the namespace
func GetProjectName(namespace string) string {
d := strings.Split(namespace, NamespacePrefix)
Expand Down
Loading