fix(pr1014): disable CAPO managed security groups for bm

ricolin · ricolin · commit 91abc998bb77 · 2026-05-28T23:57:51.000+08:00
Candidate upstream home: #1014. PR #1014 owns the server_type=bm path. Bare-metal tenant networks can have port security disabled so Ironic can use pre-created ports, but CAPO managed security groups cause generated OpenStackServer ports to include a security group. Neutron rejects that combination before Nova/Ironic can create the server. Add a ClusterClass variable that removes managedSecurityGroups only for server_type=bm clusters, keep VM clusters on the existing managed security group behavior, and cover the remove-patch path in the Rust test helper. Validation: - cargo test managed_security_groups - cargo test test_convert_values_to_cluster_topology_variables - uv run pytest magnum_cluster_api/tests/unit/test_sync.py magnum_cluster_api/tests/unit/test_utils.py magnum_cluster_api/tests/unit/test_resources.py magnum_cluster_api/tests/unit/test_driver.py -q Assisted-By: ChatGPT <noreply@openai.com> (cherry picked from commit 93e3406ff794f6e1bdd5cbee42f0ec61112691d5) (cherry picked from commit 86b807549c8a845c5e037e89ada46e1bd99e325c) Signed-off-by: Rico Lin <rlin@vexxhost.com>
diff --git a/magnum_cluster_api/resources.py b/magnum_cluster_api/resources.py
@@ -1197,6 +1197,13 @@ def get_object(self) -> dict:
                                 utils.get_cluster_floating_ip_disabled(self.cluster),
                             ),
                         },
