Skip to content

Fix SecurityGroup availability status by counting security group rules #651

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.

Sign up for GitHub

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

Open
gndrmnn wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix SecurityGroup availability status by counting security group rules #651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions internal/controllers/securitygroup/actuator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"errors"
"fmt"
"iter"
"time"

"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/groups"
"github.com/gophercloud/gophercloud/v2/openstack/networking/v2/extensions/security/rules"
Expand Down Expand Up @@ -50,6 +51,11 @@ type (
securityGroupIterator = iter.Seq2[*osResourceT, error]
)

const (
// The frequency to poll when waiting for the resource to become available
securityGroupAvailablePollingPeriod = 15 * time.Second
)

type securityGroupActuator struct {
osClient osclients.NetworkClient
k8sClient client.Client
Expand Down Expand Up @@ -134,9 +140,6 @@ func (actuator securityGroupActuator) CreateResource(ctx context.Context, obj *o
ProjectID: projectID,
}

// FIXME(mandre) The security group inherits the default security group
// rules. This could be a problem when we implement `update` if ORC
// does not takes these rules into account.
osResource, err := actuator.osClient.CreateSecGroup(ctx, &createOpts)
if err != nil {
// We should require the spec to be updated before retrying a create which returned a conflict
Expand Down
19 changes: 18 additions & 1 deletion internal/controllers/securitygroup/status.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,24 @@ func (securityGroupStatusWriter) ResourceAvailableStatus(orcObject orcObjectPT,
}
}

// SecurityGroup is available as soon as it exists
Copy link
Collaborator

@mandre mandre Jan 27, 2026

Choose a reason for hiding this comment

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

On second thoughts, I wonder if this is really what we want: the security group is indeed available right away, meaning we can do operations on it. The Available condition is accurate.

The Progressing condition, on the other hand, should reflect that we still have security group rules being provisioned, and that the desired state does not yet match the observed state.

Copy link
Contributor Author

@gndrmnn gndrmnn Jan 27, 2026

Choose a reason for hiding this comment

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

Issue #120 states that the current way to represent the Available status in security groups is undesired. As far as I understand, security group rules can only ever be part of a single security group. And they can not be cross-referenced. So I would agree with #120 in that a security group is only "available and matches the spec" when the security groups are actually created.

However, this design decision has to be decided by the project according to it's own goals.

Copy link
Contributor

@mdbooth mdbooth Jan 27, 2026

Choose a reason for hiding this comment

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

I agree, this hinges on understanding what 'Available' most usefully means in the API. We certainly can proceed to create resources as soon as the SecurityGroup exists, but is that creating risk?

Another thought: we create a SecurityGroup, it's fully reconciled and available. We then modify its Rules. Should it still be Available in the period while the new spec has not been applied?

Is it sufficient that a user should wait for 'Available=True AND Progressing=False'?

I don't have the answers, unfortunately. I recommend writing down all the use cases and edge conditions you can think of and deciding what the correct status should be in all cases. If it can't be defined, does that mean we need to add a new Condition?

Copy link
Collaborator

@mandre mandre Jan 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

The more I think about it, the more I believe Available=True should mean we can operate on the resource. Granted, securitygroup is a bit special because we've convoluted 2 resources into 1 controller (security groups and security group rules).

My current thinking is that:

  • the security group itself is available for use right away upon creation. Neutron did not bother adding a status field for it.
  • the security group rules, in our representation in ORC, are attributes to the security group. Their creation/update are reflected by the Progressing condition.

Likewise, I'm of the opinion that the Available condition of the security group should not change when the rules are being updated. This can again be tracked via the Progressing condition.

This is what we say in the docs:

Every ORC resource reports its status through two conditions: Available (whether the resource is ready for use) and Progressing (whether ORC is still working on it).

And also, about the ResourceAvailableStatus() method:

This sets the value of the Available condition. This should not return true unless the resource is completely ready to be used.

We should clarify what "ready to be used" means. I'll check how the other controllers implement the ResourceAvailableStatus() method.

Copy link
Collaborator

@mandre mandre Jan 27, 2026

Choose a reason for hiding this comment

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

Looking at the existing controllers, they're all returning Available as soon as they're created, except for the resources that have a status field. For these we're looking at the following conditions:

  • Floating IP, active and down statuses
  • Image, active status
  • Network, active status
  • Port, active and down status
  • Router, active status
  • Server, active status
  • Volume, active and in-use statuses

Copy link
Contributor

@eshulman2 eshulman2 Jan 27, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I agree with the spirit of this PR, but I think a better implementation would be to add a third condition rather than modifying Available.

The problem is Available=True, Progressing=True doesn't tell you what it's progressing on. Maybe you don't care if it's updating a tag, but you do care if a security rule failed. With the current situation tag updates a indistinguishable from rule creation error and doesn't provide enough info to the user:

Scenario Available Progressing User can distinguish?
SG created, rules pending True True No - could be tags updating
SG created, rule failed True True No - looks like still progressing
SG created, tag update in progress True True No - same as above

I would propose to add a third condition for subresources. A SubResourcesReady condition for controllers with sub-resources (SecurityGroup→Rules, Router→Interfaces, etc.):

  • Available: Main resource exists and can be operated on
  • Progressing: Controller is actively reconciling something
  • SubResourcesReady: All sub-resources (rules, interfaces, attachments) are in desired state

This gives users clear semantics:

Scenario Available Progressing SubResourcesReady
SG created, rules pending True True False
SG created, rule failed True False False (reason: rule error)
SG fully ready True False True
Tag update in progress True True True

This pattern could apply to other controllers too making it easy to understand sub resource failures and act on them, providing a better user experience. WDYT? @mandre @mdbooth

Copy link
Contributor Author

@gndrmnn gndrmnn Jan 28, 2026

Choose a reason for hiding this comment

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

I think the question here comes down to "What is the goal of ORC and in what context do you want its users to operate?"

You have to decide whether you want to look at the resources that users define using ORC as "bit-for-bit representation of an OpenStack resources", or if you want to see these resources as abstractions or "ORC resources".

If you look at a SecurityGroup as a bit-for-bit OpenStack resource representation, then sure, it is ready/available as soon as it is accepted and created.
In this case I personally think a SecurityGroupRule should be its own resource and not be part of the SecurityGroup. Similar to how RouterInterfaces and Routers are handled.

If, however, you look at how SecurityGroups are defined in ORC - with specific rules stated in the spec - then it becomes an abstraction of a OpenStack resource - an "ORC resource". And the available status should represent that. In that case, I don't think a third status is necessary. At least for SecurityGroups it makes no sense to be available when the rules are failing to be created.

I think both views are valid, but the project has to decide on one and clearly specify what it is.

Copy link
Contributor

@eshulman2 eshulman2 Jan 28, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I believe adding this condition offers a few key advantages:

  • User Empowerment: It lets the user decide the approach. By having a distinct condition, we can implement a policy on how to act on that status—effectively moving the decision-making power to the user while keeping the status itself informative.
  • Predictability: It establishes a common behavior for all resources with sub-resources. This consistency makes the system much more predictable for the end user. For example in a loadbalancer-pool controller I started implementing I decided to let the status move to available even if not all members are in the pool yet. IMO having different controllers implementing different behaviors would make ORC unpredictable and would "hide" the behavior from the user.
  • Visibility for Complex Resources: For complex resources like Load Balancers (which may have many sub-resources), an additional condition makes the internal state significantly more visible.

My general goal here is to find a middle ground, let’s provide the data and let the user decide how they want ORC to act in their specific environment or even on a per-resource basis.

Copy link
Collaborator

@mandre mandre Jan 29, 2026

Choose a reason for hiding this comment

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

In principle I agree we need a new condition to differentiate between when the resource is available (meaning you can operate on it) and when we consider it ready to be used (generally when sub resources are all ready). From the k8s API convention document:

Conditions should be added to explicitly convey properties that users and components care about rather than requiring those properties to be inferred from other observations.

If we wanted to give control to the user, for what condition to look for when provisioning resources that reference other resources, we'd have to change the dependency management a bit. Right now, controllers have watches for when deps become available. We would need watches for other conditions, which can quickly become heavy to manage.

Also, now that I look at it, the port controller does not even check that the security group is Available (here and here) 🤦... We should check it.

I'm inclined to accept this change, and mark the security group available only when all rules are fully provisioned, until we have a need for more granularity.

Could you then make sure the Port controller operates on available security groups?

resourceSpec := orcObject.Spec.Resource
if resourceSpec != nil && resourceSpec.Rules != nil {
// Make sure specified security group rules exist in resource

resourceStatus := orcObject.Status.Resource
if resourceStatus == nil || resourceStatus.Rules == nil {
return metav1.ConditionFalse, progress.WaitingOnOpenStack(progress.WaitingOnReady, securityGroupAvailablePollingPeriod)
}

if len(resourceSpec.Rules) != len(resourceStatus.Rules) {
Copy link
Collaborator

@mandre mandre Jan 27, 2026

Choose a reason for hiding this comment

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

We should not be comparing the number of rules in the spec and in the status, because they can be different: the security group inherits the default security group rules.
Instead, we should check that the desired state (the rules in the spec) accurately represents the observed state (the rules in the status) AND account for the default security rules. The actuator should already do exactly that for the rules update.

Copy link
Contributor Author

@gndrmnn gndrmnn Jan 27, 2026

Choose a reason for hiding this comment

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

We should not be comparing the number of rules in the spec and in the status, because they can be different: the security group inherits the default security group rules.

I am not sure I fully understand. So every security group implicitly inherits the rules of the security group default, right? But this is not shown explicitly in openstack security group show, or the Horizon Web UI. Do I understand this correctly?

But it seems like ORC also does not currently use the (inherited) default rules to populate the status of the security group, so that should not actually be a problem. Unless this is unwanted behaviour and ORC is supposed to do that in the future, in which case the change in this PR will indeed break.

Instead, we should check that the desired state (the rules in the spec) accurately represents the observed state (the rules in the status) AND account for the default security rules. The actuator should already do exactly that for the rules update.

Yes, my understanding is that the actuator will take care of making sure the rules are matching from a content perspective. That being said, the rules should only become part of the security group resource status if and when they match. So to update the Available status it should be enough to make sure the number of rules is the same, without duplicating the validation logic of the rule's content, or am I not seeing the problem right now?

Copy link
Collaborator

@mandre mandre Jan 27, 2026

Choose a reason for hiding this comment

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

OK, this confused me as well so I ran some tests:

  • when creating a security group, we indeed inherit the default security group rules, this is just how OpenStack works.
  • however, the first thing the controller does is to delete the default rules, meaning that the user never sees these rules in the status.
  • also, it appears that OpenStack is not trying to add the rules back, which is good.

I believe this is the expected behavior, and makes the experience with ORC deterministic.

This is effectively a consequence of #174, and this comment in the actuator should now be removed because it's outdated.

All that to say that in most cases, we can indeed check if the resource is fully provisioned by comparing the number of rules.

Copy link
Contributor Author

@gndrmnn gndrmnn Jan 27, 2026

Choose a reason for hiding this comment

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

OK, this confused me as well so I ran some tests:

  • when creating a security group, we indeed inherit the default security group rules, this is just how OpenStack works.

  • however, the first thing the controller does is to delete the default rules, meaning that the user never sees these rules in the status.

  • also, it appears that OpenStack is not trying to add the rules back, which is good.

I believe this is the expected behavior, and makes the experience with ORC deterministic.

Got it. At first I thought you meant like some sort of implicit inheritence

This is effectively a consequence of #174, and this comment in the actuator should now be removed because it's outdated.

I amended the commit to remove the comment

Copy link
Collaborator

@mandre mandre Jan 27, 2026

Choose a reason for hiding this comment

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

Thanks! I'm still a unsure about the ResourceAvailableStatus() changes, but we'll definitely want to get rid of the outdated and confusing comment.

return metav1.ConditionFalse, progress.WaitingOnOpenStack(progress.WaitingOnReady, securityGroupAvailablePollingPeriod)
}

if len(resourceSpec.Rules) != len(osResource.Rules) {
return metav1.ConditionFalse, progress.WaitingOnOpenStack(progress.WaitingOnReady, securityGroupAvailablePollingPeriod)
}
}

return metav1.ConditionTrue, nil
}

Expand Down