Read groups from the groups service, and fix nested-group expansion - #8
Open
johnworth wants to merge 14 commits into
Open
Read groups from the groups service, and fix nested-group expansion#8johnworth wants to merge 14 commits into
johnworth wants to merge 14 commits into
Conversation
getGroupMembers followed member.Name when descending into a nested group. That works against iplant-groups only because a group member's name is its full colon-delimited Grouper path, which doubles as a lookup key. The groups service names a group by its short name and keys membership by ID, so the same recursion would 404 on the first nested group and fail the propagation. A member entry already carries the nested group's own ID in its subject ID, and iplant-groups has served /groups/id/:id/members all along, so keying the whole recursion on IDs works against both backends. Verified against the local cluster's iplant-groups: the depth-2 chain Field Team -> Genomics Lab -> msmith's default list returns the same three members before and after. Adds the repo's first test, covering nesting at depth 0, 1, and 2 against a fake service that serves by-ID lookups only, so a regression to name-based recursion fails rather than silently returning short membership. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Repoints the client at the groups service's routes: /groups/lookup for the de-users group, /groups/:id and /groups/:id/members in place of the /groups/id/... forms, and GET / for the status check. The status check now requires the reported database connectivity rather than accepting any 200, because the groups service answers its status endpoint even when it cannot reach group storage. Keycloak connectivity is deliberately not fatal: without it names degrade, but membership -- all this service reads -- still resolves. The crawler now paginates. It listed groups in one unpaginated request, which the groups service caps at 1000; production carries 2,583, so the crawl would have propagated ACLs for the first page and silently ignored the rest. The folder/prefix scoping is gone with it: the service holds exactly one deployment's group data, so there is nothing left to filter on. Config keys move from iplant_groups.* to groups.*, dropping folder_name_prefix and replacing public_group with de_users_group, which is now a bare name resolved by type rather than a colon-delimited path. GetGroupByName is deleted -- it was already unreachable, and the groups service has no by-name lookup for it to use. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
johnworth
marked this pull request as ready for review
August 5, 2026 15:08
A public group whose membership is not public answers a non-admin caller with an empty list. data-info's member update replaces rather than merges, so propagating that empties the iRODS group -- and the log line is "Updated group X with 0 members", indistinguishable from the benign case where iRODS has no accounts for the members. The groups service now marks a withheld list, and this refuses it with a message naming the cause: the configured groups.user is not an admin of the groups service. Also replaces the embedded default config's iplant_groups block, which no longer satisfied Validate() after the switch to groups.*, so any deployment relying on defaults failed startup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Unchecked Close on the groups client, and the source-id chain reads as a tagged switch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ListAllGroups asked for 1000 groups per page and stopped on the first page shorter than that. The 1000-row cap belongs to the groups service, though, so if a future release lowers it, every page comes back "short" and the crawl silently truncates to the first response -- groups past the cap would just stop propagating, with no error anywhere. Advance the offset by the number of groups each page actually returned and terminate only on an empty page, so the crawl tracks whatever cap the server enforces. The fake servers in the tests hard-cap the request count so an offset-ignoring client fails fast instead of looping. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
getGroupMembers recursed into nested groups with no visited set. The groups service API can't create a membership cycle today, but a direct SQL edit can, and one cycle would spin the recursion forever issuing HTTP GETs -- and with the AMQP consumer concurrency at 1, that halts all group propagation, not just the poisoned group. Thread a visited set through the recursion and silently skip any group ID already seen. Silent is the right response because the same shape occurs legitimately in a diamond (two parents sharing a subgroup), whose members were already collected on the first visit. The fake groups server in the tests now hard-caps its request count so an unterminated recursion fails the suite in milliseconds, not at the test timeout. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The groups service answers a non-admin's listing requests with a 200 and an access-filtered page -- there is no marker distinguishing it from a complete listing. A propagator configured with a groups user missing from the service's admin-users list would therefore start cleanly, crawl a filtered listing, and groups would silently stop propagating. After resolving the de-users group ID at startup, verify that the same group shows up in a system-type group listing (the same listing endpoint the crawl uses, narrowed to the smallest slice that must contain it) and refuse to start otherwise, naming the probable cause in the error. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Neither package-level http.Client had a Timeout, so a downstream that accepts the connection but never answers would hang a request forever. The stakes went up on this branch: the AMQP consumer runs with concurrency 1, so one wedged request to the groups service or data-info stops all propagation, and only the crawl's context (if any deadline is ever attached upstream) could unstick it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The early return for a non-2xx status ran before the deferred close was registered, leaking the response body -- and with it the underlying connection -- every time the groups service answered with an error. Register the close as soon as the request succeeds so every exit path is covered. (The data-info client already had this right.) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The err variable in CrawlGroups was declared outside the loop and only assigned when a group was actually published. On the iteration that skips the public group, the error check re-read whatever the previous iteration left behind, logging a stale publish failure against the wrong group and re-recording it as the overall error. Scope the error to the publish call so each check sees only its own result. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Status and the Redacted field carried doc comments while Group, GroupList, GroupMembers, and Subject had none; bring the older types up to the same standard with one-sentence comments. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The unchecked deferred Close was the one lint issue left in a file this branch touches; discard the error explicitly to match how the groups client handles it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The group listing crawl ended only on an empty page, so a service that stopped honoring offset would wedge the single AMQP consumer goroutine while the accumulated slice grew, with nothing logged. Bound it: a page that contributes no group the crawl has not already seen, or a crawl that runs past maxListPages, is an error naming the likely cause. The startup admin check now pages the system listing through the same helper instead of judging admin standing from a single response. An unrecognized member source id was logged and skipped, which handed data-info a short member list; that update replaces rather than merges, so the members would have been removed from the iRODS group under a log line reading like an ordinary run. Refuse the propagation instead, as the redacted-list guard already does. Nested expansion also returned a user once per path that reached them, so deduplicate the list before returning it. Scope the crawl to the groups users create. The listing that replaced the prefix-scoped Grouper search carries the DE's own system groups too, and publishing for those has the propagator create an @grouper-<id> iRODS group per system group that nothing ever removes. Skipping the system type in the crawl keeps this to one listing request and, unlike naming the three user-created types, does not silently drop a group type the groups service adds later. Retire the remaining Grouper-era wording and names the switch left behind: the README summary, the propagation header comment, two error strings, and the crawler's publicGroup field, which holds the de-users group ID. The @grouper- iRODS name prefix stays; existing iRODS groups carry it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wjd21NTp4Ead7JYhx5sQUT
The groups service now caps a member listing and refuses an unpaged request for a group larger than the cap, rather than truncating it. Propagation needs the whole membership -- the data-info update replaces rather than merges, so a short list removes people from the iRODS group -- so ask for it a page at a time. Termination mirrors ListAllGroups: the offset advances by what the pages held and a page contributing no new member ends the crawl, because stopping at a page shorter than the requested size would truncate silently against a service whose own cap is lower than what we asked for. The reported total ends the crawl one request earlier when it is present, and a crawl that ends disagreeing with it is refused instead of propagated. Redaction is read from the page as a property of the group: an empty member list flagged redacted means the membership was withheld, not that the group is empty. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Wjd21NTp4Ead7JYhx5sQUT
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
1 participant
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.
Part of the Grouper removal: group-propagator stops reading iplant-groups and reads the new
groupsservice instead. Depends on the groups service being deployed, so this stays a draft until the cutover window.Two of these are defects in the current code, not consequences of the migration. Both produce wrong iRODS ACLs rather than errors, so neither would have shown up as a failure.
Nested-group expansion recursed by name
getGroupMembersfollowedmember.Namewhen descending into a nested group. That works against iplant-groups only because a group member's name there is its full colon-delimited Grouper path, which happens to double as a lookup key. The groups service names a group by its short name, so the same recursion would 404 on the first nested group.Both backends have served membership by ID all along (
/groups/id/:id/memberson iplant-groups), so keying the whole recursion on IDs works against either. This half is deployable today, independently of the cutover.Verified against the local cluster in both directions: the depth-2 chain
Field Team → Genomics Lab → msmith's default listreturns the same three members before and after the change, and returns all three through the deployed groups service — including the member reachable only at depth 2.The crawl was unpaginated
CrawlGroupslisted groups in a single request. The groups service caps one listing at 1000; production carries 2,583 groups. The crawl would have propagated ACLs for the first page and silently ignored the rest — the same silent-truncation shape that made the abandoned Keycloak-backed design unworkable.Everything else
iplant_groups.*togroups.*.folder_name_prefixis gone (the service holds exactly one deployment's group data, so there is nothing left to scope by) andpublic_groupbecomesde_users_group, a bare name resolved by type rather than a colon path.GetGroupByNamedeleted — already unreachable, and the groups service has no by-name lookup for it to use.Tests
The repo had none. There are now table-driven tests covering nesting at depth 0, 1, and 2 against a fake service that serves by-ID lookups only, so a regression to name-based recursion fails rather than silently returning short membership; and pagination at production scale, asserting each group appears exactly once in order.
Deployment config: cyverse-de/deployments#86.