use GET for enroll status polling #297 by IbrahimAhmed8 · Pull Request #301 · google/sam

IbrahimAhmed8 · 2026-08-24T13:50:44Z

clients used POST instead of GET for /enroll/status causing 405s.

switched clients to use GET
put peer_id in the query string
removed unused payloads and headers
renamed a url var to fix shadowing

fixes #297

google-cla · 2026-08-24T13:50:55Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

gemini-code-assist

Code Review

This pull request refactors the enrollment status polling mechanism in both internal/node/enroll.go and internal/router/router.go by replacing the POST requests (which sent protobuf payloads) with GET requests that pass the peer ID as a query parameter. A review comment points out that the implementation in internal/router/router.go uses client.Get instead of http.NewRequestWithContext, which ignores the request context, and fails to query-escape the peer ID. The reviewer suggested a code block to resolve these issues.

gemini-code-assist · 2026-08-24T13:51:28Z

 				return r.ctx.Err()
 			case <-ticker.C:
-				statusResp, err := client.Post(r.config.ControlPlaneURL+"/enroll/status", "application/x-protobuf", bytes.NewReader(statusData))
+				statusResp, err := client.Get(r.config.ControlPlaneURL + "/enroll/status?peer_id=" + peerID.String())


Using client.Get here ignores the request context (r.ctx), meaning that if the context is canceled while the HTTP request is in-flight, the request will not be aborted immediately and will instead wait for the client's 30-second timeout.

Additionally, peerID.String() is appended directly to the URL without query escaping, which is inconsistent with the implementation in internal/node/enroll.go and could lead to issues if the peer ID format contains special characters.

We should use http.NewRequestWithContext with r.ctx and url.QueryEscape instead. Note that you will need to import "net/url" in this file.

Suggested change

statusResp, err := client.Get(r.config.ControlPlaneURL + "/enroll/status?peer_id=" + peerID.String())

req, err := http.NewRequestWithContext(r.ctx, "GET", r.config.ControlPlaneURL+"/enroll/status?peer_id="+url.QueryEscape(peerID.String()), nil)

if err != nil {

logger.Warnf("failed to create enrollment status request: %v", err)

continue

}

statusResp, err := client.Do(req)

aojea · 2026-08-24T16:56:36Z

Things I specifically checked and am happy with:

Renaming the url local to enrollURL. Good catch.
Switching the router to http.NewRequestWithContext(r.ctx, ...) is an
improvement: the previous client.Post ignored cancellation entirely
and would hang for the full 30s client timeout on shutdown.
The Gateway route is type: Exact, value: /enroll/status
(.github/k8s/sam-control-plane-template.yaml:215).

Three changes before we merge

This needs a test

A verb mismatch between two components belongs at the integration
level, in tests/integration/: start a control plane with
AutoApproveEnrollment = false, run the real client polling path, approve through the
admin API, and assert the client ends up holding a Biscuit.

We already have a test that calls the handler is what we already have, and it is what missed this.

router.go: the new error branch never terminates

    req, err := http.NewRequestWithContext(...)
    if err != nil {
        logger.Warnf("failed to create enrollment status request: %v", err)
        continue
    }

NewRequestWithContext fails only on a malformed method or URL, which is
deterministic — so a bad ControlPlaneURL now logs a warning every 2s forever with no
exit path. enroll.go returns an error in the identical situation.

The URL is loop-invariant in both files. Build it once above the for/select and
return the error, which fixes the spin and removes the duplicated per-tick work:

