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{