diff --git a/api/datareading.go b/api/datareading.go index a5973181..86dd2dd8 100644 --- a/api/datareading.go +++ b/api/datareading.go @@ -30,7 +30,10 @@ type DataReading struct { } // UnmarshalJSON implements the json.Unmarshaler interface for DataReading. -// It handles the dynamic parsing of the Data field based on the DataGatherer. +// The function attempts to decode the Data field into known types in a prioritized order. +// Empty data is considered an error, because there is no way to discriminate between data types. +// TODO(wallrj): Add a discriminator field to DataReading to avoid this complex logic. +// E.g. "data_type": "discovery"|"dynamic" func (o *DataReading) UnmarshalJSON(data []byte) error { var tmp struct { ClusterID string `json:"cluster_id,omitempty"` @@ -40,45 +43,49 @@ func (o *DataReading) UnmarshalJSON(data []byte) error { SchemaVersion string `json:"schema_version"` } - d := json.NewDecoder(bytes.NewReader(data)) - d.DisallowUnknownFields() - - if err := d.Decode(&tmp); err != nil { - return err + // Decode the top-level fields of DataReading + if err := jsonUnmarshalStrict(data, &tmp); err != nil { + return fmt.Errorf("failed to parse DataReading: %s", err) } + + // Assign top-level fields to the DataReading object o.ClusterID = tmp.ClusterID o.DataGatherer = tmp.DataGatherer o.Timestamp = tmp.Timestamp o.SchemaVersion = tmp.SchemaVersion - { - var discoveryData DiscoveryData - d := json.NewDecoder(bytes.NewReader(tmp.Data)) - d.DisallowUnknownFields() - if err := d.Decode(&discoveryData); err == nil { - o.Data = &discoveryData - return nil - } + // Return an error if data is empty + if len(tmp.Data) == 0 || bytes.Equal(tmp.Data, []byte("null")) || bytes.Equal(tmp.Data, []byte("{}")) { + return fmt.Errorf("failed to parse DataReading.Data for gatherer %q: empty data", o.DataGatherer) } - { - var dynamicData DynamicData - d := json.NewDecoder(bytes.NewReader(tmp.Data)) - d.DisallowUnknownFields() - if err := d.Decode(&dynamicData); err == nil { - o.Data = &dynamicData - return nil - } + + // Define a list of decoding attempts with prioritized types + dataTypes := []struct { + target interface{} + assign func(interface{}) + }{ + {&DiscoveryData{}, func(v interface{}) { o.Data = v.(*DiscoveryData) }}, + {&DynamicData{}, func(v interface{}) { o.Data = v.(*DynamicData) }}, } - { - var genericData map[string]interface{} - d := json.NewDecoder(bytes.NewReader(tmp.Data)) - d.DisallowUnknownFields() - if err := d.Decode(&genericData); err == nil { - o.Data = genericData + + // Attempt to decode the Data field into each type + for _, dataType := range dataTypes { + if err := jsonUnmarshalStrict(tmp.Data, dataType.target); err == nil { + dataType.assign(dataType.target) return nil } } - return fmt.Errorf("failed to parse DataReading.Data for gatherer %s", o.DataGatherer) + + // Return an error if no type matches + return fmt.Errorf("failed to parse DataReading.Data for gatherer %q: unknown type", o.DataGatherer) +} + +// jsonUnmarshalStrict unmarshals JSON data into the provided interface, +// disallowing unknown fields to ensure strict adherence to the expected structure. +func jsonUnmarshalStrict(data []byte, v interface{}) error { + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.DisallowUnknownFields() + return decoder.Decode(v) } // GatheredResource wraps the raw k8s resource that is sent to the jetstack secure backend diff --git a/api/datareading_test.go b/api/datareading_test.go index 5fd6e689..c55ca034 100644 --- a/api/datareading_test.go +++ b/api/datareading_test.go @@ -4,6 +4,8 @@ import ( "encoding/json" "testing" "time" + + "github.com/stretchr/testify/assert" ) func TestJSONGatheredResourceDropsEmptyTime(t *testing.T) { @@ -34,3 +36,178 @@ func TestJSONGatheredResourceSetsTimeWhenPresent(t *testing.T) { t.Fatalf("unexpected json \ngot %s\nwant %s", string(bytes), expected) } } + +// TestDataReading_UnmarshalJSON tests the UnmarshalJSON method of DataReading +// with various scenarios including valid and invalid JSON inputs. +func TestDataReading_UnmarshalJSON(t *testing.T) { + tests := []struct { + name string + input string + wantDataType interface{} + expectError string + }{ + { + name: "DiscoveryData type", + input: `{ + "cluster_id": "61b2db64-fd70-49a6-a257-08397b9b4bae", + "data-gatherer": "discovery", + "timestamp": "2024-06-01T12:00:00Z", + "data": { + "cluster_id": "60868ebf-6e47-4184-9bc0-20bb6824e210", + "server_version": { + "major": "1", + "minor": "20", + "gitVersion": "v1.20.0" + } + }, + "schema_version": "v1" + }`, + wantDataType: &DiscoveryData{}, + }, + { + name: "DynamicData type", + input: `{ + "cluster_id": "69050b54-c61a-4384-95c3-35f890377a67", + "data-gatherer": "dynamic", + "timestamp": "2024-06-01T12:00:00Z", + "data": {"items": []}, + "schema_version": "v1" + }`, + wantDataType: &DynamicData{}, + }, + { + name: "Invalid JSON", + input: `not a json`, + expectError: "failed to parse DataReading: invalid character 'o' in literal null (expecting 'u')", + }, + { + name: "Missing data field", + input: `{ + "cluster_id": "cc5a0429-8dc4-42c8-8e3a-eece9bca15c3", + "data-gatherer": "missing-data-field", + "timestamp": "2024-06-01T12:00:00Z", + "schema_version": "v1" + }`, + expectError: `failed to parse DataReading.Data for gatherer "missing-data-field": empty data`, + }, + { + name: "Mismatched data type", + input: `{ + "cluster_id": "c272b13e-b19e-4782-833f-d55a305f3c9e", + "data-gatherer": "unknown-data-type", + "timestamp": "2024-06-01T12:00:00Z", + "data": "this should be an object", + "schema_version": "v1" + }`, + expectError: `failed to parse DataReading.Data for gatherer "unknown-data-type": unknown type`, + }, + { + name: "Empty data field", + input: `{ + "cluster_id": "07909675-113f-4b59-ba5e-529571a191e6", + "data-gatherer": "empty-data", + "timestamp": "2024-06-01T12:00:00Z", + "data": {}, + "schema_version": "v1" + }`, + expectError: `failed to parse DataReading.Data for gatherer "empty-data": empty data`, + }, + { + name: "Additional field", + input: `{ + "cluster_id": "11df7332-4b32-4f5a-903b-0cbbef381850", + "data-gatherer": "additional-field", + "timestamp": "2024-06-01T12:00:00Z", + "data": { + "cluster_id": "60868ebf-6e47-4184-9bc0-20bb6824e210" + }, + "extra_field": "should cause error", + "schema_version": "v1" + }`, + expectError: `failed to parse DataReading: json: unknown field "extra_field"`, + }, + { + name: "Additional data field", + input: `{ + "cluster_id": "ca44c338-987e-4d57-8320-63f538db4292", + "data-gatherer": "additional-data-field", + "timestamp": "2024-06-01T12:00:00Z", + "data": { + "cluster_id": "60868ebf-6e47-4184-9bc0-20bb6824e210", + "server_version": { + "major": "1", + "minor": "20", + "gitVersion": "v1.20.0" + }, + "extra_field": "should cause error" + }, + "schema_version": "v1" + }`, + expectError: `failed to parse DataReading.Data for gatherer "additional-data-field": unknown type`, + }, + { + name: "Empty JSON object", + input: `{}`, + expectError: `failed to parse DataReading.Data for gatherer "": empty data`, + }, + { + name: "Null data field", + input: `{ + "cluster_id": "36281cb3-7f3a-4efa-9879-7c988a9715b0", + "data-gatherer": "null-data", + "timestamp": "2024-06-01T12:00:00Z", + "data": null, + "schema_version": "v1" + }`, + expectError: `failed to parse DataReading.Data for gatherer "null-data": empty data`, + }, + { + name: "Empty string data field", + input: `{ + "cluster_id": "7b7aa8ee-58ac-4818-9b29-c0a76296ea1d", + "data-gatherer": "empty-string-data", + "timestamp": "2024-06-01T12:00:00Z", + "data": "", + "schema_version": "v1" + }`, + expectError: `failed to parse DataReading.Data for gatherer "empty-string-data": unknown type`, + }, + { + name: "Array instead of object in data field", + input: `{ + "cluster_id": "94d7757f-d084-4ccb-963b-f60fece0df2d", + "data-gatherer": "array-data", + "timestamp": "2024-06-01T12:00:00Z", + "data": [], + "schema_version": "v1" + }`, + expectError: `failed to parse DataReading.Data for gatherer "array-data": unknown type`, + }, + { + name: "Incorrect timestamp format", + input: `{ + "cluster_id": "d58f298d-b8c1-4d99-aa85-c27d9aec6f97", + "data-gatherer": "bad-timestamp", + "timestamp": "not-a-timestamp", + "data": { + "items": [] + }, + "schema_version": "v1" + }`, + expectError: `failed to parse DataReading: parsing time "not-a-timestamp" as "2006-01-02T15:04:05Z07:00": cannot parse "not-a-timestamp" as "2006"`, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var dr DataReading + err := dr.UnmarshalJSON([]byte(tt.input)) + if tt.expectError != "" { + assert.EqualError(t, err, tt.expectError) + return + } + assert.NoError(t, err) + assert.IsType(t, tt.wantDataType, dr.Data) + }) + } +} diff --git a/pkg/echo/echo_test.go b/pkg/echo/echo_test.go index d9cd9318..2a12d942 100644 --- a/pkg/echo/echo_test.go +++ b/pkg/echo/echo_test.go @@ -8,6 +8,8 @@ import ( "testing" "time" + "k8s.io/apimachinery/pkg/version" + "github.com/jetstack/preflight/api" ) @@ -34,8 +36,10 @@ func TestEchoServerRequestResponse(t *testing.T) { ClusterID: "test_suite_cluster", DataGatherer: "dummy", Timestamp: api.Time{Time: time.Now()}, - Data: map[string]string{ - "test": "test", + Data: &api.DiscoveryData{ + ServerVersion: &version.Info{ + GitVersion: "v1.20.0", + }, }, SchemaVersion: "2.0.0", },