Add metadata retrieved from the `context` to the user agent when a new HTTP client is created #2789

MatteoPologruto · 2024-12-18T11:10:54Z

Please check if the PR fulfills these requirements

The PR has no duplicates (please search among the Pull Requests
before creating one)
The PR follows
our contributing guidelines
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)
UPGRADING.md has been updated with a migration guide (for breaking changes)
configuration.schema.json updated 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 thenetwork.user_agent_ext if it's empty and the CLI runs in daemon mode. The complete user agent is arduino-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 is titled accordingly?

Other information

codecov · 2024-12-18T11:29:24Z

Codecov Report

Attention: Patch coverage is 88.88889% with 3 lines in your changes missing coverage. Please review.

Project coverage is 67.71%. Comparing base (e9092cc) to head (2e7d6c2).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
commands/service_check_for_updates.go	0.00%	3 Missing ⚠️

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

Flag	Coverage Δ
unit	`67.71% <88.88%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

alessio-perugini

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

internal/cli/daemon/daemon.go

This allows the extraction of the user-agent in a single place. Also it forces the context passing on all operations that requires access to network.

MatteoPologruto · 2025-01-23T12:40:02Z

@cmaglie

alessio-perugini

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

alessio-perugini · 2025-01-23T12:52:43Z

internal/integrationtest/arduino-cli.go

+	return CreateEnvForDaemonWithUserAgent(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.
+func CreateEnvForDaemonWithUserAgent(t *testing.T, userAgent string) (*Environment, *ArduinoCLI) {
 	env := NewEnvironment(t)

 	cli := NewArduinoCliWithinEnvironment(env, &ArduinoCLIConfig{
 		ArduinoCLIPath:         FindRepositoryRootPath(t).Join("arduino-cli"),
 		UseSharedStagingFolder: true,
 	})

-	_ = cli.StartDaemon(false)
+	_ = cli.StartDaemon(false, userAgent)


Suggested change

return CreateEnvForDaemonWithUserAgent(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.

func CreateEnvForDaemonWithUserAgent(t *testing.T, userAgent string) (*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 the CreateEnvForDaemon

I would simplify and put everything in the same function as it was before.
The useragent will be hardcoded inside the StartDaemon

alessio-perugini · 2025-01-23T12:54:33Z

internal/integrationtest/arduino-cli.go


 // StartDaemon starts the Arduino CLI daemon. It returns the address of the daemon.
-func (cli *ArduinoCLI) StartDaemon(verbose bool) string {
+func (cli *ArduinoCLI) StartDaemon(verbose bool, userAgent string) string {


Suggested change

func (cli *ArduinoCLI) StartDaemon(verbose bool, userAgent string) string {

func (cli *ArduinoCLI) StartDaemon(verbose bool) string {

I cannot think of uses cases that we need to pass different userAgent. The StartDaemon currently is only called in the CreateEnvForDaemon. It's responsibility is to create the grpclient and initializing grpc options.

My suggestion is to just hardcode the userAgent in the grpc option. See later suggestion

alessio-perugini · 2025-01-23T12:55:42Z

internal/integrationtest/arduino-cli.go

+		conn, err := grpc.NewClient(
+			cli.daemonAddr,
+			grpc.WithTransportCredentials(insecure.NewCredentials()),
+			grpc.WithUserAgent(userAgent),


Suggested change

grpc.WithUserAgent(userAgent),

grpc.WithUserAgent("cli-test/0.0.0"),

I'd simply hardcode the user-agent here.

alessio-perugini · 2025-01-23T13:36:44Z

internal/integrationtest/http_server.go

+func (env *Environment) HTTPServeFile(port uint16, path *paths.Path, isDaemon bool) *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())
+		if isDaemon {
+			// 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")
+		}
 	})


The HTTPServeFile 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 of TestDaemonUserAgent.

See suggestion over the TestDaemonUserAgent.
Another approach could be to simply pass some assertions function as a callback, or create another abstraction.

alessio-perugini · 2025-01-23T14:13:51Z

internal/integrationtest/daemon/daemon_test.go

+func TestDaemonUserAgent(t *testing.T) {
+	env, cli := integrationtest.CreateEnvForDaemon(t)
+	defer env.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())
+	}
+}
+


Here, if possible would be nice to extract the assertion code that is done by passing the true flag in the HTTPServerFile. 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.go index 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()) } }

MatteoPologruto added topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project labels Dec 18, 2024

MatteoPologruto self-assigned this Dec 18, 2024

MatteoPologruto marked this pull request as ready for review December 18, 2024 11:23

MatteoPologruto requested review from Xayton, alessio-perugini and cmaglie December 18, 2024 11:23

alessio-perugini reviewed Dec 18, 2024

View reviewed changes

internal/cli/daemon/daemon.go Outdated Show resolved Hide resolved

internal/cli/daemon/daemon.go Outdated Show resolved Hide resolved

cmaglie requested changes Jan 7, 2025

View reviewed changes

internal/cli/daemon/daemon.go Outdated Show resolved Hide resolved

internal/cli/daemon/daemon.go Outdated Show resolved Hide resolved

MatteoPologruto force-pushed the user-agent branch from 1dc53f7 to b78fdb5 Compare January 16, 2025 16:37

MatteoPologruto added 2 commits January 22, 2025 09:57

Set the extra user agent when a new rpc instance is created

b7b5399

Add integration test

fe84afb

MatteoPologruto force-pushed the user-agent branch from 60ad95e to fe84afb Compare January 22, 2025 08:57

cmaglie added 3 commits January 22, 2025 17:08

Moved user-agent extraction deep in configuration.HttpClient

4f73e4c

This allows the extraction of the user-agent in a single place. Also it forces the context passing on all operations that requires access to network.

Updated integration test

fc6d0a6

Restore previous user-agent for package manager

527f7d2

cmaglie approved these changes Jan 23, 2025

View reviewed changes

cmaglie requested a review from alessio-perugini January 23, 2025 11:55

alessio-perugini approved these changes Jan 23, 2025

View reviewed changes

Apply review suggestions to the integration test

2e7d6c2

MatteoPologruto changed the title ~~Add daemon to the user agent when the CLI is started in daemon mode~~ Add metadata retrieved from the context to the user agent when a new HTTP client is created Jan 23, 2025

MatteoPologruto merged commit c5cfe4a into arduino:master Jan 23, 2025
98 checks passed

MatteoPologruto deleted the user-agent branch January 23, 2025 16:41

	func (cli *ArduinoCLI) StartDaemon(verbose bool, userAgent string) string {
	func (cli *ArduinoCLI) StartDaemon(verbose bool) string {

	grpc.WithUserAgent(userAgent),
	grpc.WithUserAgent("cli-test/0.0.0"),

Uh oh!

Add metadata retrieved from the context to the user agent when a new HTTP client is created #2789

Add metadata retrieved from the context to the user agent when a new HTTP client is created #2789

Uh oh!

Conversation

MatteoPologruto commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Please check if the PR fulfills these requirements

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change, and is titled accordingly?

Other information

Uh oh!

codecov bot commented Dec 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

alessio-perugini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MatteoPologruto commented Jan 23, 2025

Uh oh!

alessio-perugini left a comment

Choose a reason for hiding this comment

Uh oh!

alessio-perugini Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

alessio-perugini Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

alessio-perugini Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

alessio-perugini Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

alessio-perugini Jan 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Add metadata retrieved from the `context` to the user agent when a new HTTP client is created #2789

Add metadata retrieved from the `context` to the user agent when a new HTTP client is created #2789

MatteoPologruto commented Dec 18, 2024 •

edited

Loading

codecov bot commented Dec 18, 2024 •

edited

Loading