Merge pull request #1753 from patrickbrophy/fix-issue-1725

Add Version info to ServerAd
PelicanPlatform · Dec 11, 2024 · ac41f77 · ac41f77
2 parents dd2b3f2 + d302ae6
commit ac41f77
Show file tree

Hide file tree

Showing 5 changed files with 210 additions and 0 deletions.
diff --git a/cache/advertise.go b/cache/advertise.go
@@ -49,6 +49,7 @@ func (server *CacheServer) CreateAdvertisement(name, originUrl, originWebUrl str
 		DataURL:        originUrl,
 		WebURL:         originWebUrl,
 		Namespaces:     server.GetNamespaceAds(),
+		Version:        config.GetVersion(),
 	}
 
 	return &ad, nil

diff --git a/director/director.go b/director/director.go
@@ -1137,6 +1137,24 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType server_s
 		adV2.DisableDirectorTest = true
 	}
 
+	// if we didn't receive a version from the ad but we were able to extract the request version from the user agent,
+	// then we can fallback to the request version
+	// otherwise, we set the version to unknown because our sources of truth are not available
+	if adV2.Version == "" && reqVer != nil {
+		adV2.Version = reqVer.String()
+	} else if adV2.Version != "" && reqVer != nil {
+		parsedAdVersion, err := version.NewVersion(adV2.Version)
+		if err != nil {
+			// ad version was not a valid version, so we fallback to the request version
+			adV2.Version = reqVer.String()
+		} else if !parsedAdVersion.Equal(reqVer) {
+			// if the reqVer doesn't match the adV2.version, we should use the adV2.version
+			adV2.Version = parsedAdVersion.String()
+		}
+	} else if adV2.Version == "" {
+		adV2.Version = "unknown"
+	}
+
 	sAd := server_structs.ServerAd{
 		Name:                adV2.Name,
 		StorageType:         st,
@@ -1147,6 +1165,7 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType server_s
 		Type:                sType.String(),
 		Caps:                adV2.Caps,
 		IOLoad:              0.0, // Explicitly set to 0. The sort algorithm takes 0.0 as unknown load
+		Version:             adV2.Version,
 	}
 
 	recordAd(engineCtx, sAd, &adV2.Namespaces)

diff --git a/director/director_test.go b/director/director_test.go
@@ -874,6 +874,193 @@ func TestDirectorRegistration(t *testing.T) {
 		assert.False(t, getAd.DisableDirectorTest)
 		teardown()
 	})
