Merge https://github.com/kubernetes/cloud-provider-aws:master (a6dc3e5) into main#136
Open
cloud-team-rebase-bot[bot] wants to merge 29 commits intoopenshift:mainfrom
Open
Merge https://github.com/kubernetes/cloud-provider-aws:master (a6dc3e5) into main#136cloud-team-rebase-bot[bot] wants to merge 29 commits intoopenshift:mainfrom
cloud-team-rebase-bot[bot] wants to merge 29 commits intoopenshift:mainfrom
Conversation
Deregister elb targets before registering targets
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis pull request updates the AWS cloud controller manager with a target reconciliation ordering fix, adds a defensive nil check for EC2 instance states with unit tests, bumps Helm chart and container image versions to v1.35.0, and extends Go module configuration with a k8s.io/streaming replacement directive. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
the
needs-ok-to-test
|
Hi @cloud-team-rebase-bot[bot]. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/providers/v1/aws_loadbalancer.go (1)
863-886:⚠️ Potential issue | 🟠 MajorDeregister-first flow can leave the target group under-provisioned on partial failure
At Line 863, targets are removed before any replacement is confirmed. If
RegisterTargetsfails at Line 882 (transient AWS/API error), the function exits with already-deregistered targets and no rollback, causing temporary capacity loss until next reconcile.Suggested mitigation (best-effort rollback on registration failure)
func (c *Cloud) ensureTargetGroupTargets(ctx context.Context, tgARN string, expectedTargets []*elbv2types.TargetDescription, actualTargets []*elbv2types.TargetDescription) error { targetsToRegister, targetsToDeregister := c.diffTargetGroupTargets(expectedTargets, actualTargets) // deregister targets prior to registering to allow instance replacements when the LB is at max instance capacity + var deregistered []elbv2types.TargetDescription if len(targetsToDeregister) > 0 { targetsToDeregisterChunks := c.chunkTargetDescriptions(targetsToDeregister, defaultDeregisterTargetsChunkSize) for _, targetsChunk := range targetsToDeregisterChunks { req := &elbv2.DeregisterTargetsInput{ TargetGroupArn: aws.String(tgARN), Targets: targetsChunk, } if _, err := c.elbv2.DeregisterTargets(ctx, req); err != nil { return fmt.Errorf("error trying to deregister targets in target group: %q", err) } + deregistered = append(deregistered, targetsChunk...) } } if len(targetsToRegister) > 0 { targetsToRegisterChunks := c.chunkTargetDescriptions(targetsToRegister, defaultRegisterTargetsChunkSize) for _, targetsChunk := range targetsToRegisterChunks { req := &elbv2.RegisterTargetsInput{ TargetGroupArn: aws.String(tgARN), Targets: targetsChunk, } if _, err := c.elbv2.RegisterTargets(ctx, req); err != nil { + // best-effort rollback to avoid leaving TG under-provisioned + if len(deregistered) > 0 { + for _, rbChunk := range c.chunkTargetDescriptions(deregistered, defaultRegisterTargetsChunkSize) { + if _, rbErr := c.elbv2.RegisterTargets(ctx, &elbv2.RegisterTargetsInput{ + TargetGroupArn: aws.String(tgARN), + Targets: rbChunk, + }); rbErr != nil { + klog.Warningf("failed to rollback deregistered targets for target group %q: %v", tgARN, rbErr) + break + } + } + } return fmt.Errorf("error trying to register targets in target group: %q", err) } } } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/providers/v1/aws_loadbalancer.go` around lines 863 - 886, The current deregister-first approach may leave the TG under-provisioned on partial RegisterTargets failures; change the flow in the block that uses targetsToDeregister/targetsToRegister so registration happens before deregistration OR implement a best-effort rollback: attempt RegisterTargetsChunks (using c.chunkTargetDescriptions, defaultRegisterTargetsChunkSize, c.elbv2.RegisterTargets and tgARN) first and only on success proceed to DeregisterTargetsChunks; if you must deregister first, capture the list of successfully deregistered targets and, if any c.elbv2.RegisterTargets call fails, re-register those previously-deregistered targets using c.elbv2.RegisterTargets in the same chunking pattern (chunkTargetDescriptions, defaultDeregisterTargetsChunkSize/defaultRegisterTargetsChunkSize) and return the original Register error if rollback attempts fail or succeed so the function does not leave the target group under-provisioned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/providers/v1/aws_loadbalancer.go`:
- Around line 863-886: The current deregister-first approach may leave the TG
under-provisioned on partial RegisterTargets failures; change the flow in the
block that uses targetsToDeregister/targetsToRegister so registration happens
before deregistration OR implement a best-effort rollback: attempt
RegisterTargetsChunks (using c.chunkTargetDescriptions,
defaultRegisterTargetsChunkSize, c.elbv2.RegisterTargets and tgARN) first and
only on success proceed to DeregisterTargetsChunks; if you must deregister
first, capture the list of successfully deregistered targets and, if any
c.elbv2.RegisterTargets call fails, re-register those previously-deregistered
targets using c.elbv2.RegisterTargets in the same chunking pattern
(chunkTargetDescriptions,
defaultDeregisterTargetsChunkSize/defaultRegisterTargetsChunkSize) and return
the original Register error if rollback attempts fail or succeed so the function
does not leave the target group under-provisioned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89adac97-ad02-44fd-a175-1ad23414c3e3
📒 Files selected for processing (2)
pkg/providers/v1/aws_loadbalancer.gopkg/providers/v1/aws_loadbalancer_test.go
The k8s.io/streaming module was recently extracted into the Kubernetes staging area. Without a replace directive, `go mod tidy` fails when building against the latest Kubernetes master because the staging version (v0.0.0) cannot be resolved from the module proxy.
…add-streaming-replace fix: add k8s.io/streaming replace directive in switch-to-latest-k8s
InstanceExistsByProviderID panics with a nil pointer dereference when DescribeInstances returns an instance with a nil State field. This can happen when an EC2 instance has been terminated and is no longer fully described by the API. The sibling method InstanceShutdownByProviderID already guards against this case. Fixes kubernetes#1365
…ists-by-provider-id fix: add nil check for instance State in InstanceExistsByProviderID
cloud-team-rebase-bot
bot
changed the title
cloud-team-bot
bot
force-pushed
the
rebase-bot-main
branch
from
0bb0e26 to
53e5b36
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/providers/v1/aws_loadbalancer_test.go (1)
1933-1938: Keep mock state unchanged when registration fails.Line 1934 mutates
NumTargetsbefore the capacity check. If the call errors, the mock still keeps the increment, which can skew later assertions/calls in the same test flow.Proposed test-mock fix
func (m *mockELBV2ClientForEnsureTargetGroupTargets) RegisterTargets(ctx context.Context, input *elbv2.RegisterTargetsInput, optFns ...func(*elbv2.Options)) (*elbv2.RegisterTargetsOutput, error) { - m.NumTargets += len(input.Targets) - - if m.NumTargets > m.MaxTargets { + next := m.NumTargets + len(input.Targets) + if next > m.MaxTargets { return nil, fmt.Errorf("TooManyTargets") } + m.NumTargets = next return m.MockedFakeELBV2.RegisterTargets(ctx, input, optFns...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/providers/v1/aws_loadbalancer_test.go` around lines 1933 - 1938, The mock RegisterTargets method mutates m.NumTargets before checking capacity, causing state to change even when it errors; change the logic in mockELBV2ClientForEnsureTargetGroupTargets.RegisterTargets to compute newCount := m.NumTargets + len(input.Targets) and perform the overflow check against m.MaxTargets first, returning the error without modifying m.NumTargets if newCount exceeds the limit, and only assign m.NumTargets = newCount when the check passes so failing calls leave mock state unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/providers/v1/aws_test.go`:
- Around line 3759-3827: The tests stub DescribeInstances on mockedEC2API but
never assert the mock was called, allowing false positives; after each call to
c.InstanceExistsByProviderID in TestInstanceExistsByProviderIDNilState,
TestInstanceExistsByProviderIDTerminated, and
TestInstanceExistsByProviderIDRunning, add an assertion on the mock (e.g., call
mockedEC2API.AssertExpectations(t) or AssertCalled/AssertNumberOfCalls for
DescribeInstances) to ensure the DescribeInstances expectation was actually
exercised.
---
Nitpick comments:
In `@pkg/providers/v1/aws_loadbalancer_test.go`:
- Around line 1933-1938: The mock RegisterTargets method mutates m.NumTargets
before checking capacity, causing state to change even when it errors; change
the logic in mockELBV2ClientForEnsureTargetGroupTargets.RegisterTargets to
compute newCount := m.NumTargets + len(input.Targets) and perform the overflow
check against m.MaxTargets first, returning the error without modifying
m.NumTargets if newCount exceeds the limit, and only assign m.NumTargets =
newCount when the check passes so failing calls leave mock state unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fbfe7030-dedd-4d13-8954-2d77a21d895f
📒 Files selected for processing (5)
hack/switch-to-latest-k8s.shpkg/providers/v1/aws.gopkg/providers/v1/aws_loadbalancer.gopkg/providers/v1/aws_loadbalancer_test.gopkg/providers/v1/aws_test.go
✅ Files skipped from review due to trivial changes (2)
- hack/switch-to-latest-k8s.sh
- pkg/providers/v1/aws.go
Comment on lines
+3759
to
+3827
| func TestInstanceExistsByProviderIDNilState(t *testing.T) { | ||
| mockedEC2API := newMockedEC2API() | ||
| c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})} | ||
|
|
||
| mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{ | ||
| Reservations: []ec2types.Reservation{ | ||
| { | ||
| Instances: []ec2types.Instance{ | ||
| { | ||
| InstanceId: aws.String("i-nil-state"), | ||
| State: nil, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, nil) | ||
|
|
||
| instanceExists, err := c.InstanceExistsByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/i-nil-state") | ||
| assert.Nil(t, err) | ||
| assert.False(t, instanceExists) | ||
| } | ||
|
|
||
| func TestInstanceExistsByProviderIDTerminated(t *testing.T) { | ||
| mockedEC2API := newMockedEC2API() | ||
| c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})} | ||
|
|
||
| mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{ | ||
| Reservations: []ec2types.Reservation{ | ||
| { | ||
| Instances: []ec2types.Instance{ | ||
| { | ||
| InstanceId: aws.String("i-terminated"), | ||
| State: &ec2types.InstanceState{ | ||
| Name: ec2types.InstanceStateNameTerminated, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, nil) | ||
|
|
||
| instanceExists, err := c.InstanceExistsByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/i-terminated") | ||
| assert.Nil(t, err) | ||
| assert.False(t, instanceExists) | ||
| } | ||
|
|
||
| func TestInstanceExistsByProviderIDRunning(t *testing.T) { | ||
| mockedEC2API := newMockedEC2API() | ||
| c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})} | ||
|
|
||
| mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{ | ||
| Reservations: []ec2types.Reservation{ | ||
| { | ||
| Instances: []ec2types.Instance{ | ||
| { | ||
| InstanceId: aws.String("i-running"), | ||
| State: &ec2types.InstanceState{ | ||
| Name: ec2types.InstanceStateNameRunning, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, nil) | ||
|
|
||
| instanceExists, err := c.InstanceExistsByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/i-running") | ||
| assert.Nil(t, err) | ||
| assert.True(t, instanceExists) | ||
| } |
There was a problem hiding this comment.
Assert EC2 mock expectations to prevent false-positive tests.
Line 3763, Line 3785, and Line 3809 stub DescribeInstances, but these tests never verify the mock was actually invoked. In the nil/terminated cases, this can still pass if provider-ID parsing regresses and returns false before reaching EC2.
Suggested patch
func TestInstanceExistsByProviderIDNilState(t *testing.T) {
mockedEC2API := newMockedEC2API()
+ t.Cleanup(func() { mockedEC2API.AssertExpectations(t) })
c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})}
@@
func TestInstanceExistsByProviderIDTerminated(t *testing.T) {
mockedEC2API := newMockedEC2API()
+ t.Cleanup(func() { mockedEC2API.AssertExpectations(t) })
c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})}
@@
func TestInstanceExistsByProviderIDRunning(t *testing.T) {
mockedEC2API := newMockedEC2API()
+ t.Cleanup(func() { mockedEC2API.AssertExpectations(t) })
c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})}As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestInstanceExistsByProviderIDNilState(t *testing.T) { | |
| mockedEC2API := newMockedEC2API() | |
| c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})} | |
| mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{ | |
| Reservations: []ec2types.Reservation{ | |
| { | |
| Instances: []ec2types.Instance{ | |
| { | |
| InstanceId: aws.String("i-nil-state"), | |
| State: nil, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, nil) | |
| instanceExists, err := c.InstanceExistsByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/i-nil-state") | |
| assert.Nil(t, err) | |
| assert.False(t, instanceExists) | |
| } | |
| func TestInstanceExistsByProviderIDTerminated(t *testing.T) { | |
| mockedEC2API := newMockedEC2API() | |
| c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})} | |
| mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{ | |
| Reservations: []ec2types.Reservation{ | |
| { | |
| Instances: []ec2types.Instance{ | |
| { | |
| InstanceId: aws.String("i-terminated"), | |
| State: &ec2types.InstanceState{ | |
| Name: ec2types.InstanceStateNameTerminated, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, nil) | |
| instanceExists, err := c.InstanceExistsByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/i-terminated") | |
| assert.Nil(t, err) | |
| assert.False(t, instanceExists) | |
| } | |
| func TestInstanceExistsByProviderIDRunning(t *testing.T) { | |
| mockedEC2API := newMockedEC2API() | |
| c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})} | |
| mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{ | |
| Reservations: []ec2types.Reservation{ | |
| { | |
| Instances: []ec2types.Instance{ | |
| { | |
| InstanceId: aws.String("i-running"), | |
| State: &ec2types.InstanceState{ | |
| Name: ec2types.InstanceStateNameRunning, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, nil) | |
| instanceExists, err := c.InstanceExistsByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/i-running") | |
| assert.Nil(t, err) | |
| assert.True(t, instanceExists) | |
| } | |
| func TestInstanceExistsByProviderIDNilState(t *testing.T) { | |
| mockedEC2API := newMockedEC2API() | |
| t.Cleanup(func() { mockedEC2API.AssertExpectations(t) }) | |
| c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})} | |
| mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{ | |
| Reservations: []ec2types.Reservation{ | |
| { | |
| Instances: []ec2types.Instance{ | |
| { | |
| InstanceId: aws.String("i-nil-state"), | |
| State: nil, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, nil) | |
| instanceExists, err := c.InstanceExistsByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/i-nil-state") | |
| assert.Nil(t, err) | |
| assert.False(t, instanceExists) | |
| } | |
| func TestInstanceExistsByProviderIDTerminated(t *testing.T) { | |
| mockedEC2API := newMockedEC2API() | |
| t.Cleanup(func() { mockedEC2API.AssertExpectations(t) }) | |
| c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})} | |
| mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{ | |
| Reservations: []ec2types.Reservation{ | |
| { | |
| Instances: []ec2types.Instance{ | |
| { | |
| InstanceId: aws.String("i-terminated"), | |
| State: &ec2types.InstanceState{ | |
| Name: ec2types.InstanceStateNameTerminated, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, nil) | |
| instanceExists, err := c.InstanceExistsByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/i-terminated") | |
| assert.Nil(t, err) | |
| assert.False(t, instanceExists) | |
| } | |
| func TestInstanceExistsByProviderIDRunning(t *testing.T) { | |
| mockedEC2API := newMockedEC2API() | |
| t.Cleanup(func() { mockedEC2API.AssertExpectations(t) }) | |
| c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})} | |
| mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{ | |
| Reservations: []ec2types.Reservation{ | |
| { | |
| Instances: []ec2types.Instance{ | |
| { | |
| InstanceId: aws.String("i-running"), | |
| State: &ec2types.InstanceState{ | |
| Name: ec2types.InstanceStateNameRunning, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, | |
| }, nil) | |
| instanceExists, err := c.InstanceExistsByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/i-running") | |
| assert.Nil(t, err) | |
| assert.True(t, instanceExists) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/providers/v1/aws_test.go` around lines 3759 - 3827, The tests stub
DescribeInstances on mockedEC2API but never assert the mock was called, allowing
false positives; after each call to c.InstanceExistsByProviderID in
TestInstanceExistsByProviderIDNilState,
TestInstanceExistsByProviderIDTerminated, and
TestInstanceExistsByProviderIDRunning, add an assertion on the mock (e.g., call
mockedEC2API.AssertExpectations(t) or AssertCalled/AssertNumberOfCalls for
DescribeInstances) to ensure the DescribeInstances expectation was actually
exercised.
update helm chart for 1.35
cloud-team-rebase-bot
bot
changed the title
cloud-team-bot
bot
force-pushed
the
rebase-bot-main
branch
from
53e5b36 to
d4851a2
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
pkg/providers/v1/aws_test.go (1)
3759-3827:⚠️ Potential issue | 🟠 MajorStrengthen these tests to prevent false positives.
At Line 3763, Line 3785, and Line 3809,
DescribeInstancesis mocked withmock.Anything, and these tests never assert expectations. That can pass even if the EC2 call is skipped or called with the wrong instance ID. Please assert expectations and matchInstanceIdsexplicitly.Suggested patch
func TestInstanceExistsByProviderIDNilState(t *testing.T) { mockedEC2API := newMockedEC2API() + t.Cleanup(func() { mockedEC2API.AssertExpectations(t) }) c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})} - mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{ + mockedEC2API.On("DescribeInstances", mock.MatchedBy(func(input *ec2.DescribeInstancesInput) bool { + return len(input.InstanceIds) == 1 && input.InstanceIds[0] == "i-nil-state" + })).Return(&ec2.DescribeInstancesOutput{ @@ func TestInstanceExistsByProviderIDTerminated(t *testing.T) { mockedEC2API := newMockedEC2API() + t.Cleanup(func() { mockedEC2API.AssertExpectations(t) }) c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})} - mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{ + mockedEC2API.On("DescribeInstances", mock.MatchedBy(func(input *ec2.DescribeInstancesInput) bool { + return len(input.InstanceIds) == 1 && input.InstanceIds[0] == "i-terminated" + })).Return(&ec2.DescribeInstancesOutput{ @@ func TestInstanceExistsByProviderIDRunning(t *testing.T) { mockedEC2API := newMockedEC2API() + t.Cleanup(func() { mockedEC2API.AssertExpectations(t) }) c := &Cloud{ec2: &awsSdkEC2{ec2: mockedEC2API}, describeInstanceBatcher: newdescribeInstanceBatcher(context.Background(), &awsSdkEC2{ec2: mockedEC2API})} - mockedEC2API.On("DescribeInstances", mock.Anything).Return(&ec2.DescribeInstancesOutput{ + mockedEC2API.On("DescribeInstances", mock.MatchedBy(func(input *ec2.DescribeInstancesInput) bool { + return len(input.InstanceIds) == 1 && input.InstanceIds[0] == "i-running" + })).Return(&ec2.DescribeInstancesOutput{As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/providers/v1/aws_test.go` around lines 3759 - 3827, The DescribeInstances mock uses mock.Anything which allows false positives; update each test (TestInstanceExistsByProviderIDNilState, TestInstanceExistsByProviderIDTerminated, TestInstanceExistsByProviderIDRunning) to expect DescribeInstances called with a request whose InstanceIds exactly match the target ID (match the ec2.DescribeInstancesInput.InstanceIds for "i-nil-state", "i-terminated", "i-running") instead of mock.Anything, and after calling c.InstanceExistsByProviderID invoke mockedEC2API.AssertExpectations(t) (or AssertCalled/AssertNumberOfCalls) to ensure the call actually happened; reference the mocked method DescribeInstances, the cloud method InstanceExistsByProviderID, and the mock factory newMockedEC2API to locate and modify the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/providers/v1/aws_test.go`:
- Around line 3759-3827: The DescribeInstances mock uses mock.Anything which
allows false positives; update each test
(TestInstanceExistsByProviderIDNilState,
TestInstanceExistsByProviderIDTerminated, TestInstanceExistsByProviderIDRunning)
to expect DescribeInstances called with a request whose InstanceIds exactly
match the target ID (match the ec2.DescribeInstancesInput.InstanceIds for
"i-nil-state", "i-terminated", "i-running") instead of mock.Anything, and after
calling c.InstanceExistsByProviderID invoke mockedEC2API.AssertExpectations(t)
(or AssertCalled/AssertNumberOfCalls) to ensure the call actually happened;
reference the mocked method DescribeInstances, the cloud method
InstanceExistsByProviderID, and the mock factory newMockedEC2API to locate and
modify the tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de37f92b-5d60-4cb3-80af-8fba72f760d5
📒 Files selected for processing (7)
charts/aws-cloud-controller-manager/Chart.yamlcharts/aws-cloud-controller-manager/values.yamlhack/switch-to-latest-k8s.shpkg/providers/v1/aws.gopkg/providers/v1/aws_loadbalancer.gopkg/providers/v1/aws_loadbalancer_test.gopkg/providers/v1/aws_test.go
✅ Files skipped from review due to trivial changes (4)
- hack/switch-to-latest-k8s.sh
- charts/aws-cloud-controller-manager/Chart.yaml
- charts/aws-cloud-controller-manager/values.yaml
- pkg/providers/v1/aws_loadbalancer_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/providers/v1/aws.go
Don't read LDFLAGS from env, in brew we set C specific flags that cause the build to break.
# Conflicts: # .github/workflows/helm_chart_release.yaml
Adds .spec file for building the ecr credential provider. We build this using brew, and tooling from ART. This means we don't need to worry about setting env vars (e.g OS_GIT_VERSION) and where Source0 is set.
…r image to be consistent with ART for 4.18 Reconciling with https://github.com/openshift/ocp-build-data/tree/827ab4ccce9cbbcf82c9dbaf6398b61d6cff8d7a/images/ose-aws-cloud-controller-manager.yml
…r image to be consistent with ART for 4.19 Reconciling with https://github.com/openshift/ocp-build-data/tree/2ea3e6158c93ca104b9d59fd58a71536fa01fb2d/images/ose-aws-cloud-controller-manager.yml
…r image to be consistent with ART for 4.20 Reconciling with https://github.com/openshift/ocp-build-data/tree/dfb5c7d531490cfdc61a3b88bc533702b9624997/images/ose-aws-cloud-controller-manager.yml
…r image to be consistent with ART for 4.21 Reconciling with https://github.com/openshift/ocp-build-data/tree/4fbe3fab45239dc4be6f5d9d98a0bf36e0274ec9/images/ose-aws-cloud-controller-manager.yml
…r image to be consistent with ART for 4.22 Reconciling with https://github.com/openshift/ocp-build-data/tree/087d1930e36b609f77d73bd8a313d85c940cff4d/images/ose-aws-cloud-controller-manager.yml
…r image to be consistent with ART for 4.22 Reconciling with https://github.com/openshift/ocp-build-data/tree/192ad5fa93a86aad43064b825aaa70d17c54913a/images/ose-aws-cloud-controller-manager.yml
- Create load balancers according to the Kubernetes Service API Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com> Assisted-by: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Changing the IP address type would invalidate the target group name, because you cannot have an IPv4 and IPv6 target for the same port/protocol set. Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
Signed-off-by: Nolan Brubaker <nolan@nbrubaker.com>
cloud-team-bot
bot
force-pushed
the
rebase-bot-main
branch
from
d4851a2 to
78a248f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
11 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.