Skip to content

Merge remote-tracking branch 'origin/dev/v2.1' into refactor/canonical-path-classifiers - #478

Merged
scttbnsn merged 3 commits into
dev/v2.1from
refactor/canonical-path-classifiers
Sep 5, 2026
Merged

Merge remote-tracking branch 'origin/dev/v2.1' into refactor/canonical-path-classifiers#478
scttbnsn merged 3 commits into
dev/v2.1from
refactor/canonical-path-classifiers

Conversation

@scttbnsn

@scttbnsn scttbnsn commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

This is a behavior-preserving refactor, no functional change.

isNodeUpdatePath was defined identically in internal/filter and internal/ownership, and isLibpodPath was defined identically (just a different prefix-slash convention) in internal/filter and internal/responsefilter. Neither of those packages can host the shared copy for the others without an import cycle, since internal/ownership and internal/responsefilter both already import internal/filter. So this adds a new leaf package, internal/apipath, with no sockguard-internal imports, and moves both predicates there. The old package-local isNodeUpdatePath/isLibpodPath names stay as one-line wrappers, so every existing call site is unchanged. Separately, internal/ownership/paths.go had nine near-identical identifier extractors (containerIdentifier, execIdentifier, networkIdentifier, volumeIdentifier, serviceIdentifier, taskIdentifier, secretIdentifier, configIdentifier, nodeIdentifier) that all did the same prefix-strip-and-cut, differing only in the resource prefix and which collection-action names (create, prune, json) don't count as an identifier. Those now delegate to one parameterized resourceIdentifier helper, again with each of the nine kept as a one-line wrapper. imageIdentifier wasn't touched, its suffix-trimming for slash-containing image references doesn't fit the shared shape.

Tests: TestIsLibpodPath and TestIsNodeUpdatePath in the new internal/apipath package cover the moved predicates directly (TestIsLibpodPath moved over from internal/filter's libpod_normalize_test.go; TestIsNodeUpdatePath is new, since neither internal/filter nor internal/ownership had a standalone test for it before, only coverage through larger flows). The existing tests that exercised these predicates as part of broader behavior keep passing unchanged: internal/filter's TestLibpodMatchersNeverMatchDockerPathsAndViceVersa and TestLibpodPathAdversarial, internal/responsefilter's TestLibpodPathPredicates, and internal/ownership's TestDockerCollectionRoutesAreNotResourceIdentifiers, TestContainerIdentifierCollectionKeywordBoundaries, TestNetworkIdentifierCollectionKeywordBoundaries, TestVolumeIdentifierCollectionKeywordBoundaries, TestNodeIdentifierEmpty, and TestIdentifierHelpers. CHANGELOG entry is under Changed.

Changelog

  • ✨ Added internal/apipath with shared IsLibpodPath and IsNodeUpdatePath predicates.
  • 🔧 Changed filter, ownership, and response-filter packages to use the shared predicates.
  • 🔧 Changed nine ownership resource identifier extractors to use the parameterized resourceIdentifier helper.
  • 🔧 Preserved existing local wrappers and function signatures.
  • 🔧 Preserved imageIdentifier.
  • ✨ Added direct table-driven tests for shared predicates.
  • 🔧 Preserved broader path behavior tests.
  • ✨ Added a CHANGELOG entry under “Changed.”

Concerns

  • Verify all callers pass normalized paths to apipath.IsLibpodPath and apipath.IsNodeUpdatePath.
  • Confirm wrapper-level behavior remains covered after removing TestIsLibpodPath.
  • Confirm resourceIdentifier excludes every collection action previously excluded by the nine dedicated implementations.

isNodeUpdatePath was defined identically in internal/filter and
internal/ownership, and isLibpodPath was defined identically (modulo a
prefix-slash convention difference) in internal/filter and
internal/responsefilter. Neither package can host the canonical copy for
the others without an import cycle, since internal/ownership and
internal/responsefilter both already import internal/filter. A new leaf
package, internal/apipath, has no sockguard-internal dependencies, so all
three can import it. The old package-local names stay as one-line wrappers
so call sites are unchanged.
…ractors

containerIdentifier, execIdentifier, networkIdentifier, volumeIdentifier,
serviceIdentifier, taskIdentifier, secretIdentifier, configIdentifier, and
nodeIdentifier in internal/ownership/paths.go each repeated the same
prefix-strip-and-cut shape, differing only in the resource prefix and which
collection-action names (create, prune, json) are excluded from being read
as an identifier. They now delegate to one parameterized
resourceIdentifier helper; each of the nine keeps its name and signature as
a one-line wrapper, so no call site changes. imageIdentifier is untouched:
its suffix-trimming logic for slash-containing image references does not
fit the shared shape.
@vercel

vercel Bot commented Sep 5, 2026

Copy link
Copy Markdown

Deployment failed for project sockguard-website with the following error:

Resource is limited - try again in 24 hours (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/codeswhat?upgradeToPro=build-rate-limit

@vercel

vercel Bot commented Sep 5, 2026

Copy link
Copy Markdown

Deployment failed for project sockguard-website with the following error:

Resource is limited - try again in 1 day (more than 100, code: "api-deployments-free-per-day").

Learn More: https://vercel.com/codeswhat?upgradeToPro=build-rate-limit

@biggest-littlest biggest-littlest left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed against the CHANGELOG entry and the diff; CI green outside the qlty/Vercel quota noise.

@ALARGECOMPANY ALARGECOMPANY left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Reviewed against the CHANGELOG entry and the diff; CI green outside the qlty/Vercel quota noise.

@scttbnsn
scttbnsn merged commit 9db817a into dev/v2.1 Sep 5, 2026
16 of 21 checks passed
@scttbnsn
scttbnsn deleted the refactor/canonical-path-classifiers branch September 5, 2026 14:20
@coderabbitai

coderabbitai Bot commented Sep 5, 2026

Copy link
Copy Markdown

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Team

Run ID: 66bdb95f-5129-4b7c-a4e8-e3d360e7a63a

📥 Commits

Reviewing files that changed from the base of the PR and between 7306545 and 1cc2b98.

⛔ Files ignored due to path filters (1)
  • CHANGELOG.md is excluded by !CHANGELOG.md
📒 Files selected for processing (7)
  • app/internal/apipath/apipath.go
  • app/internal/apipath/apipath_test.go
  • app/internal/filter/libpod_normalize.go
  • app/internal/filter/libpod_normalize_test.go
  • app/internal/filter/node.go
  • app/internal/ownership/paths.go
  • app/internal/responsefilter/libpod_filter.go

📝 Walkthrough

Walkthrough

The change adds a leaf apipath package with shared predicates for normalized libpod and node-update paths. Table-driven tests cover valid, invalid, boundary, and empty inputs. The filter and response-filter packages delegate path checks to these predicates. The ownership package centralizes resource identifier extraction and preserves action exclusions.

Suggested labels: second-opinion

✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch refactor/canonical-path-classifiers

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

second-opinion Summons Greptile as an independent second-opinion reviewer

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants