Merge pull request PelicanPlatform#1304 from jhiemstrawisc/issue-1247

Reduce number of caches in `Link` header on director cache redirects to 6
jhiemstrawisc · May 21, 2024 · d41ef3b · d41ef3b
2 parents 5cafa6b + 798b1c7
commit d41ef3b
Show file tree

Hide file tree

Showing 4 changed files with 204 additions and 89 deletions.
diff --git a/director/advertise_test.go b/director/advertise_test.go
@@ -19,6 +19,7 @@
 package director
 
 import (
+	_ "embed"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -32,6 +33,11 @@ import (
 	"github.com/pelicanplatform/pelican/utils"
 )
 
+var (
+	//go:embed resources/mock_topology.json
+	mockTopology string
+)
+
 func TestParseServerAd(t *testing.T) {
 
 	server := utils.Server{
@@ -48,10 +54,10 @@ func TestParseServerAd(t *testing.T) {
 	// Check that we populate all of the fields correctly -- note that lat/long don't get updated
 	// until right before the ad is recorded, so we don't check for that here.
 	ad := parseServerAd(server, server_structs.OriginType)
-	assert.Equal(t, ad.URL.String(), "http://my-endpoint.com")
-	assert.Equal(t, ad.WebURL.String(), "")
-	assert.Equal(t, ad.Name, "MY_SERVER")
-	assert.Equal(t, ad.AuthURL, url.URL{})
+	assert.Equal(t, "http://my-endpoint.com", ad.URL.String())
+	assert.Equal(t, "", ad.WebURL.String())
+	assert.Equal(t, "MY_SERVER", ad.Name)
+	assert.Equal(t, url.URL{}, ad.AuthURL)
 	assert.True(t, ad.Type == server_structs.OriginType)
 
 	// A quick check that type is set correctly
@@ -60,90 +66,17 @@ func TestParseServerAd(t *testing.T) {
 
 	// A check that the authurl is set correctly for parsing a server ad from topology
 	ad = parseServerAd(osdf_server, server_structs.OriginType)
-	assert.Equal(t, ad.URL.String(), "http://my-endpoint.com")
-	assert.Equal(t, ad.AuthURL.String(), "https://my-auth-endpoint.com")
+	assert.Equal(t, "http://my-endpoint.com", ad.URL.String())
+	assert.Equal(t, "https://my-auth-endpoint.com", ad.AuthURL.String())
 }
 
 func JSONHandler(w http.ResponseWriter, r *http.Request) {
-	jsonResponse := `
-	{
-		"caches": [
-			{
-				"auth_endpoint": "https://cache-auth-endpoint.com",
-				"endpoint": "http://cache-endpoint.com",
-				"resource": "MY_CACHE"
-			}
-		],
-		"namespaces": [
-			{
-				"caches": [
-					{
-						"auth_endpoint": "https://cache-auth-endpoint.com",
-						"endpoint": "http://cache-endpoint.com",
-						"resource": "MY_CACHE"
-					}
-				],
-				"credential_generation": {
-					"base_path": "/server",
-					"issuer": "https://my-issuer.com",
-					"max_scope_depth": 3,
-					"strategy": "OAuth2",
-					"vault_issuer": null,
-					"vault_server": null
-				},
-				"scitokens": [
-					{
-						"base_path": ["/server"],
-						"issuer": "https://my-issuer.com",
-						"restricted_path": []
-					}
-				],
-				"dirlisthost": null,
-				"origins": [
-					{
-						"auth_endpoint": "https://origin1-auth-endpoint.com",
-						"endpoint": "http://origin1-endpoint.com",
-						"resource": "MY_ORIGIN1"
-					}
-				],
-				"path": "/my/server",
-				"readhttps": true,
-				"usetokenonread": true,
-				"writebackhost": "https://writeback.my-server.com"
-			},
-			{
-				"caches": [
-					{
-						"auth_endpoint": "https://cache-auth-endpoint.com",
-						"endpoint": "http://cache-endpoint.com",
-						"resource": "MY_CACHE"
-					}
-				],
-				"credential_generation": null,
-				"scitokens": [],
-				"dirlisthost": null,
-				"origins": [
-					{
-						"auth_endpoint": "https://origin2-auth-endpoint.com",
-						"endpoint": "http://origin2-endpoint.com",
-						"resource": "MY_ORIGIN2"
-					}
-				],
-				"path": "/my/server/2",
-				"readhttps": true,
-				"usetokenonread": false,
-				"writebackhost": null
-			}
-		]
-	}
-	`
-
 	// Set the Content-Type header to indicate JSON.
 	w.Header().Set("Content-Type", "application/json")
 
 	// Write the JSON response to the response body.
 	w.WriteHeader(http.StatusOK)
-	_, _ = w.Write([]byte(jsonResponse))
+	_, _ = w.Write([]byte(mockTopology))
 }
 func TestAdvertiseOSDF(t *testing.T) {
 	viper.Reset()
@@ -168,16 +101,16 @@ func TestAdvertiseOSDF(t *testing.T) {
 
 	// Test a few values. If they're correct, it indicates the whole process likely succeeded
 	nsAd, oAds, cAds := getAdsForPath("/my/server/path/to/file")
-	assert.Equal(t, nsAd.Path, "/my/server")
-	assert.Equal(t, nsAd.Generation[0].MaxScopeDepth, uint(3))
-	assert.Equal(t, oAds[0].AuthURL.String(), "https://origin1-auth-endpoint.com")
-	assert.Equal(t, cAds[0].URL.String(), "http://cache-endpoint.com")
+	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())
 
 	nsAd, oAds, cAds = getAdsForPath("/my/server/2/path/to/file")
-	assert.Equal(t, nsAd.Path, "/my/server/2")
-	assert.Equal(t, nsAd.PublicRead, true)
-	assert.Equal(t, oAds[0].AuthURL.String(), "https://origin2-auth-endpoint.com")
-	assert.Equal(t, cAds[0].URL.String(), "http://cache-endpoint.com")
+	assert.Equal(t, "/my/server/2", nsAd.Path)
+	assert.Equal(t, true, nsAd.PublicRead)
+	assert.Equal(t, "https://origin2-auth-endpoint.com", oAds[0].AuthURL.String())
+	assert.Equal(t, "http://cache-endpoint.com", cAds[0].URL.String())
 }
 
 func TestFindDownedTopologyCache(t *testing.T) {

diff --git a/director/director.go b/director/director.go
@@ -87,6 +87,11 @@ var (
 
 	originStatUtils      = make(map[string]originStatUtil)
 	originStatUtilsMutex = sync.RWMutex{}
+
+	// The number of caches to send in the Link header. As discussed in issue
+	// https://github.com/PelicanPlatform/pelican/issues/1247, the client stops
+	// after three attempts, so there's really no need to send every cache we know
+	cachesToSend = 6
 )
 
 func getRedirectURL(reqPath string, ad server_structs.ServerAd, requiresAuth bool) (redirectURL url.URL) {
@@ -281,7 +286,10 @@ func redirectToCache(ginCtx *gin.Context) {
 
 	linkHeader := ""
 	first := true
-	for idx, ad := range cacheAds {
+	if numCAds := len(cacheAds); numCAds < cachesToSend {
+		cachesToSend = numCAds
+	}
+	for idx, ad := range cacheAds[:cachesToSend] {
 		if first {
 			first = false
 		} else {

diff --git a/director/director_test.go b/director/director_test.go
@@ -1112,6 +1112,50 @@ func TestRedirects(t *testing.T) {
 		assert.NotEmpty(t, w.Header().Get("Location"))
 		assert.Equal(t, "https://example.com/api/v1.0/director/healthTest/pelican/monitoring/test.txt", w.Header().Get("Location"))
 	})
+
+	t.Run("redirect-link-header-length", func(t *testing.T) {
+		viper.Reset()
+		serverAds.DeleteAll()
+		t.Cleanup(func() {
+			viper.Reset()
+			serverAds.DeleteAll()
+		})
+
+		// Use ads generated via mock topology for generating list of caches
+		topoServer := httptest.NewServer(http.HandlerFunc(JSONHandler))
+		defer topoServer.Close()
+		viper.Set("Federation.TopologyNamespaceUrl", topoServer.URL)
+		viper.Set("Director.CacheSortMethod", "random")
+		// Populate ads for redirectToCache to use
+		err := AdvertiseOSDF()
+		require.NoError(t, err)
+
+		req, _ := http.NewRequest("GET", "/my/server", nil)
+		// Provide a few things so that redirectToCache doesn't choke
+		req.Header.Add("User-Agent", "pelican-v7.999.999")
+		req.Header.Add("X-Real-Ip", "128.104.153.60")
+
+		recorder := httptest.NewRecorder()
+		c, _ := gin.CreateTestContext(recorder)
+		c.Request = req
+
+		redirectToCache(c)
+		// We should have a random collection of 6 caches in the header
+		assert.Contains(t, c.Writer.Header().Get("Link"), "pri=6")
+		// We should not have a 7th cache in the header
+		assert.NotContains(t, c.Writer.Header().Get("Link"), "pri=7")
+
+		// Make sure we can still get a cache list with a smaller number of caches
+		req, _ = http.NewRequest("GET", "/my/server/2", nil)
+		req.Header.Add("User-Agent", "pelican-v7.999.999")
+		req.Header.Add("X-Real-Ip", "128.104.153.60")
+		c.Request = req
+
+		redirectToCache(c)
+		assert.Contains(t, c.Writer.Header().Get("Link"), "pri=1")
+		assert.NotContains(t, c.Writer.Header().Get("Link"), "pri=2")
+	})
+
 }
 
 func TestGetHealthTestFile(t *testing.T) {

diff --git a/director/resources/mock_topology.json b/director/resources/mock_topology.json
@@ -0,0 +1,130 @@
+    {
+        "caches": [
+            {
+                "auth_endpoint": "https://cache-auth-endpoint.com",
+                "endpoint": "http://cache-endpoint.com",
+                "resource": "MY_CACHE"
+            },
+            {
+                "auth_endpoint": "https://cache2.com",
+                "endpoint": "https://cache2.com",
+                "resource": "CACHE2"
+            },
+            {
+                "auth_endpoint": "https://cache3.com",
+                "endpoint": "https://cache3.com",
+                "resource": "CACHE3"
+            },
+            {
+                "auth_endpoint": "https://cache4.com",
+                "endpoint": "https://cache4.com",
+                "resource": "CACHE4"
+            },
+            {
+                "auth_endpoint": "https://cache5.com",
+                "endpoint": "https://cache5.com",
+                "resource": "CACHE5"
+            },
+            {
+                "auth_endpoint": "https://cache6.com",
+                "endpoint": "https://cache6.com",
+                "resource": "CACHE6"
+            },
+            {
+                "auth_endpoint": "https://cache7.com",
+                "endpoint": "https://cache7.com",
+                "resource": "CACHE7"
+            }
+        ],
+        "namespaces": [
+            {
+                "caches": [
+                    {
+                        "auth_endpoint": "https://cache-auth-endpoint.com",
+                        "endpoint": "http://cache-endpoint.com",
+                        "resource": "MY_CACHE"
+                    },
+                    {
+                        "auth_endpoint": "https://cache2.com",
+                        "endpoint": "https://cache2.com",
+                        "resource": "CACHE2"
+                    },
+                    {
+                        "auth_endpoint": "https://cache3.com",
+                        "endpoint": "https://cache3.com",
+                        "resource": "CACHE3"
+                    },
+                    {
+                        "auth_endpoint": "https://cache4.com",
+                        "endpoint": "https://cache4.com",
+                        "resource": "CACHE4"
+                    },
+                    {
+                        "auth_endpoint": "https://cache5.com",
+                        "endpoint": "https://cache5.com",
+                        "resource": "CACHE5"
+                    },
+                    {
+                        "auth_endpoint": "https://cache6.com",
+                        "endpoint": "https://cache6.com",
+                        "resource": "CACHE6"
+                    },
+                    {
+                        "auth_endpoint": "https://cache7.com",
+                        "endpoint": "https://cache7.com",
+                        "resource": "CACHE7"
+                    }
+                ],
+                "credential_generation": {
+                    "base_path": "/server",
+                    "issuer": "https://my-issuer.com",
+                    "max_scope_depth": 3,
+                    "strategy": "OAuth2",
+                    "vault_issuer": null,
+                    "vault_server": null
+                },
+                "scitokens": [
+                    {
+                        "base_path": ["/server"],
+                        "issuer": "https://my-issuer.com",
+                        "restricted_path": []
+                    }
+                ],
+                "dirlisthost": null,
+                "origins": [
+                    {
+                        "auth_endpoint": "https://origin1-auth-endpoint.com",
+                        "endpoint": "http://origin1-endpoint.com",
+                        "resource": "MY_ORIGIN1"
+                    }
+                ],
+                "path": "/my/server",
+                "readhttps": true,
+                "usetokenonread": true,
+                "writebackhost": "https://writeback.my-server.com"
+            },
+            {
+                "caches": [
+                    {
+                        "auth_endpoint": "https://cache-auth-endpoint.com",
+                        "endpoint": "http://cache-endpoint.com",
+                        "resource": "MY_CACHE"
+                    }
+                ],
+                "credential_generation": null,
+                "scitokens": [],
+                "dirlisthost": null,
+                "origins": [
+                    {
+                        "auth_endpoint": "https://origin2-auth-endpoint.com",
+                        "endpoint": "http://origin2-endpoint.com",
+                        "resource": "MY_ORIGIN2"
+                    }
+                ],
+                "path": "/my/server/2",
+                "readhttps": true,
+                "usetokenonread": false,
+                "writebackhost": null
+            }
+        ]
+    }