+
+	t.Run("origin-advertise-with-version-and-ua-test", func(t *testing.T) {
+		c, r, w := setupContext()
+		pKey, token, _ := generateToken()
+		publicKey, err := jwk.PublicKeyOf(pKey)
+		assert.NoError(t, err, "Error creating public key from private key")
+		setupJwksCache(t, "/foo/bar", publicKey)
+
+		isurl := url.URL{}
+		isurl.Path = ts.URL
+
+		ad := server_structs.OriginAdvertiseV2{
+			BrokerURL: "https://broker-url.org",
+			DataURL:   "https://or-url.org",
+			Name:      "test",
+			Namespaces: []server_structs.NamespaceAdV2{{
+				Path:   "/foo/bar",
+				Issuer: []server_structs.TokenIssuer{{IssuerUrl: isurl}},
+			}},
+			Version: "7.0.0",
+		}
+
+		jsonad, err := json.Marshal(ad)
+		assert.NoError(t, err, "Error marshalling OriginAdvertise")
+
+		setupRequest(c, r, jsonad, token, server_structs.OriginType)
+
+		r.ServeHTTP(w, c.Request)
+
+		assert.Equal(t, 200, w.Result().StatusCode, "Expected status code of 200")
+
+		get := serverAds.Get("https://or-url.org")
+		getAd := get.Value()
+		require.NotNil(t, get, "Coudln't find server in the director cache.")
+		assert.Equal(t, ad.Version, getAd.Version)
+		teardown()
+
+	})
+	t.Run("origin-advertise-with-ua-version-test", func(t *testing.T) {
+		c, r, w := setupContext()
+		pKey, token, _ := generateToken()
+		publicKey, err := jwk.PublicKeyOf(pKey)
+		assert.NoError(t, err, "Error creating public key from private key")
+		setupJwksCache(t, "/foo/bar", publicKey)
+
+		isurl := url.URL{}
+		isurl.Path = ts.URL
+
+		ad := server_structs.OriginAdvertiseV2{
+			BrokerURL: "https://broker-url.org",
+			DataURL:   "https://or-url.org",
+			Name:      "test",
+			Namespaces: []server_structs.NamespaceAdV2{{
+				Path:   "/foo/bar",
+				Issuer: []server_structs.TokenIssuer{{IssuerUrl: isurl}},
+			}},
+		}
+
+		jsonad, err := json.Marshal(ad)
+		assert.NoError(t, err, "Error marshalling OriginAdvertise")
+
+		setupRequest(c, r, jsonad, token, server_structs.OriginType)
+
+		r.ServeHTTP(w, c.Request)
+
+		assert.Equal(t, 200, w.Result().StatusCode, "Expected status code of 200")
+
+		get := serverAds.Get("https://or-url.org")
+		getAd := get.Value()
+		require.NotNil(t, get, "Coudln't find server in the director cache.")
+		assert.Equal(t, "7.0.0", getAd.Version)
+		teardown()
+	})
+
+	t.Run("origin-advertise-with-no-version-info-test", func(t *testing.T) {
+		c, r, w := setupContext()
+		pKey, token, _ := generateToken()
+		publicKey, err := jwk.PublicKeyOf(pKey)
+		assert.NoError(t, err, "Error creating public key from private key")
+		setupJwksCache(t, "/foo/bar", publicKey)
+
+		isurl := url.URL{}
+		isurl.Path = ts.URL
+
+		ad := server_structs.OriginAdvertiseV2{
+			BrokerURL: "https://broker-url.org",
+			DataURL:   "https://or-url.org",
+			Name:      "test",
+			Namespaces: []server_structs.NamespaceAdV2{{
+				Path:   "/foo/bar",
+				Issuer: []server_structs.TokenIssuer{{IssuerUrl: isurl}},
+			}},
+		}
+
+		jsonad, err := json.Marshal(ad)
+		assert.NoError(t, err, "Error marshalling OriginAdvertise")
+
+		setupRequest(c, r, jsonad, token, server_structs.OriginType)
+		// set header so that it doesn't have any version info
+		c.Request.Header.Set("User-Agent", "fake-curl")
+
+		r.ServeHTTP(w, c.Request)
+
+		assert.Equal(t, 200, w.Result().StatusCode, "Expected status code of 200")
+
+		get := serverAds.Get("https://or-url.org")
+		getAd := get.Value()
+		require.NotNil(t, get, "Coudln't find server in the director cache.")
+		assert.Equal(t, "unknown", getAd.Version)
+		teardown()
+	})
+
+	t.Run("origin-advertise-with-old-ad-test", func(t *testing.T) {
+		c, r, w := setupContext()
+		pKey, token, _ := generateToken()
+		publicKey, err := jwk.PublicKeyOf(pKey)
+		assert.NoError(t, err, "Error creating public key from private key")
+		setupJwksCache(t, "/foo/bar", publicKey)
+
+		isurl := url.URL{}
+		isurl.Path = ts.URL
+
+		ad := server_structs.OriginAdvertiseV1{
+			Name: "test",
+			URL:  "https://or-url.org",
+			Namespaces: []server_structs.NamespaceAdV1{{
+				Path:   "/foo/bar",
+				Issuer: isurl,
+			}},
+		}
+
+		jsonad, err := json.Marshal(ad)
+		assert.NoError(t, err, "Error marshalling OriginAdvertise")
+
+		setupRequest(c, r, jsonad, token, server_structs.OriginType)
+
+		r.ServeHTTP(w, c.Request)
+
+		assert.Equal(t, 200, w.Result().StatusCode, "Expected status code of 200")
+
+		get := serverAds.Get("https://or-url.org")
+		getAd := get.Value()
+		require.NotNil(t, get, "Coudln't find server in the director cache.")
+		assert.Equal(t, "7.0.0", getAd.Version)
+		teardown()
+
+	})
+
+	t.Run("origin-advertise-with-mismatch-versions", func(t *testing.T) {
+		c, r, w := setupContext()
+		pKey, token, _ := generateToken()
+		publicKey, err := jwk.PublicKeyOf(pKey)
+		assert.NoError(t, err, "Error creating public key from private key")
+		setupJwksCache(t, "/foo/bar", publicKey)
+
+		isurl := url.URL{}
+		isurl.Path = ts.URL
+
+		ad := server_structs.OriginAdvertiseV2{
+			BrokerURL: "https://broker-url.org",
+			DataURL:   "https://or-url.org",
+			Name:      "test",
+			Namespaces: []server_structs.NamespaceAdV2{{
+				Path:   "/foo/bar",
+				Issuer: []server_structs.TokenIssuer{{IssuerUrl: isurl}},
+			}},
+			Version: "7.0.0",
+		}
+
+		jsonad, err := json.Marshal(ad)
+		assert.NoError(t, err, "Error marshalling OriginAdvertise")
+
+		setupRequest(c, r, jsonad, token, server_structs.OriginType)
+
+		// 7.0.1 != 7.0.0
+		c.Request.Header.Set("User-Agent", "pelican-origin/7.0.1")
+
+		r.ServeHTTP(w, c.Request)
+
+		assert.Equal(t, 200, w.Result().StatusCode, "Expected status code of 200")
+
+		get := serverAds.Get("https://or-url.org")
+		getAd := get.Value()
+		require.NotNil(t, get, "Coudln't find server in the director cache.")
+		assert.Equal(t, "7.0.0", getAd.Version)
+		teardown()
+	})
 }
 
 func TestGetAuthzEscaped(t *testing.T) {

diff --git a/origin/advertise.go b/origin/advertise.go
@@ -146,6 +146,7 @@ func (server *OriginServer) CreateAdvertisement(name, originUrlStr, originWebUrl
 		}},
 		StorageType:         ost,
 		DisableDirectorTest: !param.Origin_DirectorTest.GetBool(),
+		Version:             config.GetVersion(),
 	}
 
 	if len(prefixes) == 0 {

diff --git a/server_structs/director.go b/server_structs/director.go
@@ -87,6 +87,7 @@ type (
 		Caps                Capabilities      `json:"capabilities"`
 		FromTopology        bool              `json:"from_topology"`
 		IOLoad              float64           `json:"io_load"`
+		Version             string            `json:"version"`
 	}
 
 	// The struct holding a server's advertisement (including ServerAd and NamespaceAd)
@@ -114,6 +115,7 @@ type (
 		Issuer              []TokenIssuer     `json:"token-issuer"`
 		StorageType         OriginStorageType `json:"storageType"`
 		DisableDirectorTest bool              `json:"directorTest"` // Use negative attribute (disable instead of enable) to be BC with legacy servers where they don't have this field
+		Version             string            `json:"version"`
 	}
 
 	OriginAdvertiseV1 struct {