From 5b2dca106847eb3255015f71280e2925a576ca45 Mon Sep 17 00:00:00 2001 From: Salvatore Dario Minonne Date: Thu, 30 Apr 2026 17:05:09 +0200 Subject: [PATCH] fix(aws): use stable error message for endpoint service adoption failure The error returned from reconcileAWSEndpointServiceStatus when endpoint service adoption fails now includes the underlying cause from findExistingVpcEndpointService. The error is stable across reconcile loops because the adoption lookup only fails with deterministic messages (API error codes, static strings, or fixed LB ARNs) and never includes variable content like AWS request IDs. Add tests covering the endpoint service creation error paths: InvalidParameter with successful adoption, failed adoption, describe failure, and non-InvalidParameter API errors. OCPBUGS-83514 Co-Authored-By: Claude Opus 4.6 --- .../controllers/platform/aws/AGENTS.md | 20 +++ .../controllers/platform/aws/controller.go | 2 +- .../platform/aws/controller_test.go | 123 ++++++++++++++++++ 3 files changed, 144 insertions(+), 1 deletion(-) create mode 100644 hypershift-operator/controllers/platform/aws/AGENTS.md diff --git a/hypershift-operator/controllers/platform/aws/AGENTS.md b/hypershift-operator/controllers/platform/aws/AGENTS.md new file mode 100644 index 000000000000..e3ab7415d070 --- /dev/null +++ b/hypershift-operator/controllers/platform/aws/AGENTS.md @@ -0,0 +1,20 @@ +# AWS Endpoint Service Controller + +## Condition Message Stability + +Errors returned from `reconcileAWSEndpointServiceStatus` are set directly as condition messages on `AWSEndpointService` resources (see `AWSEndpointServiceAvailable` condition). If an error message contains variable output (e.g., AWS request IDs, timestamps), the condition will flip on every reconciliation loop, causing unnecessary status updates and API churn. + +Rules: +- **Never wrap raw AWS SDK errors with `%w` in return paths that feed condition messages.** Raw errors from the AWS SDK can contain request IDs and other per-call metadata that change on every invocation. +- **Use stable, deterministic error messages** for all returned errors. Include only fixed strings, error codes (`apiErr.ErrorCode()`), and deterministic identifiers (e.g., resource ARNs from input). +- **Log the full error for debugging, return a stable summary.** Use `log.Info(...)` with the full error before returning a sanitized message. This preserves debuggability without causing condition flapping. + +Example: +```go +// Good: log full error, return stable message +log.Info("adoption failed", "err", err) +return fmt.Errorf("endpoint service adoption failed (trigger: %s): failed to find existing endpoint service", apiErr.ErrorCode()) + +// Bad: wraps potentially variable error into condition message +return fmt.Errorf("endpoint service adoption failed: %w", err) +``` diff --git a/hypershift-operator/controllers/platform/aws/controller.go b/hypershift-operator/controllers/platform/aws/controller.go index a2aeb890b2c1..3197d8268a15 100644 --- a/hypershift-operator/controllers/platform/aws/controller.go +++ b/hypershift-operator/controllers/platform/aws/controller.go @@ -581,7 +581,7 @@ func (r *AWSEndpointServiceReconciler) reconcileAWSEndpointServiceStatus(ctx con serviceName, serviceID, err = findExistingVpcEndpointService(ctx, ec2Client, aws.ToString(lbARN)) if err != nil { log.Info("existing endpoint service not found, adoption failed", "err", err) - return errors.New(apiErr.ErrorCode()) + return fmt.Errorf("endpoint service adoption failed: %v", err) } } else { return errors.New(apiErr.ErrorCode()) diff --git a/hypershift-operator/controllers/platform/aws/controller_test.go b/hypershift-operator/controllers/platform/aws/controller_test.go index a3e6e12ad794..6cee55803109 100644 --- a/hypershift-operator/controllers/platform/aws/controller_test.go +++ b/hypershift-operator/controllers/platform/aws/controller_test.go @@ -20,6 +20,7 @@ import ( ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2" elbv2types "github.com/aws/aws-sdk-go-v2/service/elasticloadbalancingv2/types" + "github.com/aws/smithy-go" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -185,6 +186,128 @@ func TestReconcileAWSEndpointServiceStatus(t *testing.T) { } } +func TestReconcileAWSEndpointServiceStatusCreationErrors(t *testing.T) { + const ( + mockControlPlaneOperatorRoleArn = "arn:aws:12345678910::iam:role/fakeRoleARN" + testLBArn = "arn:aws:elasticloadbalancing:us-east-1:123456789:loadbalancer/net/test-lb/abc123" + ) + + activeLB := &elasticloadbalancingv2.DescribeLoadBalancersOutput{ + LoadBalancers: []elbv2types.LoadBalancer{{ + LoadBalancerArn: aws.String(testLBArn), + State: &elbv2types.LoadBalancerState{Code: elbv2types.LoadBalancerStateEnumActive}, + }}, + } + + hostedCluster := &hyperv1.HostedCluster{ + Spec: hyperv1.HostedClusterSpec{ + Platform: hyperv1.PlatformSpec{ + AWS: &hyperv1.AWSPlatformSpec{ + RolesRef: hyperv1.AWSRolesRef{ + ControlPlaneOperatorARN: mockControlPlaneOperatorRoleArn, + }, + }, + }, + }, + } + + tests := []struct { + name string + createErr error + adoptionDescribeOut *ec2.DescribeVpcEndpointServiceConfigurationsOutput + adoptionDescribeErr error + wantErrContains string + wantServiceName string + }{ + { + name: "When CreateVpcEndpointServiceConfiguration fails with InvalidParameter and adoption succeeds, it should use the adopted service", + createErr: &smithy.GenericAPIError{Code: "InvalidParameter", Message: "LBs are already associated with another VPC Endpoint Service"}, + adoptionDescribeOut: &ec2.DescribeVpcEndpointServiceConfigurationsOutput{ + ServiceConfigurations: []ec2types.ServiceConfiguration{{ + ServiceName: aws.String("com.amazonaws.vpce.adopted-service"), + ServiceId: aws.String("vpce-svc-adopted"), + NetworkLoadBalancerArns: []string{testLBArn}, + }}, + }, + wantServiceName: "com.amazonaws.vpce.adopted-service", + }, + { + name: "When CreateVpcEndpointServiceConfiguration fails with InvalidParameter and no matching service exists, it should return the adoption error", + createErr: &smithy.GenericAPIError{Code: "InvalidParameter", Message: "LBs are already associated with another VPC Endpoint Service"}, + adoptionDescribeOut: &ec2.DescribeVpcEndpointServiceConfigurationsOutput{ + ServiceConfigurations: []ec2types.ServiceConfiguration{}, + }, + wantErrContains: "endpoint service adoption failed", + }, + { + name: "When CreateVpcEndpointServiceConfiguration fails with InvalidParameter and DescribeVpcEndpointServiceConfigurations also fails, it should return the adoption error", + createErr: &smithy.GenericAPIError{Code: "InvalidParameter", Message: "LBs are already associated with another VPC Endpoint Service"}, + adoptionDescribeErr: fmt.Errorf("describe configurations unavailable"), + wantErrContains: "endpoint service adoption failed", + }, + { + name: "When CreateVpcEndpointServiceConfiguration fails with a non-InvalidParameter API error, it should return the API error code", + createErr: &smithy.GenericAPIError{Code: "AccessDenied", Message: "not authorized"}, + wantErrContains: "AccessDenied", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + mockCtrl := gomock.NewController(t) + + elbClient := awsapi.NewMockELBV2API(mockCtrl) + elbClient.EXPECT().DescribeLoadBalancers(gomock.Any(), gomock.Any()).Return(activeLB, nil) + + infra := &configv1.Infrastructure{ + ObjectMeta: metav1.ObjectMeta{Name: "cluster"}, + Status: configv1.InfrastructureStatus{InfrastructureName: "management-cluster-infra-id"}, + } + fakeClient := fake.NewClientBuilder().WithScheme(hyperapi.Scheme).WithObjects(infra).Build() + + mockEC2 := awsapi.NewMockEC2API(mockCtrl) + mockEC2.EXPECT().CreateVpcEndpointServiceConfiguration(gomock.Any(), gomock.Any()). + Return(nil, tt.createErr) + + // Set up adoption-related mocks only for InvalidParameter cases. + if tt.adoptionDescribeOut != nil || tt.adoptionDescribeErr != nil { + mockEC2.EXPECT().DescribeVpcEndpointServiceConfigurations(gomock.Any(), gomock.Any()). + Return(tt.adoptionDescribeOut, tt.adoptionDescribeErr) + } + + // When adoption succeeds, the function continues to reconcile permissions. + isAdoptionSuccess := tt.wantErrContains == "" && tt.adoptionDescribeOut != nil + if isAdoptionSuccess { + mockEC2.EXPECT().DescribeVpcEndpointServicePermissions(gomock.Any(), gomock.Any()). + Return(&ec2.DescribeVpcEndpointServicePermissionsOutput{}, nil) + mockEC2.EXPECT().ModifyVpcEndpointServicePermissions(gomock.Any(), gomock.Any()). + Return(&ec2.ModifyVpcEndpointServicePermissionsOutput{}, nil) + } + + r := AWSEndpointServiceReconciler{ + Client: fakeClient, + ManagementClusterCapabilities: &capabilities.MockCapabilityChecker{ + MockHas: func(caps ...capabilities.CapabilityType) bool { + return false + }, + }, + } + awsEPS := &hyperv1.AWSEndpointService{} + + err := r.reconcileAWSEndpointServiceStatus(t.Context(), awsEPS, hostedCluster, mockEC2, elbClient) + + if tt.wantErrContains != "" { + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring(tt.wantErrContains)) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(awsEPS.Status.EndpointServiceName).To(Equal(tt.wantServiceName)) + } + }) + } +} + func TestDeleteAWSEndpointService(t *testing.T) { existingConnectionsDeleteOut := &ec2.DeleteVpcEndpointServiceConfigurationsOutput{ Unsuccessful: []ec2types.UnsuccessfulItem{