From acc7f2809843af116444d1bf54d8946ef5feaa1d Mon Sep 17 00:00:00 2001 From: Emma Turetsky Date: Tue, 6 Aug 2024 03:19:50 +0000 Subject: [PATCH 1/6] Fixed error where queries were being added to filename on get/copy --- client/fed_test.go | 76 ++++++++++++++++++++++++++++++++++++++++++++-- client/main.go | 10 ++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/client/fed_test.go b/client/fed_test.go index 1587bc2bc..b4697a5e5 100644 --- a/client/fed_test.go +++ b/client/fed_test.go @@ -156,6 +156,42 @@ func TestGetAndPutAuth(t *testing.T) { } }) + t.Run("testPelicanObjectPutAndGetWithQueryAndDestDir", func(t *testing.T) { + oldPref, err := config.SetPreferredPrefix(config.PelicanPrefix) + defer func() { + _, err := config.SetPreferredPrefix(oldPref) + require.NoError(t, err) + }() + assert.NoError(t, err) + + // Set path for object to upload/download + for _, export := range fed.Exports { + tempPath := tempFile.Name() + fileName := filepath.Base(tempPath) + uploadURL := fmt.Sprintf("pelican://%s:%s%s/%s/%s", param.Server_Hostname.GetString(), strconv.Itoa(param.Server_WebPort.GetInt()), + export.FederationPrefix, "osdf_osdf", fileName) + + // Upload the file with PUT + transferResultsUpload, err := client.DoPut(fed.Ctx, tempFile.Name(), uploadURL, false, client.WithTokenLocation(tempToken.Name())) + assert.NoError(t, err) + if err == nil { + assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) + } + + queryURL := uploadURL + "?directread" + tempDir := t.TempDir() + // Download that same file with GET + transferResultsDownload, err := client.DoGet(fed.Ctx, queryURL, tempDir, false, client.WithTokenLocation(tempToken.Name())) + assert.NoError(t, err) + if err == nil { + assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) + stats, err := os.Stat(filepath.Join(tempDir, fileName)) + assert.NoError(t, err) + assert.NotNil(t, stats) + } + } + }) + // We ran into a bug with the token option not working how it should. This test ensures that transfer option works how it should t.Run("testPelicanObjectPutAndGetWithWithTokenOption", func(t *testing.T) { oldPref, err := config.SetPreferredPrefix(config.PelicanPrefix) @@ -306,14 +342,14 @@ func TestCopyAuth(t *testing.T) { uploadURL := fmt.Sprintf("pelican://%s:%s%s/%s/%s", param.Server_Hostname.GetString(), strconv.Itoa(param.Server_WebPort.GetInt()), export.FederationPrefix, "osdf_osdf", fileName) - // Upload the file with PUT + // Upload the file with COPY transferResultsUpload, err := client.DoCopy(fed.Ctx, tempFile.Name(), uploadURL, false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) if err == nil { assert.Equal(t, int64(17), transferResultsUpload[0].TransferredBytes) } - // Download that same file with GET + // Download that same file with COPY transferResultsDownload, err := client.DoCopy(fed.Ctx, uploadURL, t.TempDir(), false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) if err == nil { @@ -322,6 +358,42 @@ func TestCopyAuth(t *testing.T) { } }) + t.Run("testPelicanObjectCopyWithQueryAndDestDir", func(t *testing.T) { + oldPref, err := config.SetPreferredPrefix(config.PelicanPrefix) + assert.NoError(t, err) + defer func() { + _, err := config.SetPreferredPrefix(oldPref) + require.NoError(t, err) + }() + + // Set path for object to upload/download + for _, export := range fed.Exports { + tempPath := tempFile.Name() + fileName := filepath.Base(tempPath) + uploadURL := fmt.Sprintf("pelican://%s:%s%s/%s/%s", param.Server_Hostname.GetString(), strconv.Itoa(param.Server_WebPort.GetInt()), + export.FederationPrefix, "osdf_osdf", fileName) + + // Upload the file with COPY + transferResultsUpload, err := client.DoCopy(fed.Ctx, tempFile.Name(), uploadURL, false, client.WithTokenLocation(tempToken.Name())) + assert.NoError(t, err) + if err == nil { + assert.Equal(t, int64(17), transferResultsUpload[0].TransferredBytes) + } + + queryURL := uploadURL + "?directread" + tempDir := t.TempDir() + // Download that same file with COPY + transferResultsDownload, err := client.DoCopy(fed.Ctx, queryURL, tempDir, false, client.WithTokenLocation(tempToken.Name())) + assert.NoError(t, err) + if err == nil { + assert.Equal(t, int64(17), transferResultsDownload[0].TransferredBytes) + stats, err := os.Stat(filepath.Join(tempDir, fileName)) + assert.NoError(t, err) + assert.NotNil(t, stats) + } + } + }) + // This tests object get/put with a pelican:// url t.Run("testOsdfObjectCopyWithPelicanUrl", func(t *testing.T) { oldPref, err := config.SetPreferredPrefix(config.OsdfPrefix) diff --git a/client/main.go b/client/main.go index 02eee9ecd..9d891b9a6 100644 --- a/client/main.go +++ b/client/main.go @@ -706,6 +706,11 @@ func DoGet(ctx context.Context, remoteObject string, localDestination string, re remoteObject = "/" + remoteObject } + qIndex := strings.Index(remoteObject, "?") + if qIndex != -1 { + remoteObject = remoteObject[:qIndex] + } + // get absolute path localDestPath, _ := filepath.Abs(localDestination) @@ -875,6 +880,11 @@ func DoCopy(ctx context.Context, sourceFile string, destination string, recursiv sourceFile = "/" + sourceFile } + qIndex := strings.Index(sourceFile, "?") + if qIndex != -1 { + sourceFile = sourceFile[:qIndex] + } + // get absolute path destPath, _ := filepath.Abs(destination) From 1ce32dfcd780804b171fd72b8a35148f571eea74 Mon Sep 17 00:00:00 2001 From: Emma Turetsky Date: Fri, 9 Aug 2024 16:24:02 +0000 Subject: [PATCH 2/6] Object get/copy now creates a url with the provided fed-discovery --- client/fed_test.go | 109 ++++++++++++++++--------------------------- client/main.go | 10 ---- cmd/object_copy.go | 5 ++ cmd/object_get.go | 5 ++ utils/client_test.go | 29 ++++++++++++ utils/utils.go | 22 +++++++++ 6 files changed, 101 insertions(+), 79 deletions(-) diff --git a/client/fed_test.go b/client/fed_test.go index b4697a5e5..bec5e65ed 100644 --- a/client/fed_test.go +++ b/client/fed_test.go @@ -47,6 +47,7 @@ import ( "github.com/pelicanplatform/pelican/test_utils" "github.com/pelicanplatform/pelican/token" "github.com/pelicanplatform/pelican/token_scopes" + "github.com/pelicanplatform/pelican/utils" ) var ( @@ -143,16 +144,12 @@ func TestGetAndPutAuth(t *testing.T) { // Upload the file with PUT transferResultsUpload, err := client.DoPut(fed.Ctx, tempFile.Name(), uploadURL, false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) - } + assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) // Download that same file with GET transferResultsDownload, err := client.DoGet(fed.Ctx, uploadURL, t.TempDir(), false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) - } + assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) } }) @@ -163,32 +160,32 @@ func TestGetAndPutAuth(t *testing.T) { require.NoError(t, err) }() assert.NoError(t, err) + viper.Set(param.Federation_DiscoveryUrl.GetName(), fmt.Sprintf("%s:%s", param.Server_Hostname.GetString(), strconv.Itoa(param.Server_WebPort.GetInt()))) // Set path for object to upload/download for _, export := range fed.Exports { tempPath := tempFile.Name() fileName := filepath.Base(tempPath) - uploadURL := fmt.Sprintf("pelican://%s:%s%s/%s/%s", param.Server_Hostname.GetString(), strconv.Itoa(param.Server_WebPort.GetInt()), + uploadURL := fmt.Sprintf("%s/%s/%s", export.FederationPrefix, "osdf_osdf", fileName) + uploadURL, err := utils.UrlWithFederation(uploadURL) + assert.NoError(t, err) + // Upload the file with PUT transferResultsUpload, err := client.DoPut(fed.Ctx, tempFile.Name(), uploadURL, false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) - } + assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) queryURL := uploadURL + "?directread" tempDir := t.TempDir() // Download that same file with GET transferResultsDownload, err := client.DoGet(fed.Ctx, queryURL, tempDir, false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) - stats, err := os.Stat(filepath.Join(tempDir, fileName)) - assert.NoError(t, err) - assert.NotNil(t, stats) - } + assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) + stats, err := os.Stat(filepath.Join(tempDir, fileName)) + assert.NoError(t, err) + assert.NotNil(t, stats) } }) @@ -211,16 +208,12 @@ func TestGetAndPutAuth(t *testing.T) { // Upload the file with PUT transferResultsUpload, err := client.DoPut(fed.Ctx, tempFile.Name(), uploadURL, false, client.WithToken(tmpTkn)) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) - } + assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) // Download that same file with GET transferResultsDownload, err := client.DoGet(fed.Ctx, uploadURL, t.TempDir(), false, client.WithToken(tmpTkn)) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) - } + assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) } }) @@ -243,16 +236,12 @@ func TestGetAndPutAuth(t *testing.T) { // Upload the file with PUT transferResultsUpload, err := client.DoPut(fed.Ctx, tempFile.Name(), uploadURL, false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) - } + assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) // Download that same file with GET transferResultsDownload, err := client.DoGet(fed.Ctx, uploadURL, t.TempDir(), false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) - } + assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) } }) @@ -283,16 +272,12 @@ func TestGetAndPutAuth(t *testing.T) { // Upload the file with PUT transferResultsUpload, err := client.DoPut(fed.Ctx, tempFile.Name(), uploadURL, false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) - } + assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) // Download that same file with GET transferResultsDownload, err := client.DoGet(fed.Ctx, uploadURL, t.TempDir(), false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) - } + assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) } }) t.Cleanup(func() { @@ -345,16 +330,12 @@ func TestCopyAuth(t *testing.T) { // Upload the file with COPY transferResultsUpload, err := client.DoCopy(fed.Ctx, tempFile.Name(), uploadURL, false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, int64(17), transferResultsUpload[0].TransferredBytes) - } + assert.Equal(t, int64(17), transferResultsUpload[0].TransferredBytes) // Download that same file with COPY transferResultsDownload, err := client.DoCopy(fed.Ctx, uploadURL, t.TempDir(), false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, int64(17), transferResultsDownload[0].TransferredBytes) - } + assert.Equal(t, int64(17), transferResultsDownload[0].TransferredBytes) } }) @@ -366,31 +347,32 @@ func TestCopyAuth(t *testing.T) { require.NoError(t, err) }() + viper.Set(param.Federation_DiscoveryUrl.GetName(), fmt.Sprintf("%s:%s", param.Server_Hostname.GetString(), strconv.Itoa(param.Server_WebPort.GetInt()))) + // Set path for object to upload/download for _, export := range fed.Exports { tempPath := tempFile.Name() fileName := filepath.Base(tempPath) - uploadURL := fmt.Sprintf("pelican://%s:%s%s/%s/%s", param.Server_Hostname.GetString(), strconv.Itoa(param.Server_WebPort.GetInt()), + uploadURL := fmt.Sprintf("%s/%s/%s", export.FederationPrefix, "osdf_osdf", fileName) + uploadURL, err := utils.UrlWithFederation(uploadURL) + assert.NoError(t, err) + // Upload the file with COPY transferResultsUpload, err := client.DoCopy(fed.Ctx, tempFile.Name(), uploadURL, false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, int64(17), transferResultsUpload[0].TransferredBytes) - } + assert.Equal(t, int64(17), transferResultsUpload[0].TransferredBytes) queryURL := uploadURL + "?directread" tempDir := t.TempDir() // Download that same file with COPY transferResultsDownload, err := client.DoCopy(fed.Ctx, queryURL, tempDir, false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, int64(17), transferResultsDownload[0].TransferredBytes) - stats, err := os.Stat(filepath.Join(tempDir, fileName)) - assert.NoError(t, err) - assert.NotNil(t, stats) - } + assert.Equal(t, int64(17), transferResultsDownload[0].TransferredBytes) + stats, err := os.Stat(filepath.Join(tempDir, fileName)) + assert.NoError(t, err) + assert.NotNil(t, stats) } }) @@ -413,16 +395,12 @@ func TestCopyAuth(t *testing.T) { // Upload the file with PUT transferResultsUpload, err := client.DoCopy(fed.Ctx, tempFile.Name(), uploadURL, false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) - } + assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) // Download that same file with GET transferResultsDownload, err := client.DoCopy(fed.Ctx, uploadURL, t.TempDir(), false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) - } + assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) } }) @@ -453,16 +431,13 @@ func TestCopyAuth(t *testing.T) { // Upload the file with PUT transferResultsUpload, err := client.DoCopy(fed.Ctx, tempFile.Name(), uploadURL, false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) - } + assert.Equal(t, transferResultsUpload[0].TransferredBytes, int64(17)) // Download that same file with GET transferResultsDownload, err := client.DoCopy(fed.Ctx, uploadURL, t.TempDir(), false, client.WithTokenLocation(tempToken.Name())) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) - } + assert.Equal(t, transferResultsDownload[0].TransferredBytes, transferResultsUpload[0].TransferredBytes) + } }) t.Cleanup(func() { @@ -502,9 +477,7 @@ func TestGetPublicRead(t *testing.T) { // Download the file with GET. Shouldn't need a token to succeed transferResults, err := client.DoGet(fed.Ctx, uploadURL, t.TempDir(), false) assert.NoError(t, err) - if err == nil { - assert.Equal(t, transferResults[0].TransferredBytes, int64(17)) - } + assert.Equal(t, transferResults[0].TransferredBytes, int64(17)) } }) t.Cleanup(func() { @@ -614,10 +587,8 @@ func TestObjectStat(t *testing.T) { // Stat the file statInfo, err := client.DoStat(fed.Ctx, statUrl) assert.NoError(t, err) - if err == nil { - assert.Equal(t, int64(17), int64(statInfo.Size)) - assert.Equal(t, "test.txt", statInfo.Name) - } + assert.Equal(t, int64(17), int64(statInfo.Size)) + assert.Equal(t, "test.txt", statInfo.Name) }) // Ensure stat fails if it does not recognize the url scheme diff --git a/client/main.go b/client/main.go index 9d891b9a6..02eee9ecd 100644 --- a/client/main.go +++ b/client/main.go @@ -706,11 +706,6 @@ func DoGet(ctx context.Context, remoteObject string, localDestination string, re remoteObject = "/" + remoteObject } - qIndex := strings.Index(remoteObject, "?") - if qIndex != -1 { - remoteObject = remoteObject[:qIndex] - } - // get absolute path localDestPath, _ := filepath.Abs(localDestination) @@ -880,11 +875,6 @@ func DoCopy(ctx context.Context, sourceFile string, destination string, recursiv sourceFile = "/" + sourceFile } - qIndex := strings.Index(sourceFile, "?") - if qIndex != -1 { - sourceFile = sourceFile[:qIndex] - } - // get absolute path destPath, _ := filepath.Abs(destination) diff --git a/cmd/object_copy.go b/cmd/object_copy.go index e5678017e..13c8615b0 100644 --- a/cmd/object_copy.go +++ b/cmd/object_copy.go @@ -202,6 +202,11 @@ func copyMain(cmd *cobra.Command, args []string) { lastSrc := "" for _, src := range source { + src, err = utils.UrlWithFederation(src) + if err != nil { + lastSrc = src + break + } isRecursive, _ := cmd.Flags().GetBool("recursive") _, result = client.DoCopy(ctx, src, dest, isRecursive, client.WithCallback(pb.callback), client.WithTokenLocation(tokenLocation), client.WithCaches(caches...)) if result != nil { diff --git a/cmd/object_get.go b/cmd/object_get.go index 98d2783c1..eeab65d7c 100644 --- a/cmd/object_get.go +++ b/cmd/object_get.go @@ -121,6 +121,11 @@ func getMain(cmd *cobra.Command, args []string) { lastSrc := "" for _, src := range source { + src, err = utils.UrlWithFederation(src) + if err != nil { + lastSrc = src + break + } isRecursive, _ := cmd.Flags().GetBool("recursive") _, result = client.DoGet(ctx, src, dest, isRecursive, client.WithCallback(pb.callback), client.WithTokenLocation(tokenLocation), client.WithCaches(caches...)) if result != nil { diff --git a/utils/client_test.go b/utils/client_test.go index 20efd4c21..0cdde9d6f 100644 --- a/utils/client_test.go +++ b/utils/client_test.go @@ -22,7 +22,10 @@ import ( "net/url" "testing" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" + + "github.com/pelicanplatform/pelican/param" ) // Test the functionality of CheckValidQuery and all its edge cases @@ -238,3 +241,29 @@ func TestExtractVersionAndServiceFromUserAgent(t *testing.T) { assert.Equal(t, 0, len(service)) }) } + +func TestUrlWithFederation(t *testing.T) { + viper.Reset() + defer viper.Reset() + pelUrl := "pelican://somefederation/namespace/test.txt" + + t.Run("testNoFederation", func(t *testing.T) { + str, err := UrlWithFederation(pelUrl) + assert.NoError(t, err) + assert.Equal(t, pelUrl, str) + }) + + t.Run("testFederationAndHost", func(t *testing.T) { + viper.Set(param.Federation_DiscoveryUrl.GetName(), "somefederation") + _, err := UrlWithFederation(pelUrl) + assert.Error(t, err) + assert.Equal(t, "Source URL should not have a host when the Federation_DiscoveryUrl is set", err.Error()) + }) + + t.Run("testFederationNoHost", func(t *testing.T) { + namespaceOnly := "/namespace/test.txt" + str, err := UrlWithFederation(namespaceOnly) + assert.NoError(t, err) + assert.Equal(t, pelUrl, str) + }) +} diff --git a/utils/utils.go b/utils/utils.go index 0628e78d4..7db12ab52 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -19,6 +19,7 @@ package utils import ( + "fmt" "net" "net/url" "regexp" @@ -29,6 +30,8 @@ import ( log "github.com/sirupsen/logrus" "golang.org/x/text/cases" "golang.org/x/text/language" + + "github.com/pelicanplatform/pelican/param" ) // snakeCaseToCamelCase converts a snake case string to camel case. @@ -127,3 +130,22 @@ func ExtractVersionAndServiceFromUserAgent(userAgent string) (reqVer, service st service = (strings.Split(userAgentSplit[0], "-"))[1] return reqVer, service } + +func UrlWithFederation(remoteUrl string) (string, error) { + if param.Federation_DiscoveryUrl.IsSet() { + parsedUrl, err := url.Parse(remoteUrl) + if err != nil { + newErr := errors.New(fmt.Sprintf("error parsing source url: %s", err)) + return "", newErr + } + if parsedUrl.Host != "" { + newErr := errors.New("Source URL should not have a host when the Federation_DiscoveryUrl is set") + return "", newErr + } + + parsedUrl.Host = param.Federation_DiscoveryUrl.GetString() + parsedUrl.Scheme = "pelican" + return parsedUrl.String(), nil + } + return remoteUrl, nil +} From 4d8125af084710ab656684d3725c485775976160 Mon Sep 17 00:00:00 2001 From: Emma Turetsky Date: Fri, 9 Aug 2024 20:15:00 +0000 Subject: [PATCH 3/6] Addressed PR comments --- utils/client_test.go | 9 --------- utils/utils.go | 7 ++++--- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/utils/client_test.go b/utils/client_test.go index 0cdde9d6f..ca960fac0 100644 --- a/utils/client_test.go +++ b/utils/client_test.go @@ -24,8 +24,6 @@ import ( "github.com/spf13/viper" "github.com/stretchr/testify/assert" - - "github.com/pelicanplatform/pelican/param" ) // Test the functionality of CheckValidQuery and all its edge cases @@ -253,13 +251,6 @@ func TestUrlWithFederation(t *testing.T) { assert.Equal(t, pelUrl, str) }) - t.Run("testFederationAndHost", func(t *testing.T) { - viper.Set(param.Federation_DiscoveryUrl.GetName(), "somefederation") - _, err := UrlWithFederation(pelUrl) - assert.Error(t, err) - assert.Equal(t, "Source URL should not have a host when the Federation_DiscoveryUrl is set", err.Error()) - }) - t.Run("testFederationNoHost", func(t *testing.T) { namespaceOnly := "/namespace/test.txt" str, err := UrlWithFederation(namespaceOnly) diff --git a/utils/utils.go b/utils/utils.go index 7db12ab52..c6f7d61ca 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -131,6 +131,9 @@ func ExtractVersionAndServiceFromUserAgent(userAgent string) (reqVer, service st return reqVer, service } +// Given a remote url with no host (a namespace path, essentially), then if the Federation.DiscoveryURL is +// set (either via a yaml file or via the command line flags), create a full pelican protocol URL by combining +// the two func UrlWithFederation(remoteUrl string) (string, error) { if param.Federation_DiscoveryUrl.IsSet() { parsedUrl, err := url.Parse(remoteUrl) @@ -139,10 +142,8 @@ func UrlWithFederation(remoteUrl string) (string, error) { return "", newErr } if parsedUrl.Host != "" { - newErr := errors.New("Source URL should not have a host when the Federation_DiscoveryUrl is set") - return "", newErr + return remoteUrl, nil } - parsedUrl.Host = param.Federation_DiscoveryUrl.GetString() parsedUrl.Scheme = "pelican" return parsedUrl.String(), nil From af9b11c604103afa104b5b2c8e3d4f804d8afd5e Mon Sep 17 00:00:00 2001 From: Emma Turetsky Date: Fri, 9 Aug 2024 20:29:13 +0000 Subject: [PATCH 4/6] Fixed test --- utils/client_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/utils/client_test.go b/utils/client_test.go index ca960fac0..02540bc25 100644 --- a/utils/client_test.go +++ b/utils/client_test.go @@ -24,6 +24,8 @@ import ( "github.com/spf13/viper" "github.com/stretchr/testify/assert" + + "github.com/pelicanplatform/pelican/param" ) // Test the functionality of CheckValidQuery and all its edge cases @@ -252,6 +254,7 @@ func TestUrlWithFederation(t *testing.T) { }) t.Run("testFederationNoHost", func(t *testing.T) { + viper.Set(param.Federation_DiscoveryUrl.GetName(), "somefederation") namespaceOnly := "/namespace/test.txt" str, err := UrlWithFederation(namespaceOnly) assert.NoError(t, err) From 5fabd631e3fc7a3ea534d6a18be6dbdc49d2e29b Mon Sep 17 00:00:00 2001 From: Emma Turetsky Date: Mon, 12 Aug 2024 16:41:43 +0000 Subject: [PATCH 5/6] Fixed url creation from provided federation -- Now only puts in the host into the resultant pelican url -- Adjusted tests to account for this -- Fixed the return value for UrlWithFederation to give the proper string for error messages -- Added path checking in the provided federation url as well as a test for it --- client/fed_test.go | 4 ++-- cmd/object_copy.go | 4 ++-- cmd/object_get.go | 4 ++-- utils/client_test.go | 15 ++++++++++++--- utils/utils.go | 13 +++++++++++-- 5 files changed, 29 insertions(+), 11 deletions(-) diff --git a/client/fed_test.go b/client/fed_test.go index bec5e65ed..9a849b371 100644 --- a/client/fed_test.go +++ b/client/fed_test.go @@ -160,7 +160,7 @@ func TestGetAndPutAuth(t *testing.T) { require.NoError(t, err) }() assert.NoError(t, err) - viper.Set(param.Federation_DiscoveryUrl.GetName(), fmt.Sprintf("%s:%s", param.Server_Hostname.GetString(), strconv.Itoa(param.Server_WebPort.GetInt()))) + viper.Set(param.Federation_DiscoveryUrl.GetName(), fmt.Sprintf("%s://%s:%s", "https", param.Server_Hostname.GetString(), strconv.Itoa(param.Server_WebPort.GetInt()))) // Set path for object to upload/download for _, export := range fed.Exports { @@ -347,7 +347,7 @@ func TestCopyAuth(t *testing.T) { require.NoError(t, err) }() - viper.Set(param.Federation_DiscoveryUrl.GetName(), fmt.Sprintf("%s:%s", param.Server_Hostname.GetString(), strconv.Itoa(param.Server_WebPort.GetInt()))) + viper.Set(param.Federation_DiscoveryUrl.GetName(), fmt.Sprintf("%s://%s:%s", "https", param.Server_Hostname.GetString(), strconv.Itoa(param.Server_WebPort.GetInt()))) // Set path for object to upload/download for _, export := range fed.Exports { diff --git a/cmd/object_copy.go b/cmd/object_copy.go index 13c8615b0..344194dd2 100644 --- a/cmd/object_copy.go +++ b/cmd/object_copy.go @@ -202,8 +202,8 @@ func copyMain(cmd *cobra.Command, args []string) { lastSrc := "" for _, src := range source { - src, err = utils.UrlWithFederation(src) - if err != nil { + src, result = utils.UrlWithFederation(src) + if result != nil { lastSrc = src break } diff --git a/cmd/object_get.go b/cmd/object_get.go index eeab65d7c..fba902cb5 100644 --- a/cmd/object_get.go +++ b/cmd/object_get.go @@ -121,8 +121,8 @@ func getMain(cmd *cobra.Command, args []string) { lastSrc := "" for _, src := range source { - src, err = utils.UrlWithFederation(src) - if err != nil { + src, result = utils.UrlWithFederation(src) + if result != nil { lastSrc = src break } diff --git a/utils/client_test.go b/utils/client_test.go index 02540bc25..44358f9af 100644 --- a/utils/client_test.go +++ b/utils/client_test.go @@ -19,6 +19,7 @@ package utils import ( + "fmt" "net/url" "testing" @@ -245,7 +246,7 @@ func TestExtractVersionAndServiceFromUserAgent(t *testing.T) { func TestUrlWithFederation(t *testing.T) { viper.Reset() defer viper.Reset() - pelUrl := "pelican://somefederation/namespace/test.txt" + pelUrl := "pelican://somefederation.org/namespace/test.txt" t.Run("testNoFederation", func(t *testing.T) { str, err := UrlWithFederation(pelUrl) @@ -253,11 +254,19 @@ func TestUrlWithFederation(t *testing.T) { assert.Equal(t, pelUrl, str) }) - t.Run("testFederationNoHost", func(t *testing.T) { - viper.Set(param.Federation_DiscoveryUrl.GetName(), "somefederation") + t.Run("testFederationWithFedHost", func(t *testing.T) { + viper.Set(param.Federation_DiscoveryUrl.GetName(), "https://somefederation.org") namespaceOnly := "/namespace/test.txt" str, err := UrlWithFederation(namespaceOnly) assert.NoError(t, err) assert.Equal(t, pelUrl, str) }) + + t.Run("testFederationWithPathComponent", func(t *testing.T) { + viper.Set(param.Federation_DiscoveryUrl.GetName(), "somefederation.org/path") + namespaceOnly := "/namespace/test.txt" + _, err := UrlWithFederation(namespaceOnly) + assert.Error(t, err) + assert.EqualError(t, err, fmt.Sprintf("provided federation url %s has a path component", param.Federation_DiscoveryUrl.GetString())) + }) } diff --git a/utils/utils.go b/utils/utils.go index c6f7d61ca..d1d3654fd 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -139,12 +139,21 @@ func UrlWithFederation(remoteUrl string) (string, error) { parsedUrl, err := url.Parse(remoteUrl) if err != nil { newErr := errors.New(fmt.Sprintf("error parsing source url: %s", err)) - return "", newErr + return remoteUrl, newErr } if parsedUrl.Host != "" { return remoteUrl, nil } - parsedUrl.Host = param.Federation_DiscoveryUrl.GetString() + parsedDiscUrl, err := url.Parse(param.Federation_DiscoveryUrl.GetString()) + if err != nil { + newErr := errors.New(fmt.Sprintf("error parsing discovery url: %s", err)) + return remoteUrl, newErr + } + if parsedDiscUrl.Path != "" { + newErr := errors.New(fmt.Sprintf("provided federation url %s has a path component", param.Federation_DiscoveryUrl.GetString())) + return remoteUrl, newErr + } + parsedUrl.Host = parsedDiscUrl.Host parsedUrl.Scheme = "pelican" return parsedUrl.String(), nil } From 6199f703c47d9b82bfc4b7712fb5dcd2b202fe28 Mon Sep 17 00:00:00 2001 From: Emma Turetsky Date: Mon, 12 Aug 2024 18:16:26 +0000 Subject: [PATCH 6/6] Fixed an issue that occurred with the federation url had not host but also had a path --- utils/client_test.go | 16 ++++++++++++++++ utils/utils.go | 5 +++++ 2 files changed, 21 insertions(+) diff --git a/utils/client_test.go b/utils/client_test.go index 44358f9af..38bef2ca4 100644 --- a/utils/client_test.go +++ b/utils/client_test.go @@ -254,6 +254,14 @@ func TestUrlWithFederation(t *testing.T) { assert.Equal(t, pelUrl, str) }) + t.Run("testFederationNoHost", func(t *testing.T) { + viper.Set(param.Federation_DiscoveryUrl.GetName(), "somefederation.org") + namespaceOnly := "/namespace/test.txt" + str, err := UrlWithFederation(namespaceOnly) + assert.NoError(t, err) + assert.Equal(t, pelUrl, str) + }) + t.Run("testFederationWithFedHost", func(t *testing.T) { viper.Set(param.Federation_DiscoveryUrl.GetName(), "https://somefederation.org") namespaceOnly := "/namespace/test.txt" @@ -269,4 +277,12 @@ func TestUrlWithFederation(t *testing.T) { assert.Error(t, err) assert.EqualError(t, err, fmt.Sprintf("provided federation url %s has a path component", param.Federation_DiscoveryUrl.GetString())) }) + + t.Run("testFederationPathComponentWithHost", func(t *testing.T) { + viper.Set(param.Federation_DiscoveryUrl.GetName(), "https://somefederation.org/path") + namespaceOnly := "/namespace/test.txt" + _, err := UrlWithFederation(namespaceOnly) + assert.Error(t, err) + assert.EqualError(t, err, fmt.Sprintf("provided federation url %s has a path component", param.Federation_DiscoveryUrl.GetString())) + }) } diff --git a/utils/utils.go b/utils/utils.go index d1d3654fd..a2d3464f1 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -149,6 +149,11 @@ func UrlWithFederation(remoteUrl string) (string, error) { newErr := errors.New(fmt.Sprintf("error parsing discovery url: %s", err)) return remoteUrl, newErr } + if parsedDiscUrl.Scheme == "" { + parsedDiscUrl.Scheme = "https" + updatedDiscString := parsedDiscUrl.String() + parsedDiscUrl, _ = url.Parse(updatedDiscString) + } if parsedDiscUrl.Path != "" { newErr := errors.New(fmt.Sprintf("provided federation url %s has a path component", param.Federation_DiscoveryUrl.GetString())) return remoteUrl, newErr