Merge pull request #301 from MbolotSuse/projects-deleting

Make project.spec.ClusterName immutable
rancher · Sep 29, 2023 · 23cc288 · 23cc288
2 parents afd79ef + 4015d3e
commit 23cc288
Show file tree

Hide file tree

Showing 4 changed files with 74 additions and 92 deletions.
diff --git a/docs.md b/docs.md
@@ -179,7 +179,7 @@ This admission webhook prevents the disabling or deletion of a NodeDriver if the
 
 #### ClusterName validation
 
-ClusterName must be equal to the namespace, and must refer to an existing management.cattle.io/v3.Cluster object.
+ClusterName must be equal to the namespace, and must refer to an existing management.cattle.io/v3.Cluster object. In addition, users cannot update the field after creation. 
 
 #### Protects system project
 

diff --git a/pkg/resources/management.cattle.io/v3/project/Project.md b/pkg/resources/management.cattle.io/v3/project/Project.md
@@ -2,7 +2,7 @@
 
 ### ClusterName validation
 
-ClusterName must be equal to the namespace, and must refer to an existing management.cattle.io/v3.Cluster object.
+ClusterName must be equal to the namespace, and must refer to an existing management.cattle.io/v3.Cluster object. In addition, users cannot update the field after creation. 
 
 ### Protects system project
 

diff --git a/pkg/resources/management.cattle.io/v3/project/validator.go b/pkg/resources/management.cattle.io/v3/project/validator.go
@@ -81,10 +81,16 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 		return nil, fmt.Errorf("failed to get old and new projects from request: %w", err)
 	}
 
-	if request.Operation == admissionv1.Delete {
+	switch request.Operation {
+	case admissionv1.Create:
+		return a.admitCreate(newProject)
+	case admissionv1.Update:
+		return a.admitUpdate(oldProject, newProject)
+	case admissionv1.Delete:
 		return a.admitDelete(oldProject)
+	default:
+		return nil, admission.ErrUnsupportedOperation
 	}
-	return a.admitCreateOrUpdate(oldProject, newProject)
 }
 
 func (a *admitter) admitDelete(project *v3.Project) (*admissionv1.AdmissionResponse, error) {
@@ -94,22 +100,35 @@ func (a *admitter) admitDelete(project *v3.Project) (*admissionv1.AdmissionRespo
 	return admission.ResponseAllowed(), nil
 }
 
-func (a *admitter) admitCreateOrUpdate(oldProject, newProject *v3.Project) (*admissionv1.AdmissionResponse, error) {
-	fieldErr, err := a.checkClusterName(newProject)
+func (a *admitter) admitCreate(project *v3.Project) (*admissionv1.AdmissionResponse, error) {
+	fieldErr, err := a.checkClusterExists(project)
 	if err != nil {
 		return nil, fmt.Errorf("error checking cluster name: %w", err)
 	}
 	if fieldErr != nil {
 		return admission.ResponseBadRequest(fieldErr.Error()), nil
 	}
+	return a.admitCommonCreateUpdate(nil, project)
+}
+
+func (a *admitter) admitUpdate(oldProject, newProject *v3.Project) (*admissionv1.AdmissionResponse, error) {
+	if oldProject.Spec.ClusterName != newProject.Spec.ClusterName {
+		fieldErr := field.Invalid(projectSpecFieldPath.Child(clusterNameField), newProject.Spec.ClusterName, "field is immutable")
+		return admission.ResponseBadRequest(fieldErr.Error()), nil
+	}
+	return a.admitCommonCreateUpdate(oldProject, newProject)
+
+}
+
+func (a *admitter) admitCommonCreateUpdate(oldProject, newProject *v3.Project) (*admissionv1.AdmissionResponse, error) {
 	projectQuota := newProject.Spec.ResourceQuota
 	nsQuota := newProject.Spec.NamespaceDefaultResourceQuota
 	if projectQuota == nil && nsQuota == nil {
 		return admission.ResponseAllowed(), nil
 	}
-	fieldErr, err = checkQuotaFields(projectQuota, nsQuota)
+	fieldErr, err := checkQuotaFields(projectQuota, nsQuota)
 	if err != nil {
-		return nil, fmt.Errorf("error checking project fields: %w", err)
+		return nil, fmt.Errorf("error checking project quota fields: %w", err)
 	}
 	if fieldErr != nil {
 		return admission.ResponseBadRequest(fieldErr.Error()), nil
@@ -124,7 +143,7 @@ func (a *admitter) admitCreateOrUpdate(oldProject, newProject *v3.Project) (*adm
 	return admission.ResponseAllowed(), nil
 }
 
-func (a *admitter) checkClusterName(project *v3.Project) (*field.Error, error) {
+func (a *admitter) checkClusterExists(project *v3.Project) (*field.Error, error) {
 	if project.Spec.ClusterName == "" {
 		return field.Required(projectSpecFieldPath.Child(clusterNameField), "clusterName is required"), nil
 	}

diff --git a/pkg/resources/management.cattle.io/v3/project/validator_test.go b/pkg/resources/management.cattle.io/v3/project/validator_test.go
@@ -426,13 +426,6 @@ func TestProjectValidation(t *testing.T) {
 					ClusterName: "testcluster",
 				},
 			},
-			stateSetup: func(state *testState) {
-				state.clusterCache.EXPECT().Get("testcluster").Return(&v3.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "testcluster",
-					},
-				}, nil)
-			},
 			wantAllowed: true,
 		},
 		{
@@ -468,13 +461,6 @@ func TestProjectValidation(t *testing.T) {
 					NamespaceDefaultResourceQuota: nil,
 				},
 			},
-			stateSetup: func(state *testState) {
-				state.clusterCache.EXPECT().Get("testcluster").Return(&v3.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "testcluster",
-					},
-				}, nil)
-			},
 			wantAllowed: true,
 		},
 		{
@@ -508,13 +494,6 @@ func TestProjectValidation(t *testing.T) {
 					},
 				},
 			},
