[release/v0.5] Prevent deletion of local cluster (#551) (#593)

* Prevent deletion of `local` cluster (#551) * Prevent deletion of `local` cluster It prevents deletion of both clusters.provisioning.cattle.io and cluster.management.cattle.io of the name `local`. Signed-off-by: Dharmit Shah <[email protected]> * Prevent deletion of `local` and `fleet-local` namespaces Signed-off-by: Dharmit Shah <[email protected]> * Parameter type grouping Signed-off-by: Dharmit Shah <[email protected]> --------- Signed-off-by: Dharmit Shah <[email protected]> * Backport unit test change from main The unit test change made in ac777f7#diff-27385eb9113237f9a5e650a62ae7a3cc56fedf10b78449f890670221804d344fR172-R174 caused the unit tests to fail. This commit backports the `if` condition from that commit. --------- Signed-off-by: Dharmit Shah <[email protected]>
rancher · Feb 5, 2025 · 61b0e74 · 61b0e74
1 parent 56d1ef6
commit 61b0e74
Show file tree

Hide file tree

Showing 7 changed files with 85 additions and 20 deletions.
diff --git a/pkg/resources/core/v1/namespace/projectannotations.go b/pkg/resources/core/v1/namespace/projectannotations.go
@@ -15,6 +15,8 @@ import (
 )
 
 const (
+	fleetLocalNs        = "fleet-local"
+	localNs             = "local"
 	manageNSVerb        = "manage-namespaces"
 	projectNSAnnotation = "field.cattle.io/projectId"
 )
@@ -23,8 +25,10 @@ type projectNamespaceAdmitter struct {
 	sar authorizationv1.SubjectAccessReviewInterface
 }
 
-// Admit ensures that the user has permission to change the namespace annotation for
-// project membership, effectively moving a project from one namespace to another.
+// Admit ensures that the:
+//   - user has permission to change the namespace annotation for project membership, effectively moving a project from
+//     one namespace to another.
+//   - deletion of `local` and `fleet-local` namespace is not allowed
 func (p *projectNamespaceAdmitter) Admit(request *admission.Request) (*admissionv1.AdmissionResponse, error) {
 	listTrace := trace.New("Namespace Admit", trace.Field{Key: "user", Value: request.UserInfo.Username})
 	defer listTrace.LogIfLong(admission.SlowTraceDuration)
@@ -36,6 +40,11 @@ func (p *projectNamespaceAdmitter) Admit(request *admission.Request) (*admission
 		return nil, fmt.Errorf("failed to decode namespace from request: %w", err)
 	}
 
+	if request.Operation == admissionv1.Delete {
+		if oldNs.Name == localNs || oldNs.Name == fleetLocalNs {
+			return admission.ResponseBadRequest(fmt.Sprintf("deletion of namespace %q is not allowed\n", request.Name)), nil
+		}
+	}
 	projectAnnoValue, ok := newNs.Annotations[projectNSAnnotation]
 	if !ok {
 		// this namespace doesn't belong to a project, let standard RBAC handle it

diff --git a/pkg/resources/core/v1/namespace/projectannotations_test.go b/pkg/resources/core/v1/namespace/projectannotations_test.go
@@ -30,6 +30,7 @@ func TestValidateProjectNamespaceAnnotations(t *testing.T) {
 		sarError                  bool
 		wantError                 bool
 		wantAllowed               bool
+		namespaceName             string
 	}{
 		{
 			name:                     "user can access, create",
@@ -189,6 +190,26 @@ func TestValidateProjectNamespaceAnnotations(t *testing.T) {
 			wantError:                 true,
 			wantAllowed:               false,
 		},
+		{
+			name:          "Prevent deletion of 'local' namespace",
+			operationType: v1.Delete,
+			wantError:     false,
+			wantAllowed:   false,
+			namespaceName: "local",
+		},
+		{
+			name:          "Prevent deletion of 'fleet-local' namespace",
+			operationType: v1.Delete,
+			wantError:     false,
+			wantAllowed:   false,
+			namespaceName: "fleet-local",
+		},
+		{
+			name:          "Allow deletion of namespace",
+			operationType: v1.Delete,
+			wantAllowed:   true,
+			wantError:     false,
+		},
 	}
 	for _, test := range tests {
 		test := test
@@ -212,7 +233,7 @@ func TestValidateProjectNamespaceAnnotations(t *testing.T) {
 				// if this wasn't for our project, don't handle the response
 				return false, nil, nil
 			})
-			request, err := createAnnotationNamespaceRequest(test.projectAnnotationValue, test.oldProjectAnnotationValue, test.includeProjectAnnotation, test.operationType)
+			request, err := createAnnotationNamespaceRequest(test.projectAnnotationValue, test.oldProjectAnnotationValue, test.includeProjectAnnotation, test.operationType, test.namespaceName)
 			assert.NoError(t, err)
 			response, err := admitter.Admit(request)
 			if test.wantError {
@@ -231,12 +252,16 @@ func sarIsForProjectGVR(sarSpec authorizationv1.SubjectAccessReviewSpec) bool {
 		sarSpec.ResourceAttributes.Resource == projectsGVR.Resource
 }
 
-func createAnnotationNamespaceRequest(newProjectAnnotation, oldProjectAnnotation string, includeProjectAnnotation bool, operation v1.Operation) (*admission.Request, error) {
+func createAnnotationNamespaceRequest(newProjectAnnotation, oldProjectAnnotation string, includeProjectAnnotation bool, operation v1.Operation, namespaceName string) (*admission.Request, error) {
 	gvk := metav1.GroupVersionKind{Version: "v1", Kind: "Namespace"}
 	gvr := metav1.GroupVersionResource{Version: "v1", Resource: "namespace"}
+	nsName := "test-ns"
+	if namespaceName != "" {
+		nsName = namespaceName
+	}
 	ns := corev1.Namespace{
 		ObjectMeta: metav1.ObjectMeta{
-			Name: "test-ns",
+			Name: nsName,
 		},
 	}
 	if includeProjectAnnotation {
@@ -266,7 +291,7 @@ func createAnnotationNamespaceRequest(newProjectAnnotation, oldProjectAnnotation
 	if err != nil {
 		return nil, err
 	}
-	if operation == v1.Update {
+	if operation != v1.Create {
 		if includeProjectAnnotation {
 			ns.Annotations[projectNSAnnotation] = oldProjectAnnotation
 		}

diff --git a/pkg/resources/core/v1/namespace/validator.go b/pkg/resources/core/v1/namespace/validator.go
@@ -47,6 +47,7 @@ func (v *Validator) Operations() []admissionv1.OperationType {
 	return []admissionv1.OperationType{
 		admissionv1.Update,
 		admissionv1.Create,
+		admissionv1.Delete,
 	}
 }
 
@@ -85,7 +86,10 @@ func (v *Validator) ValidatingWebhook(clientConfig admissionv1.WebhookClientConf
 	}
 	kubeSystemCreateWebhook.FailurePolicy = admission.Ptr(admissionv1.Ignore)
 
-	return []admissionv1.ValidatingWebhook{*standardWebhook, *createWebhook, *kubeSystemCreateWebhook}
+	deleteWebhook := admission.NewDefaultValidatingWebhook(v, clientConfig, admissionv1.ClusterScope, []admissionv1.OperationType{admissionv1.Delete})
+	deleteWebhook.Name = admission.CreateWebhookName(v, "delete-only")
+
+	return []admissionv1.ValidatingWebhook{*standardWebhook, *createWebhook, *kubeSystemCreateWebhook, *deleteWebhook}
 }
 
 // Admitters returns the psaAdmitter and the projectNamespaceAdmitter for namespaces.

diff --git a/pkg/resources/core/v1/namespace/validator_test.go b/pkg/resources/core/v1/namespace/validator_test.go
@@ -20,7 +20,7 @@ func TestGVR(t *testing.T) {
 func TestOperations(t *testing.T) {
 	validator := NewValidator(nil)
 	operations := validator.Operations()
-	assert.Len(t, operations, 2)
+	assert.Len(t, operations, 3)
 	assert.Contains(t, operations, v1.Update)
 	assert.Contains(t, operations, v1.Create)
 }
@@ -56,7 +56,7 @@ func TestValidatingWebhook(t *testing.T) {
 	wantURL := "test.cattle.io/namespaces"
 	validator := NewValidator(nil)
 	webhooks := validator.ValidatingWebhook(clientConfig)
-	assert.Len(t, webhooks, 3)
+	assert.Len(t, webhooks, 4)
 	hasAllUpdateWebhook := false
 	hasCreateNonKubeSystemWebhook := false
 	hasCreateKubeSystemWebhook := false
@@ -71,7 +71,7 @@ func TestValidatingWebhook(t *testing.T) {
 		operation := operations[0]
 		assert.Equal(t, v1.ClusterScope, *rule.Scope)
 
-		assert.Contains(t, []v1.OperationType{v1.Create, v1.Update}, operation, "only expected webhooks for create and update")
+		assert.Contains(t, []v1.OperationType{v1.Create, v1.Update, v1.Delete}, operation, "only expected webhooks for create, update and delete")
 		if operation == v1.Update {
 			assert.False(t, hasAllUpdateWebhook, "had more than one webhook validating update calls, exepcted only one")
 			hasAllUpdateWebhook = true
@@ -81,7 +81,7 @@ func TestValidatingWebhook(t *testing.T) {
 				// failure policy defaults to fail, but if we specify one it needs to be fail
 				assert.Equal(t, v1.Fail, *webhook.FailurePolicy)
 			}
-		} else {
+		} else if operation == v1.Create {
 			assert.NotNil(t, webhook.NamespaceSelector)
 			matchExpressions := webhook.NamespaceSelector.MatchExpressions
 			assert.Len(t, matchExpressions, 1)

diff --git a/pkg/resources/management.cattle.io/v3/cluster/validator.go b/pkg/resources/management.cattle.io/v3/cluster/validator.go
@@ -26,6 +26,8 @@ import (
 var parsedRangeLessThan125 = semver.MustParseRange("< 1.25.0-rancher0")
 var parsedRangeLessThan123 = semver.MustParseRange("< 1.23.0-rancher0")
 
+const localCluster = "local"
+
 // NewValidator returns a new validator for management clusters.
 func NewValidator(sar authorizationv1.SubjectAccessReviewInterface, cache v3.PodSecurityAdmissionConfigurationTemplateCache) *Validator {
 	return &Validator{
@@ -70,6 +72,16 @@ type admitter struct {
 
 // Admit handles the webhook admission request sent to this webhook.
 func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResponse, error) {
+	oldCluster, newCluster, err := objectsv3.ClusterOldAndNewFromRequest(&request.AdmissionRequest)
+	if err != nil {
+		return nil, fmt.Errorf("failed get old and new clusters from request: %w", err)
+	}
+
+	if request.Operation == admissionv1.Delete && oldCluster.Name == localCluster {
+		// deleting "local" cluster could corrupt the cluster Rancher is deployed in
+		return admission.ResponseBadRequest("cannot delete the local cluster"), nil
+	}
+
 	response, err := a.validateFleetPermissions(request)
 	if err != nil {
 		return nil, fmt.Errorf("failed to validate fleet permissions: %w", err)
@@ -79,12 +91,9 @@ func (a *admitter) Admit(request *admission.Request) (*admissionv1.AdmissionResp
 	}
 
 	if request.Operation == admissionv1.Create || request.Operation == admissionv1.Update {
-		cluster, err := objectsv3.ClusterFromRequest(&request.AdmissionRequest)
-		if err != nil {
-			return nil, fmt.Errorf("failed to get cluster from request: %w", err)
-		}
-		// no need to validate the local cluster, or imported cluster which represents a KEv2 cluster (GKE/EKS/AKS) or v1 Provisioning Cluster
-		if cluster.Name == "local" || cluster.Spec.RancherKubernetesEngineConfig == nil {
+		// no need to validate the PodSecurityAdmissionConfigurationTemplate on a local cluster,
+		// or imported cluster which represents a KEv2 cluster (GKE/EKS/AKS) or v1 Provisioning Cluster
+		if newCluster.Name == localCluster || newCluster.Spec.RancherKubernetesEngineConfig == nil {
 			return admission.ResponseAllowed(), nil
 		}
 		response, err = a.validatePSACT(request)

diff --git a/pkg/resources/management.cattle.io/v3/cluster/validator_test.go b/pkg/resources/management.cattle.io/v3/cluster/validator_test.go
@@ -72,6 +72,16 @@ func TestAdmit(t *testing.T) {
 			operation:     admissionv1.Delete,
 			expectAllowed: true,
 		},
+		{
+			name:      "Delete local cluster where Rancher is deployed",
+			operation: admissionv1.Delete,
+			oldCluster: v3.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "local",
+				},
+			},
+			expectAllowed: false,
+		},
 	}
 
 	for _, tt := range tests {
@@ -105,7 +115,9 @@ func TestAdmit(t *testing.T) {
 			assert.Equal(t, tt.expectAllowed, res.Allowed)
 
 			if !tt.expectAllowed {
-				assert.Equal(t, tt.expectedReason, res.Result.Reason)
+				if tt.expectedReason != "" {
+					assert.Equal(t, tt.expectedReason, res.Result.Reason)
+				}
 			}
 		})
 	}

diff --git a/pkg/resources/provisioning.cattle.io/v1/cluster/validator.go b/pkg/resources/provisioning.cattle.io/v1/cluster/validator.go
@@ -35,6 +35,7 @@ import (
 
 const (
 	globalNamespace         = "cattle-global-data"
+	localCluster            = "local"
 	systemAgentVarDirEnvVar = "CATTLE_AGENT_VAR_DIR"
 	failureStatus           = "Failure"
 )
@@ -92,6 +93,11 @@ func (p *provisioningAdmitter) Admit(request *admission.Request) (*admissionv1.A
 	listTrace := trace.New("provisioningClusterValidator Admit", trace.Field{Key: "user", Value: request.UserInfo.Username})
 	defer listTrace.LogIfLong(admission.SlowTraceDuration)
 
+	if request.Operation == admissionv1.Delete && request.Name == localCluster {
+		// deleting "local" cluster could corrupt the cluster Rancher is deployed in
+		return admission.ResponseBadRequest("can't delete local cluster"), nil
+	}
+
 	oldCluster, cluster, err := objectsv1.ClusterOldAndNewFromRequest(&request.AdmissionRequest)
 	if err != nil {
 		return nil, err
@@ -416,7 +422,7 @@ func (p *provisioningAdmitter) validateMachinePoolNames(request *admission.Reque
 
 // validatePSACT validate if the cluster and underlying secret are configured properly when PSACT is enabled or disabled
 func (p *provisioningAdmitter) validatePSACT(request *admission.Request, response *admissionv1.AdmissionResponse, cluster *v1.Cluster) error {
-	if cluster.Name == "local" || cluster.Spec.RKEConfig == nil {
+	if cluster.Name == localCluster || cluster.Spec.RKEConfig == nil {
 		return nil
 	}
 
@@ -664,7 +670,7 @@ func validateACEConfig(cluster *v1.Cluster) *metav1.Status {
 
 func isValidName(clusterName, clusterNamespace string, clusterExists bool) bool {
 	// A provisioning cluster with name "local" is only expected to be created in the "fleet-local" namespace.
-	if clusterName == "local" {
+	if clusterName == localCluster {
 		return clusterNamespace == "fleet-local"
 	}