Conversation
|
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[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the /debug/network-info diagnostic endpoint to return announced addresses instead of observed addresses, defining a new NetworkInfoResponse protobuf message in api/sam.proto and updating the corresponding generated bindings, handlers, and tests. The code review feedback correctly identifies a violation of §5 of the Repository Style Guide, which states that diagnostic endpoints should use JSON with shapes defined as Go structs in api/ rather than polluting the protobuf mesh contract. The reviewer recommends defining NetworkInfoResponse as a standard Go struct, reverting to standard JSON marshaling, and cleaning up the resulting unused protobuf imports and helper functions across the codebase.
| // ============================================================================ | ||
| // Node diagnostics API | ||
| // ============================================================================ | ||
| // | ||
| // Read-only operator diagnostics served by the node sidecar. These addresses | ||
| // are what the node listens on and announces to other peers; they are not the | ||
| // external addresses that remote peers report observing through Identify. | ||
|
|
||
| message NetworkInfoResponse { | ||
| repeated string listen_addresses = 1; | ||
| repeated string announced_addresses = 2; | ||
| } |
There was a problem hiding this comment.
5. API surfaces and secrets
According to the repository style guide (§5), the operator plane and diagnostic endpoints should use JSON with shapes defined as Go structs in api/, while api/sam.proto is reserved for the mesh protocol.
Defining NetworkInfoResponse in api/sam.proto unnecessarily pollutes the protobuf contract and forces the regeneration of Go, JavaScript, and Python bindings for a purely local, unversioned operator diagnostic endpoint. This also contradicts the comment in internal/node/debug_handlers.go stating that /debug endpoints are not part of the api/sam.proto mesh contract.
Recommended Fix:
Remove NetworkInfoResponse from api/sam.proto and instead define it as a standard Go struct in the api package (e.g., in a new file api/network.go or an existing operator Go file in api/).
package api
type NetworkInfoResponse struct {
ListenAddresses []string `json:"listen_addresses"`
AnnouncedAddresses []string `json:"announced_addresses"`
}References
- According to §5 of the Repository Style Guide, the operator plane and diagnostic endpoints should use JSON with shapes defined as Go structs in api/, while api/sam.proto is reserved for the mesh protocol. (link)
| "google.golang.org/protobuf/encoding/protojson" | ||
| "google.golang.org/protobuf/proto" |
There was a problem hiding this comment.
5. API surfaces and secrets
Remove the unused protojson and proto imports since writeDebugProtoJSON is being removed.
References
- According to §5 of the Repository Style Guide, the operator plane and diagnostic endpoints should use JSON with shapes defined as Go structs in api/, while api/sam.proto is reserved for the mesh protocol. (link)
| }) | ||
| mux.HandleFunc("GET /debug/network-info", func(w http.ResponseWriter, r *http.Request) { | ||
| writeDebugJSON(w, n.networkInfo()) | ||
| writeDebugProtoJSON(w, n.networkInfo()) |
There was a problem hiding this comment.
5. API surfaces and secrets
Since NetworkInfoResponse should be defined as a standard Go struct in the api package rather than a protobuf message, we can use the standard writeDebugJSON helper instead of writeDebugProtoJSON.
| writeDebugProtoJSON(w, n.networkInfo()) | |
| writeDebugJSON(w, n.networkInfo()) |
References
- According to §5 of the Repository Style Guide, the operator plane and diagnostic endpoints should use JSON with shapes defined as Go structs in api/, while api/sam.proto is reserved for the mesh protocol. (link)
| func writeDebugProtoJSON(w http.ResponseWriter, value proto.Message) { | ||
| data, err := (protojson.MarshalOptions{UseProtoNames: true}).Marshal(value) | ||
| if err != nil { | ||
| http.Error(w, "Failed to encode response", http.StatusInternalServerError) | ||
| logger.Errorf("Failed to encode response: %v", err) | ||
| return | ||
| } | ||
| w.Header().Set("Content-Type", "application/json") | ||
| if _, err := w.Write(append(data, '\n')); err != nil { | ||
| logger.Errorf("Failed to write response: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
5. API surfaces and secrets
With NetworkInfoResponse defined as a Go struct, the writeDebugProtoJSON helper is no longer needed and can be completely removed.
References
- According to §5 of the Repository Style Guide, the operator plane and diagnostic endpoints should use JSON with shapes defined as Go structs in api/, while api/sam.proto is reserved for the mesh protocol. (link)
|
|
||
| "github.com/google/sam/api" | ||
| "github.com/modelcontextprotocol/go-sdk/mcp" | ||
| "google.golang.org/protobuf/encoding/protojson" |
There was a problem hiding this comment.
5. API surfaces and secrets
Remove the unused protojson import since we are switching to standard json.Unmarshal.
References
- According to §5 of the Repository Style Guide, the operator plane and diagnostic endpoints should use JSON with shapes defined as Go structs in api/, while api/sam.proto is reserved for the mesh protocol. (link)
| var networkInfoResponse api.NetworkInfoResponse | ||
| if err := protojson.Unmarshal(networkInfo.Body.Bytes(), &networkInfoResponse); err != nil { | ||
| t.Fatalf("decode network info response: %v", err) |
There was a problem hiding this comment.
5. API surfaces and secrets
Since NetworkInfoResponse is a standard Go struct, use the standard json.Unmarshal instead of protojson.Unmarshal.
| var networkInfoResponse api.NetworkInfoResponse | |
| if err := protojson.Unmarshal(networkInfo.Body.Bytes(), &networkInfoResponse); err != nil { | |
| t.Fatalf("decode network info response: %v", err) | |
| var networkInfoResponse api.NetworkInfoResponse | |
| if err := json.Unmarshal(networkInfo.Body.Bytes(), &networkInfoResponse); err != nil { | |
| t.Fatalf("decode network info response: %v", err) |
References
- According to §5 of the Repository Style Guide, the operator plane and diagnostic endpoints should use JSON with shapes defined as Go structs in api/, while api/sam.proto is reserved for the mesh protocol. (link)
| "testing" | ||
|
|
||
| "github.com/google/sam/api" | ||
| "google.golang.org/protobuf/encoding/protojson" |
There was a problem hiding this comment.
5. API surfaces and secrets
Remove the unused protojson import since we are switching to standard json.Unmarshal.
References
- According to §5 of the Repository Style Guide, the operator plane and diagnostic endpoints should use JSON with shapes defined as Go structs in api/, while api/sam.proto is reserved for the mesh protocol. (link)
| var info api.NetworkInfoResponse | ||
| if err := protojson.Unmarshal([]byte(resp), &info); err != nil { | ||
| t.Fatalf("failed to unmarshal JSON: %v", err) | ||
| } |
There was a problem hiding this comment.
5. API surfaces and secrets
Since NetworkInfoResponse is a standard Go struct, use the standard json.Unmarshal instead of protojson.Unmarshal.
| var info api.NetworkInfoResponse | |
| if err := protojson.Unmarshal([]byte(resp), &info); err != nil { | |
| t.Fatalf("failed to unmarshal JSON: %v", err) | |
| } | |
| var info api.NetworkInfoResponse | |
| if err := json.Unmarshal([]byte(resp), &info); err != nil { | |
| t.Fatalf("failed to unmarshal JSON: %v", err) | |
| } |
References
- According to §5 of the Repository Style Guide, the operator plane and diagnostic endpoints should use JSON with shapes defined as Go structs in api/, while api/sam.proto is reserved for the mesh protocol. (link)
|
@kaisoz gemini-code-assist recommends using a Go struct based on .gemini/styleguide.md, but the newer AGENTS.md requires all API responses to use api/sam.proto. I followed AGENTS.md. Could you confirm which rule applies to /debug/network-info and review the PR when you have time? Thanks! |
026a39c to
f7f9e82
Compare
|
@kaisoz could you please take a look at my above comment? |
Hey @Mukezh! First of all, my apologies for not having a look at this earlier! Somehow I missed it.. and thanks so much for the PR! Yes, With that in mind, I think we can make it smaller. The The variable rename, the CLI help, the SKILL.md line and the tests you added can all stay. The tests just need the two Could you also add the That way the next person (or agent) doesn't run into the same thing. Thanks again, looking forward to the next one! 😊 |
|
@kaisoz |
This branch has not been deployed
2 participants
Fixes #484
Summary
observed_addressestoannounced_addressesin/debug/network-infoapi/sam.protoand encode it using protojsonThe endpoint continues to return addresses from
Host.Addrs(); this change gives that existing data the correct name.This PR intentionally implements only proposal 1 from the issue. It does not add collection or reporting of peer-observed external addresses.
Testing
go test ./internal/node -run 'TestNetworkInfo|TestDebugHandlerHTTP' -count=1make lintgo test ./tests/integration -run '^TestDebugEndpoints$' -count=1 -vin Linux