-			stateSetup: func(state *testState) {
-				state.clusterCache.EXPECT().Get("testcluster").Return(&v3.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "testcluster",
-					},
-				}, nil)
-			},
 			wantAllowed: true,
 		},
 		{
@@ -543,13 +522,6 @@ func TestProjectValidation(t *testing.T) {
 					},
 				},
 			},
-			stateSetup: func(state *testState) {
-				state.clusterCache.EXPECT().Get("testcluster").Return(&v3.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "testcluster",
-					},
-				}, nil)
-			},
 			wantAllowed: false,
 		},
 		{
@@ -578,13 +550,6 @@ func TestProjectValidation(t *testing.T) {
 					},
 				},
 			},
-			stateSetup: func(state *testState) {
-				state.clusterCache.EXPECT().Get("testcluster").Return(&v3.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "testcluster",
-					},
-				}, nil)
-			},
 			wantAllowed: false,
 		},
 		{
@@ -619,13 +584,6 @@ func TestProjectValidation(t *testing.T) {
 					},
 				},
 			},
-			stateSetup: func(state *testState) {
-				state.clusterCache.EXPECT().Get("testcluster").Return(&v3.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "testcluster",
-					},
-				}, nil)
-			},
 			wantAllowed: false,
 		},
 		{
@@ -660,13 +618,6 @@ func TestProjectValidation(t *testing.T) {
 					},
 				},
 			},
-			stateSetup: func(state *testState) {
-				state.clusterCache.EXPECT().Get("testcluster").Return(&v3.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "testcluster",
-					},
-				}, nil)
-			},
 			wantAllowed: false,
 		},
 		{
@@ -702,13 +653,6 @@ func TestProjectValidation(t *testing.T) {
 					},
 				},
 			},
-			stateSetup: func(state *testState) {
-				state.clusterCache.EXPECT().Get("testcluster").Return(&v3.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "testcluster",
-					},
-				}, nil)
-			},
 			wantAllowed: false,
 		},
 		{
@@ -742,13 +686,6 @@ func TestProjectValidation(t *testing.T) {
 					},
 				},
 			},
-			stateSetup: func(state *testState) {
-				state.clusterCache.EXPECT().Get("testcluster").Return(&v3.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "testcluster",
-					},
-				}, nil)
-			},
 			wantAllowed: false,
 		},
 		{
@@ -798,13 +735,6 @@ func TestProjectValidation(t *testing.T) {
 					},
 				},
 			},
-			stateSetup: func(state *testState) {
-				state.clusterCache.EXPECT().Get("testcluster").Return(&v3.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "testcluster",
-					},
-				}, nil)
-			},
 			wantAllowed: false,
 		},
 		{
@@ -854,13 +784,6 @@ func TestProjectValidation(t *testing.T) {
 					},
 				},
 			},
-			stateSetup: func(state *testState) {
-				state.clusterCache.EXPECT().Get("testcluster").Return(&v3.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "testcluster",
-					},
-				}, nil)
-			},
 			wantAllowed: true,
 		},
 		{
@@ -929,15 +852,55 @@ func TestProjectValidation(t *testing.T) {
 					},
 				},
 			},
-			stateSetup: func(state *testState) {
-				state.clusterCache.EXPECT().Get("testcluster").Return(&v3.Cluster{
-					ObjectMeta: metav1.ObjectMeta{
-						Name: "testcluster",
-					},
-				}, nil)
+			wantAllowed: false,
+		},
+		{
+			name:      "update changing clusterName",
+			operation: admissionv1.Update,
+			oldProject: &v3.Project{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "testcluster",
+				},
+				Spec: v3.ProjectSpec{
+					ClusterName: "testcluster",
+				},
+			},
+			newProject: &v3.Project{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "testothercluster",
+				},
+				Spec: v3.ProjectSpec{
+					ClusterName: "testothercluster",
+				},
 			},
 			wantAllowed: false,
 		},
+		{
+			name:      "invalid operation",
+			operation: admissionv1.Connect,
+			oldProject: &v3.Project{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "testcluster",
+				},
+				Spec: v3.ProjectSpec{
+					ClusterName: "testcluster",
+				},
+			},
+			newProject: &v3.Project{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "testcluster",
+				},
+				Spec: v3.ProjectSpec{
+					ClusterName: "testcluster",
+				},
+			},
+			wantAllowed: false,
+			wantErr:     true,
+		},
 	}
 	for _, test := range tests {
 		test := test