Skip to content

Add metadata retrieved from the context 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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 23, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b7b5399
Set the extra user agent when a new rpc instance is created
MatteoPologruto Jan 16, 2025
fe84afb
Add integration test
MatteoPologruto Jan 22, 2025
4f73e4c
Moved user-agent extraction deep in configuration.HttpClient
cmaglie Jan 22, 2025
fc6d0a6
Updated integration test
cmaglie Jan 22, 2025
527f7d2
Restore previous user-agent for package manager
cmaglie Jan 22, 2025
2e7d6c2
Apply review suggestions to the integration test
MatteoPologruto Jan 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add integration test
  • Loading branch information
@MatteoPologruto
MatteoPologruto committed Jan 22, 2025
commit fe84afb6529dec54e4e98e5c57601486de2f5d4b
20 changes: 10 additions & 10 deletions internal/integrationtest/core/core_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func TestCoreSearch(t *testing.T) {

// Set up an http server to serve our custom index file
test_index := paths.New("..", "testdata", "test_index.json")
url := env.HTTPServeFile(8000, test_index)
url := env.HTTPServeFile(8000, test_index, false)

// Run update-index with our test index
_, _, err := cli.Run("core", "update-index", "--additional-urls="+url.String())
Expand Down Expand Up @@ -161,7 +161,7 @@ func TestCoreSearchNoArgs(t *testing.T) {

// Set up an http server to serve our custom index file
testIndex := paths.New("..", "testdata", "test_index.json")
url := env.HTTPServeFile(8000, testIndex)
url := env.HTTPServeFile(8000, testIndex, false)

// update custom index and install test core (installed cores affect `core search`)
_, _, err := cli.Run("core", "update-index", "--additional-urls="+url.String())
Expand Down Expand Up @@ -749,7 +749,7 @@ func TestCoreSearchSortedResults(t *testing.T) {

// Set up the server to serve our custom index file
testIndex := paths.New("..", "testdata", "test_index.json")
url := env.HTTPServeFile(8000, testIndex)
url := env.HTTPServeFile(8000, testIndex, false)

// update custom index
_, _, err := cli.Run("core", "update-index", "--additional-urls="+url.String())
Expand Down Expand Up @@ -821,7 +821,7 @@ func TestCoreListSortedResults(t *testing.T) {

// Set up the server to serve our custom index file
testIndex := paths.New("..", "testdata", "test_index.json")
url := env.HTTPServeFile(8000, testIndex)
url := env.HTTPServeFile(8000, testIndex, false)

// update custom index
_, _, err := cli.Run("core", "update-index", "--additional-urls="+url.String())
Expand Down Expand Up @@ -892,7 +892,7 @@ func TestCoreListDeprecatedPlatformWithInstalledJson(t *testing.T) {

// Set up the server to serve our custom index file
testIndex := paths.New("..", "testdata", "test_index.json")
url := env.HTTPServeFile(8000, testIndex)
url := env.HTTPServeFile(8000, testIndex, false)

// update custom index
_, _, err := cli.Run("core", "update-index", "--additional-urls="+url.String())
Expand Down Expand Up @@ -1110,8 +1110,8 @@ func TestCoreInstallRunsToolPostInstallScript(t *testing.T) {
env, cli := integrationtest.CreateArduinoCLIWithEnvironment(t)
defer env.CleanUp()

url := env.HTTPServeFile(8080, paths.New("testdata", "package_with_postinstall_index.json"))
env.HTTPServeFile(8081, paths.New("testdata", "core_with_postinst.zip"))
url := env.HTTPServeFile(8080, paths.New("testdata", "package_with_postinstall_index.json"), false)
env.HTTPServeFile(8081, paths.New("testdata", "core_with_postinst.zip"), false)

_, _, err := cli.Run("core", "update-index", "--additional-urls", url.String())
require.NoError(t, err)
Expand All @@ -1129,7 +1129,7 @@ func TestCoreBrokenDependency(t *testing.T) {

// Set up an http server to serve our custom index file
test_index := paths.New("..", "testdata", "test_index.json")
url := env.HTTPServeFile(8000, test_index)
url := env.HTTPServeFile(8000, test_index, false)

// Run update-index with our test index
_, _, err := cli.Run("core", "update-index", "--additional-urls="+url.String())
Expand All @@ -1145,7 +1145,7 @@ func TestCoreUpgradeWarningWithPackageInstalledButNotIndexed(t *testing.T) {
env, cli := integrationtest.CreateArduinoCLIWithEnvironment(t)
defer env.CleanUp()

url := env.HTTPServeFile(8000, paths.New("..", "testdata", "test_index.json")).String()
url := env.HTTPServeFile(8000, paths.New("..", "testdata", "test_index.json"), false).String()

t.Run("missing additional-urls", func(t *testing.T) {
// update index
Expand Down Expand Up @@ -1187,7 +1187,7 @@ func TestCoreHavingIncompatibleDepTools(t *testing.T) {
env, cli := integrationtest.CreateArduinoCLIWithEnvironment(t)
defer env.CleanUp()

url := env.HTTPServeFile(8000, paths.New("..", "testdata", "test_index.json")).String()
url := env.HTTPServeFile(8000, paths.New("..", "testdata", "test_index.json"), false).String()
additionalURLs := "--additional-urls=" + url

_, _, err := cli.Run("core", "update-index", additionalURLs)
Expand Down
28 changes: 28 additions & 0 deletions internal/integrationtest/daemon/daemon_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,34 @@ func TestDaemonCoreUpgradePlatform(t *testing.T) {
})
}

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())
}
}

Copy link
Contributor

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 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())
 	}
 }

func analyzeUpdateIndexClient(t *testing.T, cl commands.ArduinoCoreService_UpdateIndexClient) (map[string]*commands.DownloadProgressEnd, error) {
analyzer := NewDownloadProgressAnalyzer(t)
for {
Expand Down
8 changes: 6 additions & 2 deletions internal/integrationtest/http_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,21 @@ import (

// HTTPServeFile spawn an http server that serve a single file. The server
// is started on the given port. The URL to the file and a cleanup function are returned.
func (env *Environment) HTTPServeFile(port uint16, path *paths.Path) *url.URL {
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
require.Contains(t, r.Header.Get("User-Agent"), "arduino-cli/git-snapshot grpc-go")
}
})
server := &http.Server{
Addr: fmt.Sprintf(":%d", port),
Handler: mux,
}

t := env.T()
fileURL, err := url.Parse(fmt.Sprintf("http://127.0.0.1:%d/%s", port, path.Base()))
require.NoError(t, err)

Expand Down
Loading