Skip to content

Conversation

bertiethorpe
Copy link
Member

@bertiethorpe bertiethorpe commented Mar 4, 2025

Previous fix for #598 doesn't work.

lookup(each.value, "port_security_enabled", null) != false ? var.security_group_ids : []
still applies the security groups even when port_security_enabled is false.

EDIT:

Solution is to circumvent toggling port security entirely with https://registry.terraform.io/providers/vtdc/viettelidcops/latest/docs/resources/networking_port_v2#no_security_groups-1

Then disabling security groups is done by:

cluster_networks = [
  {
    network = "some_net"
    subnet = "some_subnet"
    no_security_groups = true
  }
]

@sjpb
Copy link
Collaborator

sjpb commented Mar 5, 2025

So the actual error without this PR, when settting port_security_enabled: false is:

│ Error: Error creating openstack_networking_port_v2: Expected HTTP response code [201 202] when accessing [POST https://create.leaf.cloud:9696/v2.0/ports], but got 400 instead: {"NeutronError": {"type": "PortSecurityAndIPRequiredForSecurityGroups", "message": "Port security must be enabled and port must have an IP address in order to use security groups.", "detail": ""}}
│ 

even screwier the logic appears to be evaluated properly:

> var.cluster_networks[0]
{
  "network" = "slurmapp-ci"
  "port_security_enabled" = false
  "subnet" = "slurmapp-ci"
}
> lookup(var.cluster_networks[0], "port_security_enabled", null) != false
false

@bertiethorpe bertiethorpe marked this pull request as ready for review March 5, 2025 09:36
@bertiethorpe bertiethorpe requested a review from a team as a code owner March 5, 2025 09:36
@jovial
Copy link
Collaborator

jovial commented Mar 5, 2025

Can we omit the security groups if port_security is false?

@jovial
Copy link
Collaborator

jovial commented Mar 5, 2025

For me, it used to work when using admin creds. When I switched to non-admin creds, I actually just enabled port security on the network (even though I don't think it is implemented).

@bertiethorpe bertiethorpe force-pushed the fix/port-security-2 branch from cef3ced to aa12167 Compare March 5, 2025 10:18
JohnGarbutt
JohnGarbutt previously approved these changes Mar 5, 2025
Copy link
Member

@JohnGarbutt JohnGarbutt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, I think this makes sense. In some places we can skip adding security groups on some networks.

I like keeping port security on, unless we find a good use case to turn that off.

@JohnGarbutt
Copy link
Member

Do we put docs for this extra flag somewhere?

@sjpb
Copy link
Collaborator

sjpb commented Mar 5, 2025

Do we put docs for this extra flag somewhere?

Yeah this is not ready to merge. It needs documenting in the variables.tf file (in both places it applies)

@sjpb
Copy link
Collaborator

sjpb commented Mar 5, 2025

Tested as working on leafcloud with no_security_groups = true.

Copy link
Collaborator

@sjpb sjpb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@sjpb sjpb merged commit 0dcf774 into main Mar 5, 2025
2 checks passed
@sjpb sjpb deleted the fix/port-security-2 branch March 5, 2025 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants