OCPBUGS-83514: fix inconsistent error return in endpoint service adoption - #8306
OCPBUGS-83514: fix inconsistent error return in endpoint service adoption#8306sdminonne wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/valid-reference
|
@sdminonne: This pull request references Jira Issue OCPBUGS-83514, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWhen 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci-robot
added
jira/valid-bug
|
@sdminonne: This pull request references Jira Issue OCPBUGS-83514, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
area/hypershift-operator
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8306 +/- ##
==========================================
+ Coverage 36.42% 36.71% +0.29%
==========================================
Files 765 765
Lines 93302 93301 -1
==========================================
+ Hits 33981 34257 +276
+ Misses 56606 56361 -245
+ Partials 2715 2683 -32
... and 4 files with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@sdminonne: This pull request references Jira Issue OCPBUGS-83514, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hypershift-operator/controllers/platform/aws/controller_test.go (1)
261-264: Assert wrapped root-cause text, not only the wrapper prefix.On Line 263, the assertion checks only
"endpoint service adoption failed". Adding expectations for the inner cause (for example,"no endpoint services found"or"describe configurations unavailable") will better guarantee the bug fix intent (propagating the actual adoption failure reason).🔧 Suggested test hardening
tests := []struct { name string createErr error adoptionDescribeOut *ec2.DescribeVpcEndpointServiceConfigurationsOutput adoptionDescribeErr error wantErrContains string + wantErrCause string wantServiceName string }{ @@ { 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", + wantErrCause: "no endpoint services found", }, { name: "When CreateVpcEndpointServiceConfiguration fails with InvalidParameter and DescribeVpcEndpointServiceConfigurations also fails, it should return the adoption error wrapping the describe 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", + wantErrCause: "describe configurations unavailable", }, @@ if tt.wantErrContains != "" { g.Expect(err).To(HaveOccurred()) g.Expect(err.Error()).To(ContainSubstring(tt.wantErrContains)) + if tt.wantErrCause != "" { + g.Expect(err.Error()).To(ContainSubstring(tt.wantErrCause)) + } } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hypershift-operator/controllers/platform/aws/controller_test.go` around lines 261 - 264, The test currently only asserts the wrapper message ("endpoint service adoption failed"); update the assertion to also check the wrapped root-cause text from err (e.g., "no endpoint services found" or "describe configurations unavailable") so the test verifies propagation of the inner error; modify the block that checks tt.wantErrContains to either include the expected inner substring in tt.wantErrContains for each case or add an additional assertion like g.Expect(err.Error()).To(ContainSubstring(<expectedInnerCause>)) referencing the local err and the table-driven test case values to locate and validate the inner cause alongside the wrapper.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hypershift-operator/controllers/platform/aws/controller_test.go`:
- Around line 261-264: The test currently only asserts the wrapper message
("endpoint service adoption failed"); update the assertion to also check the
wrapped root-cause text from err (e.g., "no endpoint services found" or
"describe configurations unavailable") so the test verifies propagation of the
inner error; modify the block that checks tt.wantErrContains to either include
the expected inner substring in tt.wantErrContains for each case or add an
additional assertion like
g.Expect(err.Error()).To(ContainSubstring(<expectedInnerCause>)) referencing the
local err and the table-driven test case values to locate and validate the inner
cause alongside the wrapper.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6f58726d-42dc-41f8-bba4-916dae1378fe
📒 Files selected for processing (1)
hypershift-operator/controllers/platform/aws/controller_test.go
f46c3a6 to
6e7fbfe
Compare
mehabhalodiya
left a comment
mehabhalodiya
left a comment
There was a problem hiding this comment.
Overall:
✅ The core error fix is correct and well-targeted. The test suite is solid. One thing to follow up on before approving:
- Confirm the
AccessDeniedtest case doesn't accidentally expect a describe mock call that won't be set up.
jparrill
left a comment
jparrill
left a comment
There was a problem hiding this comment.
/approve
Dropped a comment, otherwise lgtm
| } | ||
| } | ||
|
|
||
| func TestReconcileAWSEndpointServiceStatusCreationErrors(t *testing.T) { |
There was a problem hiding this comment.
It's possible to merge with TestReconcileAWSEndpointServiceStatus test function?
There was a problem hiding this comment.
Hmmm, good question. Looking
There was a problem hiding this comment.
No, it can't mocking setup is pretty different
openshift-ci
Bot
added
the
approved
|
@mehabhalodiya thanks! PTAL |
mehabhalodiya
left a comment
mehabhalodiya
left a comment
There was a problem hiding this comment.
/lgtm
/approve
Thank you!
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jparrill, mehabhalodiya, sdminonne The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
Scheduling tests matching the |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
687d7f4 to
029cf34
Compare
|
New changes are detected. LGTM label has been removed. |
|
/test e2e-aws-4-22 |
|
/test e2e-azure-self-managed |
|
lgtm, will defer to others to tag in case they have more feedback |
| 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 (trigger: %s): failed to find existing endpoint service", apiErr.ErrorCode()) |
There was a problem hiding this comment.
shouldn't err be included here instead of apiErr.ErrorCode()?
There was a problem hiding this comment.
Good point double checking
There was a problem hiding this comment.
Thanks for this @muraee ! I looked into it and AFAIK the err comes from serviceName, serviceID, err = findExistingVpcEndpointService(ctx, ec2Client, aws.ToString(lbARN)) and that err may contain the err ID from AWs and this is may trigger re-conciliation since the err is bubble-up in condition.
It's true that a static message doesn't add a lot of info but resync should be avoided.
I don't have a strong opinion but I would keep as is
Thoughts?
There was a problem hiding this comment.
findExistingVpcEndpointService returns parsses the aws error and returns only the ErrorCode. It doesn't include any err ID
There was a problem hiding this comment.
Right, taking back and amending the code. TY!
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 <noreply@anthropic.com>
029cf34 to
5b2dca1
Compare
|
@sdminonne: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
Nirshal
left a comment
Nirshal
left a comment
There was a problem hiding this comment.
It seems fine to me, I just have a couple of doubts that are probably coming from my lack of experience, if you would be so nice to explain them to me, I would gladly approve the PR.
| if errors.As(err, &apiErr) { | ||
| return "", "", errors.New(apiErr.ErrorCode()) | ||
| } | ||
| return "", "", err |
There was a problem hiding this comment.
I have a doubt here:
- this row returns the error in its raw form, without "sanitization". Failing the describe could still happens for non-api related issues like network error, right? Do those kind of errors can have a variable part that can again induce flapping?
- if we apply the AGENTS file suggestion here, we log and then we return a %T of the error, but then at row 583 we log again the "sanitized" version, and that is misleading (at first glance they may seem 2 different problems).
- If that's not the case, and we want to keep the code as it is now, we should care to have a way to indicate to the agents that in this particular case we do not want to follow the rule indicated by AGENTS.md, otherwise they are going to flag this as an inconsistency and try to correct this.
There was a problem hiding this comment.
My understanding is that only AWS errs need to be sanitized. AWS errs contains a variable ID. Same err but different ID. If we propagate it to the condition we may have an unwanted re-sync.
@muraee please confirm or educate me in case I'm wrong.
| 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) |
There was a problem hiding this comment.
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?
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
|
Stale PRs are closed after 21d of inactivity. If this PR is still relevant, comment to refresh it or remove the stale label. If this PR is safe to close now please do so with /lifecycle stale |
openshift-ci
Bot
added
lifecycle/stale
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Stale PRs rot after 14d of inactivity. Mark the PR as fresh by commenting If this PR is safe to close now please do so with /lifecycle rotten |
openshift-ci
Bot
added
lifecycle/rotten
|
The background task completed — it was the pending Prow jobs query that I had already incorporated into the analysis above. The report is complete with all findings included. To summarize: this is not a test failure — it's a merge conflict. The |
|
Rotten PRs close after 7d of inactivity. Reopen the PR by commenting /close |
|
@openshift-ci[bot]: Closed this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@sdminonne: This pull request references Jira Issue OCPBUGS-83514. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
8 participants
Summary
reconcileAWSEndpointServiceStatus, the code was returning the outerCreateVpcEndpointServiceConfigurationerror code ("InvalidParameter") instead of the actual error fromfindExistingVpcEndpointService. This made debugging harder since the logged error and returned error were inconsistent.Additional context
This PR addresses item 2 from OCPBUGS-83514. Item 1 (the
//go:generatedirective insupport/awsapi/iam.go) was investigated and confirmed to be intentionally placed by thedelegatingclientgeneratortemplate — no code change needed.Fixes: https://redhat.atlassian.net/browse/OCPBUGS-83514
Ref: PR #7871 review comment
Test plan
make buildpassesmake verifypasses🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests