Skip to content

Never let an accept file stop the agent - #130

Merged
shawnburke merged 3 commits into
mainfrom
grpc/accept-2-tolerance
Aug 27, 2026
Merged

Never let an accept file stop the agent#130
shawnburke merged 3 commits into
mainfrom
grpc/accept-2-tolerance

Conversation

@shawnburke

@shawnburke shawnburke commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Context

This is a stack of PRs to get the new gRPC tunnel up to spec with snyk-broker --- with no snyk-broker in the picture we have to implement all of the accept file parsing and routing in Go.

Specifically I had Claude audit all of the test cases in snyk-broker, figure out which ones we care about, and make sure that the new tunnel supports them properly. In addition, this folds in the new wildcards support.

This PR just makes sure that we have rigourous parsing of accept files and WARN on things that we don't support - snyk-broker has lots of stuff we don't care about but hard to know if it's in some customer file. No real functional change here.


⚠️ Heads up for re-review: this picked up a second commit after approval. main is currently broken and this fixes it — see "Also fixes main" below. The warn-and-ignore commit is unchanged.

Claude Stuff

Stack 2/4. 1/4 (#129) has merged, so this sits directly on main.

Enabling the tunnel switches deployments that are running on snyk-broker today, and that switch has to be transparent: a file the broker accepts must still start. Constructs the Router cannot carry are warned about and ignored, never refused.

What gets warned about

Constructs snyk-broker honours that the Router does not implement — body and query valid filters, requiredCapabilities. Ignoring one widens the rule, so the warning says exactly that rather than leaving an operator to discover it from traffic.

Inbound public rules are warned about at parse and ignored. They can't affect outbound routing, and enough accept files carry a block copied from a snyk-broker config that refusing them would break working deployments over a section that routes nothing. The warning names the section and says support for it is going away. An empty block stays silent — that's the shape Render itself emits, and warning about it would train everyone to ignore the warning.

Where the warnings live, and why it matters

Rule warnings fire at Router construction, not at parse. Parsing is shared with the snyk-broker path, where the Node broker honours all of these — warning at parse would tell an operator their working rule is being dropped when it isn't.

TestSnykBrokerConstructsStillParse pins that boundary from the other side: those files parse, and parse silently.

TestNoAcceptFileConstructStopsTheAgent pins the headline rule, including a file that uses every unsupported construct at once.

Also here

TestOriginContract. Origin() is the one accessor this package shares with the reflector — relay_instance_manager.go reads it to detect a wildcard rule, build the proxy URI and report a bad origin. It's pinned here, ahead of the routing work in 3/4 and 4/4 that refactors it, so a change to the shared accessor can't quietly alter what the reflector sees.


Also fixes main (second commit)

main does not compile as of #127 (f3b10e8):

server/snykbroker/acceptfile/router.go:95:21: multiple-value
ruleHeaders.ToStringMap() (value of type (map[string]string, error))
in single-value context

#127 changed ResolverMap.ToStringMap to return an error — so a credential provider that produces no value stops the request instead of leaving its placeholder to travel upstream as the credential — and updated the reflector, but not acceptfile/router.go, which calls the same method on the tunnel path. The PR's CI evidently ran against a base that predated that call site.

Fixed here rather than in a separate PR so the unbreak rides the first thing that merges.

The handling matches the reflector's serve(): log the failure against the rule and refuse. A plain error lands in the default arm of grpctunnel's RouteError mapping, which is a 502 — the same status the reflector answers, so both transports refuse a failed provider identically.

Two tests added, since the tunnel path had no coverage of provider failure in either direction — which is part of why the stale call site read as fine:

  • a failing provider makes Route refuse (verified red when the error is swallowed again)
  • a succeeding one still reaches the upstream, so the first is refusing on the failure and not merely on having a plugin

make test is green on this branch, and on main with just this one hunk applied.


The stack

PR
1 #129 flake fixes — merged
2 this one warn-and-ignore + unbreak main
3 #131 matching parity
4 #128 wildcard origins, auth, pools

🤖 Generated with Claude Code

https://claude.ai/code/session_01DFYsRuHoCbCBWEFvuaj9kV

@shawnburke
shawnburke marked this pull request as ready for review August 27, 2026 00:15
@shawnburke
shawnburke force-pushed the grpc/accept-2-tolerance branch from 1aa4aa6 to 02abc2f Compare August 27, 2026 00:32
Base automatically changed from grpc/accept-1-flake to main August 27, 2026 01:47
Enabling the tunnel switches deployments that are running on snyk-broker
today, and that switch has to be transparent: a file the broker accepts
must still start. Constructs the Router cannot carry are warned about and
ignored rather than refused.

What lands here are the ones snyk-broker honours and the Router does not
implement — body and query "valid" filters, requiredCapabilities. Ignoring
one widens the rule, so the warning says exactly that rather than leaving
an operator to discover it from traffic.

Inbound "public" rules are warned about at parse and ignored. They cannot
affect outbound routing, and enough accept files carry a block copied from
a snyk-broker config that refusing them would break working deployments
over a section that routes nothing. The warning names the section and says
support for it will be removed. An empty block stays silent — that is the
shape Render itself emits, and warning about it would train everyone to
ignore the warning.

The rule warnings are at Router construction, not at parse. Parsing is
shared with the snyk-broker path, where the Node broker honours these, so
warning at parse would tell an operator their working rule is being
dropped when it is not. TestSnykBrokerConstructsStillParse pins that
boundary from the other side: those files parse, and parse silently.

Also adds TestOriginContract. Origin() is the one accessor this package
shares with the reflector — relay_instance_manager.go reads it to detect a
wildcard rule, build the proxy URI and report a bad origin — and it is
pinned here, ahead of the routing work that refactors it, so a change to
the shared accessor cannot quietly alter what the reflector sees.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DFYsRuHoCbCBWEFvuaj9kV
@shawnburke
shawnburke force-pushed the grpc/accept-2-tolerance branch from 02abc2f to 73ea43f Compare August 27, 2026 01:47
aszarama
aszarama previously approved these changes Aug 27, 2026
claude added 2 commits August 27, 2026 06:09
#127 changed ResolverMap.ToStringMap to return an error so a provider that
produces no value stops the request instead of leaving its placeholder in the
header, but it did not update acceptfile/router.go, which calls that method on
the tunnel path. main does not compile:

    server/snykbroker/acceptfile/router.go:95:21: multiple-value
    ruleHeaders.ToStringMap() (value of type (map[string]string, error))
    in single-value context

Handle the error the way the reflector's serve() does: log the failure against
the rule and refuse. A plain error lands in the default arm of grpctunnel's
RouteError mapping, which is a 502 — the same status the reflector answers, so
both transports refuse a failed provider identically.

The gap was a compile error rather than a behaviour one, so a test would not
have prevented it; the tunnel path had no coverage of provider failure either
way, which is what let the call site read as fine. Added both directions, and
confirmed the failing one goes red when the error is swallowed again.

Merges current main in, so this carries the fix at the bottom of the stack.
@shawnburke

Copy link
Copy Markdown
Collaborator Author

@aszarama somehow the prior PR broke main so i put the fix in here

@shawnburke
shawnburke merged commit 23066a3 into main Aug 27, 2026
20 checks passed
@shawnburke
shawnburke deleted the grpc/accept-2-tolerance branch August 27, 2026 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants