diff --git a/README.md b/README.md index e6dfdc7..e29e470 100644 --- a/README.md +++ b/README.md @@ -191,3 +191,4 @@ The following environment variables can be configured in the rest-dynamic-contro | REST_CONTROLLER_MIN_ERROR_RETRY_INTERVAL | The minimum interval between retries when an error occurs. This should be less than max-error-retry-interval. | `1s` | | REST_CONTROLLER_MAX_ERROR_RETRIES | How many times to retry the processing of a resource when an error occurs before giving up and dropping the resource. | `5` | | REST_CONTROLLER_METRICS_SERVER_PORT | The port where the metrics server will be listening. If not set, the metrics server is disabled. | | +| REST_CONTROLLER_IDENTIFIER_MATCH_POLICY | Policy to match identifier fields when checking if a remote resource exists. Possible values are "AND" (all fields must match) and "OR" (at least one field must match). | `OR` | diff --git a/internal/controllers/helpers.go b/internal/controllers/helpers.go index 97977be..c68a648 100644 --- a/internal/controllers/helpers.go +++ b/internal/controllers/helpers.go @@ -3,6 +3,7 @@ package restResources import ( "fmt" "math" + "strings" "github.com/krateoplatformops/rest-dynamic-controller/internal/tools/comparison" getter "github.com/krateoplatformops/rest-dynamic-controller/internal/tools/definitiongetter" @@ -35,8 +36,9 @@ func isCRUpdated(mg *unstructured.Unstructured, rm map[string]interface{}) (comp return comparison.CompareExisting(m, rm) } -// populateStatusFields populates the status fields in the mg object with the values from the body -// It checks both the `identifiers` and `additionalStatusFields` defined in the resource +// populateStatusFields populates the status fields in the mg object with the values from the response body of the API call. +// It supports dot notation for nested fields and performs necessary type conversions. +// It uses the identifiers and additionalStatusFields from the clientInfo to determine which fields to populate. func populateStatusFields(clientInfo *getter.Info, mg *unstructured.Unstructured, body map[string]interface{}) error { // Handle nil inputs if mg == nil { @@ -49,34 +51,40 @@ func populateStatusFields(clientInfo *getter.Info, mg *unstructured.Unstructured return nil // Nothing to populate, but not an error } + // Combine identifiers and additionalStatusFields into one list. + allFields := append(clientInfo.Resource.Identifiers, clientInfo.Resource.AdditionalStatusFields...) // Early return if no fields to populate - if len(clientInfo.Resource.Identifiers) == 0 && len(clientInfo.Resource.AdditionalStatusFields) == 0 { + if len(allFields) == 0 { return nil } - // Create a set of all fields we need to look for - fieldsToPopulate := make(map[string]struct{}) + for _, fieldName := range allFields { + //log.Printf("Managing field: %s", fieldName) + // Split the field name by '.' to handle nested paths. + path := strings.Split(fieldName, ".") + //log.Printf("Field path split: %v", path) - // Add identifiers to the set - for _, identifier := range clientInfo.Resource.Identifiers { - fieldsToPopulate[identifier] = struct{}{} - } - - // Add additionalStatusFields to the set - for _, additionalField := range clientInfo.Resource.AdditionalStatusFields { - fieldsToPopulate[additionalField] = struct{}{} - } + // Extract the raw value from the response body without copying. + value, found, err := unstructured.NestedFieldNoCopy(body, path...) + if err != nil || !found { + // An error here means the path was invalid or not found. + // We can safely continue to the next field. + //log.Printf("Field '%s' not found in response body or error occurred: %v", fieldName, err) + continue + } + //log.Printf("Extracted value for field '%s': %v", fieldName, value) - // Single pass through the body map - for k, v := range body { - if _, exists := fieldsToPopulate[k]; exists { - // Convert the value to a format that unstructured can handle - convertedValue := deepCopyJSONValue(v) + // Perform deep copy and type conversions (e.g., float64 to int64). + convertedValue := deepCopyJSONValue(value) + //log.Printf("Converted value for field '%s': %v", fieldName, convertedValue) - if err := unstructured.SetNestedField(mg.Object, convertedValue, "status", k); err != nil { - return fmt.Errorf("setting nested field '%s' in status: %w", k, err) - } + // The destination path in the status should mirror the source path. + statusPath := append([]string{"status"}, path...) + //log.Printf("Setting value for field '%s' at status path: %v", fieldName, statusPath) + if err := unstructured.SetNestedField(mg.Object, convertedValue, statusPath...); err != nil { + return fmt.Errorf("setting nested field '%s' in status: %w", fieldName, err) } + //log.Printf("Successfully set field '%s' with value: %v at path: %v", fieldName, convertedValue, statusPath) } return nil diff --git a/internal/controllers/helpers_test.go b/internal/controllers/helpers_test.go index fc7abe2..3fbc9f8 100644 --- a/internal/controllers/helpers_test.go +++ b/internal/controllers/helpers_test.go @@ -531,6 +531,232 @@ func TestPopulateStatusFields(t *testing.T) { }, }, }, + { + name: "nested identifiers and additional status fields", + clientInfo: &getter.Info{ + Resource: getter.Resource{ + Identifiers: []string{"metadata.uid"}, + AdditionalStatusFields: []string{"spec.host", "status.phase"}, + }, + }, + mg: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + body: map[string]interface{}{ + "metadata": map[string]interface{}{ + "uid": "xyz-123", + }, + "spec": map[string]interface{}{ + "host": "example.com", + }, + "status": map[string]interface{}{ + "phase": "Running", + }, + }, + wantErr: false, + expected: map[string]interface{}{ + "status": map[string]interface{}{ + "metadata": map[string]interface{}{ + "uid": "xyz-123", + }, + "spec": map[string]interface{}{ + "host": "example.com", + }, + "status": map[string]interface{}{ + "phase": "Running", + }, + }, + }, + }, + { + name: "mixed top-level and nested fields", + clientInfo: &getter.Info{ + Resource: getter.Resource{ + Identifiers: []string{"id"}, + AdditionalStatusFields: []string{"metadata.name"}, + }, + }, + mg: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + body: map[string]interface{}{ + "id": "abc-456", + "metadata": map[string]interface{}{ + "name": "my-resource", + }, + }, + wantErr: false, + expected: map[string]interface{}{ + "status": map[string]interface{}{ + "id": "abc-456", + "metadata": map[string]interface{}{ + "name": "my-resource", + }, + }, + }, + }, + { + name: "nested field not found in body", + clientInfo: &getter.Info{ + Resource: getter.Resource{ + AdditionalStatusFields: []string{"spec.nonexistent"}, + }, + }, + mg: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + body: map[string]interface{}{ + "spec": map[string]interface{}{ + "host": "example.com", + }, + }, + wantErr: false, + expected: map[string]interface{}{}, + }, + { + name: "complex nested object", + clientInfo: &getter.Info{ + Resource: getter.Resource{ + AdditionalStatusFields: []string{"data.config.spec"}, + }, + }, + mg: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + body: map[string]interface{}{ + "data": map[string]interface{}{ + "config": map[string]interface{}{ + "spec": map[string]interface{}{ + "key": "value", + "num": float64(123), + }, + }, + }, + }, + wantErr: false, + expected: map[string]interface{}{ + "status": map[string]interface{}{ + "data": map[string]interface{}{ + "config": map[string]interface{}{ + "spec": map[string]interface{}{ + "key": "value", + "num": int64(123), + }, + }, + }, + }, + }, + }, + { + name: "slice of strings", + clientInfo: &getter.Info{ + Resource: getter.Resource{ + AdditionalStatusFields: []string{"spec.tags"}, + }, + }, + mg: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + body: map[string]interface{}{ + "spec": map[string]interface{}{ + "tags": []interface{}{"tag1", "tag2"}, + }, + }, + wantErr: false, + expected: map[string]interface{}{ + "status": map[string]interface{}{ + "spec": map[string]interface{}{ + "tags": []interface{}{"tag1", "tag2"}, + }, + }, + }, + }, + { + name: "slice of objects with mixed numeric types", + clientInfo: &getter.Info{ + Resource: getter.Resource{ + AdditionalStatusFields: []string{"spec.ports"}, + }, + }, + mg: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + body: map[string]interface{}{ + "spec": map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{"name": "http", "port": 80}, + map[string]interface{}{"name": "https", "port": float32(443)}, + }, + }, + }, + wantErr: false, + expected: map[string]interface{}{ + "status": map[string]interface{}{ + "spec": map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{"name": "http", "port": int64(80)}, + map[string]interface{}{"name": "https", "port": int64(443)}, + }, + }, + }, + }, + }, + { + name: "object with nil value", + clientInfo: &getter.Info{ + Resource: getter.Resource{ + AdditionalStatusFields: []string{"config"}, + }, + }, + mg: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + body: map[string]interface{}{ + "config": map[string]interface{}{ + "settingA": "valueA", + "settingB": nil, + }, + }, + wantErr: false, + expected: map[string]interface{}{ + "status": map[string]interface{}{ + "config": map[string]interface{}{ + "settingA": "valueA", + "settingB": nil, + }, + }, + }, + }, + { + name: "slice of objects with mixed numeric types", + clientInfo: &getter.Info{ + Resource: getter.Resource{ + AdditionalStatusFields: []string{"spec.ports"}, + }, + }, + mg: &unstructured.Unstructured{ + Object: map[string]interface{}{}, + }, + body: map[string]interface{}{ + "spec": map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{"name": "http", "port": 80}, + map[string]interface{}{"name": "https", "port": float32(443)}, + }, + }, + }, + wantErr: false, + expected: map[string]interface{}{ + "status": map[string]interface{}{ + "spec": map[string]interface{}{ + "ports": []interface{}{ + map[string]interface{}{"name": "http", "port": int64(80)}, + map[string]interface{}{"name": "https", "port": int64(443)}, + }, + }, + }, + }, + }, } for _, tt := range tests { @@ -542,52 +768,8 @@ func TestPopulateStatusFields(t *testing.T) { } if !tt.wantErr { - // Validate the results - if len(tt.expected) == 0 { - // No status should be created or should be empty - status, exists, _ := unstructured.NestedMap(tt.mg.Object, "status") - if exists && len(status) > 0 { - // Check if there were pre-existing status fields ("existing" field in status) - hasPreExisting := false - for _, test := range tests { - if test.name == tt.name { - if statusObj, ok := test.mg.Object["status"]; ok { - if statusMap, ok := statusObj.(map[string]interface{}); ok && len(statusMap) > 0 { - hasPreExisting = true - break - } - } - } - } - if !hasPreExisting { - t.Errorf("populateStatusFields() unexpected status field created: %v while expected is empty", status) - } - } - } else { - // Validate expected status fields - status, exists, _ := unstructured.NestedMap(tt.mg.Object, "status") - if !exists { - t.Errorf("populateStatusFields() status field not created while length of expected is %d", len(tt.expected)) - return - } - - expectedStatus := tt.expected["status"].(map[string]interface{}) - - // Check that all expected fields are present with correct values - for k, expectedVal := range expectedStatus { - if actualVal, ok := status[k]; !ok { - t.Errorf("populateStatusFields() status.%s not found", k) - } else if actualVal != expectedVal { - t.Errorf("populateStatusFields() status.%s = %v, want %v", k, actualVal, expectedVal) - } - } - - // For tests with existing status, ensure they're preserved - if tt.name == "existing status fields should be preserved" { - if status["existing"] != "existingValue" { - t.Errorf("populateStatusFields() existing status field not preserved") - } - } + if diff := cmp.Diff(tt.expected, tt.mg.Object); diff != "" { + t.Errorf("populateStatusFields() mismatch (-want +got):\n%s", diff) } } }) diff --git a/internal/controllers/restResources.go b/internal/controllers/restResources.go index 5eb435f..a9da119 100644 --- a/internal/controllers/restResources.go +++ b/internal/controllers/restResources.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "net/http" + "os" customcondition "github.com/krateoplatformops/rest-dynamic-controller/internal/controllers/condition" restclient "github.com/krateoplatformops/rest-dynamic-controller/internal/tools/client" @@ -101,13 +102,14 @@ func (h *handler) Observe(ctx context.Context, mg *unstructured.Unstructured) (c cli.Debug = meta.IsVerbose(mg) cli.SetAuth = clientInfo.SetAuth cli.IdentifierFields = clientInfo.Resource.Identifiers + cli.IdentifierMatchPolicy = os.Getenv("REST_CONTROLLER_IDENTIFIER_MATCH_POLICY") // If not set, it will default to "OR" cli.Resource = mg var response restclient.Response // Tries to tries to build the GET API Call, with the given statusFields and specFields values, if it is able to validate the GET request, returns true isKnown := builder.IsResourceKnown(cli, clientInfo, mg) if isKnown { - // Getting the external resource by its identifier + // Getting the external resource by its identifier (e.g GET /resource/{id}). apiCall, callInfo, err := builder.APICallBuilder(cli, clientInfo, apiaction.Get) if apiCall == nil || callInfo == nil { log.Error(fmt.Errorf("API action get not found"), "action", apiaction.Get) @@ -143,6 +145,8 @@ func (h *handler) Observe(ctx context.Context, mg *unstructured.Unstructured) (c return controller.ExternalObservation{}, err } } else { + // Resource is not known, we try to find it by its fields in the items returned by a "list" API call (e.g GET /resources). + // Typically used when the resource does not have an identifier yet, e.g: before creation (first reconcile loop). apiCall, callInfo, err := builder.APICallBuilder(cli, clientInfo, apiaction.FindBy) if apiCall == nil { if !unstructuredtools.IsConditionSet(mg, condition.Creating()) && !unstructuredtools.IsConditionSet(mg, condition.Available()) { @@ -231,7 +235,7 @@ func (h *handler) Observe(ctx context.Context, mg *unstructured.Unstructured) (c if b != nil { err = populateStatusFields(clientInfo, mg, b) if err != nil { - log.Error(err, "Updating identifiers") + log.Error(err, "Updating status fields (identifiers and additionalStatusFields)") return controller.ExternalObservation{}, err } diff --git a/internal/tools/client/builder/builder.go b/internal/tools/client/builder/builder.go index 3421bc7..be5864d 100644 --- a/internal/tools/client/builder/builder.go +++ b/internal/tools/client/builder/builder.go @@ -103,14 +103,14 @@ func BuildCallConfig(callInfo *CallInfo, mg *unstructured.Unstructured, configSp // If the status is not found, it means that the resource is not created yet // The error is not returned here, as it is not critical for the validation // log.Debug("Status not found") - statusFields = make(map[string]interface{}) + statusFields = make(map[string]interface{}) // Initialize as empty map } specFields, err := unstructuredtools.GetFieldsFromUnstructured(mg, "spec") if err != nil { // If the spec is not found, it means that the resource is not created yet // The error is not returned here, as it is not critical for the validation // log.Debug("Spec not found") - specFields = make(map[string]interface{}) + specFields = make(map[string]interface{}) // Initialize as empty map } processFields(callInfo, specFields, reqConfiguration, mapBody) @@ -120,7 +120,7 @@ func BuildCallConfig(callInfo *CallInfo, mg *unstructured.Unstructured, configSp return reqConfiguration } -// applyConfigSpec populates the request configuration from a configuration spec map. +// applyConfigSpec populates the request configuration from a configuration spec map (coming from the Configuration CR) func applyConfigSpec(req *restclient.RequestConfiguration, configSpec map[string]interface{}, action string) { if configSpec == nil { return @@ -147,8 +147,11 @@ func applyConfigSpec(req *restclient.RequestConfiguration, configSpec map[string process("path", req.Parameters) } -// IsResourceKnown tries to build the GET API Call, with the given statusFields and specFields values, if it is able to validate the GET request, returns true -// This function is used during a 'findby' action +// IsResourceKnown tries to build the GET API Call, with the given statusFields and specFields values +// If it is able to validate the GET request, returns true +// This function is used during the reconciliation to decide: +// - if the resource can be retrieved by its identifier (e.g GET /resource/{id}) +// - or if it needs to be found by its fields in a list of resources (e.g GET /resources) func IsResourceKnown(cli restclient.UnstructuredClientInterface, clientInfo *getter.Info, mg *unstructured.Unstructured) bool { if mg == nil || clientInfo == nil { return false @@ -168,7 +171,7 @@ func IsResourceKnown(cli restclient.UnstructuredClientInterface, clientInfo *get actionGetMethod := "GET" for _, descr := range clientInfo.Resource.VerbsDescription { - if strings.EqualFold(descr.Action, apiaction.Get.String()) { + if strings.EqualFold(descr.Action, apiaction.Get.String()) { // Needed if the action `get` in RestDefinition is not mapped to GET method actionGetMethod = descr.Method } } diff --git a/internal/tools/client/clienttools.go b/internal/tools/client/clienttools.go index cbbe2db..e01cc40 100644 --- a/internal/tools/client/clienttools.go +++ b/internal/tools/client/clienttools.go @@ -9,13 +9,12 @@ import ( "os" pathutil "path" "path/filepath" - "reflect" "strconv" "strings" stringset "github.com/krateoplatformops/rest-dynamic-controller/internal/text" + "github.com/krateoplatformops/rest-dynamic-controller/internal/tools/comparison" fgetter "github.com/krateoplatformops/rest-dynamic-controller/internal/tools/filegetter" - unstructuredtools "github.com/krateoplatformops/unstructured-runtime/pkg/tools/unstructured" "github.com/pb33f/libopenapi" "github.com/pb33f/libopenapi/datamodel/high/base" v3 "github.com/pb33f/libopenapi/datamodel/high/v3" @@ -97,12 +96,13 @@ type UnstructuredClientInterface interface { } type UnstructuredClient struct { - IdentifierFields []string - Resource *unstructured.Unstructured - DocScheme *libopenapi.DocumentModel[v3.Document] - Server string - Debug bool - SetAuth func(req *http.Request) + IdentifierFields []string + IdentifierMatchPolicy string + Resource *unstructured.Unstructured + DocScheme *libopenapi.DocumentModel[v3.Document] + Server string + Debug bool + SetAuth func(req *http.Request) } type RequestConfiguration struct { @@ -114,47 +114,47 @@ type RequestConfiguration struct { Method string } -// isInResource checks if the given value is present in the resource's spec or status fields. -func (u *UnstructuredClient) isInResource(value string, fields ...string) (bool, error) { +// isInResource compares a value from an API response with the corresponding value +// in the local Unstructured resource. It checks for the identifier's presence +// and correctness in 'spec' first, then falls back to checking 'status'. +func (u *UnstructuredClient) isInResource(responseValue interface{}, fieldPath ...string) (bool, error) { if u.Resource == nil { return false, fmt.Errorf("resource is nil") } - specs, err := unstructuredtools.GetFieldsFromUnstructured(u.Resource, "spec") - if err != nil { - return false, fmt.Errorf("getting fields from unstructured: %w", err) - } - - val, ok, err := unstructured.NestedFieldCopy(specs, fields...) - if err != nil { - return false, fmt.Errorf("getting nested field: %w", err) - } - if ok && reflect.DeepEqual(val, value) { - return true, nil - } - - // if value is not found in spec, we check the status (if it exists) - if u.Resource.Object["status"] == nil { - return false, nil - } - status, err := unstructuredtools.GetFieldsFromUnstructured(u.Resource, "status") - if err != nil { - return false, fmt.Errorf("getting fields from unstructured: %w", err) - } + //log.Printf("isInResource: comparing field '%s', API value: '%v'", strings.Join(fieldPath, "."), responseValue) - val, ok, err = unstructured.NestedFieldCopy(status, fields...) - if err != nil { - return false, fmt.Errorf("getting nested field: %w", err) - } - if !ok { - return false, nil + // Check 1: Look for the identifier in the 'spec'. + if localValue, found, err := unstructured.NestedFieldNoCopy(u.Resource.Object, append([]string{"spec"}, fieldPath...)...); err == nil && found { + //log.Printf("isInResource: found field in spec: '%v'", localValue) + // If the field is found in the spec, we compare it. + // If it matches, we have a definitive match and can return true. + if comparison.DeepEqual(localValue, responseValue) { + //log.Printf("isInResource: match found in spec.") + return true, nil + } + //log.Printf("isInResource: value in spec did not match.") + } else if err != nil { + return false, fmt.Errorf("error searching for identifier in spec: %w", err) + } + + // Check 2: If the identifier was not found in spec, or if it was found but did not match, + // we proceed to check the 'status'. This is common for server-assigned identifiers. + if localValue, found, err := unstructured.NestedFieldNoCopy(u.Resource.Object, append([]string{"status"}, fieldPath...)...); err == nil && found { + //log.Printf("isInResource: found field in status: '%v'", localValue) + // If found in status, we compare it. This is the last chance for a match. + if comparison.DeepEqual(localValue, responseValue) { + //log.Printf("isInResource: match found in status.") + return true, nil + } + //log.Printf("isInResource: value in status did not match.") + } else if err != nil { + return false, fmt.Errorf("error searching for identifier in status: %w", err) } - if reflect.DeepEqual(val, value) { - return true, nil - } + // No match. + //log.Printf("isInResource: value for field '%s' not found in spec or status.", strings.Join(fieldPath, ".")) - // end of the search, if we reach this point, the value is not found return false, nil } diff --git a/internal/tools/client/clienttools_test.go b/internal/tools/client/clienttools_test.go index e20d754..1f29082 100644 --- a/internal/tools/client/clienttools_test.go +++ b/internal/tools/client/clienttools_test.go @@ -478,11 +478,6 @@ func TestUnstructuredClient_RequestedBody(t *testing.T) { } if got != nil { - if got == nil { - t.Errorf("RequestedBody() returned nil, want %v", tt.want) - return - } - for _, wantItem := range tt.want { if !got.Contains(wantItem) { t.Errorf("RequestedBody() missing expected item %q", wantItem) diff --git a/internal/tools/client/restclient.go b/internal/tools/client/restclient.go index c01fced..c0ff3e0 100644 --- a/internal/tools/client/restclient.go +++ b/internal/tools/client/restclient.go @@ -159,8 +159,11 @@ func (u *UnstructuredClient) Call(ctx context.Context, cli *http.Client, path st }, nil } -// It support both list and single item responses +// FindBy locates a specific resource within an API response it retrieves. +// It serves as the primary orchestrator for the find operation, +// delegating response parsing and item matching to helper functions. func (u *UnstructuredClient) FindBy(ctx context.Context, cli *http.Client, path string, opts *RequestConfiguration) (Response, error) { + // Execute the initial API call. response, err := u.Call(ctx, cli, path, opts) if err != nil { return Response{}, err @@ -172,59 +175,164 @@ func (u *UnstructuredClient) FindBy(ctx context.Context, cli *http.Client, path } } - list := response.ResponseBody + // Normalize the API response to get a consistent list of items to search. + // This handles multiple response shapes (e.g., raw list, wrapped list, single object). + // Shapes handled: + // 1. A standard JSON array: `[{"id": 1}, {"id": 2}]` + // 2. An object wrapping the array: `{"items": [{"id": 1}, {"id": 2}], "count": 2}` Note: we take the first array we find in the object as we don't know the property name in advance. + // 3. A single object, for endpoints that don't use an array for single-item results: `{"id": 1}` (e.g. when the collection only has one item at the moment) + itemList, err := u.extractItemsFromResponse(response.ResponseBody) + if err != nil { + return Response{}, err + } + + // Delegate the search logic to a dedicated helper function. + if matchedItem, found := u.findItemInList(itemList); found { + return Response{ + ResponseBody: matchedItem, + statusCode: response.statusCode, + }, nil + } + + // If no match is found after checking all items, return a Not Found error. + return Response{}, &StatusError{ + StatusCode: http.StatusNotFound, + Inner: fmt.Errorf("item not found"), + } +} + +// extractItemsFromResponse parses the body of an API response and extracts a list of items. +// It is designed to handle three common API response patterns for list operations: +// 1. A standard JSON array: `[{"id": 1}, {"id": 2}]` +// 2. An object wrapping the array: `{"items": [{"id": 1}, {"id": 2}]}` +// 3. A single object, for endpoints that don't use an array for single-item results: `{"id": 1}` +func (u *UnstructuredClient) extractItemsFromResponse(body interface{}) ([]interface{}, error) { + // Case 1: The body is already a standard list (JSON array). + if list, ok := body.([]interface{}); ok { + return list, nil + } - var li map[string]interface{} - if _, ok := list.([]interface{}); ok { - li = map[string]interface{}{ - "items": list, + // Case 2 and 3: The body is an object (map). + if body == nil { + return nil, fmt.Errorf("response body is nil") + } + if bodyMap, ok := body.(map[string]interface{}); ok { + if len(bodyMap) == 0 { + return []interface{}{}, nil } - } else { - li, ok = list.(map[string]interface{}) + + // Case 2: The body is an object, which may contain a list. + // Iterate through its values to find the first one that is a list. + for _, v := range bodyMap { + if list, ok := v.([]interface{}); ok { + return list, nil + } + } + + // Case 3: If no list was found inside the object, assume the object + // itself is the single item we are looking for e.g. `{"id": 1}`. + // Wrap it in a slice to create a list of one. + return []interface{}{bodyMap}, nil + } + + // If the body is not a list or an object, it's an unexpected type. + return nil, fmt.Errorf("unexpected response type: %T", body) +} + +// findItemInList iterates through a slice of items and checks if any of them +// match the identifiers of the local resource. +func (u *UnstructuredClient) findItemInList(items []interface{}) (map[string]interface{}, bool) { + if len(items) == 0 { + return nil, false + } + + for _, item := range items { + itemMap, ok := item.(map[string]interface{}) if !ok { - return Response{}, fmt.Errorf("unexpected response type: %T", list) + // Skip any elements in the list that are not JSON objects. + continue } + + // Delegate the matching logic for a single item to a dedicated helper. + isMatch, err := u.isItemMatch(itemMap) + if err != nil { + // If an error occurs during comparison, we cannot consider it a match. + // For now, we log the error and continue searching. + // log.Printf("error matching item: %v", err) // Optional: for debugging + continue + } + + if isMatch { + // If a match is found, return the item immediately. + return itemMap, true + } + } + + // Return false if no match was found in the entire list. + return nil, false +} + +// isItemMatch checks if a single item (from an API response) matches the local resource +// by comparing all configured identifier fields. +func (u *UnstructuredClient) isItemMatch(itemMap map[string]interface{}) (bool, error) { + // Normalize the policy string to lowercase + policy := strings.ToLower(u.IdentifierMatchPolicy) + + // If no identifiers are specified, no match is possible. + if len(u.IdentifierFields) == 0 { + return false, nil } - for _, v := range li { - if vli, ok := v.([]interface{}); ok { - if len(vli) > 0 { - for _, item := range vli { - itMap, ok := item.(map[string]interface{}) - if !ok { - continue // skip this item if it's not a map - } - - for _, ide := range u.IdentifierFields { - idepath := strings.Split(ide, ".") // split the identifier field by '.' - responseValue, _, err := unstructured.NestedString(itMap, idepath...) - if err != nil { - val, _, err := unstructured.NestedFieldNoCopy(itMap, idepath...) - if err != nil { - return Response{}, fmt.Errorf("getting nested field: %w", err) - } - responseValue = fmt.Sprintf("%v", val) - } - ok, err = u.isInResource(responseValue, idepath...) - if err != nil { - return Response{}, err - } - if ok { - return Response{ - ResponseBody: itMap, - statusCode: response.statusCode, - }, nil - } - } - - } + if policy == "and" { + // AND Logic: Return false on the first failed match. + for _, ide := range u.IdentifierFields { + idepath := strings.Split(ide, ".") + + val, found, err := unstructured.NestedFieldNoCopy(itemMap, idepath...) + if err != nil || !found { + // If any identifier is missing, it's not an AND match. + return false, nil + } + + ok, err := u.isInResource(val, idepath...) + if err != nil { + // A hard error during comparison should be propagated up. + return false, err + } + if !ok { + // If any identifier does not match, it's not an AND match. + return false, nil } - break } - } - return Response{}, &StatusError{ - StatusCode: http.StatusNotFound, - Inner: fmt.Errorf("item not found"), + + // If the loop completes, it means all identifiers matched. + return true, nil + + } else { + // OR Logic (default): Return true on the first successful identifier match. + for _, ide := range u.IdentifierFields { + idepath := strings.Split(ide, ".") + + val, found, err := unstructured.NestedFieldNoCopy(itemMap, idepath...) + if err != nil || !found { + // If field is not found or there is an error, it's not a match for this identifier, so we continue. + continue + } + + ok, err := u.isInResource(val, idepath...) + if err != nil { + // A hard error during comparison should be propagated up. + return false, err + } + + if ok { + // On the first match, we can return true. + return true, nil + } + } + + // If the loop completes, no identifiers matched. + return false, nil } } diff --git a/internal/tools/client/restclient_test.go b/internal/tools/client/restclient_test.go index 3ba9554..f22ba07 100644 --- a/internal/tools/client/restclient_test.go +++ b/internal/tools/client/restclient_test.go @@ -559,7 +559,7 @@ func TestFindBy(t *testing.T) { Method: "GET", }, identifierFields: []string{"id"}, - mg: map[string]interface{}{"spec": map[string]interface{}{"id": "456"}}, // Will be converted to string for comparison + mg: map[string]interface{}{"spec": map[string]interface{}{"id": 456}}, expected: map[string]interface{}{"id": 456, "name": "item2"}, }, } @@ -657,3 +657,370 @@ func (m *mockTransportImpl) RoundTrip(req *http.Request) (*http.Response, error) // Return the recorded response return rr.Result(), nil } + +func TestResponse_IsPending(t *testing.T) { + tests := []struct { + name string + response *Response + want bool + }{ + { + name: "should be true for StatusProcessing", + response: &Response{statusCode: http.StatusProcessing}, + want: true, + }, + { + name: "should be true for StatusContinue", + response: &Response{statusCode: http.StatusContinue}, + want: true, + }, + { + name: "should be true for StatusAccepted", + response: &Response{statusCode: http.StatusAccepted}, + want: true, + }, + { + name: "should be false for StatusOK", + response: &Response{statusCode: http.StatusOK}, + want: false, + }, + { + name: "should be false for StatusBadRequest", + response: &Response{statusCode: http.StatusBadRequest}, + want: false, + }, + { + name: "should be false for nil response", + response: nil, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := tt.response.IsPending(); got != tt.want { + t.Errorf("Response.IsPending() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestExtractItemsFromResponse(t *testing.T) { + client := &UnstructuredClient{} + singleItem := map[string]interface{}{"id": 1} + tests := []struct { + name string + body interface{} + want []interface{} + wantErr bool + }{ + { + name: "standard list response", + body: []interface{}{singleItem, map[string]interface{}{"id": 2}}, + want: []interface{}{singleItem, map[string]interface{}{"id": 2}}, + }, + { + name: "wrapped list response", + body: map[string]interface{}{"items": []interface{}{singleItem}, "count": 1}, + want: []interface{}{singleItem}, + }, + { + name: "single object response", + body: singleItem, + want: []interface{}{singleItem}, + }, + { + name: "empty list response", + body: []interface{}{}, + want: []interface{}{}, + }, + { + name: "empty object response", + body: map[string]interface{}{}, + want: []interface{}{}, + }, + { + name: "nil body", + body: nil, + wantErr: true, + }, + { + name: "invalid body type", + body: "a string", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := client.extractItemsFromResponse(tt.body) + if (err != nil) != tt.wantErr { + t.Errorf("extractItemsFromResponse() error = %v, wantErr %v", err, tt.wantErr) + return + } + assert.Equal(t, tt.want, got, "extractItemsFromResponse() got = %v, want %v", got, tt.want) + }) + } +} + +func TestFindItemInList(t *testing.T) { + item1 := map[string]interface{}{"name": "one", "value": 1} + item2 := map[string]interface{}{"name": "two", "value": 2} + + tests := []struct { + name string + items []interface{} + client *UnstructuredClient + wantItem map[string]interface{} + wantFound bool + }{ + { + name: "match found", + items: []interface{}{item1, item2}, + client: &UnstructuredClient{ + IdentifierFields: []string{"name"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"name": "two"}, + }}, + }, + wantItem: item2, + wantFound: true, + }, + { + name: "no match found", + items: []interface{}{item1, item2}, + client: &UnstructuredClient{ + IdentifierFields: []string{"name"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"name": "three"}, + }}, + }, + wantItem: nil, + wantFound: false, + }, + { + name: "empty item list", + items: []interface{}{}, + client: &UnstructuredClient{}, + wantItem: nil, + wantFound: false, + }, + { + name: "list with non-object items", + items: []interface{}{"a string", 123, nil, item2}, + client: &UnstructuredClient{ + IdentifierFields: []string{"name"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"name": "two"}, + }}, + }, + wantItem: item2, + wantFound: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotItem, gotFound := tt.client.findItemInList(tt.items) + assert.Equal(t, tt.wantFound, gotFound) + assert.Equal(t, tt.wantItem, gotItem) + }) + } +} + +func TestIsItemMatch(t *testing.T) { + tests := []struct { + name string + client *UnstructuredClient + itemMap map[string]interface{} + wantMatch bool + wantErr bool + }{ + // --- AND Policy Tests --- + { + name: "AND policy, all identifiers match", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "AND", + IdentifierFields: []string{"name", "region"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"name": "test-vm", "region": "us-east-1"}, + }}, + }, + itemMap: map[string]interface{}{"name": "test-vm", "region": "us-east-1"}, + wantMatch: true, + }, + { + name: "AND policy, one identifier does not match", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "AND", + IdentifierFields: []string{"name", "region"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"name": "test-vm", "region": "us-east-1"}, + }}, + }, + itemMap: map[string]interface{}{"name": "test-vm", "region": "us-west-2"}, + wantMatch: false, + }, + { + name: "AND policy, one identifier is missing from item", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "AND", + IdentifierFields: []string{"name", "region"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"name": "test-vm", "region": "us-east-1"}, + }}, + }, + itemMap: map[string]interface{}{"name": "test-vm"}, + wantMatch: false, + }, + + // --- OR Policy Tests --- + { + name: "OR policy, first identifier matches", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "OR", + IdentifierFields: []string{"name", "id"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"name": "test-vm", "id": "vm-stale"}, + }}, + }, + itemMap: map[string]interface{}{"name": "test-vm", "id": "vm-123"}, + wantMatch: true, + }, + { + name: "OR policy, second identifier matches", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "OR", + IdentifierFields: []string{"name", "id"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"name": "stale-name", "id": "vm-123"}, + }}, + }, + itemMap: map[string]interface{}{"name": "test-vm", "id": "vm-123"}, + wantMatch: true, + }, + { + name: "OR policy, no identifiers match", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "OR", + IdentifierFields: []string{"name", "id"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"name": "stale-name", "id": "vm-stale"}, + }}, + }, + itemMap: map[string]interface{}{"name": "test-vm", "id": "vm-123"}, + wantMatch: false, + }, + + // --- Edge Cases --- + { + name: "No identifiers specified", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "AND", + IdentifierFields: []string{}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{}}, + }, + itemMap: map[string]interface{}{"name": "test-vm"}, + wantMatch: false, + }, + { + name: "Default policy (empty) is OR, match succeeds on partial match", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "", // Default should be OR + IdentifierFields: []string{"name", "region"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"name": "test-vm", "region": "us-east-1"}, + }}, + }, + itemMap: map[string]interface{}{"name": "test-vm", "region": "us-west-2"}, // name matches, region doesn't + wantMatch: true, // Should be true with OR logic + }, + { + name: "AND policy explicitly set, match fails on partial match", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "And", + IdentifierFields: []string{"name", "region"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"name": "test-vm", "region": "us-east-1"}, + }}, + }, + itemMap: map[string]interface{}{"name": "test-vm", "region": "us-west-2"}, + wantMatch: false, // Should be false with AND logic + }, + + // --- Complex Identifier Tests --- + { + name: "AND policy, nested object identifier matches", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "AND", + IdentifierFields: []string{"metadata.labels"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"metadata": map[string]interface{}{ + "labels": map[string]string{"app": "database", "tier": "backend"}, + }}, + }}, + }, + itemMap: map[string]interface{}{"metadata": map[string]interface{}{ + "labels": map[string]string{"tier": "backend", "app": "database"}, // Key order is different + }}, + wantMatch: true, // DeepEqual should handle map key order + }, + { + name: "AND policy, nested object identifier fails", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "AND", + IdentifierFields: []string{"metadata.labels"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{"metadata": map[string]interface{}{ + "labels": map[string]string{"app": "database", "tier": "backend"}, + }}, + }}, + }, + itemMap: map[string]interface{}{"metadata": map[string]interface{}{ + "labels": map[string]string{"app": "database", "tier": "frontend"}, // One value is different + }}, + wantMatch: false, + }, + { + name: "OR policy, array identifier matches", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "OR", + IdentifierFields: []string{"ports", "name"}, + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "name": "stale-name", + "ports": []interface{}{80, 443}, + }, + }}, + }, + itemMap: map[string]interface{}{"name": "test-svc", "ports": []interface{}{80, 443}}, + wantMatch: true, + }, + { + name: "OR policy, array identifier fails (order matters)", + client: &UnstructuredClient{ + IdentifierMatchPolicy: "OR", + IdentifierFields: []string{"ports"}, // Only test the array + Resource: &unstructured.Unstructured{Object: map[string]interface{}{ + "spec": map[string]interface{}{ + "name": "test-svc", + "ports": []interface{}{80, 443}, + }, + }}, + }, + itemMap: map[string]interface{}{"name": "test-svc", "ports": []interface{}{443, 80}}, // Order is different + wantMatch: false, // DeepEqual for slices is order-sensitive + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotMatch, err := tt.client.isItemMatch(tt.itemMap) + if (err != nil) != tt.wantErr { + t.Errorf("isItemMatch() error = %v, wantErr %v", err, tt.wantErr) + return + } + assert.Equal(t, tt.wantMatch, gotMatch) + }) + } +} diff --git a/internal/tools/comparison/comparison.go b/internal/tools/comparison/comparison.go index 2a53eee..7632a72 100644 --- a/internal/tools/comparison/comparison.go +++ b/internal/tools/comparison/comparison.go @@ -225,3 +225,30 @@ func compareAny(a any, b any) bool { return cmp.Equal(a, b) } + +// DeepEqual performs a deep comparison between two values. +// It is suitable for comparing also complex structures like maps and slices. +// For maps (objects), key order does not matter. +// For slices (arrays), element order and content are strictly compared. +func DeepEqual(a, b interface{}) bool { + // PROBABLY NOT NEEDED + // For complex types, a direct recursive comparison is correct and respects + // the nuances of map and slice comparison. + //aKind := reflect.TypeOf(a).Kind() + //bKind := reflect.TypeOf(b).Kind() + //if aKind == reflect.Map || aKind == reflect.Slice || bKind == reflect.Map || bKind == reflect.Slice { + // return cmp.Equal(a, b) + //} + // + //// For primary types (string, bool, numbers), we use a normalization + //// step to handle type discrepancies, such as int64 from a CRD vs. + //// float64 from a JSON response. + //strA := fmt.Sprintf("%v", a) + //strB := fmt.Sprintf("%v", b) + // + //normA := jqutil.InferType(strA) + //normB := jqutil.InferType(strB) + + //return cmp.Equal(normA, normB) + return cmp.Equal(a, b) +} diff --git a/internal/tools/comparison/comparison_test.go b/internal/tools/comparison/comparison_test.go index df4d606..a8018ad 100644 --- a/internal/tools/comparison/comparison_test.go +++ b/internal/tools/comparison/comparison_test.go @@ -665,3 +665,55 @@ func TestCompareExisting_SliceTypeAssertionFailure(t *testing.T) { t.Error("expected IsEqual=false for type assertion failure") } } + +func TestDeepEqual(t *testing.T) { + tests := []struct { + name string + a interface{} + b interface{} + want bool + }{ + // Primary Types & Normalization + {name: "equal integers", a: 42, b: 42, want: true}, + {name: "different integers", a: 42, b: 43, want: false}, + {name: "equal floats", a: 3.14, b: 3.14, want: true}, + {name: "equal strings", a: "hello", b: "hello", want: true}, + {name: "different strings", a: "hello", b: "world", want: false}, + {name: "equal booleans", a: true, b: true, want: true}, + {name: "nil vs nil", a: nil, b: nil, want: true}, + {name: "nil vs non-nil", a: nil, b: 42, want: false}, + // For the following we need to decide on the desired behavior + //{name: "int vs float64 (equal)", a: 42, b: 42.0, want: true}, + //{name: "int64 vs float64 (equal)", a: int64(42), b: 42.0, want: true}, + //{name: "int vs float32 (equal)", a: 42, b: float32(42.0), want: true}, + {name: "int vs float (unequal)", a: 42, b: 42.1, want: false}, + + // Slices (will use direct cmp.Equal) + {name: "equal int slices", a: []int{1, 2, 3}, b: []int{1, 2, 3}, want: true}, + {name: "unequal int slices (content)", a: []int{1, 2, 3}, b: []int{1, 2, 4}, want: false}, + {name: "unequal int slices (order)", a: []int{1, 2, 3}, b: []int{3, 2, 1}, want: false}, + {name: "equal interface slices", a: []interface{}{1, "a"}, b: []interface{}{1, "a"}, want: true}, + // This case is expected to FAIL with the current DeepEqual implementation, demonstrating its limitation. + {name: "int vs float in slices", a: []interface{}{1, "a"}, b: []interface{}{1.0, "a"}, want: false}, + + // Maps (will use direct cmp.Equal) + {name: "equal maps", a: map[string]int{"a": 1}, b: map[string]int{"a": 1}, want: true}, + {name: "equal maps (different key order)", a: map[string]int{"a": 1, "b": 2}, b: map[string]int{"b": 2, "a": 1}, want: true}, + {name: "unequal maps (value)", a: map[string]int{"a": 1}, b: map[string]int{"a": 2}, want: false}, + {name: "unequal maps (key)", a: map[string]int{"a": 1}, b: map[string]int{"c": 1}, want: false}, + // This case is expected to FAIL with the current DeepEqual implementation, demonstrating its limitation. + {name: "int vs float in maps", a: map[string]interface{}{"a": 1}, b: map[string]interface{}{"a": 1.0}, want: false}, + + // Nested Structures (will use direct cmp.Equal) + {name: "equal nested maps", a: map[string]interface{}{"data": map[string]int{"a": 1}}, b: map[string]interface{}{"data": map[string]int{"a": 1}}, want: true}, + {name: "equal map with slice", a: map[string]interface{}{"data": []int{1, 2}}, b: map[string]interface{}{"data": []int{1, 2}}, want: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := DeepEqual(tt.a, tt.b); got != tt.want { + t.Errorf("DeepEqual() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/testdata/restdefinitions/sample.yaml b/testdata/restdefinitions/sample.yaml index 93f3209..7ee444a 100644 --- a/testdata/restdefinitions/sample.yaml +++ b/testdata/restdefinitions/sample.yaml @@ -8,8 +8,9 @@ spec: resourceGroup: sample.krateo.io resource: identifiers: - - id - name + additionalStatusFields: + - id kind: Sample verbsDescription: - action: create