From 3b41c4f26b2b19b2306e91d9c40a25212d0715c6 Mon Sep 17 00:00:00 2001 From: Emma Turetsky Date: Fri, 17 Jan 2025 18:18:39 +0000 Subject: [PATCH] The director only redirects to topology caches when auth is required --- director/advertise_test.go | 2 +- director/director.go | 13 ++++- director/director_test.go | 70 +++++++++++++++++++++++++++ director/resources/mock_topology.json | 52 +++++++++++++++----- 4 files changed, 123 insertions(+), 14 deletions(-) diff --git a/director/advertise_test.go b/director/advertise_test.go index 10a6a73ee..9f119be50 100644 --- a/director/advertise_test.go +++ b/director/advertise_test.go @@ -233,7 +233,7 @@ func TestAdvertiseOSDF(t *testing.T) { assert.Equal(t, "/my/server", nsAd.Path) assert.Equal(t, uint(3), nsAd.Generation[0].MaxScopeDepth) assert.Equal(t, "https://origin1-auth-endpoint.com", oAds[0].AuthURL.String()) - assert.Equal(t, "https://cache2.com", cAds[0].URL.String()) + assert.Equal(t, "http://cache2.com", cAds[0].URL.String()) // Check that various capabilities have survived until this point. Because these are from topology, // origin and namespace caps should be the same assert.True(t, oAds[0].Caps.Writes) diff --git a/director/director.go b/director/director.go index 13e2c487e..d457be91f 100644 --- a/director/director.go +++ b/director/director.go @@ -377,7 +377,7 @@ func redirectToCache(ginCtx *gin.Context) { // If either disableStat or skipstat is set, then skip the stat query skipStat := ginCtx.Request.URL.Query().Has("skipstat") || disableStat - namespaceAd, originAds, cacheAds := getAdsForPath(reqPath) + namespaceAd, originAds, prelim_cacheAds := getAdsForPath(reqPath) // if GetAdsForPath doesn't find any ads because the prefix doesn't exist, we should // report the lack of path first -- this is most important for the user because it tells them // they're trying to get an object that simply doesn't exist @@ -395,6 +395,17 @@ func redirectToCache(ginCtx *gin.Context) { log.Errorf("Failed to get depth attribute for the redirecting request to %q, with best match namespace prefix %q", reqPath, namespaceAd.Path) } + // If the namespace doesn't require a token for reads, remove all topology only ads in order to + // prevent redirection to the non-auth version of these caches + cacheAds := []server_structs.ServerAd{} + for _, pAd := range prelim_cacheAds { + if namespaceAd.Caps.PublicReads && pAd.FromTopology { + continue + } else { + cacheAds = append(cacheAds, pAd) + } + } + // If the namespace requires a token yet there's no token available, skip the stat. if !namespaceAd.Caps.PublicReads && reqParams.Get("authz") == "" { skipStat = true diff --git a/director/director_test.go b/director/director_test.go index 1ffaceb0d..e5ea132c6 100644 --- a/director/director_test.go +++ b/director/director_test.go @@ -1771,6 +1771,76 @@ func TestRedirects(t *testing.T) { assert.NotContains(t, c.Writer.Header().Get("Link"), "pri=2") }) + t.Run("no-redirect-to-topology-cache-public-reads", func(t *testing.T) { + server_utils.ResetTestState() + t.Cleanup(func() { + server_utils.ResetTestState() + }) + + viper.Set("Director.CacheSortMethod", "random") + + // Make sure the http cache from topology isn't included in the cache list + req, _ := http.NewRequest("GET", "/my/server/3", nil) + req.Header.Add("User-Agent", "pelican-client/7.6.1") + req.Header.Add("X-Real-Ip", "128.104.153.60") + + pelCacheAd := server_structs.ServerAd{ + Name: "pel-cache", + URL: url.URL{ + Scheme: "https", + Host: "pelcache.test.edu", + }, + Type: server_structs.CacheType.String(), + } + + nsAd := server_structs.NamespaceAdV2{ + Caps: server_structs.Capabilities{PublicReads: true}, + Path: "/my/server/3", + Issuer: []server_structs.TokenIssuer{{ + IssuerUrl: url.URL{ + Scheme: "https", + Host: "wisc.edu", + }, + }, + }, + } + + cSlice := []server_structs.NamespaceAdV2{nsAd} + recordAd(context.Background(), pelCacheAd, &cSlice) + + recorder := httptest.NewRecorder() + c, _ := gin.CreateTestContext(recorder) + c.Request = req + + redirectToCache(c) + + assert.Contains(t, c.Writer.Header().Get("Link"), "pelcache.test.edu") + assert.NotContains(t, c.Writer.Header().Get("Link"), "http:") + }) + + t.Run("redirect-to-topology-caches-auth-reads", func(t *testing.T) { + server_utils.ResetTestState() + t.Cleanup(func() { + server_utils.ResetTestState() + }) + + viper.Set("Director.CacheSortMethod", "random") + + // Make sure the http cache from topology isn't included in the cache list + req, _ := http.NewRequest("GET", "/my/server", nil) + req.Header.Add("User-Agent", "pelican-client/7.6.1") + req.Header.Add("X-Real-Ip", "128.104.153.60") + + recorder := httptest.NewRecorder() + c, _ := gin.CreateTestContext(recorder) + c.Request = req + + redirectToCache(c) + + assert.Contains(t, c.Writer.Header().Get("Link"), "pri=6") + assert.NotContains(t, c.Writer.Header().Get("Link"), "http:") + }) + // Make sure collections-url is correctly populated when the ns/origin comes from topology t.Run("collections-url-from-topology", func(t *testing.T) { server_utils.ResetTestState() diff --git a/director/resources/mock_topology.json b/director/resources/mock_topology.json index 30b1c9b59..cc411093b 100644 --- a/director/resources/mock_topology.json +++ b/director/resources/mock_topology.json @@ -7,32 +7,32 @@ }, { "auth_endpoint": "https://cache2.com", - "endpoint": "https://cache2.com", + "endpoint": "http://cache2.com", "resource": "CACHE2" }, { "auth_endpoint": "https://cache3.com", - "endpoint": "https://cache3.com", + "endpoint": "http://cache3.com", "resource": "CACHE3" }, { "auth_endpoint": "https://cache4.com", - "endpoint": "https://cache4.com", + "endpoint": "http://cache4.com", "resource": "CACHE4" }, { "auth_endpoint": "https://cache5.com", - "endpoint": "https://cache5.com", + "endpoint": "http://cache5.com", "resource": "CACHE5" }, { "auth_endpoint": "https://cache6.com", - "endpoint": "https://cache6.com", + "endpoint": "http://cache6.com", "resource": "CACHE6" }, { "auth_endpoint": "https://cache7.com", - "endpoint": "https://cache7.com", + "endpoint": "http://cache7.com", "resource": "CACHE7" } ], @@ -46,32 +46,32 @@ }, { "auth_endpoint": "https://cache2.com", - "endpoint": "https://cache2.com", + "endpoint": "http://cache2.com", "resource": "CACHE2" }, { "auth_endpoint": "https://cache3.com", - "endpoint": "https://cache3.com", + "endpoint": "http://cache3.com", "resource": "CACHE3" }, { "auth_endpoint": "https://cache4.com", - "endpoint": "https://cache4.com", + "endpoint": "http://cache4.com", "resource": "CACHE4" }, { "auth_endpoint": "https://cache5.com", - "endpoint": "https://cache5.com", + "endpoint": "http://cache5.com", "resource": "CACHE5" }, { "auth_endpoint": "https://cache6.com", - "endpoint": "https://cache6.com", + "endpoint": "http://cache6.com", "resource": "CACHE6" }, { "auth_endpoint": "https://cache7.com", - "endpoint": "https://cache7.com", + "endpoint": "http://cache7.com", "resource": "CACHE7" } ], @@ -125,6 +125,34 @@ "readhttps": true, "usetokenonread": false, "writebackhost": null + }, + { + "caches": [ + { + "auth_endpoint": "https://cache3.com", + "endpoint": "http://cache3.com", + "resource": "CACHE3" + }, + { + "auth_endpoint": "https://cache4.com", + "endpoint": "http://cache4.com", + "resource": "CACHE4" + } + ], + "credential_generation": null, + "scitokens": [], + "dirlisthost": null, + "origins": [ + { + "auth_endpoint": "https://origin3-auth-endpoint.com", + "endpoint": "http://origin3-endpoint.com", + "resource": "MY_ORIGIN3" + } + ], + "path": "/my/server/3", + "readhttps": true, + "usetokenonread": false, + "writebackhost": null } ] }