UPSTREAM-SYNC: Fix merge https://github.com/kubernetes/cloud-provider-aws:master (c34d66e) into main#158
Merged
openshift-merge-bot[bot] merged 64 commits intoJun 16, 2026
Conversation
Deregister elb targets before registering targets
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
update helm chart for 1.35
Configure WithHTTPClient on all LoadDefaultConfig calls to prevent clock skew overcorrection from slow API responses (COE-389792). Add AST test to ensure future SDK clients include timeout.
Replace http.Client with awshttp.NewBuildableClient().WithTimeout() to preserve SDK default transport settings.
…-timeout fix: Add explicit HTTP request timeouts to all AWS SDK clients
Pin all GitHub Actions to full-length commit SHAs to comply with Kubernetes organization security policy that requires all actions must be pinned to prevent supply chain attacks. This change addresses the CI failures: "The actions actions/checkout@v4 and actions/dependency-review-action@v4 are not allowed in kubernetes/cloud-provider-aws because all actions must be pinned to a full-length commit SHA." Actions pinned with release verification: - actions/checkout@v4 → @34e114876b0b11c390a56381ad16ebd13914f8d5 # v4.3.1 ``` Release: https://github.com/actions/checkout/releases/tag/v4.3.1 Commit: actions/checkout@34e1148 ``` - actions/dependency-review-action@v4 → @2031cfc080254a8a887f58cffee85186f0e49e48 # v4.9.0 ``` Release: https://github.com/actions/dependency-review-action/releases/tag/v4.9.0 Commit: actions/dependency-review-action@2031cfc ``` - golang/govulncheck-action@v1 → @31f7c5463448f83528bd771c2d978d940080c9fd # master (post-v1.0.4) ``` Commit: golang/govulncheck-action@31f7c54 Note: Using master HEAD instead of v1.0.4 because v1.0.4 contains unpinned transitive dependencies (actions/checkout@v4.1.1, actions/setup-go@v5.0.0). The master branch includes a fix from Feb 2026 that pins these dependencies. See: golang/govulncheck-action@31f7c54 ``` - helm/chart-releaser-action@v1.7.0 → @a0d2dc62c5e491af8ef6ba64a2e02bcf3fb33aa1 # v1.7.0 ``` Release: https://github.com/helm/chart-releaser-action/releases/tag/v1.7.0 Commit: helm/chart-releaser-action@a0d2dc6 ``` - actions/github-script@v7 → @f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 ``` Release: https://github.com/actions/github-script/releases/tag/v7.1.0 Commit: actions/github-script@f28e40c ``` Files modified: - .github/workflows/deps.yml - .github/workflows/tag.yml - .github/workflows/helm_chart_release.yaml - .github/workflows/kpromo-reminder.yaml Justification: Pinning actions to commit SHAs instead of mutable tags (v4, v1.7.0, etc.) prevents potential security vulnerabilities where a tag could be moved to point to malicious code. This is a required security practice in the Kubernetes organization to ensure supply chain integrity and is enforced by GitHub Actions policy for kubernetes/* repositories. GitHub enforces that not only direct action dependencies must be pinned, but also transitive dependencies (actions used within composite actions). This is why govulncheck-action required using the master branch commit instead of the latest release tag. Each SHA has been verified against the official release tags to ensure we're using the intended versions while meeting security requirements. Reviewed-by: Claude Sonnet 4.5 <noreply@anthropic.com>
fix(ci): pin GitHub Actions to full-length commit SHAs
This commit adds surgical fixes to improve DNS resilience in hairpin tests, particularly for AWS EUSC regions where DNS propagation can be slower than in public regions. Changes: 1. Add AWS SDK retry configuration with 10 attempts and 30s max backoff to handle transient DNS failures when calling DescribeLoadBalancers 2. Wrap AWS API calls with wait.PollUntilContextTimeout (25min timeout, 5s interval) to retry DNS failures for elasticloadbalancing endpoint 3. Increase curl retry parameters to not fall into 15 DNS cache as well keep resilience on EUSC region. These changes address the following failures observed in EUSC: - NLB hairpin: DNS lookup failure for elasticloadbalancing.eusc-de-east-1.amazonaws.com taking 10+ minutes before timeout in hookPreTest - CLB hairpin: curl exit code 6 (DNS resolution failure) for LB endpoint
fix(e2e): increase test timeout and load balancer propagation to cover EUSC region
Add AWS API metrics middleware for status codes and error tracking
1.36.0 rc release
Fix the managed (controller-owned) security group leak when user provided security group (SG) annotation(1) is added to an existing Service type-loadBalancer Classic Load Balancer (CLB). Previously, the controller was leak a managed security group resource when the annotation is added to existing Service loadBalancer CLB (default mode). This change detects the changes correctly, trigger the SG removal and it's dependencies - other SGs referencing the managed security group that will be removed. Unit tests functions added to validate Service update to BYO Security Group annotations from a managed SG state on CLB. Issue kubernetes#1208
1.36 dependency update
Fix leak managed/owned security group on Service update with BYO SG on CLB
The cloud-provider-aws-push-images postsubmit has been failing across all branches (master and release-*) since early April 2026 because cloudbuild.yaml pins gcr.io/k8s-staging-test-infra/gcb-docker-gcloud:v20221214-1b4dd4d69a, and that tag has been garbage-collected out of the staging registry. Cloud Build retries the pull 10 times and fails with 'manifest unknown: Failed to fetch "v20221214-1b4dd4d69a"'. Example failed run (v1.36.0 tag): https://prow.k8s.io/view/gs/kubernetes-ci-logs/logs/cloud-provider-aws-push-images/2051449456091467776 Bumping to v20260205-38cfa9523f (digest sha256:ff388e0dc16351e96f8464e2e185b74a7578a5ccb7a112cf3393468e59e6e2d2), currently the newest tag in gcr.io/k8s-staging-test-infra/gcb-docker-gcloud and aliased to 'latest'. This image still provides /buildx-entrypoint used by the build step. Signed-off-by: Ganesh Putta <ganiredi@amazon.com>
…d-image cloudbuild: bump gcb-docker-gcloud to v20260205-38cfa9523f
Adds support for Bring Your Own (BYO) Security Groups (SGs) for AWS Network Load Balancers (NLBs). The feature is only enabled when NLBSecurityGroupMode=Managed in the AWS CCM cloud config. The feature allows associating BYO groups to NLBs via a service annotation. These groups replace the default managed security groups otherwise created by the AWS CCM, and the user is responsible for configuring the security group rules as needed for the functioning of the service.
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>
…r image to be consistent with ART for 5.0 Reconciling with https://github.com/openshift-eng/ocp-build-data/tree/7691ed4dc0b6585b358f9e73fb736ace9a48a286/images/ose-aws-cloud-controller-manager.yml
mfbonfigli
force-pushed
the
rebase-bot-main
branch
from
c7b8d44 to
cd5ffc6
Compare
Author
|
@RadekManak Thanks! Should be fixed now if I did it correctly. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile.openshift (1)
1-16:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Container runs as root — violate security guidelines.
No
USERdirective is specified in either the builder or runtime stage. The container will executeaws-cloud-controller-manageras root, which violates the coding guidelines requirement that containers must run as non-root users. As per coding guidelines, "USER non-root; never run as root" is a mandatory container security requirement.🔒 Proposed fix to add non-root USER
FROM registry.ci.openshift.org/ocp/5.0:base-rhel9 LABEL description="AWS Cloud Controller Manager" +USER 1001 + COPY --from=builder /build/aws-cloud-controller-manager /bin/aws-cloud-controller-manager ENTRYPOINT [ "/bin/aws-cloud-controller-manager" ]Note: UID 1001 is a common convention for non-root users in OpenShift/Red Hat base images. Verify this UID is appropriate for the
ocp/5.0:base-rhel9base image or adjust accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile.openshift` around lines 1 - 16, The Dockerfile currently leaves both stages running as root (builder and final) so the aws-cloud-controller-manager will run as root; add a non-root user and switch to it in the final stage (and optionally in the builder stage) by creating a user/group with a non-root UID (e.g. 1001), chowning the installed binary (/bin/aws-cloud-controller-manager) to that user, and adding a USER directive before ENTRYPOINT so the process runs non-root; reference the final-stage commands (COPY --from=builder /build/aws-cloud-controller-manager /bin/aws-cloud-controller-manager and ENTRYPOINT [ "/bin/aws-cloud-controller-manager" ]) and ensure ownership/permissions are adjusted to allow execution by the chosen non-root user.Sources: Coding guidelines, Linters/SAST tools
♻️ Duplicate comments (1)
pkg/providers/v1/aws_validations_test.go (1)
333-338:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe "empty value" test case still uses a non-empty error string.
Line 336 sets
ServiceAnnotationLoadBalancerExtraSecurityGroupsto the full error message instead of an empty string"". This duplicates the "annotation present" validation path rather than testing the empty-value branch.🐛 Proposed fix
{ name: "NLB with BYO extra SG with empty value - error (not supported)", annotations: map[string]string{ ServiceAnnotationLoadBalancerType: "nlb", - ServiceAnnotationLoadBalancerExtraSecurityGroups: "BYO extra security group annotation \"service.beta.kubernetes.io/aws-load-balancer-extra-security-groups\" is not supported by NLB", + ServiceAnnotationLoadBalancerExtraSecurityGroups: "", }, expectedError: "BYO extra security group annotation \"service.beta.kubernetes.io/aws-load-balancer-extra-security-groups\" is not supported by NLB", },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/providers/v1/aws_validations_test.go` around lines 333 - 338, The test case "NLB with BYO extra SG with empty value - error (not supported)" incorrectly sets ServiceAnnotationLoadBalancerExtraSecurityGroups to the full error string instead of an empty string; update that annotation value to "" and keep expectedError as the full message so the test exercises the empty-value validation branch (look for the test case name and the symbols ServiceAnnotationLoadBalancerType, ServiceAnnotationLoadBalancerExtraSecurityGroups, and expectedError in the test table).
🧹 Nitpick comments (1)
pkg/providers/v1/aws_validations_test.go (1)
745-746: 💤 Low valueSwap
assert.Equalargument order and remove redundantassert.Contains.Line 745 passes
(actual, expected)instead of the conventional(expected, actual), which will produce confusing failure messages. Line 746 is redundant—iferr.Error()equalstt.expectedError, it always contains it.♻️ Suggested cleanup
} else { assert.Error(t, err, "Expected error for test case: %s", tt.name) - assert.Equal(t, err.Error(), tt.expectedError, "Expected error for test case: %s", tt.name) - assert.Contains(t, err.Error(), tt.expectedError, "Error message should contain expected text for test case: %s", tt.name) + assert.Equal(t, tt.expectedError, err.Error(), "Expected error for test case: %s", tt.name) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/providers/v1/aws_validations_test.go` around lines 745 - 746, Swap the arguments to the test assertion so assert.Equal receives (tt.expectedError, err.Error()) instead of (err.Error(), tt.expectedError) to follow the conventional (expected, actual) order, and remove the redundant assert.Contains assertion that checks err.Error() against tt.expectedError (since equality already guarantees containment); update the two assertions in pkg/providers/v1/aws_validations_test.go that reference err.Error() and tt.expectedError accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/providers/v1/aws_validations_test.go`:
- Around line 699-721: Remove the duplicate test entries and correct the policy
values and test names so both PreferDualStack and RequireDualStack paths are
covered: keep a single case named "PreferDualStack with two entries works" with
ipFamilyPolicy: v1.IPFamilyPolicyPreferDualStack and ipFamilies:
[]v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, and change the two
"RequireDualStack" cases to a single case named "RequireDualStack with two
entries works" with ipFamilyPolicy: v1.IPFamilyPolicyRequireDualStack (and the
same ipFamilies/expectedError); ensure the test slice contains one unique entry
per scenario and update any duplicated entries referencing ipFamilyPolicy,
ipFamilies, and expectedError accordingly.
---
Outside diff comments:
In `@Dockerfile.openshift`:
- Around line 1-16: The Dockerfile currently leaves both stages running as root
(builder and final) so the aws-cloud-controller-manager will run as root; add a
non-root user and switch to it in the final stage (and optionally in the builder
stage) by creating a user/group with a non-root UID (e.g. 1001), chowning the
installed binary (/bin/aws-cloud-controller-manager) to that user, and adding a
USER directive before ENTRYPOINT so the process runs non-root; reference the
final-stage commands (COPY --from=builder /build/aws-cloud-controller-manager
/bin/aws-cloud-controller-manager and ENTRYPOINT [
"/bin/aws-cloud-controller-manager" ]) and ensure ownership/permissions are
adjusted to allow execution by the chosen non-root user.
---
Duplicate comments:
In `@pkg/providers/v1/aws_validations_test.go`:
- Around line 333-338: The test case "NLB with BYO extra SG with empty value -
error (not supported)" incorrectly sets
ServiceAnnotationLoadBalancerExtraSecurityGroups to the full error string
instead of an empty string; update that annotation value to "" and keep
expectedError as the full message so the test exercises the empty-value
validation branch (look for the test case name and the symbols
ServiceAnnotationLoadBalancerType,
ServiceAnnotationLoadBalancerExtraSecurityGroups, and expectedError in the test
table).
---
Nitpick comments:
In `@pkg/providers/v1/aws_validations_test.go`:
- Around line 745-746: Swap the arguments to the test assertion so assert.Equal
receives (tt.expectedError, err.Error()) instead of (err.Error(),
tt.expectedError) to follow the conventional (expected, actual) order, and
remove the redundant assert.Contains assertion that checks err.Error() against
tt.expectedError (since equality already guarantees containment); update the two
assertions in pkg/providers/v1/aws_validations_test.go that reference
err.Error() and tt.expectedError accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4df2d62d-f910-4157-9559-2fd6ea3cc8f2
⛔ Files ignored due to path filters (44)
go.sumis excluded by!**/*.sumtests/e2e/go.sumis excluded by!**/*.sumtests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/config.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/credentials.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/go_module_metadata.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/middleware/user_agent.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/protocol/ec2query/error_utils.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/protocol/query/middleware.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/retry/middleware.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/signer/v4/stream.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/transport/http/client.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/transport/http/timeout_read_closer.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/configsources/CHANGELOG.mdis excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/configsources/config.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/configsources/endpoints.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/configsources/go_module_metadata.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/awsrulesfn/partitions.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/awsrulesfn/partitions.jsonis excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/v2/CHANGELOG.mdis excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/v2/endpoints.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/v2/go_module_metadata.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/middleware/middleware.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/CHANGELOG.mdis excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/LICENSE.txtis excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_client.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptAddressTransfer.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptCapacityReservationBillingOwnership.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptReservedInstancesExchangeQuote.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptTransitGatewayClientVpnAttachment.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptTransitGatewayMulticastDomainAssociations.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptTransitGatewayPeeringAttachment.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptTransitGatewayVpcAttachment.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptVpcEndpointConnections.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptVpcPeeringConnection.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AdvertiseByoipCidr.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AllocateAddress.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AllocateHosts.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AllocateIpamPoolCidr.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_ApplySecurityGroupsToClientVpnTargetNetwork.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AssignIpv6Addresses.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AssignPrivateIpAddresses.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AssignPrivateNatGatewayAddress.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AssociateAddress.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AssociateCapacityReservationBillingOwner.gois excluded by!**/vendor/**
📒 Files selected for processing (11)
.ci-operator.yamlDockerfile.openshiftdocs/service_controller.mdpkg/providers/v1/aws.gopkg/providers/v1/aws_fakes.gopkg/providers/v1/aws_loadbalancer.gopkg/providers/v1/aws_loadbalancer_test.gopkg/providers/v1/aws_test.gopkg/providers/v1/aws_validations.gopkg/providers/v1/aws_validations_test.gotests/e2e/go.mod
✅ Files skipped from review due to trivial changes (1)
- .ci-operator.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/go.mod
- pkg/providers/v1/aws_validations.go
- pkg/providers/v1/aws_loadbalancer_test.go
- pkg/providers/v1/aws_loadbalancer.go
- pkg/providers/v1/aws.go
- pkg/providers/v1/aws_test.go
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Dockerfile.openshift (1)
1-16:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Container runs as root — violate security guidelines.
No
USERdirective is specified in either the builder or runtime stage. The container will executeaws-cloud-controller-manageras root, which violates the coding guidelines requirement that containers must run as non-root users. As per coding guidelines, "USER non-root; never run as root" is a mandatory container security requirement.🔒 Proposed fix to add non-root USER
FROM registry.ci.openshift.org/ocp/5.0:base-rhel9 LABEL description="AWS Cloud Controller Manager" +USER 1001 + COPY --from=builder /build/aws-cloud-controller-manager /bin/aws-cloud-controller-manager ENTRYPOINT [ "/bin/aws-cloud-controller-manager" ]Note: UID 1001 is a common convention for non-root users in OpenShift/Red Hat base images. Verify this UID is appropriate for the
ocp/5.0:base-rhel9base image or adjust accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Dockerfile.openshift` around lines 1 - 16, The Dockerfile currently leaves both stages running as root (builder and final) so the aws-cloud-controller-manager will run as root; add a non-root user and switch to it in the final stage (and optionally in the builder stage) by creating a user/group with a non-root UID (e.g. 1001), chowning the installed binary (/bin/aws-cloud-controller-manager) to that user, and adding a USER directive before ENTRYPOINT so the process runs non-root; reference the final-stage commands (COPY --from=builder /build/aws-cloud-controller-manager /bin/aws-cloud-controller-manager and ENTRYPOINT [ "/bin/aws-cloud-controller-manager" ]) and ensure ownership/permissions are adjusted to allow execution by the chosen non-root user.Sources: Coding guidelines, Linters/SAST tools
♻️ Duplicate comments (1)
pkg/providers/v1/aws_validations_test.go (1)
333-338:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe "empty value" test case still uses a non-empty error string.
Line 336 sets
ServiceAnnotationLoadBalancerExtraSecurityGroupsto the full error message instead of an empty string"". This duplicates the "annotation present" validation path rather than testing the empty-value branch.🐛 Proposed fix
{ name: "NLB with BYO extra SG with empty value - error (not supported)", annotations: map[string]string{ ServiceAnnotationLoadBalancerType: "nlb", - ServiceAnnotationLoadBalancerExtraSecurityGroups: "BYO extra security group annotation \"service.beta.kubernetes.io/aws-load-balancer-extra-security-groups\" is not supported by NLB", + ServiceAnnotationLoadBalancerExtraSecurityGroups: "", }, expectedError: "BYO extra security group annotation \"service.beta.kubernetes.io/aws-load-balancer-extra-security-groups\" is not supported by NLB", },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/providers/v1/aws_validations_test.go` around lines 333 - 338, The test case "NLB with BYO extra SG with empty value - error (not supported)" incorrectly sets ServiceAnnotationLoadBalancerExtraSecurityGroups to the full error string instead of an empty string; update that annotation value to "" and keep expectedError as the full message so the test exercises the empty-value validation branch (look for the test case name and the symbols ServiceAnnotationLoadBalancerType, ServiceAnnotationLoadBalancerExtraSecurityGroups, and expectedError in the test table).
🧹 Nitpick comments (1)
pkg/providers/v1/aws_validations_test.go (1)
745-746: 💤 Low valueSwap
assert.Equalargument order and remove redundantassert.Contains.Line 745 passes
(actual, expected)instead of the conventional(expected, actual), which will produce confusing failure messages. Line 746 is redundant—iferr.Error()equalstt.expectedError, it always contains it.♻️ Suggested cleanup
} else { assert.Error(t, err, "Expected error for test case: %s", tt.name) - assert.Equal(t, err.Error(), tt.expectedError, "Expected error for test case: %s", tt.name) - assert.Contains(t, err.Error(), tt.expectedError, "Error message should contain expected text for test case: %s", tt.name) + assert.Equal(t, tt.expectedError, err.Error(), "Expected error for test case: %s", tt.name) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/providers/v1/aws_validations_test.go` around lines 745 - 746, Swap the arguments to the test assertion so assert.Equal receives (tt.expectedError, err.Error()) instead of (err.Error(), tt.expectedError) to follow the conventional (expected, actual) order, and remove the redundant assert.Contains assertion that checks err.Error() against tt.expectedError (since equality already guarantees containment); update the two assertions in pkg/providers/v1/aws_validations_test.go that reference err.Error() and tt.expectedError accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/providers/v1/aws_validations_test.go`:
- Around line 699-721: Remove the duplicate test entries and correct the policy
values and test names so both PreferDualStack and RequireDualStack paths are
covered: keep a single case named "PreferDualStack with two entries works" with
ipFamilyPolicy: v1.IPFamilyPolicyPreferDualStack and ipFamilies:
[]v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, and change the two
"RequireDualStack" cases to a single case named "RequireDualStack with two
entries works" with ipFamilyPolicy: v1.IPFamilyPolicyRequireDualStack (and the
same ipFamilies/expectedError); ensure the test slice contains one unique entry
per scenario and update any duplicated entries referencing ipFamilyPolicy,
ipFamilies, and expectedError accordingly.
---
Outside diff comments:
In `@Dockerfile.openshift`:
- Around line 1-16: The Dockerfile currently leaves both stages running as root
(builder and final) so the aws-cloud-controller-manager will run as root; add a
non-root user and switch to it in the final stage (and optionally in the builder
stage) by creating a user/group with a non-root UID (e.g. 1001), chowning the
installed binary (/bin/aws-cloud-controller-manager) to that user, and adding a
USER directive before ENTRYPOINT so the process runs non-root; reference the
final-stage commands (COPY --from=builder /build/aws-cloud-controller-manager
/bin/aws-cloud-controller-manager and ENTRYPOINT [
"/bin/aws-cloud-controller-manager" ]) and ensure ownership/permissions are
adjusted to allow execution by the chosen non-root user.
---
Duplicate comments:
In `@pkg/providers/v1/aws_validations_test.go`:
- Around line 333-338: The test case "NLB with BYO extra SG with empty value -
error (not supported)" incorrectly sets
ServiceAnnotationLoadBalancerExtraSecurityGroups to the full error string
instead of an empty string; update that annotation value to "" and keep
expectedError as the full message so the test exercises the empty-value
validation branch (look for the test case name and the symbols
ServiceAnnotationLoadBalancerType,
ServiceAnnotationLoadBalancerExtraSecurityGroups, and expectedError in the test
table).
---
Nitpick comments:
In `@pkg/providers/v1/aws_validations_test.go`:
- Around line 745-746: Swap the arguments to the test assertion so assert.Equal
receives (tt.expectedError, err.Error()) instead of (err.Error(),
tt.expectedError) to follow the conventional (expected, actual) order, and
remove the redundant assert.Contains assertion that checks err.Error() against
tt.expectedError (since equality already guarantees containment); update the two
assertions in pkg/providers/v1/aws_validations_test.go that reference
err.Error() and tt.expectedError accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4df2d62d-f910-4157-9559-2fd6ea3cc8f2
⛔ Files ignored due to path filters (44)
go.sumis excluded by!**/*.sumtests/e2e/go.sumis excluded by!**/*.sumtests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/config.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/credentials.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/go_module_metadata.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/middleware/user_agent.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/protocol/ec2query/error_utils.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/protocol/query/middleware.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/retry/middleware.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/signer/v4/stream.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/transport/http/client.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/aws/transport/http/timeout_read_closer.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/configsources/CHANGELOG.mdis excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/configsources/config.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/configsources/endpoints.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/configsources/go_module_metadata.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/awsrulesfn/partitions.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/awsrulesfn/partitions.jsonis excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/v2/CHANGELOG.mdis excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/v2/endpoints.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/endpoints/v2/go_module_metadata.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/internal/middleware/middleware.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/CHANGELOG.mdis excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/LICENSE.txtis excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_client.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptAddressTransfer.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptCapacityReservationBillingOwnership.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptReservedInstancesExchangeQuote.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptTransitGatewayClientVpnAttachment.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptTransitGatewayMulticastDomainAssociations.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptTransitGatewayPeeringAttachment.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptTransitGatewayVpcAttachment.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptVpcEndpointConnections.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AcceptVpcPeeringConnection.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AdvertiseByoipCidr.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AllocateAddress.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AllocateHosts.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AllocateIpamPoolCidr.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_ApplySecurityGroupsToClientVpnTargetNetwork.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AssignIpv6Addresses.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AssignPrivateIpAddresses.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AssignPrivateNatGatewayAddress.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AssociateAddress.gois excluded by!**/vendor/**tests/e2e/vendor/github.com/aws/aws-sdk-go-v2/service/ec2/api_op_AssociateCapacityReservationBillingOwner.gois excluded by!**/vendor/**
📒 Files selected for processing (11)
.ci-operator.yamlDockerfile.openshiftdocs/service_controller.mdpkg/providers/v1/aws.gopkg/providers/v1/aws_fakes.gopkg/providers/v1/aws_loadbalancer.gopkg/providers/v1/aws_loadbalancer_test.gopkg/providers/v1/aws_test.gopkg/providers/v1/aws_validations.gopkg/providers/v1/aws_validations_test.gotests/e2e/go.mod
✅ Files skipped from review due to trivial changes (1)
- .ci-operator.yaml
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/e2e/go.mod
- pkg/providers/v1/aws_validations.go
- pkg/providers/v1/aws_loadbalancer_test.go
- pkg/providers/v1/aws_loadbalancer.go
- pkg/providers/v1/aws.go
- pkg/providers/v1/aws_test.go
🛑 Comments failed to post (1)
pkg/providers/v1/aws_validations_test.go (1)
699-721:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate test cases with wrong policy values.
Lines 699–709 are exact duplicates (both named "PreferDualStack with two entries works" with identical configuration). Lines 711–721 are also duplicates; additionally, both claim to test "RequireDualStack" but incorrectly use
v1.IPFamilyPolicyPreferDualStack, leaving theRequireDualStackpolicy path untested.🧹 Proposed fix
- { - name: "PreferDualStack with two entries works", - ipFamilyPolicy: v1.IPFamilyPolicyPreferDualStack, - ipFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, - expectedError: "", - }, - { - name: "PreferDualStack with two entries works", - ipFamilyPolicy: v1.IPFamilyPolicyPreferDualStack, - ipFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, - expectedError: "", - }, { name: "RequireDualStack with two entries works", - ipFamilyPolicy: v1.IPFamilyPolicyPreferDualStack, - ipFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, - expectedError: "", - }, - { - name: "RequireDualStack with two entries works", - ipFamilyPolicy: v1.IPFamilyPolicyPreferDualStack, + ipFamilyPolicy: v1.IPFamilyPolicyRequireDualStack, ipFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, expectedError: "", },📝 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.{ name: "RequireDualStack with two entries works", ipFamilyPolicy: v1.IPFamilyPolicyRequireDualStack, ipFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, expectedError: "", },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/providers/v1/aws_validations_test.go` around lines 699 - 721, Remove the duplicate test entries and correct the policy values and test names so both PreferDualStack and RequireDualStack paths are covered: keep a single case named "PreferDualStack with two entries works" with ipFamilyPolicy: v1.IPFamilyPolicyPreferDualStack and ipFamilies: []v1.IPFamily{v1.IPv6Protocol, v1.IPv4Protocol}, and change the two "RequireDualStack" cases to a single case named "RequireDualStack with two entries works" with ipFamilyPolicy: v1.IPFamilyPolicyRequireDualStack (and the same ipFamilies/expectedError); ensure the test slice contains one unique entry per scenario and update any duplicated entries referencing ipFamilyPolicy, ipFamilies, and expectedError accordingly.
|
@mfbonfigli: 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. |
|
/retitle UPSTREAM-SYNC: Fix merge https://github.com/kubernetes/cloud-provider-aws:master (c34d66e) into main The regression test failure is constant. Can you take a look? |
openshift-ci
Bot
changed the title
openshift-ci-robot
added
jira/valid-reference
|
@mfbonfigli: This pull request is an upstream sync and explicitly references no jira issue. 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. |
Author
|
@RadekManak Those tests appear to fail also on an empty PR as you can double check here:
Going deeper the test cases that fail are:
You can check the test logic here: I would assume that both are more related to CCCMO behavior than CCM. If a regression happened I would assume it is on CCCMO rather than CCM. Taking a look at past test run on the latest merged PRs:
|
|
/lgtm |
openshift-ci-robot
added
the
verified
|
@RadekManak: This PR has been marked as verified by 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. |
Author
|
/assign elmiko |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elmiko, RadekManak 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 |
openshift-ci
Bot
added
the
approved
openshift-merge-bot
Bot
merged commit 5060934
into
openshift:main
9 of 10 checks passed
1 task
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
17 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.
What
Manual rebase to fix the rebasebot issues of #136
Rebasebot failed the merge, leaving the PR in a state were it could not compile. This rebase has been done manually and finalized with a fresh
go mod vendorto update dependencies both in the main package and intests/e2eSummary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores