From b7b539986c96593b9b3311ffd8f58e1cb462d7ce Mon Sep 17 00:00:00 2001 From: MatteoPologruto Date: Thu, 16 Jan 2025 17:34:53 +0100 Subject: [PATCH 1/6] Set the extra user agent when a new rpc instance is created --- commands/instances.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/commands/instances.go b/commands/instances.go index d5534494bae..098e03f76ae 100644 --- a/commands/instances.go +++ b/commands/instances.go @@ -67,6 +67,24 @@ func (s *arduinoCoreServerImpl) Create(ctx context.Context, req *rpc.CreateReque var userAgent string if md, ok := metadata.FromIncomingContext(ctx); ok { userAgent = strings.Join(md.Get("user-agent"), " ") + if userAgent != "" { + // s.SettingsGetValue() returns an error if the key does not exist and for this reason we are accessing + // network.user_agent_ext directly from s.settings.ExtraUserAgent() to set it + if s.settings.ExtraUserAgent() == "" { + if strings.Contains(userAgent, "arduino-ide/2") { + // needed for analytics purposes + userAgent = userAgent + " daemon" + } + _, err := s.SettingsSetValue(ctx, &rpc.SettingsSetValueRequest{ + Key: "network.user_agent_ext", + ValueFormat: "cli", + EncodedValue: userAgent, + }) + if err != nil { + return nil, err + } + } + } } // Setup downloads directory From fe84afb6529dec54e4e98e5c57601486de2f5d4b Mon Sep 17 00:00:00 2001 From: MatteoPologruto Date: Wed, 22 Jan 2025 09:23:28 +0100 Subject: [PATCH 2/6] Add integration test --- internal/integrationtest/core/core_test.go | 20 ++++++------- .../integrationtest/daemon/daemon_test.go | 28 +++++++++++++++++++ internal/integrationtest/http_server.go | 8 ++++-- 3 files changed, 44 insertions(+), 12 deletions(-) diff --git a/internal/integrationtest/core/core_test.go b/internal/integrationtest/core/core_test.go index a27b5f6d15a..4818997e996 100644 --- a/internal/integrationtest/core/core_test.go +++ b/internal/integrationtest/core/core_test.go @@ -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()) @@ -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()) @@ -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()) @@ -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()) @@ -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()) @@ -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) @@ -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()) @@ -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 @@ -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) diff --git a/internal/integrationtest/daemon/daemon_test.go b/internal/integrationtest/daemon/daemon_test.go index 47225784361..7d800c84fa7 100644 --- a/internal/integrationtest/daemon/daemon_test.go +++ b/internal/integrationtest/daemon/daemon_test.go @@ -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()) + } +} + func analyzeUpdateIndexClient(t *testing.T, cl commands.ArduinoCoreService_UpdateIndexClient) (map[string]*commands.DownloadProgressEnd, error) { analyzer := NewDownloadProgressAnalyzer(t) for { diff --git a/internal/integrationtest/http_server.go b/internal/integrationtest/http_server.go index c5f06e9557a..6d04a2a4489 100644 --- a/internal/integrationtest/http_server.go +++ b/internal/integrationtest/http_server.go @@ -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) From 4f73e4c8c68f5536e1829e66b9f1fefeded2cda6 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Wed, 22 Jan 2025 17:08:03 +0100 Subject: [PATCH 3/6] Moved user-agent extraction deep in configuration.HttpClient 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. --- commands/instances.go | 34 ++++------------------ commands/service_board_identify.go | 18 ++++++------ commands/service_board_identify_test.go | 15 +++++----- commands/service_check_for_updates.go | 6 ++-- commands/service_library_download.go | 12 +++++--- internal/arduino/resources/helpers_test.go | 2 +- internal/cli/configuration/network.go | 17 ++++++++--- internal/cli/configuration/network_test.go | 7 +++-- 8 files changed, 51 insertions(+), 60 deletions(-) diff --git a/commands/instances.go b/commands/instances.go index 098e03f76ae..8264f23bae4 100644 --- a/commands/instances.go +++ b/commands/instances.go @@ -43,7 +43,6 @@ import ( paths "github.com/arduino/go-paths-helper" "github.com/sirupsen/logrus" "google.golang.org/grpc/codes" - "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" ) @@ -64,29 +63,6 @@ func installTool(ctx context.Context, pm *packagemanager.PackageManager, tool *c // Create a new Instance ready to be initialized, supporting directories are also created. func (s *arduinoCoreServerImpl) Create(ctx context.Context, req *rpc.CreateRequest) (*rpc.CreateResponse, error) { - var userAgent string - if md, ok := metadata.FromIncomingContext(ctx); ok { - userAgent = strings.Join(md.Get("user-agent"), " ") - if userAgent != "" { - // s.SettingsGetValue() returns an error if the key does not exist and for this reason we are accessing - // network.user_agent_ext directly from s.settings.ExtraUserAgent() to set it - if s.settings.ExtraUserAgent() == "" { - if strings.Contains(userAgent, "arduino-ide/2") { - // needed for analytics purposes - userAgent = userAgent + " daemon" - } - _, err := s.SettingsSetValue(ctx, &rpc.SettingsSetValueRequest{ - Key: "network.user_agent_ext", - ValueFormat: "cli", - EncodedValue: userAgent, - }) - if err != nil { - return nil, err - } - } - } - } - // Setup downloads directory downloadsDir := s.settings.DownloadsDir() if downloadsDir.NotExist() { @@ -107,11 +83,11 @@ func (s *arduinoCoreServerImpl) Create(ctx context.Context, req *rpc.CreateReque } } - config, err := s.settings.DownloaderConfig() + config, err := s.settings.DownloaderConfig(ctx) if err != nil { return nil, err } - inst, err := instances.Create(dataDir, packagesDir, userPackagesDir, downloadsDir, userAgent, config) + inst, err := instances.Create(dataDir, packagesDir, userPackagesDir, downloadsDir, "", config) if err != nil { return nil, err } @@ -395,7 +371,7 @@ func (s *arduinoCoreServerImpl) Init(req *rpc.InitRequest, stream rpc.ArduinoCor responseError(err.GRPCStatus()) continue } - config, err := s.settings.DownloaderConfig() + config, err := s.settings.DownloaderConfig(ctx) if err != nil { taskCallback(&rpc.TaskProgress{Name: i18n.Tr("Error downloading library %s", libraryRef)}) e := &cmderrors.FailedLibraryInstallError{Cause: err} @@ -516,7 +492,7 @@ func (s *arduinoCoreServerImpl) UpdateLibrariesIndex(req *rpc.UpdateLibrariesInd } // Perform index update - config, err := s.settings.DownloaderConfig() + config, err := s.settings.DownloaderConfig(stream.Context()) if err != nil { return err } @@ -626,7 +602,7 @@ func (s *arduinoCoreServerImpl) UpdateIndex(req *rpc.UpdateIndexRequest, stream } } - config, err := s.settings.DownloaderConfig() + config, err := s.settings.DownloaderConfig(stream.Context()) if err != nil { downloadCB.Start(u, i18n.Tr("Downloading index: %s", filepath.Base(URL.Path))) downloadCB.End(false, i18n.Tr("Invalid network configuration: %s", err)) diff --git a/commands/service_board_identify.go b/commands/service_board_identify.go index 2f4b1f54dce..787de81cee3 100644 --- a/commands/service_board_identify.go +++ b/commands/service_board_identify.go @@ -48,7 +48,7 @@ func (s *arduinoCoreServerImpl) BoardIdentify(ctx context.Context, req *rpc.Boar defer release() props := properties.NewFromHashmap(req.GetProperties()) - res, err := identify(pme, props, s.settings, !req.GetUseCloudApiForUnknownBoardDetection()) + res, err := identify(ctx, pme, props, s.settings, !req.GetUseCloudApiForUnknownBoardDetection()) if err != nil { return nil, err } @@ -58,7 +58,7 @@ func (s *arduinoCoreServerImpl) BoardIdentify(ctx context.Context, req *rpc.Boar } // identify returns a list of boards checking first the installed platforms or the Cloud API -func identify(pme *packagemanager.Explorer, properties *properties.Map, settings *configuration.Settings, skipCloudAPI bool) ([]*rpc.BoardListItem, error) { +func identify(ctx context.Context, pme *packagemanager.Explorer, properties *properties.Map, settings *configuration.Settings, skipCloudAPI bool) ([]*rpc.BoardListItem, error) { if properties == nil { return nil, nil } @@ -90,7 +90,7 @@ func identify(pme *packagemanager.Explorer, properties *properties.Map, settings // if installed cores didn't recognize the board, try querying // the builder API if the board is a USB device port if len(boards) == 0 && !skipCloudAPI && !settings.SkipCloudApiForBoardDetection() { - items, err := identifyViaCloudAPI(properties, settings) + items, err := identifyViaCloudAPI(ctx, properties, settings) if err != nil { // this is bad, but keep going logrus.WithError(err).Debug("Error querying builder API") @@ -119,14 +119,14 @@ func identify(pme *packagemanager.Explorer, properties *properties.Map, settings return boards, nil } -func identifyViaCloudAPI(props *properties.Map, settings *configuration.Settings) ([]*rpc.BoardListItem, error) { +func identifyViaCloudAPI(ctx context.Context, props *properties.Map, settings *configuration.Settings) ([]*rpc.BoardListItem, error) { // If the port is not USB do not try identification via cloud if !props.ContainsKey("vid") || !props.ContainsKey("pid") { return nil, nil } logrus.Debug("Querying builder API for board identification...") - return cachedAPIByVidPid(props.Get("vid"), props.Get("pid"), settings) + return cachedAPIByVidPid(ctx, props.Get("vid"), props.Get("pid"), settings) } var ( @@ -134,7 +134,7 @@ var ( validVidPid = regexp.MustCompile(`0[xX][a-fA-F\d]{4}`) ) -func cachedAPIByVidPid(vid, pid string, settings *configuration.Settings) ([]*rpc.BoardListItem, error) { +func cachedAPIByVidPid(ctx context.Context, vid, pid string, settings *configuration.Settings) ([]*rpc.BoardListItem, error) { var resp []*rpc.BoardListItem cacheKey := fmt.Sprintf("cache.builder-api.v3/boards/byvid/pid/%s/%s", vid, pid) @@ -148,7 +148,7 @@ func cachedAPIByVidPid(vid, pid string, settings *configuration.Settings) ([]*rp } } - resp, err := apiByVidPid(vid, pid, settings) // Perform API requrest + resp, err := apiByVidPid(ctx, vid, pid, settings) // Perform API requrest if err == nil { if cachedResp, err := json.Marshal(resp); err == nil { @@ -160,7 +160,7 @@ func cachedAPIByVidPid(vid, pid string, settings *configuration.Settings) ([]*rp return resp, err } -func apiByVidPid(vid, pid string, settings *configuration.Settings) ([]*rpc.BoardListItem, error) { +func apiByVidPid(ctx context.Context, vid, pid string, settings *configuration.Settings) ([]*rpc.BoardListItem, error) { // ensure vid and pid are valid before hitting the API if !validVidPid.MatchString(vid) { return nil, errors.New(i18n.Tr("Invalid vid value: '%s'", vid)) @@ -173,7 +173,7 @@ func apiByVidPid(vid, pid string, settings *configuration.Settings) ([]*rpc.Boar req, _ := http.NewRequest("GET", url, nil) req.Header.Set("Content-Type", "application/json") - httpClient, err := settings.NewHttpClient() + httpClient, err := settings.NewHttpClient(ctx) if err != nil { return nil, fmt.Errorf("%s: %w", i18n.Tr("failed to initialize http client"), err) } diff --git a/commands/service_board_identify_test.go b/commands/service_board_identify_test.go index 98dc8e40278..31687359885 100644 --- a/commands/service_board_identify_test.go +++ b/commands/service_board_identify_test.go @@ -16,6 +16,7 @@ package commands import ( + "context" "fmt" "net/http" "net/http/httptest" @@ -48,7 +49,7 @@ func TestGetByVidPid(t *testing.T) { vidPidURL = ts.URL settings := configuration.NewSettings() - res, err := apiByVidPid("0xf420", "0XF069", settings) + res, err := apiByVidPid(context.Background(), "0xf420", "0XF069", settings) require.Nil(t, err) require.Len(t, res, 1) require.Equal(t, "Arduino/Genuino MKR1000", res[0].GetName()) @@ -56,7 +57,7 @@ func TestGetByVidPid(t *testing.T) { // wrong vid (too long), wrong pid (not an hex value) - _, err = apiByVidPid("0xfffff", "0xDEFG", settings) + _, err = apiByVidPid(context.Background(), "0xfffff", "0xDEFG", settings) require.NotNil(t, err) } @@ -69,7 +70,7 @@ func TestGetByVidPidNotFound(t *testing.T) { defer ts.Close() vidPidURL = ts.URL - res, err := apiByVidPid("0x0420", "0x0069", settings) + res, err := apiByVidPid(context.Background(), "0x0420", "0x0069", settings) require.NoError(t, err) require.Empty(t, res) } @@ -84,7 +85,7 @@ func TestGetByVidPid5xx(t *testing.T) { defer ts.Close() vidPidURL = ts.URL - res, err := apiByVidPid("0x0420", "0x0069", settings) + res, err := apiByVidPid(context.Background(), "0x0420", "0x0069", settings) require.NotNil(t, err) require.Equal(t, "the server responded with status 500 Internal Server Error", err.Error()) require.Len(t, res, 0) @@ -99,7 +100,7 @@ func TestGetByVidPidMalformedResponse(t *testing.T) { defer ts.Close() vidPidURL = ts.URL - res, err := apiByVidPid("0x0420", "0x0069", settings) + res, err := apiByVidPid(context.Background(), "0x0420", "0x0069", settings) require.NotNil(t, err) require.Equal(t, "wrong format in server response", err.Error()) require.Len(t, res, 0) @@ -107,7 +108,7 @@ func TestGetByVidPidMalformedResponse(t *testing.T) { func TestBoardDetectionViaAPIWithNonUSBPort(t *testing.T) { settings := configuration.NewSettings() - items, err := identifyViaCloudAPI(properties.NewMap(), settings) + items, err := identifyViaCloudAPI(context.Background(), properties.NewMap(), settings) require.NoError(t, err) require.Empty(t, items) } @@ -156,7 +157,7 @@ func TestBoardIdentifySorting(t *testing.T) { defer release() settings := configuration.NewSettings() - res, err := identify(pme, idPrefs, settings, true) + res, err := identify(context.Background(), pme, idPrefs, settings, true) require.NoError(t, err) require.NotNil(t, res) require.Len(t, res, 4) diff --git a/commands/service_check_for_updates.go b/commands/service_check_for_updates.go index 2b5c7a51b4e..cce5a4a02a1 100644 --- a/commands/service_check_for_updates.go +++ b/commands/service_check_for_updates.go @@ -43,7 +43,7 @@ func (s *arduinoCoreServerImpl) CheckForArduinoCLIUpdates(ctx context.Context, r inventory.WriteStore() }() - latestVersion, err := semver.Parse(s.getLatestRelease()) + latestVersion, err := semver.Parse(s.getLatestRelease(ctx)) if err != nil { return nil, err } @@ -82,8 +82,8 @@ func (s *arduinoCoreServerImpl) shouldCheckForUpdate(currentVersion *semver.Vers // getLatestRelease queries the official Arduino download server for the latest release, // if there are no errors or issues a version string is returned, in all other case an empty string. -func (s *arduinoCoreServerImpl) getLatestRelease() string { - client, err := s.settings.NewHttpClient() +func (s *arduinoCoreServerImpl) getLatestRelease(ctx context.Context) string { + client, err := s.settings.NewHttpClient(ctx) if err != nil { return "" } diff --git a/commands/service_library_download.go b/commands/service_library_download.go index 2384d59396f..4253be8cca1 100644 --- a/commands/service_library_download.go +++ b/commands/service_library_download.go @@ -82,11 +82,15 @@ func (s *arduinoCoreServerImpl) LibraryDownload(req *rpc.LibraryDownloadRequest, }) } -func downloadLibrary(ctx context.Context, downloadsDir *paths.Path, libRelease *librariesindex.Release, - downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB, queryParameter string, settings *configuration.Settings) error { - +func downloadLibrary( + ctx context.Context, + downloadsDir *paths.Path, libRelease *librariesindex.Release, + downloadCB rpc.DownloadProgressCB, taskCB rpc.TaskProgressCB, + queryParameter string, + settings *configuration.Settings, +) error { taskCB(&rpc.TaskProgress{Name: i18n.Tr("Downloading %s", libRelease)}) - config, err := settings.DownloaderConfig() + config, err := settings.DownloaderConfig(ctx) if err != nil { return &cmderrors.FailedDownloadError{Message: i18n.Tr("Can't download library"), Cause: err} } diff --git a/internal/arduino/resources/helpers_test.go b/internal/arduino/resources/helpers_test.go index 611de8dd518..ad1d6805254 100644 --- a/internal/arduino/resources/helpers_test.go +++ b/internal/arduino/resources/helpers_test.go @@ -55,7 +55,7 @@ func TestDownloadApplyUserAgentHeaderUsingConfig(t *testing.T) { settings := configuration.NewSettings() settings.Set("network.user_agent_ext", goldUserAgentValue) - config, err := settings.DownloaderConfig() + config, err := settings.DownloaderConfig(context.Background()) require.NoError(t, err) err = r.Download(context.Background(), tmp, config, "", func(progress *rpc.DownloadProgress) {}, "") require.NoError(t, err) diff --git a/internal/cli/configuration/network.go b/internal/cli/configuration/network.go index c570d0a3b82..43b502a03fb 100644 --- a/internal/cli/configuration/network.go +++ b/internal/cli/configuration/network.go @@ -16,18 +16,21 @@ package configuration import ( + "context" "errors" "fmt" "net/http" "net/url" "os" "runtime" + "strings" "time" "github.com/arduino/arduino-cli/commands/cmderrors" "github.com/arduino/arduino-cli/internal/i18n" "github.com/arduino/arduino-cli/internal/version" "go.bug.st/downloader/v2" + "google.golang.org/grpc/metadata" ) // UserAgent returns the user agent (mainly used by HTTP clients) @@ -84,17 +87,23 @@ func (settings *Settings) NetworkProxy() (*url.URL, error) { } // NewHttpClient returns a new http client for use in the arduino-cli -func (settings *Settings) NewHttpClient() (*http.Client, error) { +func (settings *Settings) NewHttpClient(ctx context.Context) (*http.Client, error) { proxy, err := settings.NetworkProxy() if err != nil { return nil, err } + userAgent := settings.UserAgent() + if md, ok := metadata.FromIncomingContext(ctx); ok { + if extraUserAgent := strings.Join(md.Get("user-agent"), " "); extraUserAgent != "" { + userAgent += " " + extraUserAgent + } + } return &http.Client{ Transport: &httpClientRoundTripper{ transport: &http.Transport{ Proxy: http.ProxyURL(proxy), }, - userAgent: settings.UserAgent(), + userAgent: userAgent, }, Timeout: settings.ConnectionTimeout(), }, nil @@ -111,8 +120,8 @@ func (h *httpClientRoundTripper) RoundTrip(req *http.Request) (*http.Response, e } // DownloaderConfig returns the downloader configuration based on current settings. -func (settings *Settings) DownloaderConfig() (downloader.Config, error) { - httpClient, err := settings.NewHttpClient() +func (settings *Settings) DownloaderConfig(ctx context.Context) (downloader.Config, error) { + httpClient, err := settings.NewHttpClient(ctx) if err != nil { return downloader.Config{}, &cmderrors.InvalidArgumentError{ Message: i18n.Tr("Could not connect via HTTP"), diff --git a/internal/cli/configuration/network_test.go b/internal/cli/configuration/network_test.go index 68cc84fdd59..563c0414589 100644 --- a/internal/cli/configuration/network_test.go +++ b/internal/cli/configuration/network_test.go @@ -16,6 +16,7 @@ package configuration_test import ( + "context" "fmt" "io" "net/http" @@ -35,7 +36,7 @@ func TestUserAgentHeader(t *testing.T) { settings := configuration.NewSettings() require.NoError(t, settings.Set("network.user_agent_ext", "test-user-agent")) - client, err := settings.NewHttpClient() + client, err := settings.NewHttpClient(context.Background()) require.NoError(t, err) request, err := http.NewRequest("GET", ts.URL, nil) @@ -59,7 +60,7 @@ func TestProxy(t *testing.T) { settings := configuration.NewSettings() settings.Set("network.proxy", ts.URL) - client, err := settings.NewHttpClient() + client, err := settings.NewHttpClient(context.Background()) require.NoError(t, err) request, err := http.NewRequest("GET", "http://arduino.cc", nil) @@ -83,7 +84,7 @@ func TestConnectionTimeout(t *testing.T) { if timeout != 0 { require.NoError(t, settings.Set("network.connection_timeout", "2s")) } - client, err := settings.NewHttpClient() + client, err := settings.NewHttpClient(context.Background()) require.NoError(t, err) request, err := http.NewRequest("GET", "http://arduino.cc", nil) From fc6d0a6cdb3b6d533d3986462bc990350cb8d651 Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Wed, 22 Jan 2025 17:09:32 +0100 Subject: [PATCH 4/6] Updated integration test --- internal/integrationtest/arduino-cli.go | 17 ++++++++++++++--- internal/integrationtest/http_server.go | 5 ++++- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/internal/integrationtest/arduino-cli.go b/internal/integrationtest/arduino-cli.go index c157a57d7a2..67c9ae61d62 100644 --- a/internal/integrationtest/arduino-cli.go +++ b/internal/integrationtest/arduino-cli.go @@ -123,6 +123,13 @@ func NewArduinoCliWithinEnvironment(env *Environment, config *ArduinoCLIConfig) // 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 CreateEnvForDaemon(t *testing.T) (*Environment, *ArduinoCLI) { + 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{ @@ -130,7 +137,7 @@ func CreateEnvForDaemon(t *testing.T) (*Environment, *ArduinoCLI) { UseSharedStagingFolder: true, }) - _ = cli.StartDaemon(false) + _ = cli.StartDaemon(false, userAgent) return env, cli } @@ -410,7 +417,7 @@ func (cli *ArduinoCLI) run(stdoutBuff, stderrBuff io.Writer, stdinBuff io.Reader } // 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 { args := []string{"daemon", "--json"} if cli.cliConfigPath != nil { args = append([]string{"--config-file", cli.cliConfigPath.String()}, args...) @@ -450,7 +457,11 @@ func (cli *ArduinoCLI) StartDaemon(verbose bool) string { for retries := 5; retries > 0; retries-- { time.Sleep(time.Second) - conn, err := grpc.NewClient(cli.daemonAddr, grpc.WithTransportCredentials(insecure.NewCredentials())) + conn, err := grpc.NewClient( + cli.daemonAddr, + grpc.WithTransportCredentials(insecure.NewCredentials()), + grpc.WithUserAgent(userAgent), + ) if err != nil { connErr = err continue diff --git a/internal/integrationtest/http_server.go b/internal/integrationtest/http_server.go index 6d04a2a4489..4255979402c 100644 --- a/internal/integrationtest/http_server.go +++ b/internal/integrationtest/http_server.go @@ -33,7 +33,10 @@ func (env *Environment) HTTPServeFile(port uint16, path *paths.Path, isDaemon bo 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") + 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") } }) server := &http.Server{ From 527f7d2568388bcfe6fa63b1126aa3467d3e76dd Mon Sep 17 00:00:00 2001 From: Cristian Maglie Date: Wed, 22 Jan 2025 17:32:34 +0100 Subject: [PATCH 5/6] Restore previous user-agent for package manager --- commands/instances.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/commands/instances.go b/commands/instances.go index 8264f23bae4..5b5fe09fcdb 100644 --- a/commands/instances.go +++ b/commands/instances.go @@ -43,6 +43,7 @@ import ( paths "github.com/arduino/go-paths-helper" "github.com/sirupsen/logrus" "google.golang.org/grpc/codes" + "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" ) @@ -63,6 +64,11 @@ func installTool(ctx context.Context, pm *packagemanager.PackageManager, tool *c // Create a new Instance ready to be initialized, supporting directories are also created. func (s *arduinoCoreServerImpl) Create(ctx context.Context, req *rpc.CreateRequest) (*rpc.CreateResponse, error) { + var userAgent string + if md, ok := metadata.FromIncomingContext(ctx); ok { + userAgent = strings.Join(md.Get("user-agent"), " ") + } + // Setup downloads directory downloadsDir := s.settings.DownloadsDir() if downloadsDir.NotExist() { @@ -87,7 +93,7 @@ func (s *arduinoCoreServerImpl) Create(ctx context.Context, req *rpc.CreateReque if err != nil { return nil, err } - inst, err := instances.Create(dataDir, packagesDir, userPackagesDir, downloadsDir, "", config) + inst, err := instances.Create(dataDir, packagesDir, userPackagesDir, downloadsDir, userAgent, config) if err != nil { return nil, err } From 2e7d6c201b1124bbec4a01b70f299896d7c1f540 Mon Sep 17 00:00:00 2001 From: MatteoPologruto Date: Thu, 23 Jan 2025 17:09:27 +0100 Subject: [PATCH 6/6] Apply review suggestions to the integration test --- internal/integrationtest/arduino-cli.go | 13 ++----- internal/integrationtest/core/core_test.go | 20 +++++----- .../integrationtest/daemon/daemon_test.go | 37 +++++++++++++++++-- internal/integrationtest/http_server.go | 9 +---- 4 files changed, 48 insertions(+), 31 deletions(-) diff --git a/internal/integrationtest/arduino-cli.go b/internal/integrationtest/arduino-cli.go index 67c9ae61d62..5236449d09a 100644 --- a/internal/integrationtest/arduino-cli.go +++ b/internal/integrationtest/arduino-cli.go @@ -123,13 +123,6 @@ func NewArduinoCliWithinEnvironment(env *Environment, config *ArduinoCLIConfig) // 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 CreateEnvForDaemon(t *testing.T) (*Environment, *ArduinoCLI) { - 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{ @@ -137,7 +130,7 @@ func CreateEnvForDaemonWithUserAgent(t *testing.T, userAgent string) (*Environme UseSharedStagingFolder: true, }) - _ = cli.StartDaemon(false, userAgent) + _ = cli.StartDaemon(false) return env, cli } @@ -417,7 +410,7 @@ func (cli *ArduinoCLI) run(stdoutBuff, stderrBuff io.Writer, stdinBuff io.Reader } // StartDaemon starts the Arduino CLI daemon. It returns the address of the daemon. -func (cli *ArduinoCLI) StartDaemon(verbose bool, userAgent string) string { +func (cli *ArduinoCLI) StartDaemon(verbose bool) string { args := []string{"daemon", "--json"} if cli.cliConfigPath != nil { args = append([]string{"--config-file", cli.cliConfigPath.String()}, args...) @@ -460,7 +453,7 @@ func (cli *ArduinoCLI) StartDaemon(verbose bool, userAgent string) string { conn, err := grpc.NewClient( cli.daemonAddr, grpc.WithTransportCredentials(insecure.NewCredentials()), - grpc.WithUserAgent(userAgent), + grpc.WithUserAgent("cli-test/0.0.0"), ) if err != nil { connErr = err diff --git a/internal/integrationtest/core/core_test.go b/internal/integrationtest/core/core_test.go index 4818997e996..a27b5f6d15a 100644 --- a/internal/integrationtest/core/core_test.go +++ b/internal/integrationtest/core/core_test.go @@ -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, false) + url := env.HTTPServeFile(8000, test_index) // Run update-index with our test index _, _, err := cli.Run("core", "update-index", "--additional-urls="+url.String()) @@ -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, false) + url := env.HTTPServeFile(8000, testIndex) // update custom index and install test core (installed cores affect `core search`) _, _, err := cli.Run("core", "update-index", "--additional-urls="+url.String()) @@ -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, false) + url := env.HTTPServeFile(8000, testIndex) // update custom index _, _, err := cli.Run("core", "update-index", "--additional-urls="+url.String()) @@ -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, false) + url := env.HTTPServeFile(8000, testIndex) // update custom index _, _, err := cli.Run("core", "update-index", "--additional-urls="+url.String()) @@ -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, false) + url := env.HTTPServeFile(8000, testIndex) // update custom index _, _, err := cli.Run("core", "update-index", "--additional-urls="+url.String()) @@ -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"), false) - env.HTTPServeFile(8081, paths.New("testdata", "core_with_postinst.zip"), false) + url := env.HTTPServeFile(8080, paths.New("testdata", "package_with_postinstall_index.json")) + env.HTTPServeFile(8081, paths.New("testdata", "core_with_postinst.zip")) _, _, err := cli.Run("core", "update-index", "--additional-urls", url.String()) require.NoError(t, err) @@ -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, false) + url := env.HTTPServeFile(8000, test_index) // Run update-index with our test index _, _, err := cli.Run("core", "update-index", "--additional-urls="+url.String()) @@ -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"), false).String() + url := env.HTTPServeFile(8000, paths.New("..", "testdata", "test_index.json")).String() t.Run("missing additional-urls", func(t *testing.T) { // update index @@ -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"), false).String() + url := env.HTTPServeFile(8000, paths.New("..", "testdata", "test_index.json")).String() additionalURLs := "--additional-urls=" + url _, _, err := cli.Run("core", "update-index", additionalURLs) diff --git a/internal/integrationtest/daemon/daemon_test.go b/internal/integrationtest/daemon/daemon_test.go index 7d800c84fa7..c90c5c7ac61 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,7 +566,33 @@ 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) { @@ -570,7 +600,8 @@ func TestDaemonUserAgent(t *testing.T) { })) // 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 +610,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()) } } diff --git a/internal/integrationtest/http_server.go b/internal/integrationtest/http_server.go index 4255979402c..dc6fd98e099 100644 --- a/internal/integrationtest/http_server.go +++ b/internal/integrationtest/http_server.go @@ -26,18 +26,11 @@ 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, isDaemon bool) *url.URL { +func (env *Environment) HTTPServeFile(port uint16, path *paths.Path) *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") - } }) server := &http.Server{ Addr: fmt.Sprintf(":%d", port),