Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork436
Add metadata retrieved from thecontext to the user agent when a new HTTP client is created#2789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecovbot commentedDec 18, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## master #2789 +/- ##==========================================+ Coverage 67.69% 67.71% +0.01%========================================== Files 238 238 Lines 22388 22392 +4 ==========================================+ Hits 15156 15163 +7+ Misses 6036 6034 -2+ Partials 1196 1195 -1
Flags with carried forward coverage won't be shown.Click here to find out more. ☔ View full report in Codecov by Sentry. |
alessio-perugini left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Left some comments/
It would be cool if we're able to test it.
Maybe we could use a proxy HTTP, and inspect the call that arrives are with the expected User agent. Something similar to:https://github.com/arduino/arduino-cli/blob/master/internal/cli/configuration/network_test.go#L29-L51
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This allows the extraction of the user-agent in a single place. Also itforces the context passing on all operations that requires access tonetwork.
MatteoPologruto commentedJan 23, 2025
alessio-perugini left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't want to block this over some minor refactoring in the integration test part.
If possible I would extract the hardcoded assertion logic in the HTTPServeFile.
The implementation is LGTM. I've also tested locally with the usage of network.proxy and without and it correctly sends the UserAgent
| returnCreateEnvForDaemonWithUserAgent(t,"cli-test/0.0.0") | ||
| } | ||
| // CreateEnvForDaemonWithUserAgent performs the minimum required operations to start the arduino-cli daemon. | ||
| // It returns a testsuite.Environment and an ArduinoCLI client to perform the integration tests. | ||
| // The Environment must be disposed by calling the CleanUp method via defer. | ||
| funcCreateEnvForDaemonWithUserAgent(t*testing.T,userAgentstring) (*Environment,*ArduinoCLI) { | ||
| env:=NewEnvironment(t) | ||
| cli:=NewArduinoCliWithinEnvironment(env,&ArduinoCLIConfig{ | ||
| ArduinoCLIPath:FindRepositoryRootPath(t).Join("arduino-cli"), | ||
| UseSharedStagingFolder:true, | ||
| }) | ||
| _=cli.StartDaemon(false) | ||
| _=cli.StartDaemon(false,userAgent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| returnCreateEnvForDaemonWithUserAgent(t,"cli-test/0.0.0") | |
| } | |
| // CreateEnvForDaemonWithUserAgent performs the minimum required operations to start the arduino-cli daemon. | |
| // It returns a testsuite.Environment and an ArduinoCLI client to perform the integration tests. | |
| // The Environment must be disposed by calling the CleanUp method via defer. | |
| funcCreateEnvForDaemonWithUserAgent(t*testing.T,userAgentstring) (*Environment,*ArduinoCLI) { | |
| env:=NewEnvironment(t) | |
| cli:=NewArduinoCliWithinEnvironment(env,&ArduinoCLIConfig{ | |
| ArduinoCLIPath:FindRepositoryRootPath(t).Join("arduino-cli"), | |
| UseSharedStagingFolder:true, | |
| }) | |
| _=cli.StartDaemon(false) | |
| _=cli.StartDaemon(false,userAgent) | |
| env:=NewEnvironment(t) | |
| cli:=NewArduinoCliWithinEnvironment(env,&ArduinoCLIConfig{ | |
| ArduinoCLIPath:FindRepositoryRootPath(t).Join("arduino-cli"), | |
| UseSharedStagingFolder:true, | |
| }) | |
| _=cli.StartDaemon(false) |
CreateEnvForDaemonWithUserAgent this function is used only by theCreateEnvForDaemon
I would simplify and put everything in the same function as it was before.
The useragent will be hardcoded inside theStartDaemon
| // StartDaemon starts the Arduino CLI daemon. It returns the address of the daemon. | ||
| func (cli*ArduinoCLI)StartDaemon(verbosebool)string { | ||
| func (cli*ArduinoCLI)StartDaemon(verbosebool,userAgentstring)string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| func (cli*ArduinoCLI)StartDaemon(verbosebool,userAgentstring)string { | |
| func (cli*ArduinoCLI)StartDaemon(verbosebool)string { |
I cannot think of uses cases that we need to pass different userAgent. TheStartDaemon currently is only called in theCreateEnvForDaemon. It's responsibility is to create the grpclient and initializing grpc options.
My suggestion is to just hardcode theuserAgent in the grpc option. See later suggestion
| conn,err:=grpc.NewClient( | ||
| cli.daemonAddr, | ||
| grpc.WithTransportCredentials(insecure.NewCredentials()), | ||
| grpc.WithUserAgent(userAgent), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| grpc.WithUserAgent(userAgent), | |
| grpc.WithUserAgent("cli-test/0.0.0"), |
I'd simply hardcode the user-agent here.
| func (env*Environment)HTTPServeFile(portuint16,path*paths.Path,isDaemonbool)*url.URL { | ||
| t:=env.T() | ||
| mux:=http.NewServeMux() | ||
| mux.HandleFunc("/"+path.Base(),func(w http.ResponseWriter,r*http.Request) { | ||
| http.ServeFile(w,r,path.String()) | ||
| ifisDaemon { | ||
| // Test that the user-agent contains metadata from the context when the CLI is in daemon mode | ||
| userAgent:=r.Header.Get("User-Agent") | ||
| require.Contains(t,userAgent,"arduino-cli/git-snapshot") | ||
| require.Contains(t,userAgent,"cli-test/0.0.0") | ||
| require.Contains(t,userAgent,"grpc-go") | ||
| } | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
TheHTTPServeFile should just serve files, and not perform any assertion based on a boolean flag.
I understand why this was done, as it is convenient to do so, but if in he future we have to make assertions based on some other http header, this function will quickly be polluted with stuff that goes out of scope of this function.
I suggest to rollback these changes, and creating a dedicated HTTP server that asserts that. I would put such server inside the dedicated test ofTestDaemonUserAgent.
See suggestion over theTestDaemonUserAgent.
Another approach could be to simply pass some assertions function as a callback, or create another abstraction.
| funcTestDaemonUserAgent(t*testing.T) { | ||
| env,cli:=integrationtest.CreateEnvForDaemon(t) | ||
| deferenv.CleanUp() | ||
| // Set up an http server to serve our custom index file | ||
| // The user-agent is tested inside the HTTPServeFile function | ||
| test_index:=paths.New("..","testdata","test_index.json") | ||
| url:=env.HTTPServeFile(8000,test_index,true) | ||
| grpcInst:=cli.Create() | ||
| require.NoError(t,grpcInst.Init("","",func(ir*commands.InitResponse) { | ||
| fmt.Printf("INIT> %v\n",ir.GetMessage()) | ||
| })) | ||
| // Set extra indexes | ||
| err:=cli.SetValue("board_manager.additional_urls",`["http://127.0.0.1:8000/test_index.json"]`) | ||
| require.NoError(t,err) | ||
| { | ||
| cl,err:=grpcInst.UpdateIndex(context.Background(),false) | ||
| require.NoError(t,err) | ||
| res,err:=analyzeUpdateIndexClient(t,cl) | ||
| require.NoError(t,err) | ||
| require.Len(t,res,2) | ||
| require.True(t,res[url.String()].GetSuccess()) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Here, if possible would be nice to extract the assertion code that is done by passing thetrue flag in theHTTPServerFile. Or make an abstraction that passes some assertion functions.
Another possible idea is by "proxing" the request to the file server:
diff --git a/internal/integrationtest/daemon/daemon_test.go b/internal/integrationtest/daemon/daemon_test.goindex 7d800c84..982ba8bb 100644--- a/internal/integrationtest/daemon/daemon_test.go+++ b/internal/integrationtest/daemon/daemon_test.go@@ -20,6 +20,10 @@ import ( "errors" "fmt" "io"+"maps"+"net/http"+"net/http/httptest"+"strings" "testing" "time"@@ -562,15 +566,41 @@ func TestDaemonUserAgent(t *testing.T) { // Set up an http server to serve our custom index file // The user-agent is tested inside the HTTPServeFile function test_index := paths.New("..", "testdata", "test_index.json")-url := env.HTTPServeFile(8000, test_index, true)+url := env.HTTPServeFile(8000, test_index)+ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {+// Test that the user-agent contains metadata from the context when the CLI is in daemon mode+userAgent := r.Header.Get("User-Agent")++require.Contains(t, userAgent, "cli-test/0.0.0")+require.Contains(t, userAgent, "grpc-go")+// Depends on how we built the client we may have git-snapshot or 0.0.0-git in dev releases+require.Condition(t, func() (success bool) {+return strings.Contains(userAgent, "arduino-cli/git-snapshot") ||+strings.Contains(userAgent, "arduino-cli/0.0.0-git")+})++proxiedReq, err := http.NewRequest(r.Method, url.String(), r.Body)+require.NoError(t, err)+maps.Copy(proxiedReq.Header, r.Header)++proxiedResp, err := http.DefaultTransport.RoundTrip(proxiedReq)+require.NoError(t, err)+defer proxiedResp.Body.Close()++// Copy the headers from the proxy response to the original response+maps.Copy(r.Header, proxiedReq.Header)+w.WriteHeader(proxiedResp.StatusCode)+io.Copy(w, proxiedResp.Body)+}))+defer ts.Close() grpcInst := cli.Create() require.NoError(t, grpcInst.Init("", "", func(ir *commands.InitResponse) { fmt.Printf("INIT> %v\n", ir.GetMessage()) }))-// Set extra indexes-err := cli.SetValue("board_manager.additional_urls", `["http://127.0.0.1:8000/test_index.json"]`)+additionalURL := ts.URL + "/test_index.json"+err := cli.SetValue("board_manager.additional_urls", fmt.Sprintf(`["%s"]`, additionalURL)) require.NoError(t, err) {@@ -579,7 +609,7 @@ func TestDaemonUserAgent(t *testing.T) { res, err := analyzeUpdateIndexClient(t, cl) require.NoError(t, err) require.Len(t, res, 2)-require.True(t, res[url.String()].GetSuccess())+require.True(t, res[additionalURL].GetSuccess()) } }
daemon to the user agent when the CLI is started in daemon modecontext to the user agent when a new HTTP client is createdc5cfe4a intoarduino:masterUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Please check if the PR fulfills these requirements
Seehow to contribute
before creating one)
our contributing guidelines
UPGRADING.mdhas been updated with a migration guide (for breaking changes)configuration.schema.jsonupdated if new parameters are added.What kind of change does this PR introduce?
Bug fix
What is the current behavior?
The user agent does not specify if the CLI runs in daemon mode. It is always something like
arduino-cli/git-snapshot (amd64; windows; go1.23.2) Commit:84fc413a.What is the new behavior?
The user agent obtained from the context metadata is propagated to the
network.user_agent_extif it's empty and the CLI runs in daemon mode. The complete user agent isarduino-cli/git-snapshot arduino-ide/2.3.4 grpc-node-js/1.9.5 daemon (amd64; windows; go1.23.4) Commit:f3dc127e.Does this PR introduce a breaking change, and istitled accordingly?
Other information