diff --git a/docs/alerts.md b/docs/alerts.md index 604e0d9d..23828f0c 100644 --- a/docs/alerts.md +++ b/docs/alerts.md @@ -45,6 +45,25 @@ annotations: description: "More than 10% of events are failing for {{ $labels.component }}." ``` +### HyperFleet API Authentication or Authorization Failures + +```yaml +alert: HyperFleetAdapterAPIAuthFailures +expr: | + sum by (component, version, adapter_name, status_code) ( + increase(hyperfleet_adapter_api_auth_failures_total[5m]) + ) >= 3 +labels: + severity: critical +annotations: + summary: "HyperFleet Adapter API authentication or authorization failures" + description: "The HyperFleet API returned HTTP {{ $labels.status_code }} to {{ $labels.adapter_name }}. Check its service-account token, API gateway configuration, and subject allowlist. When tenant enforcement is enabled, verify tenant dimensions are present and non-empty and tenant headers are propagated." +``` + +**Impact:** The affected adapter cannot complete API-backed parameter extraction, preconditions, or post-actions until its identity is accepted. + +**Response:** For HTTP 403 responses with tenant enforcement enabled, verify the gateway-injected tenant dimensions before changing the service-account token or subject allowlist. See [API Authentication and Authorization Failures](runbook.md#api-authentication-and-authorization-failures). + ### No Events Processed (Dead Man's Switch) ```yaml diff --git a/docs/metrics.md b/docs/metrics.md index fd6cb6a9..40828efc 100644 --- a/docs/metrics.md +++ b/docs/metrics.md @@ -26,6 +26,7 @@ All adapter metrics include `component` and `version` as constant labels. Event- | `hyperfleet_adapter_events_processed_total` | Counter | `component`, `version`, `adapter_name`, `status` | Total CloudEvents processed. Status: `success`, `failed`, `skipped` | | `hyperfleet_adapter_event_processing_duration_seconds` | Histogram | `component`, `version`, `adapter_name` | End-to-end event processing duration | | `hyperfleet_adapter_errors_total` | Counter | `component`, `version`, `adapter_name`, `error_type` | Total errors by execution phase | +| `hyperfleet_adapter_api_auth_failures_total` | Counter | `component`, `version`, `adapter_name`, `status_code` | Total HyperFleet API authentication and authorization failures. `status_code` is always `401` or `403` | #### Status Values @@ -103,6 +104,14 @@ Error rate by phase: sum by (error_type) (rate(hyperfleet_adapter_errors_total[5m])) ``` +HyperFleet API authentication and authorization failures: + +```promql +sum by (status_code) ( + rate(hyperfleet_adapter_api_auth_failures_total{status_code=~"401|403"}[5m]) +) +``` + ## Broker Metrics The adapter automatically registers Prometheus metrics from the [hyperfleet-broker](https://github.com/openshift-hyperfleet/hyperfleet-broker) library. diff --git a/docs/runbook.md b/docs/runbook.md index 7a7c8fba..24f08fca 100644 --- a/docs/runbook.md +++ b/docs/runbook.md @@ -181,14 +181,23 @@ All event failures are ACKed (not retried) to avoid infinite loops on non-transi The adapter retries on 5xx, 408 (Request Timeout), and 429 (Too Many Requests) with configurable backoff (exponential, linear, or constant). +#### API Authentication and Authorization Failures + +**Symptoms:** `hyperfleet_adapter_api_auth_failures_total{status_code=~"401|403"}` is increasing. Error logs include `http_status` and either `phase` (parameter extraction or preconditions) or `post_action`, with the affected resource context. + +HTTP 401 and 403 failures are ACKed under the normal event-failure policy; they are not redelivered automatically. Redelivery cannot repair invalid credentials, subject allowlists, or tenant dimensions. + +**Remediation:** For HTTP 403 responses with tenant enforcement enabled, verify the gateway-injected tenant dimensions before changing the projected service-account token or subject allowlist. If they are correct, check the token and allowlist, then rely on normal upstream reconciliation. + **Steps:** -1. Check HyperFleet API health: `kubectl get pods -l app=hyperfleet-api` -2. Check API response times from the adapter pod: +1. With tenant enforcement enabled, verify gateway-injected tenant dimensions match the adapter's tenant. +2. Check HyperFleet API health: `kubectl get pods -l app=hyperfleet-api` +3. Check API response times from the adapter pod: ```bash kubectl exec -- curl -s -o /dev/null -w "%{http_code} %{time_total}s" http://hyperfleet-api:8000/healthz ``` -3. Check for resource exhaustion on the API service -4. Review `retryAttempts` and `timeout` in adapter config +4. Check for resource exhaustion on the API service +5. Review `retryAttempts` and `timeout` in adapter config --- diff --git a/internal/executor/auth_failure_metrics_test.go b/internal/executor/auth_failure_metrics_test.go new file mode 100644 index 00000000..f0af71bd --- /dev/null +++ b/internal/executor/auth_failure_metrics_test.go @@ -0,0 +1,369 @@ +package executor + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "log/slog" + "net/http" + "testing" + + "github.com/cloudevents/sdk-go/v2/event" + hyperfleetlogger "github.com/openshift-hyperfleet/hyperfleet-logger" + "github.com/prometheus/client_golang/prometheus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/configloader" + "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/hyperfleetapi" + "github.com/openshift-hyperfleet/hyperfleet-adapter/internal/k8sclient" + "github.com/openshift-hyperfleet/hyperfleet-adapter/pkg/metrics" +) + +func TestCreateHandler_PostActionAPIAuthFailuresAreAcknowledgedAndRecorded(t *testing.T) { + tests := []struct { + name string + statusCode int + }{ + {name: "401 unauthorized", statusCode: http.StatusUnauthorized}, + {name: "403 forbidden", statusCode: http.StatusForbidden}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + registry := prometheus.NewRegistry() + recorder := metrics.NewRecorder("test-adapter", "v0.1.0", "test", registry) + mockClient := newMockAPIClient() + mockClient.PutResponse = &hyperfleetapi.Response{ + StatusCode: tt.statusCode, + Status: fmt.Sprintf("%d %s", tt.statusCode, http.StatusText(tt.statusCode)), + Body: []byte(`{"error":"authentication failed"}`), + Attempts: 1, + } + + exec, err := NewBuilder(). + WithConfig(authFailurePostActionConfig()). + WithAPIClient(mockClient). + WithTransportClient(k8sclient.NewMockK8sClient()). + Build() + require.NoError(t, err) + + handler := AlwaysAck(WithMetrics(exec.CreateHandler(), recorder)) + err = handler(context.Background(), authFailureEvent(t, "cluster-auth-failure")) + require.NoError(t, err, "authentication failures must be ACKed for external remediation") + + families, err := registry.Gather() + require.NoError(t, err) + assert.Equal(t, float64(1), getCounterValue(t, families, + "hyperfleet_adapter_events_processed_total", "status", "failed"), + "auth-failed post-action must fail the event") + assert.Equal(t, float64(1), getCounterValue(t, families, + "hyperfleet_adapter_errors_total", "error_type", "post_actions"), + "post_actions phase error metric behavior must remain unchanged") + assert.Equal(t, float64(1), getCounterValue(t, families, + "hyperfleet_adapter_api_auth_failures_total", "status_code", fmt.Sprint(tt.statusCode)), + "auth failures must be classified by exact bounded HTTP status") + }) + } +} + +func TestCreateHandler_PostActionNonAuthAPIFailureDoesNotEmitAuthMetric(t *testing.T) { + registry := prometheus.NewRegistry() + recorder := metrics.NewRecorder("test-adapter", "v0.1.0", "test", registry) + mockClient := newMockAPIClient() + mockClient.PutResponse = &hyperfleetapi.Response{ + StatusCode: http.StatusInternalServerError, + Status: "500 Internal Server Error", + Body: []byte(`{"error":"upstream unavailable"}`), + Attempts: 1, + } + + exec, err := NewBuilder(). + WithConfig(authFailurePostActionConfig()). + WithAPIClient(mockClient). + WithTransportClient(k8sclient.NewMockK8sClient()). + Build() + require.NoError(t, err) + + handler := AlwaysAck(WithMetrics(exec.CreateHandler(), recorder)) + err = handler(context.Background(), authFailureEvent(t, "cluster-non-auth-failure")) + require.NoError(t, err, "existing always-ACK behavior for non-auth failures must remain unchanged") + + families, err := registry.Gather() + require.NoError(t, err) + assert.Equal(t, float64(1), getCounterValue(t, families, + "hyperfleet_adapter_events_processed_total", "status", "failed")) + assert.Equal(t, float64(1), getCounterValue(t, families, + "hyperfleet_adapter_errors_total", "error_type", "post_actions")) + assert.Nil(t, findFamily(families, "hyperfleet_adapter_api_auth_failures_total"), + "non-auth API failures must not create an auth-failure metric series") +} + +func TestCreateHandler_OptionalAPIParameterAuthFailureIsLoggedAndRecorded(t *testing.T) { + previous := slog.Default() + var logs bytes.Buffer + slog.SetDefault(slog.New(hyperfleetlogger.NewHandler( + "test", + "test", + hyperfleetlogger.WithLevel(slog.LevelDebug), + hyperfleetlogger.WithFormat(hyperfleetlogger.FormatText), + hyperfleetlogger.WithOutput(&logs), + ))) + t.Cleanup(func() { slog.SetDefault(previous) }) + + registry := prometheus.NewRegistry() + recorder := metrics.NewRecorder("test-adapter", "v0.1.0", "test", registry) + mockClient := newMockAPIClient() + mockClient.GetResponse = &hyperfleetapi.Response{ + StatusCode: http.StatusForbidden, + Status: "403 Forbidden", + Body: []byte(`optional-api-parameter-auth-response-must-not-be-logged`), + } + + exec, err := NewBuilder(). + WithConfig(&configloader.Config{ + Adapter: configloader.AdapterInfo{Name: "test-adapter", Version: "v0.1.0"}, + Params: []configloader.Parameter{ + {Name: "clusterID", Source: configloader.StringSource("event.id"), Required: true}, + { + Name: "cluster", + Source: configloader.APICallSource(&configloader.APICall{ + Method: http.MethodGet, + URL: "/clusters/{{ .clusterID }}", + }), + Default: map[string]interface{}{"name": "fallback"}, + }, + }, + }). + WithAPIClient(mockClient). + WithTransportClient(k8sclient.NewMockK8sClient()). + Build() + require.NoError(t, err) + + result, err := WithMetrics(exec.CreateHandler(), recorder)( + context.Background(), authFailureEvent(t, "cluster-optional-auth-failure")) + require.NoError(t, err) + require.Equal(t, StatusSuccess, result.Status) + assert.Equal(t, map[string]interface{}{"name": "fallback"}, result.Params["cluster"]) + + families, err := registry.Gather() + require.NoError(t, err) + assert.Equal(t, float64(1), getCounterValue(t, families, + "hyperfleet_adapter_events_processed_total", "status", "success")) + assert.Equal(t, float64(1), getCounterValue(t, families, + "hyperfleet_adapter_api_auth_failures_total", "status_code", "403")) + assert.Contains(t, logs.String(), "http_status=403") + assert.Contains(t, logs.String(), "phase=param_extraction") + assert.Contains(t, logs.String(), "param=cluster") + assert.NotContains(t, logs.String(), "optional-api-parameter-auth-response-must-not-be-logged") +} + +func TestExecutor_PostActionAuthFailureLogsAreContextualAndRedacted(t *testing.T) { + tests := []struct { + sentinel string + name string + statusCode int + expectAuthLog bool + }{ + { + name: "401 unauthorized", + statusCode: http.StatusUnauthorized, + sentinel: "qe-auth-response-body-401-must-not-be-logged", + expectAuthLog: true, + }, + { + name: "403 forbidden", + statusCode: http.StatusForbidden, + sentinel: "qe-auth-response-body-403-must-not-be-logged", + expectAuthLog: true, + }, + { + name: "500 server error", + statusCode: http.StatusInternalServerError, + sentinel: "qe-non-auth-response-body", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + previous := slog.Default() + var logs bytes.Buffer + slog.SetDefault(slog.New(hyperfleetlogger.NewHandler( + "test", + "test", + hyperfleetlogger.WithLevel(slog.LevelDebug), + hyperfleetlogger.WithFormat(hyperfleetlogger.FormatText), + hyperfleetlogger.WithOutput(&logs), + ))) + t.Cleanup(func() { slog.SetDefault(previous) }) + + mockClient := newMockAPIClient() + mockClient.GetResponse = &hyperfleetapi.Response{ + StatusCode: http.StatusOK, + Status: "200 OK", + Body: []byte(`{}`), + } + mockClient.PutResponse = &hyperfleetapi.Response{ + StatusCode: tt.statusCode, + Status: fmt.Sprintf("%d %s", tt.statusCode, http.StatusText(tt.statusCode)), + Body: []byte(tt.sentinel), + } + + exec, err := NewBuilder(). + WithConfig(new404PostActionConfig()). + WithAPIClient(mockClient). + WithTransportClient(k8sclient.NewMockK8sClient()). + Build() + require.NoError(t, err) + + result := exec.Execute(context.Background(), map[string]interface{}{ + "id": "cluster-auth-401", + "kind": "Cluster", + }) + require.Equal(t, StatusFailed, result.Status, "post-action API failure must fail the execution") + + captured := logs.String() + assert.NotContains(t, captured, tt.sentinel, "response bodies must never be written to executor logs") + if !tt.expectAuthLog { + assert.NotContains(t, captured, "http_status=500", + "non-auth API failures must not emit the dedicated auth-failure log") + return + } + + assert.Contains(t, captured, "ERROR") + assert.Contains(t, captured, fmt.Sprintf("http_status=%d", tt.statusCode)) + assert.Contains(t, captured, "post_action=reportStatus") + assert.Contains(t, captured, "cluster_id=cluster-auth-401") + }) + } +} + +func TestExecutor_APIAuthFailuresAreLoggedAcrossExecutionPhases(t *testing.T) { + tests := []struct { + name string + phase ExecutionPhase + config *configloader.Config + responseBody string + }{ + { + name: "parameter extraction", + phase: PhaseParamExtraction, + responseBody: "parameter-auth-response-must-not-be-logged", + config: &configloader.Config{ + Adapter: configloader.AdapterInfo{Name: "test-adapter", Version: "v0.1.0"}, + Params: []configloader.Parameter{ + {Name: "clusterID", Source: configloader.StringSource("event.id"), Required: true}, + { + Name: "cluster", + Required: true, + Source: configloader.APICallSource(&configloader.APICall{ + Method: http.MethodGet, + URL: "/clusters/{{ .clusterID }}", + }), + }, + }, + }, + }, + { + name: "preconditions", + phase: PhasePreconditions, + responseBody: "precondition-auth-response-must-not-be-logged", + config: &configloader.Config{ + Adapter: configloader.AdapterInfo{Name: "test-adapter", Version: "v0.1.0"}, + Params: []configloader.Parameter{ + {Name: "clusterID", Source: configloader.StringSource("event.id"), Required: true}, + }, + Preconditions: []configloader.Precondition{ + { + ActionBase: configloader.ActionBase{ + Name: "fetch-cluster", + APICall: &configloader.APICall{ + Method: http.MethodGet, + URL: "/clusters/{{ .clusterID }}", + }, + }, + }, + }, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + previous := slog.Default() + var logs bytes.Buffer + slog.SetDefault(slog.New(hyperfleetlogger.NewHandler( + "test", + "test", + hyperfleetlogger.WithLevel(slog.LevelDebug), + hyperfleetlogger.WithFormat(hyperfleetlogger.FormatText), + hyperfleetlogger.WithOutput(&logs), + ))) + t.Cleanup(func() { slog.SetDefault(previous) }) + + registry := prometheus.NewRegistry() + recorder := metrics.NewRecorder("test-adapter", "v0.1.0", "test", registry) + mockClient := newMockAPIClient() + mockClient.GetResponse = &hyperfleetapi.Response{ + StatusCode: http.StatusForbidden, + Status: "403 Forbidden", + Body: []byte(tt.responseBody), + } + + exec, err := NewBuilder(). + WithConfig(tt.config). + WithAPIClient(mockClient). + WithTransportClient(k8sclient.NewMockK8sClient()). + Build() + require.NoError(t, err) + + result, err := WithMetrics(exec.CreateHandler(), recorder)( + context.Background(), authFailureEvent(t, "cluster-auth-failure")) + require.NoError(t, err) + require.Equal(t, StatusFailed, result.Status) + + assert.Contains(t, logs.String(), "http_status=403") + assert.Contains(t, logs.String(), "phase="+string(tt.phase)) + assert.NotContains(t, logs.String(), tt.responseBody) + + families, err := registry.Gather() + require.NoError(t, err) + assert.Equal(t, float64(1), getCounterValue(t, families, + "hyperfleet_adapter_api_auth_failures_total", "status_code", "403")) + }) + } +} + +func authFailurePostActionConfig() *configloader.Config { + return &configloader.Config{ + Adapter: configloader.AdapterInfo{Name: "test-adapter", Version: "v0.1.0"}, + Post: &configloader.PostConfig{PostActions: []configloader.PostAction{ + { + ActionBase: configloader.ActionBase{ + Name: "write-cluster-status", + APICall: &configloader.APICall{ + Method: http.MethodPut, + URL: "/clusters/{{ .clusterID }}/status", + Body: `{"status":"ready"}`, + }, + }, + }, + }}, + Params: []configloader.Parameter{ + {Name: "clusterID", Source: configloader.StringSource("event.id"), Required: true}, + }, + } +} + +func authFailureEvent(t *testing.T, clusterID string) *event.Event { + t.Helper() + evt := event.New() + evt.SetID("event-" + clusterID) + evt.SetType("com.hyperfleet.test") + evt.SetSource("qe") + payload, err := json.Marshal(map[string]interface{}{"id": clusterID}) + require.NoError(t, err) + require.NoError(t, evt.SetData(event.ApplicationJSON, payload)) + return &evt +} diff --git a/internal/executor/executor.go b/internal/executor/executor.go index 72a10c17..7e45b644 100644 --- a/internal/executor/executor.go +++ b/internal/executor/executor.go @@ -107,11 +107,14 @@ func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResu // Phase 1: Parameter Extraction slog.InfoContext(ctx, "phase running", "phase", result.CurrentPhase) - if paramErr := e.executeParamExtraction(execCtx); paramErr != nil { + paramErr := e.executeParamExtraction(execCtx) + result.APIAuthFailureStatusCodes = execCtx.APIAuthFailureStatusCodes + if paramErr != nil { result.Status = StatusFailed result.Errors[PhaseParamExtraction] = paramErr execCtx.SetError("ParameterExtractionFailed", paramErr.Error()) resErr := fmt.Errorf("parameter extraction failed: %w", paramErr) + logAPIAuthFailure(ctx, paramErr, "phase", PhaseParamExtraction) slog.ErrorContext(ctx, "phase failed", "phase", PhaseParamExtraction, "error", resErr) result.ExecutionContext = execCtx result.Params = execCtx.Params @@ -146,7 +149,11 @@ func (e *Executor) Execute(ctx context.Context, data interface{}) *ExecutionResu precondErr := fmt.Errorf("precondition evaluation failed: error=%w", precondOutcome.Error) result.Errors[result.CurrentPhase] = precondErr execCtx.SetError("PreconditionFailed", precondOutcome.Error.Error()) - slog.ErrorContext(ctx, "phase failed", "phase", result.CurrentPhase, "error", precondOutcome.Error) + logAPIAuthFailure(ctx, precondOutcome.Error, "phase", result.CurrentPhase) + slog.ErrorContext(ctx, "phase failed", + "phase", result.CurrentPhase, + "error", precondOutcome.Error, + ) result.ResourcesSkipped = true result.SkipReason = "PreconditionFailed" // Set skip metadata on adapter context without overwriting the failed execution status diff --git a/internal/executor/handler.go b/internal/executor/handler.go index 74df78af..ed4813b3 100644 --- a/internal/executor/handler.go +++ b/internal/executor/handler.go @@ -79,12 +79,18 @@ func recordMetrics(recorder *metrics.Recorder, result *ExecutionResult, duration if result == nil { return } + for _, statusCode := range result.APIAuthFailureStatusCodes { + recorder.RecordAPIAuthFailure(statusCode) + } switch { case result.Status == StatusFailed: recorder.RecordEventProcessed("failed") - for phase := range result.Errors { + for phase, err := range result.Errors { recorder.RecordError(string(phase)) + if statusCode, ok := apiAuthFailureStatusCode(err); ok { + recorder.RecordAPIAuthFailure(statusCode) + } } case result.ResourcesSkipped: recorder.RecordEventProcessed("skipped") diff --git a/internal/executor/param_extractor.go b/internal/executor/param_extractor.go index ea2f725e..12ddab58 100644 --- a/internal/executor/param_extractor.go +++ b/internal/executor/param_extractor.go @@ -31,6 +31,10 @@ func extractConfigParams( fmt.Sprintf("failed to extract required parameter '%s' from source '%s'", param.Name, param.Source.Describe()), err) } + if statusCode, ok := apiAuthFailureStatusCode(err); ok { + execCtx.APIAuthFailureStatusCodes = append(execCtx.APIAuthFailureStatusCodes, statusCode) + logAPIAuthFailure(ctx, err, "phase", PhaseParamExtraction, "param", param.Name) + } if param.Default != nil { execCtx.Params[param.Name] = param.Default } diff --git a/internal/executor/post_action_executor.go b/internal/executor/post_action_executor.go index 45ef0ffa..29966b17 100644 --- a/internal/executor/post_action_executor.go +++ b/internal/executor/post_action_executor.go @@ -69,7 +69,10 @@ func (pae *PostActionExecutor) ExecuteAll( results = append(results, result) if err != nil { - slog.ErrorContext(ctx, "post action processed: failed", "post_action", action.Name, "error", err) + slog.ErrorContext(ctx, "post action processed: failed", + "post_action", action.Name, + "error", err, + ) if execCtx.Adapter.ExecutionError == nil { execCtx.Adapter.ExecutionError = &ExecutionError{ @@ -352,6 +355,14 @@ func (pae *PostActionExecutor) executeAPICall( result.Status = StatusFailed result.Error = validationErr + attrs := []any{"post_action", result.Name} + if execCtx != nil { + if clusterID, ok := execCtx.EventData["id"].(string); ok { + attrs = append(attrs, "cluster_id", clusterID) + } + } + logAPIAuthFailure(ctx, validationErr, attrs...) + // Determine error context errorContext := "API call failed" if err == nil && resp != nil && !resp.IsSuccess() { diff --git a/internal/executor/precondition_executor.go b/internal/executor/precondition_executor.go index 089e7756..922063e8 100644 --- a/internal/executor/precondition_executor.go +++ b/internal/executor/precondition_executor.go @@ -40,7 +40,10 @@ func (pe *PreconditionExecutor) ExecuteAll( if err != nil { // Execution error (API call failed, parse error, etc.) - slog.ErrorContext(ctx, "precondition evaluated: failed", "precondition", precond.Name, "error", err) + slog.ErrorContext(ctx, "precondition evaluated: failed", + "precondition", precond.Name, + "error", err, + ) return &PreconditionsOutcome{ AllMatched: false, Results: results, diff --git a/internal/executor/types.go b/internal/executor/types.go index 0c3209a2..623bccb6 100644 --- a/internal/executor/types.go +++ b/internal/executor/types.go @@ -78,6 +78,9 @@ type Executor struct { // ExecutionResult contains the result of processing an event type ExecutionResult struct { + // APIAuthFailureStatusCodes contains authentication or authorization failures + // encountered by optional API-backed parameters that did not fail execution. + APIAuthFailureStatusCodes []int // ExecutionContext contains the full execution context (for testing and debugging) ExecutionContext *ExecutionContext // Params contains the extracted parameters @@ -169,6 +172,9 @@ type PostActionResult struct { // ExecutionContext holds runtime context during execution type ExecutionContext struct { + // APIAuthFailureStatusCodes tracks authentication or authorization failures + // from optional API-backed parameters. + APIAuthFailureStatusCodes []int // Ctx is the Go context Ctx context.Context // Config is the unified adapter configuration diff --git a/internal/executor/utils.go b/internal/executor/utils.go index 4914ae61..2293b41f 100644 --- a/internal/executor/utils.go +++ b/internal/executor/utils.go @@ -33,6 +33,32 @@ func ToConditionDefs(conditions []configloader.Condition) []criteria.ConditionDe return defs } +// apiAuthFailureStatusCode returns the status code for wrapped API authorization errors. +func apiAuthFailureStatusCode(err error) (int, bool) { + apiErr, ok := apierrors.IsAPIError(err) + if !ok { + return 0, false + } + + if !apiErr.IsUnauthorized() && !apiErr.IsForbidden() { + return 0, false + } + + return apiErr.StatusCode, true +} + +// logAPIAuthFailure records a structured log for an API authentication or +// authorization failure. Callers add context specific to their execution phase. +func logAPIAuthFailure(ctx context.Context, err error, attrs ...any) { + statusCode, ok := apiAuthFailureStatusCode(err) + if !ok { + return + } + + attrs = append([]any{"http_status", statusCode}, attrs...) + slog.ErrorContext(ctx, "hyperfleet api authentication or authorization failed", attrs...) +} + // ExecuteLogAction executes a log action with the given context // The message is rendered as a Go template with access to all params // This is a shared utility function used by both PreconditionExecutor and PostActionExecutor @@ -62,7 +88,8 @@ func ExecuteLogAction( // ExecuteAPICall executes an API call with the given configuration and returns the response and rendered URL // This is a shared utility function used by both PreconditionExecutor and PostActionExecutor -// On error, it returns an APIError with full context (method, URL, status, body, attempts, duration) +// On error, it returns an APIError with request metadata. Response bodies are inspected only +// to classify 404 errors and are not retained. // Returns: response, renderedURL, error func ExecuteAPICall( ctx context.Context, @@ -135,15 +162,11 @@ func ExecuteAPICall( } } resp, err = apiClient.Post(ctx, url, body, opts...) - // Log error message on failure for debugging purposes - if err != nil || (resp != nil && !resp.IsSuccess()) { - var logErr error - if err != nil { - logErr = err - } else { - logErr = fmt.Errorf("POST returned non-success status: %d", resp.StatusCode) - } - slog.ErrorContext(ctx, "post request failed", "error", logErr) + if err != nil { + slog.ErrorContext(ctx, "post request failed", "error", err) + } else if resp != nil && !resp.IsSuccess() { + slog.ErrorContext(ctx, "post request returned non-success status", + "status_code", resp.StatusCode, "status", resp.Status) } case http.MethodPut: body := []byte(apiCall.Body) @@ -154,15 +177,11 @@ func ExecuteAPICall( } } resp, err = apiClient.Put(ctx, url, body, opts...) - // Log error message on failure for debugging purposes - if err != nil || (resp != nil && !resp.IsSuccess()) { - var logErr error - if err != nil { - logErr = err - } else { - logErr = fmt.Errorf("PUT returned non-success status: %d", resp.StatusCode) - } - slog.ErrorContext(ctx, "put request failed", "error", logErr) + if err != nil { + slog.ErrorContext(ctx, "put request failed", "error", err) + } else if resp != nil && !resp.IsSuccess() { + slog.ErrorContext(ctx, "put request returned non-success status", + "status_code", resp.StatusCode, "status", resp.Status) } case http.MethodPatch: body := []byte(apiCall.Body) @@ -183,8 +202,12 @@ func ExecuteAPICall( // Return response AND error - response may contain useful details even on error // (e.g., HTTP status code, response body) if resp != nil { - slog.WarnContext(ctx, "api call failed", "status_code", resp.StatusCode, "status", resp.Status, "error", err) - // Wrap as APIError with full context + slog.WarnContext(ctx, "api call failed", + "status_code", resp.StatusCode, + "status", resp.Status, + "error", err, + ) + // Wrap as APIError with request and response metadata. apiErr := apierrors.NewAPIError( apiCall.Method, url, @@ -314,11 +337,7 @@ func ValidateAPIResponse(resp *hyperfleetapi.Response, err error, method, url st } if !resp.IsSuccess() { - errMsg := fmt.Sprintf("API returned non-success status: %d %s", resp.StatusCode, resp.Status) - if len(resp.Body) > 0 { - errMsg = fmt.Sprintf("%s, response body: %s", errMsg, string(resp.Body)) - } - baseErr := fmt.Errorf("%s", errMsg) + baseErr := fmt.Errorf("API returned non-success status: %d %s", resp.StatusCode, resp.Status) return apierrors.NewAPIError( method, url, diff --git a/internal/executor/utils_test.go b/internal/executor/utils_test.go index b98f0cfd..b474e0f9 100644 --- a/internal/executor/utils_test.go +++ b/internal/executor/utils_test.go @@ -94,7 +94,6 @@ func TestValidateAPIResponse_NonSuccessStatusCodes(t *testing.T) { body []byte statusCode int expectError bool - expectBody bool }{ { name: "400 Bad Request", @@ -102,7 +101,6 @@ func TestValidateAPIResponse_NonSuccessStatusCodes(t *testing.T) { status: "400 Bad Request", body: []byte(`{"error":"invalid input"}`), expectError: true, - expectBody: true, }, { name: "401 Unauthorized", @@ -110,7 +108,6 @@ func TestValidateAPIResponse_NonSuccessStatusCodes(t *testing.T) { status: "401 Unauthorized", body: []byte(`{"error":"invalid token"}`), expectError: true, - expectBody: true, }, { name: "403 Forbidden", @@ -118,7 +115,6 @@ func TestValidateAPIResponse_NonSuccessStatusCodes(t *testing.T) { status: "403 Forbidden", body: nil, expectError: true, - expectBody: false, }, { name: "404 Not Found", @@ -126,7 +122,6 @@ func TestValidateAPIResponse_NonSuccessStatusCodes(t *testing.T) { status: "404 Not Found", body: []byte(`{"message":"resource not found"}`), expectError: true, - expectBody: true, }, { name: "429 Too Many Requests", @@ -134,7 +129,6 @@ func TestValidateAPIResponse_NonSuccessStatusCodes(t *testing.T) { status: "429 Too Many Requests", body: []byte(`{"retry_after":60}`), expectError: true, - expectBody: true, }, { name: "500 Internal Server Error", @@ -142,7 +136,6 @@ func TestValidateAPIResponse_NonSuccessStatusCodes(t *testing.T) { status: "500 Internal Server Error", body: []byte(`{"error":"internal error"}`), expectError: true, - expectBody: true, }, { name: "502 Bad Gateway", @@ -150,7 +143,6 @@ func TestValidateAPIResponse_NonSuccessStatusCodes(t *testing.T) { status: "502 Bad Gateway", body: nil, expectError: true, - expectBody: false, }, { name: "503 Service Unavailable", @@ -158,7 +150,6 @@ func TestValidateAPIResponse_NonSuccessStatusCodes(t *testing.T) { status: "503 Service Unavailable", body: []byte("service temporarily unavailable"), expectError: true, - expectBody: true, }, { name: "504 Gateway Timeout", @@ -166,7 +157,6 @@ func TestValidateAPIResponse_NonSuccessStatusCodes(t *testing.T) { status: "504 Gateway Timeout", body: nil, expectError: true, - expectBody: false, }, } @@ -193,9 +183,8 @@ func TestValidateAPIResponse_NonSuccessStatusCodes(t *testing.T) { assert.Equal(t, "GET", apiErr.Method) assert.Equal(t, "http://example.com/api", apiErr.URL) - if tt.expectBody { - assert.Equal(t, tt.body, apiErr.ResponseBody) - assert.Contains(t, apiErr.Error(), string(tt.body)) + if len(tt.body) > 0 { + assert.NotContains(t, apiErr.Error(), string(tt.body)) } } else { assert.NoError(t, err) @@ -419,7 +408,7 @@ func TestValidateAPIResponse_APIErrorHelpers(t *testing.T) { }) } -func TestValidateAPIResponse_ResponseBodyString(t *testing.T) { +func TestValidateAPIResponse_DoesNotRetainResponseBody(t *testing.T) { resp := &hyperfleetapi.Response{ StatusCode: 500, Status: "500 Internal Server Error", @@ -430,8 +419,7 @@ func TestValidateAPIResponse_ResponseBodyString(t *testing.T) { apiErr, _ := apierrors.IsAPIError(err) - assert.True(t, apiErr.HasResponseBody()) - assert.Equal(t, `{"error":"database timeout","code":"DB_TIMEOUT"}`, apiErr.ResponseBodyString()) + assert.NotContains(t, apiErr.Error(), `{"error":"database timeout","code":"DB_TIMEOUT"}`) } // TestToConditionDefs tests the conversion of configloader conditions to criteria definitions diff --git a/internal/hyperfleetapi/client_test.go b/internal/hyperfleetapi/client_test.go index 9f072101..5a1333b5 100644 --- a/internal/hyperfleetapi/client_test.go +++ b/internal/hyperfleetapi/client_test.go @@ -547,10 +547,7 @@ func TestAPIError(t *testing.T) { t.Error("expected IsNotFound to return false for 503") } - // Test ResponseBodyString - bodyStr := err.ResponseBodyString() - assert.Contains(t, bodyStr, "backend is down", - "expected response body string to contain error message, got: %s", bodyStr) + assert.NotContains(t, err.Error(), "backend is down") } func TestAPIErrorStatusHelpers(t *testing.T) { @@ -660,8 +657,7 @@ func TestAPIErrorInRetryExhausted(t *testing.T) { if apiErr.Attempts != 2 { t.Errorf("expected 2 attempts, got %d", apiErr.Attempts) } - assert.Contains(t, apiErr.ResponseBodyString(), "backend overloaded", - "expected response body to contain error message, got: %s", apiErr.ResponseBodyString()) + assert.NotContains(t, apiErr.Error(), "backend overloaded") if !apiErr.IsServerError() { t.Error("expected IsServerError to return true") } diff --git a/pkg/errors/api_error.go b/pkg/errors/api_error.go index 3b539fd7..e1c72031 100644 --- a/pkg/errors/api_error.go +++ b/pkg/errors/api_error.go @@ -23,8 +23,9 @@ type APIError struct { URL string // Status is the HTTP status string (e.g., "503 Service Unavailable") Status string - // ResponseBody is the response body (may contain error details from the API) - ResponseBody []byte + // brokenEndpoint reports whether a 404 response came from the API's + // catch-all route rather than a missing resource. + brokenEndpoint bool // Duration is the total duration including retries Duration time.Duration // StatusCode is the HTTP status code (0 if request failed before getting response) @@ -43,16 +44,15 @@ type problemDetails struct { Code string `json:"code"` } -// parseProblemDetails attempts to parse the response body as RFC 9457 Problem Details. -func (e *APIError) parseProblemDetails() (problemDetails, bool) { - if !e.HasResponseBody() { - return problemDetails{}, false - } +// isBrokenEndpointResponse reports whether an RFC 9457 response identifies the +// API's catch-all route. The response body is inspected only while constructing +// an APIError and is never retained on the error. +func isBrokenEndpointResponse(body []byte) bool { var pd problemDetails - if err := json.Unmarshal(e.ResponseBody, &pd); err != nil { - return problemDetails{}, false + if err := json.Unmarshal(body, &pd); err != nil { + return false } - return pd, true + return pd.Code == brokenEndpointCode } // Error implements the error interface. @@ -99,17 +99,13 @@ func (e *APIError) IsNotFound() bool { // IsResourceNotFound returns true when the 404 represents a real resource that // was not found, as opposed to a broken/misconfigured URL. // It defaults to true for any 404 (safe fallback if proxies strip the response -// body), and only returns false when the RFC 9457 body contains the catch-all +// body), and only returns false when the RFC 9457 response contains the catch-all // error code HYPERFLEET-NTF-000, which signals no route matched the request URL. func (e *APIError) IsResourceNotFound() bool { if !e.IsNotFound() { return false } - pd, ok := e.parseProblemDetails() - if !ok { - return true - } - return pd.Code != brokenEndpointCode + return !e.brokenEndpoint } // IsUnauthorized returns true if the error was a 401 Unauthorized @@ -137,28 +133,12 @@ func (e *APIError) IsConflict() bool { return e.StatusCode == 409 } -// ----------------------------------------------------------------------------- -// Response Body Helpers -// ----------------------------------------------------------------------------- - -// ResponseBodyString returns the response body as a string -func (e *APIError) ResponseBodyString() string { - if e.ResponseBody == nil { - return "" - } - return string(e.ResponseBody) -} - -// HasResponseBody returns true if there is a response body -func (e *APIError) HasResponseBody() bool { - return len(e.ResponseBody) > 0 -} - // ----------------------------------------------------------------------------- // Constructor and Helper Functions // ----------------------------------------------------------------------------- -// NewAPIError creates a new APIError with all fields +// NewAPIError creates a new APIError. For 404 responses, body is inspected to +// preserve resource-not-found classification, but is not retained. func NewAPIError( method, url string, statusCode int, @@ -169,25 +149,19 @@ func NewAPIError( err error, ) *APIError { return &APIError{ - Method: method, - URL: url, - StatusCode: statusCode, - Status: status, - ResponseBody: body, - Attempts: attempts, - Duration: duration, - Err: err, + Method: method, + URL: url, + StatusCode: statusCode, + Status: status, + brokenEndpoint: statusCode == 404 && isBrokenEndpointResponse(body), + Attempts: attempts, + Duration: duration, + Err: err, } } // IsAPIError checks if an error is an APIError and returns it. // This function supports wrapped errors via errors.As. -// -// Example usage: -// -// if apiErr, ok := errors.IsAPIError(err); ok { -// log.Printf("API call failed: status=%d body=%s", apiErr.StatusCode, apiErr.ResponseBodyString()) -// } func IsAPIError(err error) (*APIError, bool) { var apiErr *APIError if errors.As(err, &apiErr) { diff --git a/pkg/metrics/recorder.go b/pkg/metrics/recorder.go index 8433657c..75925617 100644 --- a/pkg/metrics/recorder.go +++ b/pkg/metrics/recorder.go @@ -3,6 +3,8 @@ package metrics import ( + "net/http" + "strconv" "strings" "time" @@ -74,6 +76,7 @@ type Recorder struct { eventsProcessed *prometheus.CounterVec processingDuration prometheus.Observer errorsTotal *prometheus.CounterVec + apiAuthFailures *prometheus.CounterVec deletionTotal *prometheus.CounterVec deletionDuration *prometheus.HistogramVec deletionInProgress *prometheus.GaugeVec @@ -126,6 +129,19 @@ func NewRecorder(component, version, adapterName string, reg prometheus.Register []string{"error_type"}, ) + apiAuthFailures := prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "hyperfleet_adapter_api_auth_failures_total", + Help: "Total number of API authentication and authorization failures", + ConstLabels: prometheus.Labels{ + "component": component, + "version": version, + "adapter_name": adapterName, + }, + }, + []string{"status_code"}, + ) + deletionTotal := prometheus.NewCounterVec( prometheus.CounterOpts{ Name: "hyperfleet_adapter_resources_deleted_total", @@ -169,6 +185,7 @@ func NewRecorder(component, version, adapterName string, reg prometheus.Register reg.MustRegister(eventsProcessed) reg.MustRegister(processingDuration) reg.MustRegister(errorsTotal) + reg.MustRegister(apiAuthFailures) reg.MustRegister(deletionTotal) reg.MustRegister(deletionDuration) reg.MustRegister(deletionInProgress) @@ -177,6 +194,7 @@ func NewRecorder(component, version, adapterName string, reg prometheus.Register eventsProcessed: eventsProcessed, processingDuration: processingDuration, errorsTotal: errorsTotal, + apiAuthFailures: apiAuthFailures, deletionTotal: deletionTotal, deletionDuration: deletionDuration, deletionInProgress: deletionInProgress, @@ -210,6 +228,18 @@ func (r *Recorder) RecordError(errorType string) { r.errorsTotal.WithLabelValues(errorType).Inc() } +// RecordAPIAuthFailure increments the auth-failure counter for HTTP 401 or 403. +func (r *Recorder) RecordAPIAuthFailure(statusCode int) { + if r == nil { + return + } + + switch statusCode { + case http.StatusUnauthorized, http.StatusForbidden: + r.apiAuthFailures.WithLabelValues(strconv.Itoa(statusCode)).Inc() + } +} + // RecordDeletion increments the resources_deleted_total counter for the given resource type. // resourceType should be the Kubernetes kind (e.g., "Namespace", "ServiceAccount"). // Valid status values: DeletionStatusSuccess ("success"), DeletionStatusError ("error"). diff --git a/pkg/metrics/recorder_auth_failure_test.go b/pkg/metrics/recorder_auth_failure_test.go new file mode 100644 index 00000000..c4083d1a --- /dev/null +++ b/pkg/metrics/recorder_auth_failure_test.go @@ -0,0 +1,67 @@ +package metrics + +import ( + "testing" + + "github.com/prometheus/client_golang/prometheus" + dto "github.com/prometheus/client_model/go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRecorder_RecordAPIAuthFailure(t *testing.T) { + registry := prometheus.NewRegistry() + recorder := NewRecorder("auth-adapter", "v1.2.3", "test", registry) + + recorder.RecordAPIAuthFailure(401) + recorder.RecordAPIAuthFailure(403) + recorder.RecordAPIAuthFailure(401) + + families, err := registry.Gather() + require.NoError(t, err) + + family := authFailureMetricFamily(families) + require.NotNil(t, family, "auth failure metric family should exist") + + counts := make(map[string]float64) + for _, metric := range family.GetMetric() { + labels := metricLabels(metric) + assert.Equal(t, "auth-adapter", labels["component"]) + assert.Equal(t, "v1.2.3", labels["version"]) + counts[labels["status_code"]] = metric.GetCounter().GetValue() + } + + assert.Equal(t, float64(2), counts["401"], "401 failures must use their own bounded series") + assert.Equal(t, float64(1), counts["403"], "403 failures must use their own bounded series") + assert.Len(t, counts, 2, "only supported authentication status codes may create series") +} + +func TestRecorder_RecordAPIAuthFailure_InvalidStatusIsNoOp(t *testing.T) { + registry := prometheus.NewRegistry() + recorder := NewRecorder("test-adapter", "v0.1.0", "test", registry) + + recorder.RecordAPIAuthFailure(400) + recorder.RecordAPIAuthFailure(404) + recorder.RecordAPIAuthFailure(500) + + families, err := registry.Gather() + require.NoError(t, err) + assert.Nil(t, authFailureMetricFamily(families), "non-auth status codes must not create auth metric series") +} + +func authFailureMetricFamily(families []*dto.MetricFamily) *dto.MetricFamily { + for _, family := range families { + if family.GetName() == "hyperfleet_adapter_api_auth_failures_total" { + return family + } + } + return nil +} + +func metricLabels(metric *dto.Metric) map[string]string { + labels := make(map[string]string, len(metric.GetLabel())) + for _, label := range metric.GetLabel() { + labels[label.GetName()] = label.GetValue() + } + return labels +} diff --git a/pkg/metrics/recorder_test.go b/pkg/metrics/recorder_test.go index a3cfc326..a8ebaed6 100644 --- a/pkg/metrics/recorder_test.go +++ b/pkg/metrics/recorder_test.go @@ -320,6 +320,10 @@ func TestNilRecorderNoPanic(t *testing.T) { recorder.RecordError("test_error") }, "RecordError on nil recorder") + assert.NotPanics(t, func() { + recorder.RecordAPIAuthFailure(401) + }, "RecordAPIAuthFailure on nil recorder") + assert.NotPanics(t, func() { recorder.RecordDeletion("Namespace", DeletionStatusSuccess) }, "RecordDeletion on nil recorder") diff --git a/test/alerts/hyperfleet-adapter-alerts.yaml b/test/alerts/hyperfleet-adapter-alerts.yaml new file mode 100644 index 00000000..c6449567 --- /dev/null +++ b/test/alerts/hyperfleet-adapter-alerts.yaml @@ -0,0 +1,13 @@ +groups: + - name: hyperfleet-adapter + rules: + - alert: HyperFleetAdapterAPIAuthFailures + expr: | + sum by (component, version, adapter_name, status_code) ( + increase(hyperfleet_adapter_api_auth_failures_total[5m]) + ) >= 3 + labels: + severity: critical + annotations: + summary: "HyperFleet Adapter API authentication or authorization failures" + description: "The HyperFleet API returned HTTP {{ $labels.status_code }} to {{ $labels.adapter_name }}. Check its service-account token, API gateway configuration, and subject allowlist. When tenant enforcement is enabled, verify tenant dimensions are present and non-empty and tenant headers are propagated." diff --git a/test/alerts/hyperfleet-adapter-alerts_test.yaml b/test/alerts/hyperfleet-adapter-alerts_test.yaml new file mode 100644 index 00000000..fb4a9db4 --- /dev/null +++ b/test/alerts/hyperfleet-adapter-alerts_test.yaml @@ -0,0 +1,33 @@ +rule_files: + - hyperfleet-adapter-alerts.yaml + +evaluation_interval: 1m + +tests: + - interval: 1m + input_series: + - series: 'hyperfleet_adapter_api_auth_failures_total{component="hyperfleet-adapter",version="test",adapter_name="adapter-a",status_code="403"}' + values: '0 1 1 1 1 1 1 1 1 1 1 1' + alert_rule_test: + - eval_time: 11m + alertname: HyperFleetAdapterAPIAuthFailures + exp_alerts: [] + + - interval: 1m + input_series: + - series: 'hyperfleet_adapter_api_auth_failures_total{component="hyperfleet-adapter",version="test",adapter_name="adapter-a",status_code="403"}' + values: '0 1 2 3 3 3 3 3 3 3 3 3' + alert_rule_test: + - eval_time: 5m + alertname: HyperFleetAdapterAPIAuthFailures + exp_alerts: + - exp_labels: + alertname: HyperFleetAdapterAPIAuthFailures + severity: critical + component: hyperfleet-adapter + version: test + adapter_name: adapter-a + status_code: "403" + exp_annotations: + summary: "HyperFleet Adapter API authentication or authorization failures" + description: "The HyperFleet API returned HTTP 403 to adapter-a. Check its service-account token, API gateway configuration, and subject allowlist. When tenant enforcement is enabled, verify tenant dimensions are present and non-empty and tenant headers are propagated."