diff --git a/build/resources.go b/build/resources.go index 0158ba3c..cedefc55 100644 --- a/build/resources.go +++ b/build/resources.go @@ -9,6 +9,7 @@ import ( "strings" templatev1 "github.com/openshift/api/template/v1" + operatorconfig "github.com/openshift/managed-cluster-validating-webhooks/config" "github.com/openshift/managed-cluster-validating-webhooks/pkg/syncset" webhooks "github.com/openshift/managed-cluster-validating-webhooks/pkg/webhooks" utils "github.com/openshift/managed-cluster-validating-webhooks/pkg/webhooks/utils" @@ -633,6 +634,17 @@ func createDaemonSet() *appsv1.DaemonSet { RestartPolicy: corev1.RestartPolicyAlways, ServiceAccountName: serviceAccountName, Volumes: []corev1.Volume{ + { + Name: "validation-webhook-config", + VolumeSource: corev1.VolumeSource{ + ConfigMap: &corev1.ConfigMapVolumeSource{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: operatorconfig.ValidationWebhookConfigMapName, + }, + Optional: pointer.Bool(true), + }, + }, + }, { Name: "service-certs", VolumeSource: corev1.VolumeSource{ @@ -661,6 +673,11 @@ func createDaemonSet() *appsv1.DaemonSet { Name: "webhooks", Image: "${REGISTRY_IMG}@${IMAGE_DIGEST}", VolumeMounts: []corev1.VolumeMount{ + { + Name: "validation-webhook-config", + MountPath: operatorconfig.ValidationWebhookConfigMount, + ReadOnly: true, + }, { Name: "service-certs", MountPath: "/service-certs", diff --git a/build/resources_test.go b/build/resources_test.go new file mode 100644 index 00000000..c00a608f --- /dev/null +++ b/build/resources_test.go @@ -0,0 +1,36 @@ +package main + +import ( + "testing" + + operatorconfig "github.com/openshift/managed-cluster-validating-webhooks/config" +) + +func TestCreateDaemonSetConfigMapMount(t *testing.T) { + daemonSet := createDaemonSet() + container := daemonSet.Spec.Template.Spec.Containers[0] + + if len(container.Env) != 0 { + t.Fatalf("environment = %#v, want none", container.Env) + } + + var foundMount bool + for _, mount := range container.VolumeMounts { + if mount.Name == "validation-webhook-config" { + foundMount = mount.MountPath == operatorconfig.ValidationWebhookConfigMount && mount.ReadOnly + } + } + if !foundMount { + t.Fatalf("volume mounts = %#v, want read-only validation webhook config mount", container.VolumeMounts) + } + + var foundVolume bool + for _, volume := range daemonSet.Spec.Template.Spec.Volumes { + if volume.Name == "validation-webhook-config" && volume.ConfigMap != nil { + foundVolume = volume.ConfigMap.Name == operatorconfig.ValidationWebhookConfigMapName && volume.ConfigMap.Optional != nil && *volume.ConfigMap.Optional + } + } + if !foundVolume { + t.Fatalf("volumes = %#v, want optional validation webhook config ConfigMap", daemonSet.Spec.Template.Spec.Volumes) + } +} diff --git a/build/selectorsyncset.yaml b/build/selectorsyncset.yaml index 18dc8370..9a64c4bb 100644 --- a/build/selectorsyncset.yaml +++ b/build/selectorsyncset.yaml @@ -200,6 +200,9 @@ objects: resources: {} terminationMessagePolicy: FallbackToLogsOnError volumeMounts: + - mountPath: /etc/validation-webhook-config + name: validation-webhook-config + readOnly: true - mountPath: /service-certs name: service-certs readOnly: true @@ -215,6 +218,10 @@ objects: - effect: NoExecute key: node-role.kubernetes.io/master volumes: + - configMap: + name: validation-webhook-config + optional: true + name: validation-webhook-config - name: service-certs secret: secretName: webhook-cert diff --git a/config/config.go b/config/config.go index 57c6765d..5d5106de 100644 --- a/config/config.go +++ b/config/config.go @@ -4,4 +4,8 @@ const ( // I know this isn't the operator's name but so much stuff has been coded to use this... OperatorName = "validation-webhook" OperatorNamespace = "openshift-validation-webhook" + + ValidationWebhookConfigMapName = "validation-webhook-config" + CCSCPMSResizeConfigKey = "enableCCSCPMSResize" + ValidationWebhookConfigMount = "/etc/validation-webhook-config" ) diff --git a/docs/webhooks-short.json b/docs/webhooks-short.json index 52ae2625..64eb77ff 100644 --- a/docs/webhooks-short.json +++ b/docs/webhooks-short.json @@ -15,10 +15,22 @@ "webhookName": "customresourcedefinitions-validation", "documentString": "Managed OpenShift Customers may not change CustomResourceDefinitions managed by Red Hat." }, + { + "webhookName": "hcpnamespace-validation", + "documentString": "Validates that only authorized users and service accounts can delete protected HCP namespaces" + }, { "webhookName": "hiveownership-validation", "documentString": "Managed OpenShift customers may not edit certain managed resources. A managed resource has a \"hive.openshift.io/managed\": \"true\" label." }, + { + "webhookName": "hostedcluster-validation", + "documentString": "Validates HostedCluster deletion operations are only performed by authorized service accounts" + }, + { + "webhookName": "hostedcontrolplane-validation", + "documentString": "Validates HostedControlPlane deletion operations are only performed by authorized service accounts" + }, { "webhookName": "imagecontentpolicies-validation", "documentString": "Managed OpenShift customers may not create ImageContentSourcePolicy, ImageDigestMirrorSet, or ImageTagMirrorSet resources that configure mirrors that would conflict with system registries (e.g. quay.io, registry.redhat.io, registry.access.redhat.com, etc). For more details, see https://docs.openshift.com/" @@ -31,13 +43,17 @@ "webhookName": "ingresscontroller-validation", "documentString": "Managed OpenShift Customer may create IngressControllers without necessary taints. This can cause those workloads to be provisioned on master nodes." }, + { + "webhookName": "manifestworks-validation", + "documentString": "Validates ManifestWorks deletion operations are only performed by authorized service accounts" + }, { "webhookName": "namespace-validation", "documentString": "Managed OpenShift Customers may not modify namespaces specified in the [openshift-monitoring/managed-namespaces openshift-monitoring/ocp-namespaces] ConfigMaps because customer workloads should be placed in customer-created namespaces. Customers may not create namespaces identified by this regular expression (^com$|^io$|^in$) because it could interfere with critical DNS resolution. Additionally, customers may not set or change the values of these Namespace labels [managed.openshift.io/storage-pv-quota-exempt managed.openshift.io/service-lb-quota-exempt]." }, { "webhookName": "network-operator-validation", - "documentString": "Managed OpenShift customers may not modify critical fields in the network.operator CRD (such as spec.migration.networkType) because it can disrupt Cluster Network Operator operations and CNI migrations. Even cluster-admin users are blocked from modifying these critical fields." + "documentString": "Managed OpenShift customers may not modify critical fields in the network.operator CRD (such as spec.migration.networkType) because it can disrupt Cluster Network Operator operations and CNI migrations. Only backplane-cluster-admin, SRE, Cluster Network Operator (CNO), and Managed Upgrade Operator (MUO) service accounts are allowed to modify these critical fields. Regular cluster-admin users (system:admin) are explicitly blocked." }, { "webhookName": "networkpolicies-validation", @@ -61,7 +77,7 @@ }, { "webhookName": "regular-user-validation", - "documentString": "Managed OpenShift customers may not manage any objects in the following APIGroups [upgrade.managed.openshift.io config.openshift.io operator.openshift.io network.openshift.io admissionregistration.k8s.io addons.managed.openshift.io cloudingress.managed.openshift.io managed.openshift.io splunkforwarder.managed.openshift.io autoscaling.openshift.io machineconfiguration.openshift.io cloudcredential.openshift.io machine.openshift.io ocmagent.managed.openshift.io], nor may Managed OpenShift customers alter the APIServer, KubeAPIServer, OpenShiftAPIServer, ClusterVersion, Proxy or SubjectPermission objects." + "documentString": "Managed OpenShift customers may not manage any objects in the following APIGroups [cloudcredential.openshift.io admissionregistration.k8s.io addons.managed.openshift.io cloudingress.managed.openshift.io managed.openshift.io ocmagent.managed.openshift.io splunkforwarder.managed.openshift.io upgrade.managed.openshift.io machine.openshift.io autoscaling.openshift.io config.openshift.io machineconfiguration.openshift.io operator.openshift.io network.openshift.io], nor may Managed OpenShift customers alter the APIServer, KubeAPIServer, OpenShiftAPIServer, ClusterVersion, Proxy or SubjectPermission objects. On CCS clusters, when the trimmed enableCCSCPMSResize configuration value is exactly true, cluster administrators and dedicated administrators may update a ControlPlaneMachineSet AWS instance type from a non-metal m5 or m6i type to an equivalent or larger non-metal m5 or m6i type." }, { "webhookName": "scc-validation", diff --git a/docs/webhooks.json b/docs/webhooks.json index 51764813..339298e2 100644 --- a/docs/webhooks.json +++ b/docs/webhooks.json @@ -86,6 +86,27 @@ ], "documentString": "Managed OpenShift Customers may not change CustomResourceDefinitions managed by Red Hat." }, + { + "webhookName": "hcpnamespace-validation", + "rules": [ + { + "operations": [ + "DELETE" + ], + "apiGroups": [ + "" + ], + "apiVersions": [ + "*" + ], + "resources": [ + "namespaces" + ], + "scope": "Cluster" + } + ], + "documentString": "Validates that only authorized users and service accounts can delete protected HCP namespaces" + }, { "webhookName": "hiveownership-validation", "rules": [ @@ -113,6 +134,48 @@ }, "documentString": "Managed OpenShift customers may not edit certain managed resources. A managed resource has a \"hive.openshift.io/managed\": \"true\" label." }, + { + "webhookName": "hostedcluster-validation", + "rules": [ + { + "operations": [ + "DELETE" + ], + "apiGroups": [ + "hypershift.openshift.io" + ], + "apiVersions": [ + "*" + ], + "resources": [ + "hostedclusters" + ], + "scope": "Namespaced" + } + ], + "documentString": "Validates HostedCluster deletion operations are only performed by authorized service accounts" + }, + { + "webhookName": "hostedcontrolplane-validation", + "rules": [ + { + "operations": [ + "DELETE" + ], + "apiGroups": [ + "hypershift.openshift.io" + ], + "apiVersions": [ + "*" + ], + "resources": [ + "hostedcontrolplanes" + ], + "scope": "Namespaced" + } + ], + "documentString": "Validates HostedControlPlane deletion operations are only performed by authorized service accounts" + }, { "webhookName": "imagecontentpolicies-validation", "rules": [ @@ -198,6 +261,27 @@ ], "documentString": "Managed OpenShift Customer may create IngressControllers without necessary taints. This can cause those workloads to be provisioned on master nodes." }, + { + "webhookName": "manifestworks-validation", + "rules": [ + { + "operations": [ + "DELETE" + ], + "apiGroups": [ + "work.open-cluster-management.io" + ], + "apiVersions": [ + "*" + ], + "resources": [ + "manifestworks" + ], + "scope": "Namespaced" + } + ], + "documentString": "Validates ManifestWorks deletion operations are only performed by authorized service accounts" + }, { "webhookName": "namespace-validation", "rules": [ @@ -241,7 +325,7 @@ "scope": "Cluster" } ], - "documentString": "Managed OpenShift customers may not modify critical fields in the network.operator CRD (such as spec.migration.networkType) because it can disrupt Cluster Network Operator operations and CNI migrations. Even cluster-admin users are blocked from modifying these critical fields." + "documentString": "Managed OpenShift customers may not modify critical fields in the network.operator CRD (such as spec.migration.networkType) because it can disrupt Cluster Network Operator operations and CNI migrations. Only backplane-cluster-admin, SRE, Cluster Network Operator (CNO), and Managed Upgrade Operator (MUO) service accounts are allowed to modify these critical fields. Regular cluster-admin users (system:admin) are explicitly blocked." }, { "webhookName": "networkpolicies-validation", @@ -498,7 +582,7 @@ "scope": "*" } ], - "documentString": "Managed OpenShift customers may not manage any objects in the following APIGroups [splunkforwarder.managed.openshift.io autoscaling.openshift.io ocmagent.managed.openshift.io upgrade.managed.openshift.io config.openshift.io machineconfiguration.openshift.io operator.openshift.io network.openshift.io cloudcredential.openshift.io machine.openshift.io admissionregistration.k8s.io addons.managed.openshift.io cloudingress.managed.openshift.io managed.openshift.io], nor may Managed OpenShift customers alter the APIServer, KubeAPIServer, OpenShiftAPIServer, ClusterVersion, Proxy or SubjectPermission objects." + "documentString": "Managed OpenShift customers may not manage any objects in the following APIGroups [config.openshift.io machineconfiguration.openshift.io operator.openshift.io cloudcredential.openshift.io addons.managed.openshift.io cloudingress.managed.openshift.io managed.openshift.io autoscaling.openshift.io network.openshift.io machine.openshift.io admissionregistration.k8s.io ocmagent.managed.openshift.io splunkforwarder.managed.openshift.io upgrade.managed.openshift.io], nor may Managed OpenShift customers alter the APIServer, KubeAPIServer, OpenShiftAPIServer, ClusterVersion, Proxy or SubjectPermission objects. On CCS clusters, when the trimmed enableCCSCPMSResize configuration value is exactly true, cluster administrators and dedicated administrators may update a ControlPlaneMachineSet AWS instance type from a non-metal m5 or m6i type to an equivalent or larger non-metal m5 or m6i type." }, { "webhookName": "scc-validation", diff --git a/pkg/webhooks/hcpnamespace/hcpnamespace.go b/pkg/webhooks/hcpnamespace/hcpnamespace.go index 68a463ad..9033068a 100644 --- a/pkg/webhooks/hcpnamespace/hcpnamespace.go +++ b/pkg/webhooks/hcpnamespace/hcpnamespace.go @@ -17,7 +17,7 @@ import ( const ( WebhookName string = "hcpnamespace-validation" - docString string = "Validates HCP namespace deletion operations are only performed by authorized service accounts" + docString string = "Validates that only authorized users and service accounts can delete protected HCP namespaces" ) var ( diff --git a/pkg/webhooks/hostedcontrolplane/hostedcontrolplane.go b/pkg/webhooks/hostedcontrolplane/hostedcontrolplane.go index f07a647e..4e145acf 100644 --- a/pkg/webhooks/hostedcontrolplane/hostedcontrolplane.go +++ b/pkg/webhooks/hostedcontrolplane/hostedcontrolplane.go @@ -116,7 +116,7 @@ func (s *HostedControlPlaneWebhook) authorized(request admissionctl.Request) adm } saName := strings.Split(request.UserInfo.Username, ":") - if len(saName) > 0 && slices.Contains(allowedServiceAccountsNames, saName[len(saName)-1]) { + if len(saName) == 4 && saName[0] == "system" && saName[1] == "serviceaccount" && saName[2] != "" && slices.Contains(allowedServiceAccountsNames, saName[3]) { ret = admissionctl.Allowed("Service account is authorized to delete HostedControlPlane resources") ret.UID = request.AdmissionRequest.UID return ret diff --git a/pkg/webhooks/hostedcontrolplane/hostedcontrolplane_test.go b/pkg/webhooks/hostedcontrolplane/hostedcontrolplane_test.go index 89d9e57d..c827bfe5 100644 --- a/pkg/webhooks/hostedcontrolplane/hostedcontrolplane_test.go +++ b/pkg/webhooks/hostedcontrolplane/hostedcontrolplane_test.go @@ -22,6 +22,30 @@ func TestHostedControlPlaneAuthorized(t *testing.T) { operation: admissionv1.Delete, shouldBeAllowed: true, }, + { + name: "Allowed short-name service account can delete hostedcontrolplane", + username: "system:serviceaccount:openshift-cluster-api:cluster-api", + operation: admissionv1.Delete, + shouldBeAllowed: true, + }, + { + name: "Short name without service account username format cannot delete hostedcontrolplane", + username: "oidc:cluster-api", + operation: admissionv1.Delete, + shouldBeAllowed: false, + }, + { + name: "Short name with an empty namespace cannot delete hostedcontrolplane", + username: "system:serviceaccount::cluster-api", + operation: admissionv1.Delete, + shouldBeAllowed: false, + }, + { + name: "Short name with extra username components cannot delete hostedcontrolplane", + username: "system:serviceaccount:openshift-cluster-api:cluster-api:extra", + operation: admissionv1.Delete, + shouldBeAllowed: false, + }, { name: "Random user cannot delete hostedcontrolplane", username: "unknown-user", diff --git a/pkg/webhooks/regularuser/common/regularuser.go b/pkg/webhooks/regularuser/common/regularuser.go index 35b6d72a..b20ffc05 100644 --- a/pkg/webhooks/regularuser/common/regularuser.go +++ b/pkg/webhooks/regularuser/common/regularuser.go @@ -1,12 +1,15 @@ package common import ( + "encoding/json" "fmt" "os" + "reflect" "slices" "strings" networkv1 "github.com/openshift/api/network/v1" + operatorconfig "github.com/openshift/managed-cluster-validating-webhooks/config" hookconfig "github.com/openshift/managed-cluster-validating-webhooks/pkg/config" "github.com/openshift/managed-cluster-validating-webhooks/pkg/webhooks/namespace" "github.com/openshift/managed-cluster-validating-webhooks/pkg/webhooks/utils" @@ -27,19 +30,21 @@ import ( // in the 'osd' package. const ( - WebhookName = "regular-user-validation" - docString = `Managed OpenShift customers may not manage any objects in the following APIGroups %s, nor may Managed OpenShift customers alter the APIServer, KubeAPIServer, OpenShiftAPIServer, ClusterVersion, Proxy or SubjectPermission objects.` - mustGatherKind = "MustGather" - mustGatherGroup = "managed.openshift.io" - clusterVersionKind = "ClusterVersion" - clusterVersionGroup = "config.openshift.io" - customDomainKind = "CustomDomain" - customDomainGroup = "managed.openshift.io" - netNamespaceKind = "NetNamespace" - netNamespaceGroup = "network.openshift.io" - machineConfigKind = "MachineConfig" - machineConfigPoolKind = "MachineConfigPool" - machineConfigGroup = "machineconfiguration.openshift.io" + WebhookName = "regular-user-validation" + docString = `Managed OpenShift customers may not manage any objects in the following APIGroups %s, nor may Managed OpenShift customers alter the APIServer, KubeAPIServer, OpenShiftAPIServer, ClusterVersion, Proxy or SubjectPermission objects. On CCS clusters, when the trimmed enableCCSCPMSResize configuration value is exactly true, cluster administrators and dedicated administrators may update a ControlPlaneMachineSet AWS instance type from a non-metal m5 or m6i type to an equivalent or larger non-metal m5 or m6i type.` + mustGatherKind = "MustGather" + mustGatherGroup = "managed.openshift.io" + clusterVersionKind = "ClusterVersion" + clusterVersionGroup = "config.openshift.io" + customDomainKind = "CustomDomain" + customDomainGroup = "managed.openshift.io" + controlPlaneMachineSetKind = "ControlPlaneMachineSet" + machineGroup = "machine.openshift.io" + netNamespaceKind = "NetNamespace" + netNamespaceGroup = "network.openshift.io" + machineConfigKind = "MachineConfig" + machineConfigPoolKind = "MachineConfigPool" + machineConfigGroup = "machineconfiguration.openshift.io" ) var ( @@ -64,7 +69,26 @@ var ( // supported OSC version still relies on this identity. "system:serviceaccount:openshift-sandboxed-containers-operator:default", } - ceeGroup = "system:serviceaccounts:openshift-backplane-cee" + ceeGroup = "system:serviceaccounts:openshift-backplane-cee" + controlPlaneMachineSetInstanceCapacities = map[string]instanceCapacity{ + "m5.large": {vCPU: 2, memoryGiB: 8}, + "m5.xlarge": {vCPU: 4, memoryGiB: 16}, + "m5.2xlarge": {vCPU: 8, memoryGiB: 32}, + "m5.4xlarge": {vCPU: 16, memoryGiB: 64}, + "m5.8xlarge": {vCPU: 32, memoryGiB: 128}, + "m5.12xlarge": {vCPU: 48, memoryGiB: 192}, + "m5.16xlarge": {vCPU: 64, memoryGiB: 256}, + "m5.24xlarge": {vCPU: 96, memoryGiB: 384}, + "m6i.large": {vCPU: 2, memoryGiB: 8}, + "m6i.xlarge": {vCPU: 4, memoryGiB: 16}, + "m6i.2xlarge": {vCPU: 8, memoryGiB: 32}, + "m6i.4xlarge": {vCPU: 16, memoryGiB: 64}, + "m6i.8xlarge": {vCPU: 32, memoryGiB: 128}, + "m6i.12xlarge": {vCPU: 48, memoryGiB: 192}, + "m6i.16xlarge": {vCPU: 64, memoryGiB: 256}, + "m6i.24xlarge": {vCPU: 96, memoryGiB: 384}, + "m6i.32xlarge": {vCPU: 128, memoryGiB: 512}, + } scope = admissionregv1.AllScopes rules = []admissionregv1.RuleWithOperations{ @@ -154,8 +178,15 @@ var ( }, } log = logf.Log.WithName(WebhookName) + + ccsCPMSResizeConfigFile = operatorconfig.ValidationWebhookConfigMount + "/" + operatorconfig.CCSCPMSResizeConfigKey ) +type instanceCapacity struct { + vCPU int + memoryGiB int +} + // RegularuserWebhook protects various objects from unauthorized manipulation type RegularuserWebhook struct { s runtime.Scheme @@ -235,6 +266,10 @@ func (s *RegularuserWebhook) authorized(request admissionctl.Request) admissionc return ret } + if isControlPlaneMachineSetInstanceTypeUpdateAllowed(request) { + return utils.WebhookResponse(request, true, "ControlPlaneMachineSet instance type update is authorized") + } + // Check MachineConfig resources first - only cluster-admins group allowed if request.Kind.Group == machineConfigGroup { if isMachineConfigAuthorized(request) { @@ -324,6 +359,126 @@ func isCustomDomainAuthorized(request admissionctl.Request) bool { slices.Contains(request.UserInfo.Groups, "dedicated-admins") } +// isControlPlaneMachineSetInstanceTypeUpdateAllowed permits only supported CPMS instance type increases. +func isControlPlaneMachineSetInstanceTypeUpdateAllowed(request admissionctl.Request) bool { + if !isCCSCPMSResizeEnabled() || + request.Operation != admissionv1.Update || + !utils.RequestMatchesGroupKind(request, controlPlaneMachineSetKind, machineGroup) || + (!slices.Contains(request.UserInfo.Groups, "cluster-admins") && !slices.Contains(request.UserInfo.Groups, "dedicated-admins")) { + return false + } + + oldObject := map[string]any{} + newObject := map[string]any{} + if json.Unmarshal(request.OldObject.Raw, &oldObject) != nil || json.Unmarshal(request.Object.Raw, &newObject) != nil { + return false + } + + oldInstanceType, oldCapacity, ok := removeInstanceType(oldObject) + if !ok { + return false + } + newInstanceType, newCapacity, ok := removeInstanceType(newObject) + if !ok || oldInstanceType == newInstanceType { + return false + } + if isManagedFieldsReset(newObject) { + return false + } + removeAPIServerManagedMetadata(oldObject) + removeAPIServerManagedMetadata(newObject) + + return newCapacity.vCPU >= oldCapacity.vCPU && + newCapacity.memoryGiB >= oldCapacity.memoryGiB && + reflect.DeepEqual(oldObject, newObject) +} + +func isCCSCPMSResizeEnabled() bool { + value, err := os.ReadFile(ccsCPMSResizeConfigFile) + return err == nil && strings.TrimSpace(string(value)) == "true" +} + +// removeInstanceType extracts the supported AWS instance type and removes it before object comparison. +func removeInstanceType(object map[string]any) (string, instanceCapacity, bool) { + spec, ok := object["spec"].(map[string]any) + if !ok { + return "", instanceCapacity{}, false + } + template, ok := spec["template"].(map[string]any) + if !ok { + return "", instanceCapacity{}, false + } + machine, ok := template["machines_v1beta1_machine_openshift_io"].(map[string]any) + if !ok { + return "", instanceCapacity{}, false + } + machineSpec, ok := machine["spec"].(map[string]any) + if !ok { + return "", instanceCapacity{}, false + } + providerSpec, ok := machineSpec["providerSpec"].(map[string]any) + if !ok { + return "", instanceCapacity{}, false + } + providerSpecValue, ok := providerSpec["value"].(map[string]any) + if !ok { + return "", instanceCapacity{}, false + } + instanceType, ok := providerSpecValue["instanceType"].(string) + if !ok { + return "", instanceCapacity{}, false + } + capacity, ok := controlPlaneMachineSetInstanceCapacities[instanceType] + if !ok { + return "", instanceCapacity{}, false + } + + delete(providerSpecValue, "instanceType") + return instanceType, capacity, true +} + +// isManagedFieldsReset reports whether an update asks the API server to reset field ownership. +func isManagedFieldsReset(object map[string]any) bool { + metadata, ok := object["metadata"].(map[string]any) + if !ok { + return false + } + + managedFields, ok := metadata["managedFields"].([]any) + if !ok { + return false + } + if len(managedFields) == 0 { + return true + } + if len(managedFields) != 1 { + return false + } + + entry, ok := managedFields[0].(map[string]any) + return ok && len(entry) == 0 +} + +// removeAPIServerManagedMetadata removes metadata fields that may change during an update. +func removeAPIServerManagedMetadata(object map[string]any) { + metadata, ok := object["metadata"].(map[string]any) + if !ok { + return + } + + for _, field := range []string{ + "creationTimestamp", + "deletionGracePeriodSeconds", + "deletionTimestamp", + "generation", + "managedFields", + "resourceVersion", + "selfLink", + } { + delete(metadata, field) + } +} + // isNetNamespaceAuthorized check if request is authorized for NetNamespace CR func isNetNamespaceAuthorized(s *RegularuserWebhook, request admissionctl.Request) bool { return (slices.Contains(request.UserInfo.Groups, "cluster-admins") || diff --git a/pkg/webhooks/regularuser/common/regularuser_test.go b/pkg/webhooks/regularuser/common/regularuser_test.go index c47953c0..d5f792f4 100644 --- a/pkg/webhooks/regularuser/common/regularuser_test.go +++ b/pkg/webhooks/regularuser/common/regularuser_test.go @@ -2,6 +2,8 @@ package common import ( "fmt" + "os" + "path/filepath" "testing" "github.com/openshift/managed-cluster-validating-webhooks/pkg/testutils" @@ -152,6 +154,243 @@ func TestFirstBlock(t *testing.T) { runRegularuserTests(t, tests) } +func TestControlPlaneMachineSetInstanceTypeUpdates(t *testing.T) { + configFile := filepath.Join(t.TempDir(), "enableCCSCPMSResize") + previousConfigFile := ccsCPMSResizeConfigFile + ccsCPMSResizeConfigFile = configFile + t.Cleanup(func() { ccsCPMSResizeConfigFile = previousConfigFile }) + if err := os.WriteFile(configFile, []byte("true"), 0o600); err != nil { + t.Fatalf("writing config file: %v", err) + } + + tests := []struct { + name string + operation admissionv1.Operation + groups []string + oldInstanceType string + newInstanceType string + oldStrategy string + newStrategy string + oldMetadata string + newMetadata string + shouldAllow bool + }{ + { + name: "dedicated-admin-can-move-to-equivalent-m6i-type", + operation: admissionv1.Update, + groups: []string{"system:authenticated", "dedicated-admins"}, + oldInstanceType: "m5.2xlarge", + newInstanceType: "m6i.2xlarge", + oldStrategy: "RollingUpdate", + newStrategy: "RollingUpdate", + shouldAllow: true, + }, + { + name: "cluster-admin-can-upsize", + operation: admissionv1.Update, + groups: []string{"system:authenticated", "cluster-admins"}, + oldInstanceType: "m5.xlarge", + newInstanceType: "m6i.2xlarge", + oldStrategy: "RollingUpdate", + newStrategy: "RollingUpdate", + shouldAllow: true, + }, + { + name: "cluster-admin-cannot-update-from-metal-type", + operation: admissionv1.Update, + groups: []string{"system:authenticated", "cluster-admins"}, + oldInstanceType: "m5.metal", + newInstanceType: "m6i.24xlarge", + oldStrategy: "RollingUpdate", + newStrategy: "RollingUpdate", + shouldAllow: false, + }, + { + name: "cluster-admin-cannot-update-to-metal-type", + operation: admissionv1.Update, + groups: []string{"system:authenticated", "cluster-admins"}, + oldInstanceType: "m5.24xlarge", + newInstanceType: "m6i.metal", + oldStrategy: "RollingUpdate", + newStrategy: "RollingUpdate", + shouldAllow: false, + }, + { + name: "dedicated-admin-cannot-downsize", + operation: admissionv1.Update, + groups: []string{"system:authenticated", "dedicated-admins"}, + oldInstanceType: "m6i.2xlarge", + newInstanceType: "m5.large", + oldStrategy: "RollingUpdate", + newStrategy: "RollingUpdate", + shouldAllow: false, + }, + { + name: "dedicated-admin-cannot-use-m5-variant", + operation: admissionv1.Update, + groups: []string{"system:authenticated", "dedicated-admins"}, + oldInstanceType: "m5.xlarge", + newInstanceType: "m5a.2xlarge", + oldStrategy: "RollingUpdate", + newStrategy: "RollingUpdate", + shouldAllow: false, + }, + { + name: "regular-user-cannot-update-instance-type", + operation: admissionv1.Update, + groups: []string{"system:authenticated"}, + oldInstanceType: "m5.xlarge", + newInstanceType: "m6i.2xlarge", + oldStrategy: "RollingUpdate", + newStrategy: "RollingUpdate", + shouldAllow: false, + }, + { + name: "dedicated-admin-cannot-change-another-field", + operation: admissionv1.Update, + groups: []string{"system:authenticated", "dedicated-admins"}, + oldInstanceType: "m5.xlarge", + newInstanceType: "m6i.2xlarge", + oldStrategy: "RollingUpdate", + newStrategy: "OnDelete", + shouldAllow: false, + }, + { + name: "dedicated-admin-cannot-add-finalizer", + operation: admissionv1.Update, + groups: []string{"system:authenticated", "dedicated-admins"}, + oldInstanceType: "m5.xlarge", + newInstanceType: "m6i.2xlarge", + oldStrategy: "RollingUpdate", + newStrategy: "RollingUpdate", + newMetadata: `, "finalizers": ["example.com/protect"]`, + shouldAllow: false, + }, + { + name: "dedicated-admin-can-update-with-api-managed-metadata-changes", + operation: admissionv1.Update, + groups: []string{"system:authenticated", "dedicated-admins"}, + oldInstanceType: "m5.xlarge", + newInstanceType: "m6i.2xlarge", + oldStrategy: "RollingUpdate", + newStrategy: "RollingUpdate", + oldMetadata: `, "generation": 1, "resourceVersion": "1"`, + newMetadata: `, "generation": 2, "resourceVersion": "2"`, + shouldAllow: true, + }, + { + name: "dedicated-admin-can-update-with-managed-fields-changes", + operation: admissionv1.Update, + groups: []string{"system:authenticated", "dedicated-admins"}, + oldInstanceType: "m5.xlarge", + newInstanceType: "m6i.2xlarge", + oldStrategy: "RollingUpdate", + newStrategy: "RollingUpdate", + oldMetadata: `, "managedFields": [{"manager": "old-manager"}]`, + newMetadata: `, "managedFields": [{"manager": "new-manager"}]`, + shouldAllow: true, + }, + { + name: "dedicated-admin-cannot-reset-managed-fields", + operation: admissionv1.Update, + groups: []string{"system:authenticated", "dedicated-admins"}, + oldInstanceType: "m5.xlarge", + newInstanceType: "m6i.2xlarge", + oldStrategy: "RollingUpdate", + newStrategy: "RollingUpdate", + newMetadata: `, "managedFields": [{}]`, + shouldAllow: false, + }, + { + name: "dedicated-admin-cannot-create-control-plane-machine-set", + operation: admissionv1.Create, + groups: []string{"system:authenticated", "dedicated-admins"}, + oldInstanceType: "m5.xlarge", + newInstanceType: "m6i.2xlarge", + oldStrategy: "RollingUpdate", + newStrategy: "RollingUpdate", + shouldAllow: false, + }, + } + + gvk := metav1.GroupVersionKind{Group: machineGroup, Version: "v1", Kind: controlPlaneMachineSetKind} + gvr := metav1.GroupVersionResource{Group: machineGroup, Version: "v1", Resource: "controlplanemachinesets"} + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + hook := NewWebhook() + object := controlPlaneMachineSetObject(test.newInstanceType, test.newStrategy, test.newMetadata) + oldObject := controlPlaneMachineSetObject(test.oldInstanceType, test.oldStrategy, test.oldMetadata) + request, err := testutils.CreateHTTPRequest(hook.GetURI(), test.name, gvk, gvr, test.operation, "customer-admin", test.groups, "openshift-machine-api", object, oldObject) + if err != nil { + t.Fatalf("creating request: %v", err) + } + + response, err := testutils.SendHTTPRequest(request, hook) + if err != nil { + t.Fatalf("sending request: %v", err) + } + if response.Allowed != test.shouldAllow { + t.Fatalf("allowed = %t, want %t", response.Allowed, test.shouldAllow) + } + }) + } + + assertCCSResizeAllowed := func(t *testing.T, expected bool) { + t.Helper() + hook := NewWebhook() + object := controlPlaneMachineSetObject("m6i.2xlarge", "RollingUpdate", "") + oldObject := controlPlaneMachineSetObject("m5.xlarge", "RollingUpdate", "") + request, err := testutils.CreateHTTPRequest(hook.GetURI(), t.Name(), gvk, gvr, admissionv1.Update, "customer-admin", []string{"system:authenticated", "dedicated-admins"}, "openshift-machine-api", object, oldObject) + if err != nil { + t.Fatalf("creating request: %v", err) + } + response, err := testutils.SendHTTPRequest(request, hook) + if err != nil { + t.Fatalf("sending request: %v", err) + } + if response.Allowed != expected { + t.Fatalf("allowed = %t, want %t", response.Allowed, expected) + } + } + + t.Run("dedicated-admin-cannot-resize-when-disabled", func(t *testing.T) { + if err := os.WriteFile(configFile, []byte("false"), 0o600); err != nil { + t.Fatalf("writing config file: %v", err) + } + assertCCSResizeAllowed(t, false) + }) + t.Run("dedicated-admin-cannot-resize-when-unset", func(t *testing.T) { + if err := os.Remove(configFile); err != nil { + t.Fatalf("removing config file: %v", err) + } + assertCCSResizeAllowed(t, false) + }) + t.Run("dedicated-admin-cannot-resize-with-invalid-value", func(t *testing.T) { + if err := os.WriteFile(configFile, []byte("enabled"), 0o600); err != nil { + t.Fatalf("writing config file: %v", err) + } + assertCCSResizeAllowed(t, false) + }) +} + +func controlPlaneMachineSetObject(instanceType, strategy, metadata string) *runtime.RawExtension { + return &runtime.RawExtension{Raw: []byte(fmt.Sprintf(`{ +"apiVersion": "machine.openshift.io/v1", +"kind": "ControlPlaneMachineSet", +"metadata": {"name": "cluster", "namespace": "openshift-machine-api"%s}, +"spec": { + "strategy": {"type": %q}, + "template": { + "machineType": "machines_v1beta1_machine_openshift_io", + "machines_v1beta1_machine_openshift_io": { + "metadata": {"labels": {"machine.openshift.io/cluster-api-machine-role": "master"}}, + "spec": {"providerSpec": {"value": {"ami": "ami-123", "instanceType": %q}}} + } + } +} +}`, metadata, strategy, instanceType))} +} + // TestAutoScaling checks specific cases for autoscaling CRDs func TestAutoScaling(t *testing.T) { tests := []regularuserTests{ diff --git a/test/e2e/validation_webhook_tests.go b/test/e2e/validation_webhook_tests.go index fc8675c9..014a48fe 100644 --- a/test/e2e/validation_webhook_tests.go +++ b/test/e2e/validation_webhook_tests.go @@ -15,6 +15,7 @@ import ( configv1 "github.com/openshift/api/config/v1" quotav1 "github.com/openshift/api/quota/v1" securityv1 "github.com/openshift/api/security/v1" + operatorconfig "github.com/openshift/managed-cluster-validating-webhooks/config" "github.com/openshift/osde2e-common/pkg/clients/openshift" monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" appsv1 "k8s.io/api/apps/v1" @@ -44,6 +45,7 @@ var _ = Describe("Managed Cluster Validating Webhooks", Ordered, func() { dedicatedAdmink8s *openshift.Client userk8s *openshift.Client clusterAdmink8s *openshift.Client + clusterAdminsK8s *openshift.Client unauthenticatedk8s *openshift.Client dynamicClient dynamic.Interface testNamespace *v1.Namespace @@ -68,10 +70,10 @@ var _ = Describe("Managed Cluster Validating Webhooks", Ordered, func() { })) Expect(err).ShouldNot(HaveOccurred(), "Unable to create test namespace") - // Pre-flight: verify dedicated-admins RBAC is working at cluster level. + // Pre-flight: verify dedicated-admins cluster-level RBAC is working. // This catches broken RBAC early (seconds) instead of timing out the // 5-minute namespace-level probe, which would cascade-skip 18+ tests. - By("pre-flight: checking dedicated-admins RBAC is working at cluster level") + By("pre-flight: checking dedicated-admins can create SubjectAccessReviews") sarGVR := schema.GroupVersionResource{ Group: "authorization.k8s.io", Version: "v1", @@ -85,9 +87,9 @@ var _ = Describe("Managed Cluster Validating Webhooks", Ordered, func() { "user": "test-user@redhat.com", "groups": []interface{}{"dedicated-admins", "system:authenticated"}, "resourceAttributes": map[string]interface{}{ - "verb": "create", - "resource": "configmaps", - "namespace": "default", + "group": "authorization.k8s.io", + "resource": "subjectaccessreviews", + "verb": "create", }, }, }) @@ -103,7 +105,7 @@ var _ = Describe("Managed Cluster Validating Webhooks", Ordered, func() { } else if !allowed { reason, _, _ := unstructured.NestedString(sarResult.Object, "status", "reason") fmt.Fprintf(GinkgoWriter, "\n=== RBAC Pre-flight Check Failed ===\n") - fmt.Fprintf(GinkgoWriter, "SubjectAccessReview: dedicated-admins cannot create configmaps in 'default' namespace\n") + fmt.Fprintf(GinkgoWriter, "SubjectAccessReview: dedicated-admins cannot create SubjectAccessReviews\n") if reason != "" { fmt.Fprintf(GinkgoWriter, "Reason: %s\n", reason) } @@ -132,7 +134,7 @@ var _ = Describe("Managed Cluster Validating Webhooks", Ordered, func() { fmt.Fprintf(GinkgoWriter, "=== End RBAC Pre-flight Check ===\n\n") Skip("Cluster does not have working dedicated-admins RBAC — skipping (likely a lease pool issue, not a webhook test failure)") } - fmt.Fprintf(GinkgoWriter, "Pre-flight check passed: dedicated-admins can create configmaps in 'default' namespace\n") + fmt.Fprintf(GinkgoWriter, "Pre-flight check passed: dedicated-admins can create SubjectAccessReviews\n") } By("waiting for namespace permissions to be ready") @@ -193,6 +195,8 @@ var _ = Describe("Managed Cluster Validating Webhooks", Ordered, func() { Expect(err).ShouldNot(HaveOccurred(), "Unable to setup impersonated dedicated admin client") clusterAdmink8s, err = client.Impersonate("system:admin", "cluster-admins") Expect(err).ShouldNot(HaveOccurred(), "Unable to setup impersonated cluster admin client") + clusterAdminsK8s, err = client.Impersonate("cluster-admin@redhat.com", "cluster-admins") + Expect(err).ShouldNot(HaveOccurred(), "Unable to setup impersonated cluster-admins client") userk8s, err = client.Impersonate("majora", "system:authenticated") Expect(err).ShouldNot(HaveOccurred(), "Unable to setup impersonated user client") unauthenticatedk8s, err = client.Impersonate("system:unauthenticated") @@ -417,6 +421,46 @@ var _ = Describe("Managed Cluster Validating Webhooks", Ordered, func() { Expect(err).NotTo(HaveOccurred(), "Expected to delete ConfigMap in test namespace") }) + It("blocks CPMS instance type updates when the CCS config is absent", func(ctx context.Context) { + configMap := &v1.ConfigMap{} + err := client.Get(ctx, operatorconfig.ValidationWebhookConfigMapName, namespaceName, configMap) + if err == nil { + Skip("CCS CPMS resize ConfigMap is present") + } + Expect(errors.IsNotFound(err)).To(BeTrue(), "getting CCS CPMS resize ConfigMap") + + cpmsClient, err := dynamic.NewForConfig(clusterAdminsK8s.GetConfig()) + Expect(err).NotTo(HaveOccurred(), "creating cluster-admin CPMS client") + controlPlaneMachineSets := cpmsClient.Resource(schema.GroupVersionResource{ + Group: "machine.openshift.io", Version: "v1", Resource: "controlplanemachinesets", + }).Namespace("openshift-machine-api") + cpms, err := controlPlaneMachineSets.Get(ctx, "cluster", metav1.GetOptions{}) + if errors.IsNotFound(err) { + Skip("ControlPlaneMachineSet is not present") + } + Expect(err).NotTo(HaveOccurred(), "getting ControlPlaneMachineSet") + + instanceTypePath := []string{"spec", "template", "machines_v1beta1_machine_openshift_io", "spec", "providerSpec", "value", "instanceType"} + instanceType, found, err := unstructured.NestedString(cpms.Object, instanceTypePath...) + Expect(err).NotTo(HaveOccurred(), "getting ControlPlaneMachineSet instance type") + Expect(found).To(BeTrue(), "ControlPlaneMachineSet instance type is missing") + + supportedInstanceTypes := map[string]struct{}{ + "m5.large": {}, "m5.xlarge": {}, "m5.2xlarge": {}, "m5.4xlarge": {}, "m5.8xlarge": {}, "m5.12xlarge": {}, "m5.16xlarge": {}, "m5.24xlarge": {}, + "m6i.large": {}, "m6i.xlarge": {}, "m6i.2xlarge": {}, "m6i.4xlarge": {}, "m6i.8xlarge": {}, "m6i.12xlarge": {}, "m6i.16xlarge": {}, "m6i.24xlarge": {}, + } + if _, ok := supportedInstanceTypes[instanceType]; !ok { + Skip(fmt.Sprintf("ControlPlaneMachineSet instance type %q cannot be validly increased for this test", instanceType)) + } + + updatedCPMS := cpms.DeepCopy() + err = unstructured.SetNestedField(updatedCPMS.Object, "m6i.32xlarge", instanceTypePath...) + Expect(err).NotTo(HaveOccurred(), "setting ControlPlaneMachineSet instance type") + _, err = controlPlaneMachineSets.Update(ctx, updatedCPMS, metav1.UpdateOptions{DryRun: []string{metav1.DryRunAll}}) + Expect(errors.IsForbidden(err)).To(BeTrue(), "expected ControlPlaneMachineSet update to be denied") + Expect(err.Error()).To(ContainSubstring(`admission webhook "regular-user-validation.managed.openshift.io" denied the request`)) + }) + It("blocks modifications to nodes", func(ctx context.Context) { var nodes v1.NodeList selectInfraNodes := resources.WithLabelSelector(labels.FormatLabels(map[string]string{"node-role.kubernetes.io": "infra"})) @@ -434,6 +478,14 @@ var _ = Describe("Managed Cluster Validating Webhooks", Ordered, func() { // TODO: test "system:serviceaccounts:openshift-backplane-cee" group can use NetNamespace CR It("allows dedicated-admin to manage CustomDomain CRs", func(ctx context.Context) { + crdClient, err := apiextensionsclientset.NewForConfig(clusterAdmink8s.GetConfig()) + Expect(err).ShouldNot(HaveOccurred(), "Unable to create CRD client") + _, err = crdClient.ApiextensionsV1().CustomResourceDefinitions().Get(ctx, "customdomains.managed.openshift.io", metav1.GetOptions{}) + if errors.IsNotFound(err) { + Skip("Skipping test: CustomDomain CRD not found in cluster") + } + Expect(err).NotTo(HaveOccurred(), "getting CustomDomain CRD") + dynamicClient, err := dynamic.NewForConfig(dedicatedAdmink8s.GetConfig()) Expect(err).ShouldNot(HaveOccurred(), "failed creating the dynamic client: %w", err)