From f8c07a281a1fd7e788c2aa5def23af9ea59f3260 Mon Sep 17 00:00:00 2001 From: Patrick Date: Wed, 13 Nov 2024 20:22:27 +0000 Subject: [PATCH 1/8] Add Version field to OriginAdvertiseV2 --- server_structs/director.go | 1 + 1 file changed, 1 insertion(+) diff --git a/server_structs/director.go b/server_structs/director.go index 316df7d43..d7011a919 100644 --- a/server_structs/director.go +++ b/server_structs/director.go @@ -114,6 +114,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 { From 3d27e8082d6919f5e154c7eb4cbe8e5c89d40390 Mon Sep 17 00:00:00 2001 From: Patrick Date: Wed, 13 Nov 2024 20:29:54 +0000 Subject: [PATCH 2/8] Added Version to CreateAdvertisement methods (cache/origin) --- cache/advertise.go | 1 + origin/advertise.go | 1 + 2 files changed, 2 insertions(+) diff --git a/cache/advertise.go b/cache/advertise.go index a0c404e1a..c52e1dba5 100644 --- 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/origin/advertise.go b/origin/advertise.go index 54bb370ab..fa9272622 100644 --- 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 { From 390632e2f42ad43c7273aa9e958e64ebf062a033 Mon Sep 17 00:00:00 2001 From: Patrick Date: Wed, 13 Nov 2024 20:57:04 +0000 Subject: [PATCH 3/8] Add Version field to ServerAd --- server_structs/director.go | 1 + 1 file changed, 1 insertion(+) diff --git a/server_structs/director.go b/server_structs/director.go index d7011a919..1d0c78a14 100644 --- 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) From 54de7ec7283da6459a18b697d57d13345fe09954 Mon Sep 17 00:00:00 2001 From: Patrick Date: Wed, 13 Nov 2024 20:58:43 +0000 Subject: [PATCH 4/8] Propogate the adV2 Version up to the ServerAd --- director/director.go | 1 + 1 file changed, 1 insertion(+) diff --git a/director/director.go b/director/director.go index f8f8f6344..94d5725fe 100644 --- a/director/director.go +++ b/director/director.go @@ -1147,6 +1147,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) From 6e31d25150d573bbad05bfe75c46a1dc987a83e4 Mon Sep 17 00:00:00 2001 From: Patrick Date: Mon, 2 Dec 2024 17:55:50 +0000 Subject: [PATCH 5/8] Use request version as fallback --- director/director.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/director/director.go b/director/director.go index 94d5725fe..cee51fc3e 100644 --- a/director/director.go +++ b/director/director.go @@ -1137,6 +1137,15 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType server_s adV2.DisableDirectorTest = true } + // if we didn't receive a version but we were able to extract the request version from the user agent, + // then we can use the request version as the server version (they should be the same) + // 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 { + adV2.Version = "unknown" + } + sAd := server_structs.ServerAd{ Name: adV2.Name, StorageType: st, From 5c65cf3e0711ce16fc81530c1d6ca317a152ed4b Mon Sep 17 00:00:00 2001 From: Patrick Date: Fri, 6 Dec 2024 19:29:13 +0000 Subject: [PATCH 6/8] Improve logic for setting registered ad version --- director/director.go | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/director/director.go b/director/director.go index cee51fc3e..313995d91 100644 --- a/director/director.go +++ b/director/director.go @@ -1137,12 +1137,18 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType server_s adV2.DisableDirectorTest = true } - // if we didn't receive a version but we were able to extract the request version from the user agent, - // then we can use the request version as the server version (they should be the same) + // 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 { + } else if adV2.Version != "" && reqVer != nil { + _, 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 adV2.Version == "" { adV2.Version = "unknown" } From 428d9f280dfb369c2d7cebc0920dc8ed89a36c6a Mon Sep 17 00:00:00 2001 From: Patrick Date: Fri, 6 Dec 2024 19:30:26 +0000 Subject: [PATCH 7/8] Added tests for tracking if the version was properly registered --- director/director_test.go | 147 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 147 insertions(+) diff --git a/director/director_test.go b/director/director_test.go index 8ea01a203..551052aaf 100644 --- a/director/director_test.go +++ b/director/director_test.go @@ -874,6 +874,153 @@ 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() + + }) } func TestGetAuthzEscaped(t *testing.T) { From d302ae6c24aa04deb338f4d2abc5f65cee7f5f43 Mon Sep 17 00:00:00 2001 From: Patrick Date: Mon, 9 Dec 2024 16:23:59 +0000 Subject: [PATCH 8/8] Added test for miss matched version. Adjusted logic to handle mismatch --- director/director.go | 5 ++++- director/director_test.go | 40 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/director/director.go b/director/director.go index 313995d91..4bf10efaf 100644 --- a/director/director.go +++ b/director/director.go @@ -1143,10 +1143,13 @@ func registerServeAd(engineCtx context.Context, ctx *gin.Context, sType server_s if adV2.Version == "" && reqVer != nil { adV2.Version = reqVer.String() } else if adV2.Version != "" && reqVer != nil { - _, err := version.NewVersion(adV2.Version) + 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" diff --git a/director/director_test.go b/director/director_test.go index 551052aaf..31597d9ac 100644 --- a/director/director_test.go +++ b/director/director_test.go @@ -1021,6 +1021,46 @@ func TestDirectorRegistration(t *testing.T) { 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) {