statusURL := r.config.ControlPlaneURL + "/enroll/status?peer_id=" + url.QueryEscape(peerID.String())
for {
    select {
    ...
    case <-ticker.C:
        req, err := http.NewRequestWithContext(r.ctx, http.MethodGet, statusURL, nil)
        if err != nil {
            return fmt.Errorf("failed to create status request: %w", err)
        }

Please make enroll.go symmetric. While you're there, use http.MethodGet rather
than the "GET" string literal in both files.

api.EnrollmentStatusRequest seems now dead. Check if we no longer need and we can delete it. Just check you recreate the protos

IbrahimAhmed8 · 2026-08-24T18:53:15Z

thanks @aojea. I pushed the changes:

moved the url build out of the loops and used http.MethodGet
router.go error handling now matches enroll.go
dropped the dead EnrollmentStatusRequest and regenerated protos
added the integration test

i also set the protoc version string to v3.21.12 so the verify passes.
it looks like the bats and e2e android jobs are failing to start because of a github billing limit on this repo

aojea · 2026-08-25T09:42:36Z

it looks like the bats and e2e android jobs are failing to start because of a github billing limit on this repo

@IbrahimAhmed8 can you please rebase to fix that?

aojea · 2026-08-25T09:46:41Z

/gemini review

gemini-code-assist

Code Review

This pull request removes the redundant EnrollmentStatusRequest protobuf message and transitions the enrollment status polling mechanism to use standard HTTP GET requests with query parameters instead of POST requests with protobuf bodies. It also includes a new integration test to verify the enrollment status polling flow. The review feedback suggests improving the robustness of URL construction by using url.Parse and JoinPath to avoid potential issues with double slashes, and recommends making the integration test polling loop more resilient to transient network or decoding errors.

gemini-code-assist · 2026-08-25T09:49:30Z

-			return fmt.Errorf("failed to marshal status request: %w", err)
-		}

+		statusURL := controlPlaneURL + "/enroll/status?peer_id=" + url.QueryEscape(n.Host.ID().String())


Constructing the status URL via simple string concatenation is fragile and can result in double slashes (e.g., http://example.com//enroll/status) if controlPlaneURL has a trailing slash. This can cause unexpected 404s or redirects depending on the server or reverse proxy configuration. It is safer and more robust to use url.Parse and JoinPath to build the URL defensively.

u, err := url.Parse(controlPlaneURL) if err != nil { return fmt.Errorf("invalid control plane URL: %w", err) } u = u.JoinPath("enroll", "status") q := u.Query() q.Set("peer_id", n.Host.ID().String()) u.RawQuery = q.Encode() statusURL := u.String()

gemini-code-assist · 2026-08-25T09:49:31Z

-			return fmt.Errorf("failed to marshal status request: %w", err)
-		}

+		statusURL := r.config.ControlPlaneURL + "/enroll/status?peer_id=" + url.QueryEscape(peerID.String())


Constructing the status URL via simple string concatenation is fragile and can result in double slashes (e.g., http://example.com//enroll/status) if ControlPlaneURL has a trailing slash. This can cause unexpected 404s or redirects depending on the server or reverse proxy configuration. It is safer and more robust to use url.Parse and JoinPath to build the URL defensively.

u, err := url.Parse(r.config.ControlPlaneURL) if err != nil { return fmt.Errorf("invalid control plane URL: %w", err) } u = u.JoinPath("enroll", "status") q := u.Query() q.Set("peer_id", peerID.String()) u.RawQuery = q.Encode() statusURL := u.String()

gemini-code-assist · 2026-08-25T09:49:31Z

+		listResp, err := client.Do(listReq)
+		if err != nil {
+			t.Fatalf("failed to list enrollments: %v", err)
+		}
+		var enrollList []storage.EnrollmentRequest
+		if err := json.NewDecoder(listResp.Body).Decode(&enrollList); err != nil {
+			_ = listResp.Body.Close()
+			t.Fatalf("failed to decode enrollments: %v", err)
+		}
+		_ = listResp.Body.Close()


In the integration test polling loop, calling t.Fatalf immediately on any network error, non-200 HTTP status, or JSON decoding failure can lead to flaky tests. Since the node enrollment request runs asynchronously, the admin client might poll before the server is fully ready or before the enrollment request has been processed. It is much more robust to log the error, sleep, and continue the polling loop until the deadline is reached.

Suggested change

listResp, err := client.Do(listReq)

if err != nil {

t.Fatalf("failed to list enrollments: %v", err)

}

var enrollList []storage.EnrollmentRequest

if err := json.NewDecoder(listResp.Body).Decode(&enrollList); err != nil {

_ = listResp.Body.Close()

t.Fatalf("failed to decode enrollments: %v", err)

}

_ = listResp.Body.Close()

listResp, err := client.Do(listReq)

if err != nil {

time.Sleep(50 * time.Millisecond)

continue

}

if listResp.StatusCode != http.StatusOK {

_ = listResp.Body.Close()

time.Sleep(50 * time.Millisecond)

continue

}

var enrollList []storage.EnrollmentRequest

if err := json.NewDecoder(listResp.Body).Decode(&enrollList); err != nil {

_ = listResp.Body.Close()

time.Sleep(50 * time.Millisecond)

continue

}

_ = listResp.Body.Close()

IbrahimAhmed8 · 2026-08-25T12:19:22Z

I rebased on main and applied the bot suggestions. actually pushed an empty commit to retrigger the bats job before realizing it forces a full run of the whole suite. sorry for burning the actions minutes especially with the billing limit issue you mentioned

aojea · 2026-08-25T13:47:17Z

CI fixed in #312
Please rebase.
Apologies for the inconvenience

aojea · 2026-08-25T14:24:16Z

Two small things and then I'll merge:

Add w.Header().Set("Cache-Control", "no-store") to writeEnrollResponse
(internal/controlplane/server.go:1048). Now that this is a GET returning a Biscuit,
the response is cacheable ( it wasn't as a POST).
Please squash into a single commit

IbrahimAhmed8 · 2026-08-25T14:25:48Z

i looked at the raw logs and its failing on test 23 in relay.bats line 91 where it reports 0 peers. looks like a dht flake

aojea · 2026-08-25T14:37:18Z

i looked at the raw logs and its failing on test 23 in relay.bats line 91 where it reports 0 peers. looks like a dht flake

yeah, unrelated flake, we need to deflake that, but is not related to this change

IbrahimAhmed8 · 2026-08-25T14:52:01Z

i looked at the raw logs and its failing on test 23 in relay.bats line 91 where it reports 0 peers. looks like a dht flake

yeah, unrelated flake, we need to deflake that, but is not related to this change

i added the cache header and combined everything into one commit

aojea · 2026-08-25T14:54:59Z

Thanks

gemini-code-assist Bot reviewed Aug 24, 2026

View reviewed changes

aojea mentioned this pull request Aug 24, 2026

Bootstrap enrollment cannot complete: clients POST to /enroll/status but the server only accepts GET (405) #297

Closed

gemini-code-assist Bot reviewed Aug 25, 2026

View reviewed changes

IbrahimAhmed8 force-pushed the FixStatus branch from 8a6d9c5 to 83cf602 Compare August 25, 2026 11:43

IbrahimAhmed8 force-pushed the FixStatus branch from aacfdf9 to de457ca Compare August 25, 2026 13:52

use GET for enroll status polling google#297

ec22cfc

IbrahimAhmed8 force-pushed the FixStatus branch from de457ca to ec22cfc Compare August 25, 2026 14:36

aojea merged commit 7453c4a into google:main Aug 25, 2026
18 checks passed

Uh oh!

Conversation

IbrahimAhmed8 commented Aug 24, 2026

Uh oh!

google-cla Bot commented Aug 24, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Aug 24, 2026

Choose a reason for hiding this comment

Uh oh!

aojea commented Aug 24, 2026

Uh oh!

IbrahimAhmed8 commented Aug 24, 2026

Uh oh!

aojea commented Aug 25, 2026

Uh oh!

aojea commented Aug 25, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Aug 25, 2026

Choose a reason for hiding this comment

Uh oh!

IbrahimAhmed8 commented Aug 25, 2026

Uh oh!

aojea commented Aug 25, 2026

Uh oh!

aojea commented Aug 25, 2026

Uh oh!

IbrahimAhmed8 commented Aug 25, 2026

Uh oh!

aojea commented Aug 25, 2026

Uh oh!

IbrahimAhmed8 commented Aug 25, 2026

Uh oh!

aojea commented Aug 25, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants