PR 2: Add Alcatraz on Hoop - #1617
Open
matheusfrancisco wants to merge 5 commits into
Open
Conversation
matheusfrancisco
added
the
minor
Contributor
Migration Safety AnalysisNo database migrations were changed in this PR. Safe to deploy to sandbox. |
Contributor
📋 API ChangelogAPI Changelog unknown vs. unknownAPI ChangesGET /serverinfo
|
PR Summary by QodoWire org-gated Alcatraz DLP provider into gateway and agent
AI Description
Diagram
High-Level Assessment
Files changed (19)
|
Code Review by Qodo
1. Unnecessary GCP creds forwarded
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| @@ -447,6 +447,7 @@ func getAISessionAnalyzerParams(pctx *plugintypes.Context) (*pb.AISessionAnalyze | |||
| func (s *Server) processClientPacket(stream *streamclient.ProxyStream, pkt *pb.Packet, pctx plugintypes.Context) error { | |||
| switch pb.PacketType(pkt.Type) { | |||
| case pbagent.SessionOpen: | |||
There was a problem hiding this comment.
1. Unnecessary gcp creds forwarded 🐞 Bug ⛨ Security
When an org is switched to the Alcatraz provider via feature flag, the session-open path still forwards GOOGLE_APPLICATION_CREDENTIALS_JSON to the agent even though Alcatraz requires no credentials, increasing secret exposure surface. This happens because the effective provider is now org-scoped, but credential propagation remains unconditional.
Agent Prompt
## Issue description
`gateway/transport/client.go` resolves the effective DLP provider per-org (`services.DLPProviderForOrg`) but still forwards GCP DLP credentials to the agent regardless of that effective provider. When the org flag selects `alcatraz`, the gateway should not send `DlpGcpRawCredentialsJSON`/`SpecAgentGCPRawCredentialsKey` because Alcatraz needs no credentials.
## Issue Context
This PR introduces per-org provider override (Alcatraz) without changing how credentials are forwarded. As a result, enabling Alcatraz for one org can still distribute GCP service-account JSON to that org’s agent sessions.
## Fix Focus Areas
- gateway/transport/client.go[447-605]
- gateway/services/providers.go[31-39]
### Concrete fix
1. In `processClientPacket` (SessionOpen):
- Only set `spec[pb.SpecAgentGCPRawCredentialsKey]` when `dlpProvider == "gcp"`.
- Only set `AgentConnectionParams.DlpGcpRawCredentialsJSON` when `dlpProvider == "gcp"`.
- Similarly, only populate Presidio URLs when `dlpProvider == "mspresidio"`.
2. Add/extend a unit/integration test that asserts:
- With `dlpProvider == "alcatraz"`, the session-open payload/spec does **not** contain GCP credential fields.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // the flag off, such sessions fail closed. ALCATRAZ_NER_MODEL_PATH points at | ||
| // a local model directory for air-gapped agents; when unset the default | ||
| // model is downloaded on first use. | ||
| func configureAlcatrazNer() { |
There was a problem hiding this comment.
2. Ner flag startup race 🐞 Bug ☼ Reliability
The new Alcatraz NER backend fails closed when experimental.alcatraz_ner is not yet present in featureflagstate, but that state is only populated after a FeatureFlagUpdate packet arrives. Because the gateway publishes the agent stream as online before sending the initial flag snapshot, the first session(s) that require NER (PERSON/LOCATION/NRP) can be incorrectly refused immediately after agent connect/reconnect even when the flag is enabled.
Agent Prompt
## Issue description
The new Alcatraz NER provider is gated by `featureflagstate.IsEnabled("experimental.alcatraz_ner")`. `featureflagstate` starts empty and is only updated when the agent receives a `FeatureFlagUpdate` packet. There is a startup window where the gateway can route `SessionOpen` traffic to the agent before the initial feature-flag seed has been delivered/processed, so the first NER-using session may fail closed incorrectly.
## Issue Context
- `featureflagstate` defaults unknown flags to `false`.
- The gateway stores the agent stream in `agentStore` before sending the initial `FeatureFlagUpdate` seed.
- SessionOpen sends use the stored stream immediately.
## Fix Focus Areas
- agent/main.go[44-63]
- agent/controller/featureflagstate/featureflagstate.go[11-39]
- gateway/transport/agent.go[29-54]
- gateway/transport/streamclient/agent.go[109-126]
### Concrete fix options (pick one)
**Option A (preferred): publish-after-seed**
1. Refactor agent connection subscribe flow so the stream is not inserted into `agentStore` (and not considered online) until after the initial `FeatureFlagUpdate` seed has been successfully sent.
2. Alternatively, keep it in the store but mark it “not ready”; gate `ProxyStream.IsAgentOnline()` / `SendToAgent` on readiness.
**Option B: seed-on-session-open**
Include the feature-flag snapshot (or just `experimental.alcatraz_ner`) in `AgentConnectionParams` / SessionOpen and update `featureflagstate` before any provider checks.
Add a test/repro harness that opens a session immediately after agent connect with NER types enabled and asserts it does not fail due to missing flag seed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
p3rotto
approved these changes
Jul 21, 2026
racerxdl
approved these changes
Jul 21, 2026
racerxdl
left a comment
racerxdl
left a comment
Contributor
There was a problem hiding this comment.
I see that the default model is set to empty. Does that spawn a default internal model or just doesnt load anything?
Just thinking that maybe we should ship a default model with alcatraz.
/serverinfo reported the raw DLP_PROVIDER env while the rest of the backend already resolves the provider per organization (services.DLPProviderForOrg, used by the transport and every data-masking handler). An org running alcatraz — whether by env or by the experimental.alcatraz_dlp flag — was therefore told it had no usable provider, and the Live Data Masking page blocked the configure path with a Google Cloud DLP deprecation notice that did not apply. has_redact_credentials had the same problem: it was computed once at startup from env only, so a flag-enabled org saw false while masking was active. It now reuses services.CheckRedactProviderForOrg. serverInfoData was a package-level struct that Get mutated per request, so org-scoped fields (feature flags, and now the provider) could leak between concurrent requests from different orgs. Get now mutates a local copy; the package-level value keeps only env-derived fields. On the UI, the promotion page treats alcatraz like mspresidio (both drive masking from data-masking rules) and shows the GCP deprecation text only when the provider really is gcp — an unset provider gets the docs link with no misleading message.
Conflicts were confined to dependency bookkeeping and generated output: - agent/client/gateway go.mod + go.sum: main had bumped alcatraz to v0.7.0 while this branch is on v0.14.1. Resolved by taking main's files wholesale (they carry the 16 commits' new requirements) and recomputing each module with go mod tidy, rather than hand-merging the requirement lists. - gateway/api/openapi/openapiv3.json: minified single-line JSON, not mergeable by hand. Regenerated with swag + openapi-gen; the redact_provider enum keeps gcp/mspresidio/alcatraz. go.work moves to go 1.26.5. This is not toolchain drift: alcatraz/ner v0.14.1 declares go 1.26.5, so libhoop (which imports it) and every module above it must match. CI asks for go-version >=1.26.0 and so needs no change. The MCP gateway from main (#1661) imports libhoop/agent/mcpadapter, which landed on libhoop after this branch was cut; libhoop's add-alcatraz-agent branch was merged with its own main to match.
sandromello
reviewed
Aug 11, 2026
Comment on lines
+57
to
+58
| if !featureflagstate.IsEnabled(alcatrazNerFlagName) { | ||
| return nil, fmt.Errorf("the NER module is disabled (enable the %s feature flag)", alcatrazNerFlagName) |
Contributor
There was a problem hiding this comment.
It doesn't make sense to add feature flag if you have other things to enable the feature (e.g.: env ALCATRAZ_NER_MODEL_PATH, DLP_PROVIDER). This just adds unecessary friction to users
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
5 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.
📝 Description
Wires the new in-process Alcatraz DLP provider (libhoop
redactor/alcatraz)into the gateway and agent. Rollout is gated per organization by two feature
flags, both default-off:
experimental.alcatraz_dlpswitches an org to thealcatraz provider without changing
DLP_PROVIDER, andexperimental.alcatraz_nerlets the agent load the in-process ONNX NER modelso statistical entity types (PERSON, LOCATION, NRP) work. With the NER flag
off, sessions requesting those types fail closed with a remediation message —
no silent masking gaps.
🔗 Related Issue
Fixes #
🚀 Type of Change
📋 Changes Made
common/featureflag: registerexperimental.alcatraz_dlp(gateway) andexperimental.alcatraz_ner(agent), bothDefault: false.gateway/services/providers.go: newDLPProviderForOrg— the org flagtakes precedence over
DLP_PROVIDER;CheckRedactProviderForOrgacceptsalcatraz (no credentials needed); updated
ErrRedactProviderMissingtext.All data-masking mutation paths (connections, datamasking, rulepacks, MCP
tools) now validate through the org-scoped check.
gateway/appconfig:DLP_PROVIDER=alcatrazcounts as a configuredmasking provider.
gateway/transport/client.go: sessions resolve the effective provider perorg and forward data-masking entity types to alcatraz sessions.
agent/main.go: registers the NER provider at startup; theexperimental.alcatraz_nerflag is checked lazily per session (flag statearrives via the existing
FeatureFlagUpdatepacket), so toggling it needsno agent restart.
ALCATRAZ_NER_MODEL_PATH(optional env) points at alocal model directory for air-gapped agents.
ALCATRAZ_NER_MODEL_PATH; gatewaychart and
.env.sampledocument the provider and flags.go.mod/go.sumbumps in agent/client/gateway for the alcatraz modules.🧪 Testing
Test Configuration:
Tests performed:
common/featureflag,agent/controller/featureflagstate, libhoop redactor suites)✅ Checklist
📄 Additional Notes
everything off, so default behavior is unchanged.
flag stops new NER sessions but does not unload the model until the agent
restarts.
ALCATRAZ_NER_MODEL_PATHis reflected in the agent helm chart(
secret-config.yaml,values.yaml) and.env.sampleper repo convention.