OCPBUGS-83514: fix inconsistent error return in endpoint service adoption #8306

Nirshal · 2026-05-04T10:55:05Z

Reading the conversation, it makes sense to me to say that because we "sanitize" all errors before this point we are safe to use %v (although I found an exception to this at row 658), but the AGENTS.md indicates to always log the full error (and that is covered by row 583) and return the summarized version. This may trigger the AI to further change this row next time it is required to work on this file. If we think we want to keep it like this, should we find a way to flag the intention to keep an exception to the rule?

jparrill · 2026-04-23T06:57:25Z

It's possible to merge with TestReconcileAWSEndpointServiceStatus test function?

Hmmm, good question. Looking

No, it can't mocking setup is pretty different

-Original file line number
+Diff line change
@@ -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)
+    ```

-Original file line number
+Diff line change
@@ Expand Up @@
     					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())
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -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"
@@ Expand Down Expand Up @@
     	}
     }
+    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{
@@ Expand Down @@

Navigation Menu

Uh oh!

OCPBUGS-83514: fix inconsistent error return in endpoint service adoption #8306

Diff view

Diff view

There are no files selected for viewing

Nirshal May 4, 2026

Uh oh!

jparrill Apr 23, 2026

Uh oh!

sdminonne Apr 23, 2026

Uh oh!

sdminonne Apr 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

OCPBUGS-83514: fix inconsistent error return in endpoint service adoption #8306

OCPBUGS-83514: fix inconsistent error return in endpoint service adoption #8306

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

Nirshal May 4, 2026

Choose a reason for hiding this comment

Uh oh!

jparrill Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

sdminonne Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

sdminonne Apr 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!