Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update security group cannot remove all rules #378

Closed
freudl opened this issue Feb 8, 2024 · 4 comments
Closed

Update security group cannot remove all rules #378

freudl opened this issue Feb 8, 2024 · 4 comments

Comments

@freudl
Copy link
Contributor

freudl commented Feb 8, 2024

The v3 client enables me to update security groups.

  • Given a group with one rule
  • When the group is updated to zero rules
  • Then the group has no rules

However, the action as no effect.

As far as I understand, during the request the parameter is marshalled with option omitempty so the empty rule set never leaves the SDK.

@sneal
Copy link
Contributor

sneal commented Feb 8, 2024

That seems likely. Let me take a took.

@freudl
Copy link
Contributor Author

freudl commented Feb 8, 2024

Thinking more about this: it might be worth to consider another case.

  • Given a group with one rule
  • When the group's name is updated
  • Then the name changes and the rules remain

In such situtations a SDK user might define a struct with field name set, but rules undefined.

noRules, _ := c.Update(ctx, "guid-A", &resource.SecurityGroupUpdate{
  Name: "newName",
})
newName, _ := c.Update(ctx, "guid-B", &resource.SecurityGroupUpdate{
  Rules: []*resource.SecurityGroupRule{},
})

@sneal
Copy link
Contributor

sneal commented Feb 14, 2024

  1. PATCHing
{
  "name": "new-name",
  "rules": null
}

results in the following error: cfclient error (CF-UnprocessableEntity|10008): Rules must be an array

  1. PATCHing
{
  "name": "new-name",
  "rules": []
}

results in a rename and all rules removed

  1. PATCHing
{
  "name": "new-name",
}

results in a rename and all rules stay the same

The relevant options are either 2 or 3. Option 3 - renaming a security group without specifying all the rules seems more useful than the ability to remove all rules from a security group. It seems like a security group with no rules isn't useful and in those cases the caller should just delete it. If so, then leaving the omitempty on Rules is the right thing to do.

@sneal
Copy link
Contributor

sneal commented Feb 14, 2024

Reading through the linked cf-mgmt issue, there is an issue here. SecurityGroupGloballyEnabled should be optionally transmitted, so change it to:

type SecurityGroupGloballyEnabled struct {
	Running *bool `json:"running,omitempty"`
	Staging *bool `json:"staging,omitempty"`
}

@freudl Does that address your concern?

sneal added a commit that referenced this issue Feb 28, 2024

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Globally enabled running and staging flags should be optional for security groups allowing only the security group name or rule definitions to be updated.
sneal added a commit that referenced this issue Feb 28, 2024
@sneal sneal closed this as completed Feb 28, 2024
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

No branches or pull requests

2 participants