Repository navigation
Conversation
Add the optional security-group-rules option. When it is set, the charm creates one OpenStack security group named garm-<name>, keeps its rules equal to the configured list, and publishes the group id as security_group_id on the garm-configurator relation. All egress is added when the operator configures no egress rule: OpenStack attaches this group instead of the project's default group, so without it a runner could not reach GitHub. The group is never deleted, and leaving the option unset keeps deployments on the project's default group. A group that cannot be reconciled blocks the unit rather than publishing an id the charm could not confirm. The charm talks to the Neutron API through a small standard-library client rather than an OpenStack SDK: charmcraft builds this charm's dependencies from source, and the SDK's cryptography dependency does not build without OpenSSL development headers, which the charm build environment lacks.
Read security_group_id from the configurator databag and pass it to the OpenStack provider as the scaleset's security_groups extra spec, so a runner boots with the group the configurator manages instead of the project's default group. A databag without the key leaves the extra spec unset, so existing deployments are unchanged.
yanksyoon
force-pushed
the
feat/isolated-security-group
branch
from
October 20, 2026 04:29
fe79067 to
2135de1
Compare
yanksyoon
commented
Oct 20, 2026
yanksyoon
commented
Oct 20, 2026
…back Address the initial review: - Use the official OpenStack SDK instead of a hand-written Keystone and Neutron client. charmcraft installs requirements from source, so the charm now declares libssl-dev and pkg-config as build packages for the SDK's cryptography dependency. - Keep the reconcile control flow in _reconcile rather than a _configure helper. - Delete duplicate security groups that share the managed name, keeping the oldest, instead of blocking on the ambiguity. - Give security-group-rules a charmcraft default: all egress, no ingress, which is what a runner strictly needs. The implicit egress the reconciler added is gone; a configured list without an egress rule is rejected instead. - Restructure the port validation so each branch is visibly needed, and catch JSONDecodeError explicitly. - Shorten the changelog entry.
Duplicates of the managed group name are now resolved by looking at the ports: the survivor is the oldest group a port already attaches, or the oldest when none does, because that is the group protecting live runners. Only duplicates no port uses are deleted, so the reconcile never asks OpenStack for a delete it would refuse, and a duplicate in use is logged rather than blocking the unit. The port filter is re-applied client side, because OpenStack ignores a query parameter it does not know and would otherwise report every port in the project as attached.
…the delete The port check and the delete cannot be atomic, so OpenStack can still answer the delete with a conflict. That is the same condition the check guards against, so it is logged and left in place rather than raised: a group a port uses must never block the reconcile. Any other delete failure still propagates.
… group The charmcraft default turns the managed group on, which CI cannot exercise: the integration tests run with fake OpenStack credentials, so the configurator blocked on an unreachable Keystone and the garm and garm-configurator suites failed at setup. An empty security-group-rules list now means "manage no group": no OpenStack call, no relation key, and the runners keep the project's default group. The integration fixtures use it, and the option description and reference docs document it as the opt-out for deployments that cannot reach the OpenStack API.
yanksyoon
commented
Oct 20, 2026
Second review pass: - Keep the unit status short: the exception carries the whole single-line reason for the log, and status_reason() truncates it to 60 characters for the status message. - Shorten the security-group-rules description and keep it to one example. - Drop the test-only connect parameter: the reconciler calls openstack.connect directly and the tests stub the module attribute. - Remove the single-use helpers (_rule_fields, _observed_rule_fields, _rule_identity, _desired_rule_fields, _age). A rule's identity is now the tuple of RULE_FIELDS read off SecurityGroupRule.neutron_fields, documented where RULE_FIELDS is declared.
Lead with the isolation it gives the scale sets and keep the lifecycle details to the reference page.
yanksyoon
commented
Oct 20, 2026
yanksyoon
commented
Oct 20, 2026
…he rule tests Final review: - Point the blocked status at debug-log rather than truncating the failure into it; the warning keeps the whole reason. - Reinstate _get_identity(rule) and rename the observed reader to _get_observed_identity, so the rule diff reads as identity against identity. - Use named arguments at the calls this change introduced. - Collapse the rule and duplicate-group unit tests into two parametrised tables of inputs and expectations. - Record next to the module and in the reference docs that the remote shell and debug-ssh sessions are outbound, so neither needs an ingress rule.
yanksyoon
marked this pull request as ready for review
October 20, 2026 10:17
yanksyoon
requested review from
cbartz,
florentianayuwono,
javierdelapuente,
weiiwang01 and
yhaliaw
as code owners
October 20, 2026 10:17
The rest of the suite would pass if the group were silently dropped, because Nova would fall back to the project's default group and the runner would work exactly as before. After the runner is online, check the tenant directly: the group the configurator manages for the scale set admits no ingress, and the runner's port carries that group and no other. Sweep the run's garm-<label> group in the existing runner-cleanup step, since the charm never deletes it by design.
yanksyoon
force-pushed
the
feat/isolated-security-group
branch
from
October 20, 2026 10:26
a89fdc5 to
c683d60
Compare
…e endpoint pin Review feedback on open threads: - Catch only the SDK's and keystoneauth's error bases. Transport and auth failures come from keystoneauth and never reach the SDK's base class, so both are needed; anything else now fails the hook instead of being reported as a blocked reconcile. keystoneauth1 becomes a declared dependency. - Let the connection close itself with `with`. - Drop `interface: "public"`: it is the SDK default, and the provider resolves the same endpoints from clouds.yaml. - Inline the observed-rule identity.
…e reconcile
Adversarial review found the observed-rule identity never matched: the SDK
resource names the ethertype `ether_type`, so every configured rule looked new
and each reconcile rewrote the whole group. Tests now build rules from the SDK
resource itself, and an idempotency case pins it.
Also from the review:
- Observe update-status, so a blocked unit retries when OpenStack comes back
instead of waiting for an unrelated event.
- Skip the reconcile when no garm-configurator relation is joined: nothing
consumes the id, and an image-credentials-only deployment should not gain an
OpenStack failure mode.
- Pass api_timeout, so a blackholed endpoint blocks the unit instead of
hanging the hook.
- Delete a surplus copy of a rule Neutron accepted twice, so the group really
holds exactly the configured rules.
- Reject an emptied option ("") rather than reading it as the opt-out, which
would silently drop the isolation.
- Fix the reference page: a list with no egress rule is rejected, not widened,
and reads are needed as well as writes.
cbartz
reviewed
Oct 20, 2026
cbartz
left a comment
cbartz
left a comment
Collaborator
There was a problem hiding this comment.
Isolation design looks right and the e2e assertion + group sweep are exactly what was needed — but I think the default rules still can't converge on a real Neutron, and there's no e2e run on the branch yet. I'd also suggest moving the group to the GARM charm; details below so you can decide either way.
Design: suggest moving the security group to the GARM charm
Not blocking on its own — happy to hear other views in the thread. Reasons:
- Lifecycle: the GARM charm already creates, drains and deletes scale sets, so it can delete the group after drain. The configurator can't know when the ports are gone, which is why the group is never deleted today (renames/removals orphan groups against quota, and e2e needs a sweep).
- Blast radius is already handled there:
ScalesetReconciler.reconcileisolates per-scaleset failures and re-raises at the end, so one tenant's OpenStack trouble doesn't stall the others. - Configurator stays a config broker: no
openstacksdk/cryptographybuild, no cloud writes. The GARM charm already receives the tenant credentials over the relation. - Cost: steady state is two list calls per scale set, writes only on drift.
- Taking up your offer to hard-code the rules (all egress, no ingress) would drop the option parsing/validation and nits 4–5 below.
Everything below applies wherever the code ends up.
To address before merge
- Default egress conflicts with Neutron's default rules. A new group comes with egress IPv4/IPv6 rules whose
remote_ip_prefixisNone. Neutron treats0.0.0.0/0,::/0andNoneas the same rule (_rule_to_keyinneutron/db/securitygroups_db.py), but the identity here compares them as different. So the reconcile tries to createegress 0.0.0.0/0, Neutron answersSecurityGroupRuleExists(409), and the unit stays blocked on every hook with the default config. Normalise those three values to one in both identities, and make the unit-test fake seed the default egress rules on create and reject duplicates, so the tests model this. - A green
garm_e2erun on this branch, linked in the PR (gh workflow run garm_e2e.yaml --ref feat/isolated-security-group), after 1. The new port/ingress assertion makes it the real check for the ticket's acceptance criteria.
Nits
security_groups()is unscoped: with admin-scoped credentials it lists every project's groups, so agarm-<name>elsewhere counts as a duplicate (picked or deleted). Filter bynameandproject_id=connection.current_project_idserver-side.- Changelog: call out the upgrade impact — the option is on by default, so after refresh the provider credentials create one security group per scale set (counts against the project's quota, default 10), and
[]opts out. charms.mdstill calls the option "optional"; it is on by default.- A rule with no
remote_ip_prefixis IPv4-only, but the option text says "default every address". Fail-closed, so only the wording needs fixing (or create both ethertypes). protocol: "icmpv6"with no/IPv4 prefix passes validation and is then rejected by Neutron, blocking the unit; reject it at config validation.- Document that scale-set names must be unique per tenant: two GARMs using the same name would share
garm-<name>and keep deleting each other's rules. - Listing
cryptographyundercharm-binary-python-packageswould likely make thelibssl-dev/pkg-configsource build unnecessary.
AI-generated message.
…an the group up A new security group already carries one any-address egress rule per ethertype, reported with no remote_ip_prefix, and Neutron's duplicate check treats that, 0.0.0.0/0 and ::/0 as the same rule. The identity compared the literal prefix, so the default configuration tried to create rules that were already there, got a 409 and blocked the unit on every hook. Both identities now normalise the prefix, and the test fake seeds the default egress rules and rejects duplicates the way Neutron does. Also from review: - Scope the group listing to the managed name and the current project, so admin-scoped credentials cannot mistake another project's group for a duplicate. - Delete the group when the option is cleared or the application is removed, unless a port still attaches it. Neither path raises: they run while the charm is being removed. - Reject icmpv6 without an IPv6 prefix, and icmp with one, at configuration time rather than letting Neutron refuse the rule later. - Install cryptography from wheels, which removes the source build and the libssl-dev project. - Changelog, option text and reference page: the option is on by default, its upgrade impact, a prefix-less rule's ethertype, and that scale set names must be unique per tenant.
The charm is a configurator that now performs an integrator job. Add an ADR that states the distinction, using Juju's definitions, and records the decision with its consequences and the alternatives considered — including moving the group to the GARM charm, which the review raised.
Member
Author
ADR added:
|
The configurator cannot know when a scale set's runner ports are gone, so it could never delete a group safely: renames and removals left groups behind against the project's quota. The GARM charm owns that lifecycle, so it takes over the group. Configurator: - keeps security-group-rules as configuration, validates it, and publishes the rules on the relation; it makes no OpenStack call and needs no permissions. - loses the reconciler, the cleanup hooks, openstacksdk and keystoneauth1, and the binary-package exemption. GARM charm: - creates garm-<scale set>, converges its rules to the published list, attaches the id through the security_groups extra spec, and deletes groups no scale set claims. - groups carry the owning application in their description, so two deployments sharing a project cannot delete each other's groups. - a failed reconcile fails closed: those scale sets are claimed but skipped, and the unit reports it after the active status rather than letting it be overwritten.
Update the reference page, the changelog and the ADR: the configurator owns the rules as configuration, the GARM charm owns the group because it owns the scale set lifecycle.
Member
Author
Rewritten: the GARM charm now owns the security group lifecycleActing on the review's design point and on the lifecycle gap, this branch is rewritten rather than extended.
The group name and the extra spec are unchanged, so an upgrade reuses the group the earlier version created instead of replacing it. An e2e run is in flight on this head; the previous one failed in Nova before any runner registered (the group was created and published correctly), so this run is also checking that failure was environmental. |
…he charm part The go-framework extension rejects any override of the charm part, so the binary-package exemption used on the configurator is not available here. charmcraft builds requirements.txt from source and the build environment has no OpenSSL headers, which cryptography needs, so a part of its own installs them into the shared build environment before the charm part builds.
Adversarial review of the move: - Groups live in a project, and several configurators can share one, so each tenant is reconciled with the union of its scale sets. Reconciling one configurator at a time made them delete each other's groups. - Every tenant is reconciled even with nothing wanted, so an opt-out deletes the group it created instead of leaking it. - A configurator that publishes no rules key at all (one predating the split) is left alone rather than swept, so a GARM-first upgrade keeps isolation. - Fail closed is decided on the spec: rules without an id skip the scale set, which covers a provider no credentials were read for and unparsable rules, and drops the skip set threaded through the scaleset reconciler. - Leader-only, so two units do not race group creation or deletion; a group a concurrent pass deleted is not an error. - Duplicate names are swept rather than hidden, rules are removed before they are added so a narrowing change is never briefly wider than the policy, and the removal path retries a group whose port is still being removed. - Tests: opt-out deletion, duplicates, two configurators in one project, a configurator without the key, and the id reaching the provider extra specs.
Two units racing would create duplicate groups and delete each other's, and a group a peer deleted came back as a reconcile failure that skipped the tenant. A non-leader now leaves the groups to the leader; its scale sets carry no group id, so the scaleset reconciler leaves those to the leader as well.
The group id is part of the desired state, so _build_desired_scalesets now ensures the groups and returns the specs with their ids, instead of the caller building the specs and then patching the ids in. The per-tenant loop stays a helper of its own.
charmcraft fetched these while packing locally, and they are not part of this change: the charm libs a charm uses are tracked from main. Removes the six files the branch added under charms/garm/lib/charms.
The flow now reads in the order the lifecycle runs: 1. _ensure_security_groups creates and converges every group the configurators describe, once per tenant. 2. _build_desired_scalesets builds the specs, each carrying the id of the group ensured for it. 3. ScalesetReconciler reconciles them. 4. _remove_orphaned_security_groups deletes the groups no scale set claims. The sweep moves out of reconcile() into its own method, so creating and deleting are separate steps: a group whose scale set is deleted in this pass is only unclaimed after step 3, which is why the sweep now runs last. A configurator that publishes no usable rules is reported once, from the pass that reads them, rather than twice.
The value the ensure step produced was a wrapper: a nested id map, a tenant lookup, the claim set and a status, with one accessor. It reads better as what it is - a map from (provider name, scale set name) to group id, returned beside the status - and the sweep now derives its claim set from the specs it is given, which are the same desired state step three reconciled.
…e with scale sets Two problems the review surfaced: - reconcile() still swept, so groups were deleted at ensure time as well as after the scaleset reconcile. Only the sweep deletes now, which is the ordering the pass documents: a group is unclaimed only once its scale set has been reconciled away. - the id map was keyed by (provider name, scale set name), but a scale set name is the identity a scale set has across the model - the scalesset reconciler dedupes on it - so the map is keyed by name alone. A duplicate group name now has every copy converged before the extra ones are swept: OpenStack allows duplicates, and the copy a runner is attached to must not keep stale rules. That also removes the separate duplicate list.
The sweep decided "claim" from a non-empty rules list, so a configurator that published rules which do not parse - or that predates the split - left its scale set unclaimed, and the pass swept the group the scale set was using. A spec now claims whenever it did not explicitly opt out, which is only an empty list.
Member
Author
|
The design of this depends on the fix of the provider reconciler. When the provider reconciler is gone, the security group cannot be deleted since the information for the provider will no longer be available. |
The integration run caught it: sweep() raised, and _reconcile_runners only caught GARM and template errors, so a tenant the charm could not reach left the unit in "error (hook failed)". The scale sets are already reconciled by then, so the sweep logs and returns nothing deleted, and the next pass tries again. An opt-out deployment is the case that exposed it: with no rules, the only OpenStack call left is the sweep for a group the charm may have created earlier, and a failure there must not turn a working deployment into a blocked one. It is logged at info instead of reported in the status.
11 of 12 tasks
Member
Author
|
We're dropping this architecture for relying on infrastructure setup. Closing. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this does
Gives each GARM scale set its own OpenStack security group, so a compromised runner cannot reach its peers. The configurator owns the rules as configuration; the GARM charm owns the group's lifecycle, because only it knows when a scale set and its runners are gone.
Draft. All review threads are resolved; this branch was rewritten after review to move the lifecycle into the GARM charm.
The split, and why
A group outlives the runners it was created for: OpenStack attaches it to a port for the life of the VM. The configurator sees the definition, not the runners, so it can only guess when deletion is safe — the earlier version of this PR left groups behind on every rename. The GARM charm retires a generation, drains it, removes its runners, and deletes the scale set only when GARM reports none left, which is exactly when the group can go.
Behaviour
security-group-rulesis on by default (all egress, no ingress), validated at the boundary, and published on the relation. An invalid list blocks the unit; a list without an egress rule is rejected.garm-<scale set>, keeps its rules equal to the published list (a rule not listed is removed, including one added by hand), and attaches it throughsecurity_groups.Compatibility with existing features
Nothing the charms provide needs an ingress rule.
enable-shellrides the agent connection the runner opens,debug-sshis an outbound tmate client, and GitHub, GARM, apt, mirrors, the proxy and OTEL are all outbound. The e2e suite runs the shell against a real runner with the group attached.Tests
charms/garm: 464 unit tests, including a newtest_security_groups.pythat models Neutron (default egress rules, duplicate rejection, foreign groups, in-use deletes);fmt,lint,complexity,staticclean.charms/garm-configurator: 76 unit tests, covering rule validation and publication; the cloud code and its dependencies are gone.tox -e garm-e2e): after the runner registers, the suite asserts the group admits no ingress and that the runner's port carries that group and no other. Run in flight on this head.make -C docs spellcheckandvaleclean.Known limits
Decision record
docs/adr/004_security_group_ownership.mdrecords the split with Juju's configurator and integrator definitions, the lifecycle reason, the consequences, and the alternatives — including the configurator-side management this PR first implemented. The general rule is the configurator/integrator page incanonical/platform-engineering-docs#333.