From 81551f46d7e374caee69c90f837ccafd08be5536 Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Tue, 12 Nov 2024 19:57:20 +0000 Subject: [PATCH 1/2] Revert "Unmarshal Capabilities correctly in all structs that use them" This reverts commit 38bcb71a8e8c4e27eaa28b85281410ae0b37d19f. --- server_structs/director.go | 54 +++++++++++-------- server_structs/director_test.go | 92 ++++----------------------------- 2 files changed, 44 insertions(+), 102 deletions(-) diff --git a/server_structs/director.go b/server_structs/director.go index a443754e0..d65ddd1d5 100644 --- a/server_structs/director.go +++ b/server_structs/director.go @@ -187,36 +187,48 @@ type ( } ) -// A helper function to handle JSON->Caps unmarshalling across multiple deprecated JSON keys. +// A helper function to handle JSON->NSAdV2 unmarshalling across multiple deprecated JSON keys. // -// Old Caches/Origins will send various bits of JSON to the Director containing the JSON representation -// of the OldCapabilities struct. To make sure these old JSON representations are unmarshalled correctly -// into the new Capabilities struct, this function determines which JSON format is being sent and handles -// conversion if necessary. We can probably think about removing this function when we don't see origins/directors +// When the Director sends a list of NamespaceAdV2 structs as JSON, it may contain +// the old capabilities struct. This function checks if the raw JSON contains the +// "FallbackRead" field, indicating the old capabilities struct, and unmarshals +// the JSON into the new struct accordingly. This is to ensure backwards compatibility +// We can probably think about removing this function when we don't see origins/directors // running Pelican <= v7.11.0. -func (c *Capabilities) UnmarshalJSON(data []byte) error { - // Check if it's the old format by looking for a unique field, e.g., "FallBackRead" - if strings.Contains(string(data), "FallBackRead") { - // Detected old JSON format, so unmarshal into OldCapabilities +func (n *NamespaceAdV2) UnmarshalJSON(data []byte) error { + // Use alias struct of NamespaceAdV2 to prevent infinite recursion of UnmarshalJSON + type Alias NamespaceAdV2 + aux := &struct { + Caps json.RawMessage `json:"Caps"` + *Alias + }{ + Alias: (*Alias)(n), + } + + if err := json.Unmarshal(data, &aux); err != nil { + return err + } + + // Check if the raw Caps JSON contains "FallbackRead", indicating + // the old caps struct. Since the struct-to-JSON code has never included + // 'omitempty', we can safely assume that the field is ALWAYS present in + // old versions of the JSON. + if strings.Contains(string(aux.Caps), "FallBackRead") { var oldCaps OldCapabilities - if err := json.Unmarshal(data, &oldCaps); err != nil { + if err := json.Unmarshal(aux.Caps, &oldCaps); err != nil { return err } - // Map the old fields to the new struct - c.PublicReads = oldCaps.PublicRead - c.Reads = oldCaps.Read - c.Writes = oldCaps.Write - c.Listings = oldCaps.Listing - c.DirectReads = oldCaps.FallBackRead + // Map old capabilities to new capabilities + n.Caps.PublicReads = oldCaps.PublicRead + n.Caps.Reads = oldCaps.Read + n.Caps.Writes = oldCaps.Write + n.Caps.Listings = oldCaps.Listing + n.Caps.DirectReads = oldCaps.FallBackRead } else { - // Assume it's the new JSON format and unmarshal directly into Capabilities - type Alias Capabilities // Create an alias to avoid recursive calls to UnmarshalJSON - var newCaps Alias - if err := json.Unmarshal(data, &newCaps); err != nil { + if err := json.Unmarshal(aux.Caps, &n.Caps); err != nil { return err } - *c = Capabilities(newCaps) // Copy unmarshalled values back to the original struct } return nil diff --git a/server_structs/director_test.go b/server_structs/director_test.go index 0cb1ebaaf..3364866bf 100644 --- a/server_structs/director_test.go +++ b/server_structs/director_test.go @@ -283,47 +283,18 @@ func TestXPelTokGenParsing(t *testing.T) { }) } -func TestCapsUnmarshalJSON(t *testing.T) { - oldCaps := `{"PublicRead":true,"Read":true,"Write":false,"Listing":false,"FallBackRead":true}}` - newCaps := `{"PublicReads":false,"Reads":true,"Writes":false,"Listings":false,"DirectReads":true}}` - - nsAdV2NoCap := `{"path":"/ncar","token-generation":[{"strategy":"","vault-server":"","max-scope-depth":0,"issuer":{}}],"token-issuer":[],"from-topology":true,"Caps":` - oAdV2NoCap := `{"name": "example-server","registry-prefix": "/origins/example-server","broker-url": "http://example-broker.com","data-url": "http://example-data.com","web-url": "http://example-web.com","namespaces": [{"path": "/example-namespace","token-generation": [],"token-issuer": [],"from-topology": false}],"token-issuer": [],"storageType": "POSIX","directorTest": false,"capabilities":` - sAdV2NoCap := `{"name": "example-server","storageType": "POSIX","directorTest": false,"auth_url": {"Scheme": "http", "Host": "example-auth.com"},"broker_url": {"Scheme": "http", "Host": "example-auth.com"},"url": {"Scheme": "http", "Host": "example-auth.com"},"web_url": {"Scheme": "http", "Host": "example-auth.com"},"type": "cache","latitude": 40.7128,"longitude": -74.0060,"from_topology": true,"io_load": 0.75,"capabilities":` +func TestNSAdV2UnmarshalJSON(t *testing.T) { + oldJSON := `{"PublicRead":true,"Caps":{"PublicRead":true,"Read":true,"Write":false,"Listing":false,"FallBackRead":true},"path":"/ncar","token-generation":[{"strategy":"","vault-server":"","max-scope-depth":0,"issuer":{"Scheme":"","Opaque":"","User":null,"Host":"","Path":"","RawPath":"","OmitHost":false,"ForceQuery":false,"RawQuery":"","Fragment":"","RawFragment":""}}],"token-issuer":[],"from-topology":true}` + newJSON := `{"Caps":{"PublicReads":false,"Reads":true,"Writes":false,"Listings":false,"DirectReads":true},"path":"/ncar","token-generation":[{"strategy":"","vault-server":"","max-scope-depth":0,"issuer":{"Scheme":"","Opaque":"","User":null,"Host":"","Path":"","RawPath":"","OmitHost":false,"ForceQuery":false,"RawQuery":"","Fragment":"","RawFragment":""}}],"token-issuer":[],"from-topology":true}` tests := []struct { name string jsonData string expected Capabilities - target interface{} }{ { - name: "NamespaceAdV2 with old caps", - jsonData: nsAdV2NoCap + oldCaps, - expected: Capabilities{ - PublicReads: true, - Reads: true, - Writes: false, - Listings: false, - DirectReads: true, - }, - target: &NamespaceAdV2{}, - }, - { - name: "NamespaceAdV2 with new caps", - jsonData: nsAdV2NoCap + newCaps, - expected: Capabilities{ - PublicReads: false, - Reads: true, - Writes: false, - Listings: false, - DirectReads: true, - }, - target: &NamespaceAdV2{}, - }, - { - name: "OriginAdvertiseV2 with old caps", - jsonData: oAdV2NoCap + oldCaps, + name: "Old JSON format", + jsonData: oldJSON, expected: Capabilities{ PublicReads: true, Reads: true, @@ -331,11 +302,10 @@ func TestCapsUnmarshalJSON(t *testing.T) { Listings: false, DirectReads: true, }, - target: &OriginAdvertiseV2{}, }, { - name: "OriginAdvertiseV2 with new caps", - jsonData: oAdV2NoCap + newCaps, + name: "New JSON format", + jsonData: newJSON, expected: Capabilities{ PublicReads: false, Reads: true, @@ -343,57 +313,17 @@ func TestCapsUnmarshalJSON(t *testing.T) { Listings: false, DirectReads: true, }, - target: &OriginAdvertiseV2{}, - }, - { - name: "ServerAd with old caps", - jsonData: sAdV2NoCap + oldCaps, - expected: Capabilities{ - PublicReads: true, - Reads: true, - Writes: false, - Listings: false, - DirectReads: true, - }, - target: &ServerAd{}, - }, - { - name: "ServerAd with new caps", - jsonData: sAdV2NoCap + newCaps, - expected: Capabilities{ - PublicReads: false, - Reads: true, - Writes: false, - Listings: false, - DirectReads: true, - }, - target: &ServerAd{}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Unmarshal JSON into the appropriate struct - if err := json.Unmarshal([]byte(tt.jsonData), tt.target); err != nil { + var ns NamespaceAdV2 + if err := json.Unmarshal([]byte(tt.jsonData), &ns); err != nil { t.Fatalf("UnmarshalJSON() error = %v", err) } - - // Check Caps field in the struct - switch v := tt.target.(type) { - case *NamespaceAdV2: - if v.Caps != tt.expected { - t.Errorf("NamespaceAdV2 Caps = %v, want %v", v.Caps, tt.expected) - } - case *OriginAdvertiseV2: - if v.Caps != tt.expected { - t.Errorf("OriginAdvertiseV2 Caps = %v, want %v", v.Caps, tt.expected) - } - case *ServerAd: - if v.Caps != tt.expected { - t.Errorf("ServerAd Caps = %v, want %v", v.Caps, tt.expected) - } - default: - t.Errorf("Unknown struct type: %T", tt.target) + if ns.Caps != tt.expected { + t.Errorf("UnmarshalJSON() = %v, want %v", ns.Caps, tt.expected) } }) } From 786f4d1220130a50ece2382a05618c9ad06b82dc Mon Sep 17 00:00:00 2001 From: Justin Hiemstra Date: Tue, 12 Nov 2024 19:57:42 +0000 Subject: [PATCH 2/2] Revert "Switch to use new Caps struct but still handle old JSON from Director" This reverts commit 5d8af336a081776d37eba556adbf278e679b09c3. --- server_structs/director.go | 67 +++------------------------------ server_structs/director_test.go | 47 ----------------------- 2 files changed, 6 insertions(+), 108 deletions(-) diff --git a/server_structs/director.go b/server_structs/director.go index d65ddd1d5..316df7d43 100644 --- a/server_structs/director.go +++ b/server_structs/director.go @@ -45,21 +45,13 @@ type ( CredentialIssuer url.URL `json:"issuer"` } - // A struct for unmarshalling the old JSON while we stage the breaking change across releases - OldCapabilities struct { - PublicRead bool - Read bool - Write bool - Listing bool - FallBackRead bool - } - + // Note that the json are kept in uppercase for backward compatibility Capabilities struct { - PublicReads bool - Reads bool - Writes bool - Listings bool - DirectReads bool + PublicReads bool `json:"PublicRead"` + Reads bool `json:"Read"` + Writes bool `json:"Write"` + Listings bool `json:"Listing"` + DirectReads bool `json:"FallBackRead"` } NamespaceAdV2 struct { @@ -187,53 +179,6 @@ type ( } ) -// A helper function to handle JSON->NSAdV2 unmarshalling across multiple deprecated JSON keys. -// -// When the Director sends a list of NamespaceAdV2 structs as JSON, it may contain -// the old capabilities struct. This function checks if the raw JSON contains the -// "FallbackRead" field, indicating the old capabilities struct, and unmarshals -// the JSON into the new struct accordingly. This is to ensure backwards compatibility -// We can probably think about removing this function when we don't see origins/directors -// running Pelican <= v7.11.0. -func (n *NamespaceAdV2) UnmarshalJSON(data []byte) error { - // Use alias struct of NamespaceAdV2 to prevent infinite recursion of UnmarshalJSON - type Alias NamespaceAdV2 - aux := &struct { - Caps json.RawMessage `json:"Caps"` - *Alias - }{ - Alias: (*Alias)(n), - } - - if err := json.Unmarshal(data, &aux); err != nil { - return err - } - - // Check if the raw Caps JSON contains "FallbackRead", indicating - // the old caps struct. Since the struct-to-JSON code has never included - // 'omitempty', we can safely assume that the field is ALWAYS present in - // old versions of the JSON. - if strings.Contains(string(aux.Caps), "FallBackRead") { - var oldCaps OldCapabilities - if err := json.Unmarshal(aux.Caps, &oldCaps); err != nil { - return err - } - - // Map old capabilities to new capabilities - n.Caps.PublicReads = oldCaps.PublicRead - n.Caps.Reads = oldCaps.Read - n.Caps.Writes = oldCaps.Write - n.Caps.Listings = oldCaps.Listing - n.Caps.DirectReads = oldCaps.FallBackRead - } else { - if err := json.Unmarshal(aux.Caps, &n.Caps); err != nil { - return err - } - } - - return nil -} - func (x XPelNs) GetName() string { return "X-Pelican-Namespace" } diff --git a/server_structs/director_test.go b/server_structs/director_test.go index 3364866bf..c05e1b1f1 100644 --- a/server_structs/director_test.go +++ b/server_structs/director_test.go @@ -19,7 +19,6 @@ package server_structs import ( - "encoding/json" "fmt" "net/http" "net/url" @@ -282,49 +281,3 @@ func TestXPelTokGenParsing(t *testing.T) { assert.Len(t, xPelTokGen.BasePaths, 0) }) } - -func TestNSAdV2UnmarshalJSON(t *testing.T) { - oldJSON := `{"PublicRead":true,"Caps":{"PublicRead":true,"Read":true,"Write":false,"Listing":false,"FallBackRead":true},"path":"/ncar","token-generation":[{"strategy":"","vault-server":"","max-scope-depth":0,"issuer":{"Scheme":"","Opaque":"","User":null,"Host":"","Path":"","RawPath":"","OmitHost":false,"ForceQuery":false,"RawQuery":"","Fragment":"","RawFragment":""}}],"token-issuer":[],"from-topology":true}` - newJSON := `{"Caps":{"PublicReads":false,"Reads":true,"Writes":false,"Listings":false,"DirectReads":true},"path":"/ncar","token-generation":[{"strategy":"","vault-server":"","max-scope-depth":0,"issuer":{"Scheme":"","Opaque":"","User":null,"Host":"","Path":"","RawPath":"","OmitHost":false,"ForceQuery":false,"RawQuery":"","Fragment":"","RawFragment":""}}],"token-issuer":[],"from-topology":true}` - - tests := []struct { - name string - jsonData string - expected Capabilities - }{ - { - name: "Old JSON format", - jsonData: oldJSON, - expected: Capabilities{ - PublicReads: true, - Reads: true, - Writes: false, - Listings: false, - DirectReads: true, - }, - }, - { - name: "New JSON format", - jsonData: newJSON, - expected: Capabilities{ - PublicReads: false, - Reads: true, - Writes: false, - Listings: false, - DirectReads: true, - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - var ns NamespaceAdV2 - if err := json.Unmarshal([]byte(tt.jsonData), &ns); err != nil { - t.Fatalf("UnmarshalJSON() error = %v", err) - } - if ns.Caps != tt.expected { - t.Errorf("UnmarshalJSON() = %v, want %v", ns.Caps, tt.expected) - } - }) - } -}