+                        {
+                            "name": "disableManagedSecurityGroups",
+                            "value": getattr(
+                                self.cluster.cluster_template, "server_type", "vm"
+                            )
+                            == "bm",
+                        },
                         {
                             "name": "dnsNameservers",
                             "value": self.cluster.cluster_template.dns_nameserver.split(
diff --git a/magnum_cluster_api/tests/unit/test_resources.py b/magnum_cluster_api/tests/unit/test_resources.py
@@ -12,6 +12,8 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
+from types import SimpleNamespace
+
 import pytest
 from magnum.objects import fields
 from magnum.tests.unit.objects import utils
@@ -82,6 +84,135 @@ def test_generate_machine_deployments_for_cluster_with_deleting_node_group(
     assert len(mds) == 2
 
 
+def test_cluster_variables_disable_managed_security_groups_for_bm(context, mocker):
+    cluster = mocker.Mock()
+    cluster.cluster_template = mocker.Mock(
+        network_driver="calico",
+        dns_nameserver="1.1.1.1",
+        external_network_id="public",
+        server_type="vm",
+    )
+    cluster.default_ng_master = mocker.Mock(image_id="image")
+    cluster.docker_volume_size = None
+    cluster.fixed_network = None
+    cluster.fixed_subnet = None
+    cluster.flavor_id = "worker"
+    cluster.keypair = None
+    cluster.labels = {}
+    cluster.master_count = 1
+    cluster.master_flavor_id = "control-plane"
+    cluster.master_lb_enabled = True
+    cluster.nodegroups = []
+    cluster.stack_id = "kube-test"
+
+    default_volume_type = SimpleNamespace(name="fast")
+    openstack_client = mocker.Mock()
+    openstack_client.cinder.return_value.volume_types.default.return_value = (
+        default_volume_type
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.clients.get_openstack_api",
+        return_value=openstack_client,
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.cinder.get_default_boot_volume_type",
+        return_value=default_volume_type.name,
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.generate_machine_deployments_for_cluster",
+        return_value=[],
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.neutron.get_external_network_id",
+        return_value="external-network",
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.neutron.get_fixed_subnet_id",
+        return_value=None,
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.utils.ensure_controlplane_server_group",
+        return_value="server-group",
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.utils.generate_api_cert_san_list",
+        return_value="",
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.utils.generate_apt_proxy_config",
+        return_value="",
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.utils.generate_containerd_config",
+        return_value="",
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.utils.generate_systemd_proxy_config",
+        return_value="",
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.utils.get_cluster_container_infra_prefix",
+        return_value="",
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.utils.get_fixed_network_id",
+        return_value=None,
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.utils.get_kube_tag",
+        return_value="v1.34.3",
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.utils.get_operating_system",
+        return_value="ubuntu",
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.utils.is_controlplane_different_failure_domain",
+        return_value=False,
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.utils.lookup_flavor",
+        side_effect=lambda _osc, flavor_id: SimpleNamespace(name=flavor_id),
+    )
+    mocker.patch(
+        "magnum_cluster_api.resources.utils.lookup_image",
+        return_value={"id": "image"},
+    )
+
+    rust_driver = mocker.Mock()
+    rust_driver.resolve_immutable_fields.return_value = {
+        "apiServerLoadBalancer": {"enabled": True}
+    }
+
+    resource = resources.Cluster(
+        context,
+        mocker.Mock(),
+        mocker.Mock(),
+        cluster,
+        rust_driver,
+    ).get_object()
+
+    variables = {
+        variable["name"]: variable["value"]
+        for variable in resource["spec"]["topology"]["variables"]
+    }
+    assert variables["disableManagedSecurityGroups"] is False
+
+    cluster.cluster_template.server_type = "bm"
+    resource = resources.Cluster(
+        context,
+        mocker.Mock(),
+        mocker.Mock(),
+        cluster,
+        rust_driver,
+    ).get_object()
+    variables = {
+        variable["name"]: variable["value"]
+        for variable in resource["spec"]["topology"]["variables"]
+    }
+    assert variables["disableManagedSecurityGroups"] is True
+
+
 @pytest.mark.parametrize(
     "auto_scaling_enabled",
     [True, False, None],
diff --git a/src/features/managed_security_groups.rs b/src/features/managed_security_groups.rs
@@ -0,0 +1,105 @@
+use crate::{
+    cluster_api::{
+        clusterclasses::{
+            ClusterClassPatches, ClusterClassPatchesDefinitions,
+            ClusterClassPatchesDefinitionsJsonPatches, ClusterClassPatchesDefinitionsSelector,
+            ClusterClassPatchesDefinitionsSelectorMatchResources, ClusterClassVariables,
+            ClusterClassVariablesSchema,
+        },
+        openstackclustertemplates::OpenStackClusterTemplate,
+    },
+    features::{
+        ClusterClassVariablesSchemaExt, ClusterFeatureEntry, ClusterFeaturePatches,
+        ClusterFeatureVariables,
+    },
+};
+use cluster_feature_derive::ClusterFeatureValues;
+use kube::CustomResourceExt;
+use serde::{Deserialize, Serialize};
+
+#[derive(Serialize, Deserialize, ClusterFeatureValues)]
+#[allow(dead_code)]
+pub struct FeatureValues {
+    #[serde(rename = "disableManagedSecurityGroups")]
+    pub disable_managed_security_groups: bool,
+}
+
+pub struct Feature {}
+
+impl ClusterFeaturePatches for Feature {
+    fn patches(&self) -> Vec<ClusterClassPatches> {
+        vec![ClusterClassPatches {
+            name: "disableManagedSecurityGroups".into(),
+            enabled_if: Some("{{ if .disableManagedSecurityGroups }}true{{end}}".into()),
+            definitions: Some(vec![ClusterClassPatchesDefinitions {
+                selector: ClusterClassPatchesDefinitionsSelector {
+                    api_version: OpenStackClusterTemplate::api_resource().api_version,
+                    kind: OpenStackClusterTemplate::api_resource().kind,
+                    match_resources: ClusterClassPatchesDefinitionsSelectorMatchResources {
+                        infrastructure_cluster: Some(true),
+                        ..Default::default()
+                    },
+                },
+                json_patches: vec![ClusterClassPatchesDefinitionsJsonPatches {
+                    op: "remove".into(),
+                    path: "/spec/template/spec/managedSecurityGroups".into(),
+                    ..Default::default()
+                }],
+            }]),
+            ..Default::default()
+        }]
+    }
+}
+
+inventory::submit! {
+    ClusterFeatureEntry{ feature: &Feature {} }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{features::test::TestClusterResources, resources::fixtures::default_values};
+    use pretty_assertions::assert_eq;
+
+    #[test]
+    fn test_keeps_managed_security_groups_by_default() {
+        let feature = Feature {};
+
+        let mut values = default_values();
+        values.disable_managed_security_groups = false;
+
+        let patches = feature.patches();
+        let mut resources = TestClusterResources::new();
+        resources.apply_patches(&patches, &values);
+
+        assert!(resources
+            .openstack_cluster_template
+            .spec
+            .template
+            .spec
+            .managed_security_groups
+            .is_some());
+    }
+
+    #[test]
+    fn test_removes_managed_security_groups_for_baremetal() {
+        let feature = Feature {};
+
+        let mut values = default_values();
+        values.disable_managed_security_groups = true;
+
+        let patches = feature.patches();
+        let mut resources = TestClusterResources::new();
+        resources.apply_patches(&patches, &values);
+
+        assert_eq!(
+            resources
+                .openstack_cluster_template
+                .spec
+                .template
+                .spec
+                .managed_security_groups,
+            None
+        );
+    }
+}
diff --git a/src/features/mod.rs b/src/features/mod.rs
@@ -63,6 +63,7 @@ pub mod flavors;
 pub mod image_repository;
 pub mod images;
 pub mod keystone_auth;
+pub mod managed_security_groups;
 pub mod networks;
 pub mod openid_connect;
 pub mod operating_system;
diff --git a/src/features/test.rs b/src/features/test.rs
@@ -90,6 +90,12 @@ impl ToRenderedPatchOperation for ClusterClassPatchesDefinitionsJsonPatches {
         version: &str,
         values: &T,
     ) -> PatchOperation {
+        if self.op == "remove" {
+            return json_patch::PatchOperation::Remove(RemoveOperation {
+                path: PointerBuf::parse(&self.path).unwrap(),
+            });
+        }
+
         let value = match self.value_from {
             Some(value_from) => value_from.to_rendered_value(version, values),
             None => self.value.expect("value should be present").into(),
@@ -104,9 +110,6 @@ impl ToRenderedPatchOperation for ClusterClassPatchesDefinitionsJsonPatches {
                 path: PointerBuf::parse(&self.path).unwrap(),
                 value: value,
             }),
-            "remove" => json_patch::PatchOperation::Remove(RemoveOperation {
-                path: PointerBuf::parse(&self.path).unwrap(),
-            }),
             _ => panic!("Unsupported patch operation: {}", self.op),
         }
     }
diff --git a/src/resources.rs b/src/resources.rs
@@ -236,6 +236,7 @@ pub mod fixtures {
             .control_plane_availability_zones(vec!["zone1".into(), "zone2".into()])
             .disable_api_server_floating_ip(true)
             .disable_api_server_floating_ip_managed(true)
+            .disable_managed_security_groups(false)
             .external_network_id("external-network-id".into())
             .control_plane_flavor("control-plane".into())
             .flavor("worker".into())
@@ -310,7 +311,7 @@ mod tests {
         let values = default_values();
         let variables: Vec<ClusterTopologyVariables> = values.into();
 
-        assert_eq!(variables.len(), 39);
+        assert_eq!(variables.len(), 40);
 
         for var in &variables {
             match var.name.as_str() {
@@ -356,6 +357,12 @@ mod tests {
                         json!(default_values().disable_api_server_floating_ip_managed)
                     );
                 }
+                "disableManagedSecurityGroups" => {
+                    assert_eq!(
+                        var.value,
+                        json!(default_values().disable_managed_security_groups)
+                    );
+                }
                 "externalNetworkId" => {
                     assert_eq!(var.value, json!(default_values().external_network_id));
                 }
