STOR-2918: Rebase to upstream v1.62.0 for OCP 5.0 - #329
Conversation
Signed-off-by: Connor catlett <conncatl@amazon.com>
…ot/cherry-pick-2837-to-master [master] Release 1.55
Signed-off-by: Connor catlett <conncatl@amazon.com>
Bump test-helm-chart instance type to account for increased memory usage
Signed-off-by: Eddie Torres <torredil@amazon.com>
Signed-off-by: Eddie Torres <torredil@amazon.com>
Update CSI sidecar images to address CVE-2025-61726
Signed-off-by: Eddie Torres <torredil@amazon.com>
Fix error handling when re-fetching the node during agent-not-ready taint removal attempts
Upgrade to volume-modifier-for-k8s 0.9.2
Signed-off-by: Eddie Torres <torredil@amazon.com>
…ot/cherry-pick-2855-to-master [master] Release 1.55.1
Fix Typo in Tagging Docs
Signed-off-by: Eddie Torres <torredil@amazon.com>
Add support for windows 2025
Remove nodes/proxy from helm tester manifests
Signed-off-by: jukie <isaac.wilson514@gmail.com>
…urable helm: make healthPort configurable
Rename ports to avoid collisions
feat(plugin): configure node and volume topology segments
Bump dependencies ahead of release
Signed-off-by: Connor Catlett <conncatl@amazon.com>
…-health-check Add plugin method to override health check
Signed-off-by: Connor Catlett <conncatl@amazon.com>
Bump dependencies
Fixing warn-on-invalid-tag validation bug
This commit carries OpenShift-specific files and modifications: - OpenShift CI configuration (.ci-operator.yaml, OWNERS, OWNERS_ALIASES) - OpenShift Dockerfile (Dockerfile.openshift.rhel7) for RHEL 9 with Go 1.25 and OpenShift 4.22 - Snyk configuration (.snyk) - Vendored dependencies (vendor/) - License header script update to skip vendor directory - Remove upstream GitHub workflows and templates - Fix `make verify` not to require a license header in OCP .ci-operator.yaml in generate-license-header.sh Updated for OpenShift 4.22 release.
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@dfajmon: This pull request references STOR-2918 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target only the "5.0.0" version, but multiple target versions were set. 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. |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dfajmon 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-ci
Bot
removed
the
do-not-merge/work-in-progress
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
WalkthroughThis large release bundles multiple EBS CSI driver features: multi-card volume attachment support, node-local volumes, an alpha metadata-labeler sidecar, EBS snapshot locking, volume cloning, and a build-time plugin system. It updates Helm charts, Kubernetes manifests, dependencies, CI/build tooling, documentation, and adds extensive e2e/unit test coverage while removing legacy A1-compatibility support. ChangesCore Driver
Estimated code review effort: 5 (Critical) | ~180 minutes Helm Chart & Deploy Manifests
Documentation & CI
Tests
Sequence Diagram(s)sequenceDiagram
participant Kubelet
participant CSIDriver as EBS CSI Driver
participant DeviceManager
participant AWSCloud as pkg/cloud
participant EC2
Kubelet->>CSIDriver: ControllerPublishVolume(nodeID, volumeID)
CSIDriver->>AWSCloud: AttachDisk(volumeID, nodeID)
AWSCloud->>DeviceManager: NewDevice(instance, volumeID, numCards)
DeviceManager-->>AWSCloud: Device{Path, CardIndex}
AWSCloud->>EC2: AttachVolume(EbsCardIndex)
EC2-->>AWSCloud: VolumeAttachment
AWSCloud->>AWSCloud: WaitForAttachmentState(expectedCardIndex)
AWSCloud-->>CSIDriver: attach success
CSIDriver-->>Kubelet: PublishContext
sequenceDiagram
participant LeaderElector
participant LabelController as Metadata Labeler
participant K8sAPI
participant EC2
LeaderElector->>LabelController: elected leader
LabelController->>K8sAPI: list Nodes and PersistentVolumes
LabelController->>EC2: GetInstancesPatching(nodeIDs)
EC2-->>LabelController: Instance details (ENIs, block devices)
LabelController->>LabelController: compute non-CSI volume/ENI counts
LabelController->>K8sAPI: patch Node labels
Estimated code review effort: 5 (Critical) | ~240 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
pkg/metrics/metrics.go (1)
81-116: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
InitializeNVME/InitializeAsyncEC2Metricsignore their own receivermand mutate the global singletonrinstead.Both methods are declared on
(m *MetricRecorder)but their bodies exclusively reference the package-levelr(registerNVMECollector(r, ...),r.asyncEC2Metrics = ...,r.registry.MustRegister(...)). Every other refactored method in this file (registerHistogramVec,registerCounterVec,initializeMetricWithOperations,IncreaseCount,ObserveHistogram) correctly usesm. Now thatMetricRecorderis exported and directly instantiable (as demonstrated by the newTestMetricRecorderConcurrentAccessinmetrics_test.go, which builds aMetricRecorder{}without going throughInitializeRecorder), calling these two methods on any instance other than the singleton would silently corrupt/mutate global state instead of the receiver's own state.Based on learnings, "Follow existing option patterns and exported API conventions instead of inventing new abstractions" and "Keep public APIs backward compatible" — the exported type should behave consistently regardless of which instance it's called on.
🐛 Proposed fix to use the receiver consistently
func (m *MetricRecorder) InitializeNVME(csiMountPointPath, instanceID string) { - registerNVMECollector(r, csiMountPointPath, instanceID) + registerNVMECollector(m, csiMountPointPath, instanceID) } func (m *MetricRecorder) InitializeAsyncEC2Metrics(minimumEmissionThreshold time.Duration) { variableLabels := []string{"volume_id", "instance_id", "attachment_state"} cacheCleanupInterval := 15 * time.Minute - r.asyncEC2Metrics = &AsyncEC2Collector{ + m.asyncEC2Metrics = &AsyncEC2Collector{ ... } - r.registry.MustRegister(r.asyncEC2Metrics) + m.registry.MustRegister(m.asyncEC2Metrics) go func() { for { - <-r.asyncEC2Metrics.ticker.C - r.asyncEC2Metrics.cleanupCache(cacheCleanupInterval) + <-m.asyncEC2Metrics.ticker.C + m.asyncEC2Metrics.cleanupCache(cacheCleanupInterval) } }() }🤖 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/metrics/metrics.go` around lines 81 - 116, InitializeNVME and InitializeAsyncEC2Metrics are using the package-level singleton r instead of their receiver m, which breaks behavior for any non-singleton MetricRecorder instance. Update both methods to consistently operate on m by wiring registerNVMECollector and the AsyncEC2Collector setup through the receiver’s fields (including registry, asyncEC2Metrics, and ticker handling) rather than mutating global state. Keep the existing API surface and patterns used by registerHistogramVec, registerCounterVec, initializeMetricWithOperations, IncreaseCount, and ObserveHistogram so exported MetricRecorder instances behave identically whether created directly or via InitializeRecorder.Source: Learnings
pkg/cloud/handlers.go (1)
74-95: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winKeep non-throttle AWS API errors visible by default Non-throttle API failures are now logged at V(3), so they won’t show up with the shipped
--v=2defaults and become much harder to diagnose.🤖 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/cloud/handlers.go` around lines 74 - 95, Non-throttle AWS API failures are being hidden behind a higher verbosity level in LogServerErrorsMiddleware, so make them visible at the default logging level. Update the error logging in the middleware returned by LogServerErrorsMiddleware so the non-throttle branch logs with the standard error logger instead of klog.V(3), while keeping throttle errors on the quieter verbose path and preserving the existing smithy.APIError / retry.DefaultThrottleErrorCodes handling.pkg/driver/node.go (1)
814-847: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject
--reserved-volume-attachments < -1.Validate()only treats-1as the sentinel, so any other negative value is subtracted directly ingetVolumesLimit()and can inflateMaxVolumesPerNodeabove the instance’s real attach limit. Add a lower-bound check in the flag validation path.🤖 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/driver/node.go` around lines 814 - 847, The negative reserved-volume setting is not being rejected, and getVolumesLimit can miscompute MaxVolumesPerNode when ReservedVolumeAttachments is below the sentinel value. Update the validation path in NodeService.Validate() to enforce a lower bound so only -1 is allowed as the special auto-detect value, and reject any value less than -1 before getVolumesLimit() uses d.options.ReservedVolumeAttachments.pkg/driver/controller_modify_volume.go (1)
92-127: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMerge the new modify options into coalesced requests
mergeModifyVolumeRequeststill dropsIOPSPerGBandAllowIopsIncreaseOnResize, so a later coalesced modify request can execute without the requested IOPS-per-GB behavior.🤖 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/driver/controller_modify_volume.go` around lines 92 - 127, mergeModifyVolumeRequest currently merges only size, IOPS, throughput, volume type, and tags, but it still drops IOPSPerGB and AllowIopsIncreaseOnResize from modifyDiskOptions. Update mergeModifyVolumeRequest in controller_modify_volume.go to carry these fields through the coalescing logic, including conflict checks consistent with the existing option merges, so later requests preserve the requested IOPS-per-GB behavior. Use the modifyVolumeRequest and modifyDiskOptions fields as the place to locate and wire the missing options.hack/e2e/eksctl/eksctl.sh (1)
22-52: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass
LINUX_AMIandWINDOWS_AMIinto the initial gomplate render. The template already uses these env vars for the linux and custom-windows nodegroups, so omitting them here makes the initial eksctl cluster ignore custom AMIs; only the outpost re-render gets them.🤖 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 `@hack/e2e/eksctl/eksctl.sh` around lines 22 - 52, The initial gomplate render in eksctl_create_cluster is missing the LINUX_AMI and WINDOWS_AMI environment values, so the main cluster template cannot use custom AMIs for the linux and custom-windows nodegroups. Update the gomplate invocation in eksctl_create_cluster to pass through both LINUX_AMI and WINDOWS_AMI alongside the existing CLUSTER_NAME, REGION, K8S_VERSION, ZONES, INSTANCE_TYPE, AMI_FAMILY, and WINDOWS variables. Keep the outpost-specific render separate, but ensure the initial render has the same AMI inputs the template expects.cmd/main.go (1)
121-131: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog the actual errors in refactored failure paths.
These paths log stale variables (
err/exporterErr) instead of the local errors, which can hide the root cause during tracing or pre-stop failures.Proposed logging fix
exporter, exporterErr := driver.InitOtelTracing() if exporterErr != nil { - klog.ErrorS(err, "failed to initialize otel tracing") + klog.ErrorS(exporterErr, "failed to initialize otel tracing") klog.FlushAndExit(klog.ExitFlushTimeout, 1) } @@ defer cancel() if shutdownErr := exporter.Shutdown(ctx); shutdownErr != nil { - klog.ErrorS(exporterErr, "could not shutdown otel exporter") + klog.ErrorS(shutdownErr, "could not shutdown otel exporter") } }() @@ clientset, clientErr := metadata.DefaultKubernetesAPIClient(options.Kubeconfig)() if clientErr != nil { - klog.ErrorS(err, "unable to communicate with k8s API") + klog.ErrorS(clientErr, "unable to communicate with k8s API")Also applies to: 213-215
🤖 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 `@cmd/main.go` around lines 121 - 131, The OpenTelemetry tracing initialization and shutdown paths in main use stale error variables instead of the local failures, so update the logging in InitOtelTracing handling and the exporter.Shutdown defer to pass the actual local error values. In the klog.ErrorS calls, replace the captured outer variable with the corresponding exporterErr or shutdownErr from the current scope so the real root cause is logged for both failure paths, including the later pre-stop shutdown site.
🟠 Major comments (29)
charts/aws-ebs-csi-driver/templates/poddisruptionbudget-controller.yaml-17-27 (1)
17-27: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRendering both
maxUnavailableandminAvailableunconditionally breaks the common single-value case.When a user sets only
maxUnavailable(or onlyminAvailable), this block still emits both keys — the unset one renders as an empty value, and PodDisruptionBudget rejects specs with both fields set anyway. This will produce an invalid manifest in the most common override scenario.🐛 Proposed fix
{{- if or .Values.controller.podDisruptionBudget.maxUnavailable .Values.controller.podDisruptionBudget.minAvailable }} - maxUnavailable: {{ .Values.controller.podDisruptionBudget.maxUnavailable }} - minAvailable: {{ .Values.controller.podDisruptionBudget.minAvailable }} + {{- if .Values.controller.podDisruptionBudget.maxUnavailable }} + maxUnavailable: {{ .Values.controller.podDisruptionBudget.maxUnavailable }} + {{- end }} + {{- if .Values.controller.podDisruptionBudget.minAvailable }} + minAvailable: {{ .Values.controller.podDisruptionBudget.minAvailable }} + {{- end }} {{- else }}🤖 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 `@charts/aws-ebs-csi-driver/templates/poddisruptionbudget-controller.yaml` around lines 17 - 27, The PodDisruptionBudget template is rendering both fields at once in the controller PDB block, which breaks the single-value override case. Update the conditional logic in the poddisruptionbudget-controller template so it emits only one of maxUnavailable or minAvailable based on which value is set in .Values.controller.podDisruptionBudget, and keep the existing default fallback behavior when neither is provided.pkg/util/util.go-61-70 (1)
61-70: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
SetDriverNameloses its one-time guard on every call
sync.OnceValueis created inside the function, so each invocation gets a fresh instance and always writesdriverName. The fatal path can never fire, which means a later call with a different name silently overwrites the package state. Use a package-level once/mutex guard instead.🤖 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/util/util.go` around lines 61 - 70, SetDriverName is recreating sync.OnceValue on every call, so the one-time guard never persists and driverName can be overwritten later. Move the guard to package scope in pkg/util/util.go, using a persistent sync.Once or mutex-protected check around SetDriverName so only the first call sets driverName and later calls with a different name hit the fatal klog.ErrorS/FlushAndExit path.pkg/cloud/cloud.go-903-932 (1)
903-932: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
AvailabilityZoneandAvailabilityZoneIdmutually exclusive.pkg/driver/controller.gocan populate both onDiskOptions, so this helper can send both toCreateVolumeand hitInvalidParameterCombination. Useelse ifhere or normalize upstream.🤖 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/cloud/cloud.go` around lines 903 - 932, The createVolumeHelper method currently sets both AvailabilityZone and AvailabilityZoneId on the same CreateVolumeInput when both zone and zoneID are present, which can trigger InvalidParameterCombination. Update the logic in createVolumeHelper to make these fields mutually exclusive by preferring one and using else if (or normalize DiskOptions before calling it), so only one of aws.String(zone) or aws.String(zoneID) is assigned.pkg/cloud/cloud.go-2188-2224 (1)
2188-2224: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDecouple plugin health from the dry-run cache
pkg/cloud/cloud.go:2194-2224
plugin.HealthCheck()only runs whenc.attemptDryRun.Load()is true. After a successful dry-run orskipDriverHealthCheck, laterProbecalls return nil until the 3h reset, so a plugin failure can stay hidden for hours. Run the plugin check on every probe or cache it separately from the EC2 dry-run.🤖 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/cloud/cloud.go` around lines 2188 - 2224, The plugin health check in cloud.DryRun is currently gated by c.attemptDryRun, so plugin failures can be hidden after the dry-run is cached. Update DryRun to always invoke plugin.GetPlugin().HealthCheck() on every probe, and keep its result separate from the EC2 dry-run cache behavior. Preserve the existing skipDriverHealthCheck logic and c.attemptDryRun.Store(false) only for the EC2 dry-run path, not for the plugin health path.tests/e2e/testsuites/dynamically_provisioned_copy_volume_tester.go-29-34 (1)
29-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCall
ValidateFuncafter the cloned pod succeeds
ValidateFuncis set by callers intests/e2e/requires_aws_api.go, butRun()never invokes it. That silently skips the post-clone AWS/API assertions; mirrorDynamicallyProvisionedCmdVolumeTestby calling it aftertcpod.WaitForSuccess().🤖 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 `@tests/e2e/testsuites/dynamically_provisioned_copy_volume_tester.go` around lines 29 - 34, The cloned-volume test currently defines ValidateFunc on DynamicallyProvisionedCopyVolumeTest but never calls it, so the post-clone assertions are skipped. Update Run() in DynamicallyProvisionedCopyVolumeTest to invoke ValidateFunc after tcpod.WaitForSuccess(), mirroring the behavior in DynamicallyProvisionedCmdVolumeTest and ensuring callers like requires_aws_api.go can run their AWS/API validation.tests/e2e/parameter_tests.go-270-271 (1)
270-271: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAvoid pulling a public image in Ginkgo e2e.
public.ecr.aws/amazonlinux/amazonlinux:2023requires public registry access and can fail in disconnected/restricted CI. Use a repository-managed or mirrored test image that providescp --reflink. As per coding guidelines, flag Ginkgo e2e tests that pull images from public registries.🤖 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 `@tests/e2e/parameter_tests.go` around lines 270 - 271, The Ginkgo e2e test setup in the reflinkProbe container is pulling a public Amazon Linux image, which can break in restricted CI. Update the test’s image reference to use a repository-managed or mirrored image that still provides cp --reflink, and keep the change within the reflinkProbe / parameter_tests setup so the e2e test no longer depends on public registry access.Source: Coding guidelines
tests/e2e/parameter_tests.go-115-117 (1)
115-117: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse bounded contexts for direct AWS and Kubernetes calls.
These e2e calls use
context.Background(), so a stuck AWS/Kubernetes request can hang the spec until the outer job times out. Use a helper withcontext.WithTimeoutfor these direct API calls. As per path instructions, Go code should usecontext.Contextfor cancellation and timeouts. Based on learnings, inspect cancellation and timeout boundaries carefully.Also applies to: 125-130, 155-157, 166-171, 196-198, 208-213, 289-293, 298-300, 313-314, 331-332
🤖 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 `@tests/e2e/parameter_tests.go` around lines 115 - 117, The direct AWS/Kubernetes calls in the e2e tests are using context.Background(), which can leave requests hanging indefinitely. Update the affected test setup and helper calls in parameter_tests.go to use a bounded context created with context.WithTimeout, and make sure the context is passed through the existing config.LoadDefaultConfig, EC2/client creation, and any other direct API calls in the listed blocks. Prefer a small helper for consistent timeout handling and cancellation, and reference the affected setup patterns around config.LoadDefaultConfig, ec2.NewFromConfig, and the Kubernetes/AWS client calls so they can be updated together.Sources: Path instructions, Learnings
tests/e2e/parameter_tests.go-161-173 (1)
161-173: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftScope EC2 tag assertions to the volume created by this test.
These queries can pass if any pre-existing cluster volume has the expected cluster/extra tag. Filter by the test-created volume handle, or add a unique per-test tag/namespace filter when available, so regressions on the newly provisioned volume are caught.
Also applies to: 202-215
🤖 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 `@tests/e2e/parameter_tests.go` around lines 161 - 173, The EC2 volume tag checks in the DynamicallyProvisionedCmdVolumeTest are too broad because DescribeVolumes only filters by the cluster tag, so pre-existing volumes can make the assertion pass. Update the ValidateFunc in the volume-tag tests (including the later matching case) to scope the query to the volume created by this test, using the created volume handle from the test setup or a unique per-test tag/namespace filter, and keep the assertions on the specific volume returned by ec2Client.DescribeVolumes.pkg/driver/node.go-962-1003 (1)
962-1003: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse a separate context for the last-chance taint removal
ctxis canceled in the watch-error handler onUnauthorized/Forbidden, so the finalGethere can fail withcontext.Canceledbefore the fallback runs. Use an independent timeout context for the last-chance read/remove path; ifattemptTaintRemovalshould survive watch cancellation too, it needs the same treatment.🤖 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/driver/node.go` around lines 962 - 1003, Use a separate context for the final taint-removal fallback in the taint-watcher flow: the watch error handler in node.go cancels ctx on Unauthorized/Forbidden, so the last-chance clientset.CoreV1().Nodes().Get and any follow-up removal logic can fail early with context.Canceled. Create an independent timeout context for the “last chance” read/remove path, and pass that same independent context through attemptTaintRemoval if it also needs to run after watch cancellation.tests/e2e/metadata-labeler.go-97-108 (1)
97-108: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winError check happens after using the (possibly nil) result.
nodes.Itemsis iterated at Line 100 beforeerrfrom theListcall is checked at Line 104. If the API call fails,nodeswill benil, and dereferencingnodes.Itemspanics instead of failing cleanly viaExpect.🐛 Proposed fix
nodes, err := cs.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{}) + Expect(err).NotTo(HaveOccurred(), "Failed to list nodes") clusterInstances := []string{} for _, clusterNode := range nodes.Items { clusterInstances = append(clusterInstances, parseProviderID(clusterNode.Spec.ProviderID)) } - Expect(err).NotTo(HaveOccurred(), "Failed to list nodes") resp, err := ec2Client.DescribeInstances(context.TODO(), &ec2.DescribeInstancesInput{🤖 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 `@tests/e2e/metadata-labeler.go` around lines 97 - 108, The `nodes.Items` loop in `metadata-labeler.go` uses the result of `cs.CoreV1().Nodes().List(...)` before checking `err`, which can panic if the list call fails. Move the `Expect(err).NotTo(HaveOccurred(), ...)` check to immediately after the `List` call and before iterating `nodes.Items`, so the `clusterInstances` build only runs when `nodes` is valid.tests/e2e/metadata-labeler.go-427-440 (1)
427-440: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winError-handling logic in
createStorageClasssilently swallows real Create errors.The check
if err != nil && errors.IsNotFound(err)only asserts when the error is specifically "NotFound" — which aCreatecall never returns. Any actual failure (AlreadyExists, permission denied, etc.) is silently ignored, so the test proceeds with a broken/incomplete storage class instead of failing with a diagnosable error.🐛 Proposed fix
_, err := cs.StorageV1().StorageClasses().Create(context.Background(), storageClass, metav1.CreateOptions{}) - if err != nil && errors.IsNotFound(err) { + if err != nil && !errors.IsAlreadyExists(err) { Expect(err).NotTo(HaveOccurred(), "Failed to create storage class") }🤖 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 `@tests/e2e/metadata-labeler.go` around lines 427 - 440, The error handling in createStorageClass is checking the wrong condition, so real Create failures are ignored and the test can continue with an invalid storage class. Update createStorageClass to assert or fail whenever cs.StorageV1().StorageClasses().Create returns any non-nil err, and remove the errors.IsNotFound branch since Create should not be treated as a NotFound case. Keep the fix scoped to createStorageClass and its Create call so failures like AlreadyExists or permission errors surface immediately.Dockerfile-28-45 (1)
28-45: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftSet a non-root runtime user or document the required exception.
These final stages do not declare
USER, so they run as root by default. If root is truly required for CSI node operations, add an explicit justification/suppression; otherwise set a non-root user per target image. As per path instructions, Dockerfiles must use “USER non-root; never run as root.”🤖 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` around lines 28 - 45, The final runtime stages in the Dockerfile currently omit a USER declaration, so they default to root. Update the linux-al2023, windows-ltsc2019, windows-ltsc2022, and windows-ltsc2025 stages to run as a non-root user using the appropriate image-supported account, or add an explicit documented exception if root is unavoidable. Use the final stage blocks with the ENTRYPOINT definitions as the place to apply the USER change.Sources: Path instructions, Linters/SAST tools
pkg/driver/controller.go-668-690 (1)
668-690: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor the node-local feature gate during capability validation.
ValidateVolumeCapabilitiesconfirmslocal-ebs://volumes even whenEnableNodeLocalVolumesis false, while publish rejects them. ReturnInvalidArgumentconsistently when the feature is disabled.🤖 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/driver/controller.go` around lines 668 - 690, Update ValidateVolumeCapabilities in controller.go so node-local volumes are gated the same way as publish paths: when EnableNodeLocalVolumes is false, detect local-ebs:// volume IDs before the node-local capability check and return InvalidArgument instead of confirming them. Keep the existing GetDiskByID path for non-node-local volumes, and ensure the isNodeLocalVolume / isValidCapabilityForNodeLocal handling only runs when the feature gate is enabled.pkg/driver/controller.go-892-911 (1)
892-911: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate snapshot lock values before creating the snapshot.
Invalid mode, non-positive duration, or non-positive cool-off currently reaches AWS after the snapshot is created, then relies on cleanup. Reject invalid lock parameters as
InvalidArgumentbeforeCreateSnapshot.🤖 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/driver/controller.go` around lines 892 - 911, Validate snapshot lock inputs in the snapshot creation path before calling CreateSnapshot, using the existing LockMode, LockDuration, LockExpirationDate, and LockCoolOffPeriod handling in pkg/driver/controller.go. Reject invalid lock mode values and any non-positive LockDuration or LockCoolOffPeriod with status.Errorf(codes.InvalidArgument, ...) so bad parameters never reach AWS. Keep the parsing logic near the current snapshot lock setup so the CreateSnapshot call only runs after all lock fields have been validated.pkg/driver/controller.go-491-499 (1)
491-499: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate publish requests before the node-local fast path.
The node-local branch bypasses
validateControllerPublishVolumeRequest, so malformed requests like an emptynode_idcan reach cloud lookup and return the wrong CSI error.Move validation before branching
volumeID := req.GetVolumeId() nodeID := req.GetNodeId() + if err := validateControllerPublishVolumeRequest(req); err != nil { + return nil, err + } + if isNodeLocalVolume(volumeID) { if !d.options.EnableNodeLocalVolumes { return nil, status.Error(codes.InvalidArgument, "node-local volumes are not enabled") } return d.controllerPublishVolumeNodeLocal(ctx, req) } - - if err := validateControllerPublishVolumeRequest(req); err != nil { - return nil, err - }🤖 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/driver/controller.go` around lines 491 - 499, The controller publish path currently checks isNodeLocalVolume before validateControllerPublishVolumeRequest in controllerPublishVolume, which lets malformed requests skip validation and hit the node-local fast path. Move validateControllerPublishVolumeRequest(req) to run before the node-local branch so all publish requests are rejected consistently before calling controllerPublishVolumeNodeLocal or any cloud lookup logic.pkg/driver/controller.go-420-430 (1)
420-430: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not map unknown create failures to
Aborted.
Abortedtells the CO the operation is in-progress/conflicted. Unknown AWS/cloud failures should remainInternal, and capacity/limit failures should map toResourceExhaustedwhen available.Safer error mapping
case errors.Is(err, cloud.ErrSourceNotFound): errCode = codes.NotFound + case errors.Is(err, cloud.ErrLimitExceeded): + errCode = codes.ResourceExhausted default: - errCode = codes.Aborted + errCode = codes.Internal }🤖 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/driver/controller.go` around lines 420 - 430, The create-failure gRPC mapping in controller logic is too broad because unknown cloud errors are currently being converted to Aborted. Update the error-to-code switch in the controller’s create path so only known conflicts and validation/not-found cases map to their specific codes, while unknown AWS/cloud failures default to Internal instead of Aborted. Also add a case for capacity/limit-related cloud errors to return ResourceExhausted when those errors can be identified.pkg/batcher/batcher.go-168-168 (1)
168-168: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid blocking forever on abandoned result channels.
A direct send can leak the
executegoroutine if a caller times out or stops receiving. Use a size-1 buffered result channel at creation time, or keep this delivery non-blocking.Possible localized fix
- ch <- BatchResult[ResultType]{Result: r, Err: err} + select { + case ch <- BatchResult[ResultType]{Result: r, Err: err}: + default: + klog.V(7).InfoS("execute: no receiver for batch result", "task", task) + }🤖 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/batcher/batcher.go` at line 168, The direct send in `execute` can block forever if the caller abandons the result channel, so update the `BatchResult` delivery path to avoid leaking the goroutine. Fix this by making the result channel created for `execute`/`Batch` size-1 buffered, or by changing the send on `ch` to a non-blocking delivery pattern while preserving the existing `BatchResult[ResultType]` contract.pkg/driver/constants.go-66-67 (1)
66-67: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep
KmsKeyIdKeyas a deprecated alias. Renaming this exported constant is a compile-time break for downstream imports; preserve the old spelling unless this PR is intentionally dropping that API.Compatibility alias
KmsKeyIDKey = "kmskeyid" + // KmsKeyIdKey is deprecated; use KmsKeyIDKey. + KmsKeyIdKey = KmsKeyIDKey🤖 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/driver/constants.go` around lines 66 - 67, The exported constant rename in constants.go is a breaking API change; keep KmsKeyIDKey as the canonical name and preserve KmsKeyIdKey as a deprecated alias for downstream compatibility. Update the constants declaration to expose both symbols in pkg/driver/constants.go so existing imports continue to compile, and mark the older spelling as deprecated if needed.Source: Learnings
hack/ebs-scale-test/helpers/scale-test/volume-lifecycle-churn-test/volume-lifecycle-churn.sh-137-149 (1)
137-149: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle failed Jobs in
wait_for_wave_jobs_complete
Exit non-zero as soon as any Job in the wave reports failure; withbackoffLimit: 0, a terminal Job failure will never reachsucceeded, so this loop can wait forever and mask the churn bug.🤖 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 `@hack/ebs-scale-test/helpers/scale-test/volume-lifecycle-churn-test/volume-lifecycle-churn.sh` around lines 137 - 149, `wait_for_wave_jobs_complete` only waits for `succeeded` Jobs, so a terminal Job failure can loop forever and hide churn errors. Update the `kubectl get jobs`/`jq` logic in `wait_for_wave_jobs_complete` to also detect any Job in the current `churn-wave` with a failure state, and exit non-zero immediately when one is found; keep the existing success path for counting completed Jobs against `REPLICAS`.pkg/cloud/metadata/metadata.go-108-125 (1)
108-125: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftShorten the metadata-labeler refresh timeout.
UpdateMetadata()runsKubernetesAPIInstanceInfo(..., true)on every periodicNodeGetInforefresh, and that path can block for up to 5s with exponential backoff when the EC2/volume labels aren’t ready yet. That can make allocatable-count updates stall repeatedly under normal labeler lag. Split the refresh timeout/backoff from the initial metadata fetch, or make it separately configurable.🤖 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/cloud/metadata/metadata.go` around lines 108 - 125, The metadata-labeler refresh path in UpdateMetadata() is using the same long timeout/backoff as the initial KubernetesAPIInstanceInfo fetch, which can stall periodic NodeGetInfo updates. Adjust the KubernetesAPIInstanceInfo call path used by Metadata.UpdateMetadata (especially the metadataLabeler=true branch) to use a shorter, separately configurable refresh timeout/backoff instead of the 5s initial-fetch behavior. Keep the change localized to UpdateMetadata and KubernetesAPIInstanceInfo so the refresh path returns quickly when EC2/volume labels are not yet ready.tests/e2e/multi_card.go-45-49 (1)
45-49: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove AWS config loading out of the
Describebody
Fail()here runs during Ginkgo tree construction, so an AWS config error can panic before any spec executes. Initialize the EC2 client inBeforeEachor lazily insideItinstead.🤖 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 `@tests/e2e/multi_card.go` around lines 45 - 49, AWS config loading is happening during Ginkgo tree construction in the Describe scope, so a failure can abort before any spec runs. Move the config initialization for config.LoadDefaultConfig and ec2.NewFromConfig out of the Describe body into a BeforeEach setup or into the It body so Fail() only runs during test execution. Keep the change localized around the EC2 client setup in multi_card.go.tests/e2e/testsuites/testsuites.go-411-411 (1)
411-411: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the topology key concatenation
"topology"+util.GetDriverName()+"/zone"is missing the dot, so it buildstopologyebs.csi.aws.com/zoneinstead oftopology.ebs.csi.aws.com/zone. That preventskeyFoundfrom ever being set here and trips theFail(...)path wheneverAllowedTopologiesis used.🤖 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 `@tests/e2e/testsuites/testsuites.go` at line 411, The topology key check in the testsuites logic is concatenating the driver name without the required dot, so update the condition in the `keyFound`/`Fail(...)` path to match the actual topology key format produced by `AllowedTopologies`. Fix the string built from `util.GetDriverName()` so it compares against the correct `topology.<driver>/zone` key, and keep the check in the same `testsuites.go` block so `keyFound` can be set properly.cmd/main.go-206-209 (1)
206-209: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winFail metadata-labeler mode when its Kubernetes client or loop fails.
MetadataLabelerModepassesk8sClienteven if client creation failed, then exits with status0when leader election/label patching returns an error. Make this path fail fast and non-zero so the container status reflects the failure.Proposed failure handling
k8sClient, err = cfg.K8sAPIClient() if err != nil { + if cmd == string(driver.MetadataLabelerMode) { + klog.ErrorS(err, "failed to setup k8s client") + klog.FlushAndExit(klog.ExitFlushTimeout, 1) + } klog.V(2).InfoS("Failed to setup k8s client", "err", err) } @@ case string(driver.MetadataLabelerMode): err := metadata.ContinuousUpdateLabelsLeaderElection(k8sClient, cloud, metadata.ControllerMetadataLabelerInterval) if err != nil { klog.ErrorS(err, "failed to patch volume/ENI count on node labels") - klog.FlushAndExit(klog.ExitFlushTimeout, 0) + klog.FlushAndExit(klog.ExitFlushTimeout, 1) } + klog.FlushAndExit(klog.ExitFlushTimeout, 0)Also applies to: 226-230
🤖 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 `@cmd/main.go` around lines 206 - 209, MetadataLabelerMode currently logs K8s client setup failures and continues, then can still exit successfully even when leader election or label patching fails. Update the MetadataLabelerMode flow in main so that cfg.K8sAPIClient() failures and the subsequent labeler loop/error path both abort the mode and return a non-zero exit status, using the existing k8sClient/MetadataLabelerMode path to fail fast instead of proceeding with a nil or unusable client.pkg/cloud/metadata/k8s.go-123-145 (1)
123-145: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject negative label-derived counts.
strconv.Atoiaccepts values like-1, so node/SageMaker labels can produce impossible ENI or block-device counts. Validate these count labels as non-negative before storing them. As per path instructions, validate at trust boundaries with allow-lists.Proposed helper hardening
result, err := strconv.Atoi(val) - if err != nil { + if err != nil || result < 0 { klog.ErrorS(err, "failed to convert label to int", "label", label) - return defaultValue, err + if err == nil { + err = fmt.Errorf("%s label must be non-negative", label) + } + return defaultValue, err }Apply the same non-negative check to the SageMaker label parsing paths.
Also applies to: 246-251
🤖 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/cloud/metadata/k8s.go` around lines 123 - 145, The SageMaker label parsing in the node metadata path accepts negative integers because strconv.Atoi only checks format, not range. In the label handling inside the k8s metadata logic, add a non-negative validation for both LabelSageMakerENICount and LabelSageMakerBlockDeviceMappingsCount before assigning to numAttachedENIs and numBlockDeviceMappings, and treat negative values the same as parse failures by falling back to defaults and logging via klog.ErrorS. Apply the same hardening to the additional SageMaker label parsing path referenced by the comment so all trust-boundary label inputs are allow-listed to valid count values.Source: Path instructions
hack/e2e/kops/patch-cluster.yaml-76-77 (1)
76-77: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winKeep
ec2:LockSnapshottag-scoped.Line 77 grants snapshot locking on every snapshot ARN in the e2e account. The policy already has tag-scoped snapshot statements below; move
ec2:LockSnapshotthere instead of granting it unconditionally. AWS listsec2:LockSnapshotas supporting resource-tag condition keys, so the narrower tag-conditioned form is viable. (docs.aws.amazon.com)Proposed least-privilege adjustment
"ec2:CreateVolume", - "ec2:EnableFastSnapshotRestores", - "ec2:LockSnapshot" + "ec2:EnableFastSnapshotRestores"Also add
ec2:LockSnapshotto the existing tag-conditionedDeleteSnapshotstatements for CSI-created snapshots.🤖 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 `@hack/e2e/kops/patch-cluster.yaml` around lines 76 - 77, The `ec2:LockSnapshot` permission is currently granted broadly in the snapshot policy, so move it out of the unconditional action list and add it to the existing tag-conditioned snapshot statements alongside `DeleteSnapshot` for CSI-created snapshots. Update the relevant IAM policy blocks in `patch-cluster.yaml` so `ec2:LockSnapshot` is only allowed when the same resource tag conditions apply, using the existing snapshot tag-scoped statements as the location to modify.pkg/cloud/metadata/labels.go-310-321 (1)
310-321: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNil-map write panics
patchSingleNodeif a node has no labels.
newNode.Labels[VolumesLabel] = ...writes directly intonode.Labelswithout checking for nil.v1.Node.ObjectMeta.Labelsis a plain map that can benil;DeepCopy()preserves a nil map as nil. Writing to a nil map panics ("assignment to entry in nil map"), crashing the metadata-labeler goroutine. The test helpermakeNode()masks this by always initializingLabels: make(map[string]string), so this path isn't exercised by tests.🔧 Suggested fix
newNode := node.DeepCopy() + if newNode.Labels == nil { + newNode.Labels = make(map[string]string) + } numAttachedENIs := enisVolumeMap[instanceID].ENIs🤖 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/cloud/metadata/labels.go` around lines 310 - 321, patchSingleNode can panic when newNode.Labels is nil because DeepCopy preserves a nil label map and the code writes to it directly. In patchSingleNode, initialize newNode.Labels before assigning VolumesLabel and ENIsLabel so nodes without existing labels are handled safely, and keep the fix near the existing node.DeepCopy() and label assignment logic.hack/e2e/run.sh-239-246 (1)
239-246: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMissing
--kubeconfigflag makes the restart-count check inconsistent with the rest of the script.The
kubectl get podscall used to compute the max container restart count omits--kubeconfig "${KUBECONFIG}", while the identical query two lines later (used to print the restart table) and otherkubectlinvocations in this script consistently pass it. In environments where the default kubeconfig differs from${KUBECONFIG}(e.g. CI runners with multiple kubeconfigs), this check will query the wrong cluster/context and silently miss real restart failures.🐛 Proposed fix
- if [[ $(kubectl get pods -n kube-system -l "app.kubernetes.io/name=aws-ebs-csi-driver" -o json | + if [[ $(kubectl get pods -n kube-system -l "app.kubernetes.io/name=aws-ebs-csi-driver" -o json --kubeconfig "${KUBECONFIG}" | jq -r '.items[].status.containerStatuses[]?.restartCount // 0' | sort -nr | head -n 1) -gt 3 ]]; then🤖 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 `@hack/e2e/run.sh` around lines 239 - 246, The restart-count gate in run.sh uses kubectl without the same --kubeconfig "${KUBECONFIG}" setting used elsewhere, so it may read the wrong cluster/context. Update the kubectl get pods call inside the restart threshold check to pass the explicit kubeconfig just like the later table-printing query and the rest of the script, keeping the logic in the same block that checks aws-ebs-csi-driver pod restarts.pkg/cloud/metadata/labels.go-118-133 (1)
118-133: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
time.Tickleaks resources and a single transient error permanently halts label refresh for the leadership term.Two issues in this loop:
time.Tickdocuments that its underlying ticker "cannot be recovered by the garbage collector; it leaks". SincecontinuousUpdateLabelsis re-invoked on every leader-election acquisition (ContinuousUpdateLabelsLeaderElection), repeated leadership flapping combined with errors will accumulate leaked tickers.- On any single
updateLabelserror (e.g. a transient EC2 API throttle), the function returns and the periodic refresh stops entirely until the next leader election — there's no retry/backoff, so a transient failure silently disables node-label metadata for an indefinite period.🔧 Suggested fix
- err = updateLabels(ctx, k8sClient, cloud, pvInformer) - if err != nil { - klog.ErrorS(err, "Could not patch node labels with updated volume/ENI count") - return err - } - for range time.Tick(updateTime) { - err = updateLabels(ctx, k8sClient, cloud, pvInformer) - if err != nil { - klog.ErrorS(err, "Could not patch node labels with updated volume/ENI count") - return err - } - } - return nil + if err := updateLabels(ctx, k8sClient, cloud, pvInformer); err != nil { + klog.ErrorS(err, "Could not patch node labels with updated volume/ENI count") + } + ticker := time.NewTicker(updateTime) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return ctx.Err() + case <-ticker.C: + if err := updateLabels(ctx, k8sClient, cloud, pvInformer); err != nil { + klog.ErrorS(err, "Could not patch node labels with updated volume/ENI count") + } + } + }🤖 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/cloud/metadata/labels.go` around lines 118 - 133, The periodic refresh in continuousUpdateLabels should not use time.Tick and should not exit the whole leader term on a single updateLabels failure. Replace the time.Tick loop with an explicit ticker created and stopped inside continuousUpdateLabels, and keep retrying updateLabels on errors instead of returning immediately; use klog.ErrorS in the continuousUpdateLabels path to log failures while preserving the loop so labels continue refreshing until context cancellation.pkg/cloud/metadata/labels.go-240-249 (1)
240-249: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA single node with an unparseable provider ID aborts label updates for every node.
getMetadatareturns immediately on the firstparseProviderIDerror, discarding results for all other (valid) nodes in the batch. Contrast this withpatchNodes, which tolerates up topatchFailsper-node failures. One node in an unexpected state (e.g. mid-registration, non-AWS) will block the entire periodic ENI/volume label refresh for the whole cluster.🔧 Suggested fix
for _, node := range nodes.Items { id, err := parseProviderID(&node) if err != nil { - return nil, err + klog.ErrorS(err, "Skipping node with unparseable provider ID", "node", node.Name) + continue } if strings.HasPrefix(id, "i-") { nodeIds = append(nodeIds, id) } }🤖 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/cloud/metadata/labels.go` around lines 240 - 249, In getMetadata, a single parseProviderID failure currently aborts the entire node scan and prevents label refresh for all valid nodes. Update the loop over nodes.Items so parseProviderID errors are tolerated per node, similar to patchNodes: skip the bad node, optionally log/debug it, and continue building nodeIds for the rest. Keep the existing id prefix filter in place and only fail the whole operation for truly fatal errors outside per-node parsing.
|
/test help |
|
/retest |
|
/test help |
|
/test e2e-aws-csi-extended |
|
/retest |
|
@dfajmon: 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. |
|
/test |
|
@gnufied: The The following commands are available to trigger optional jobs: Use 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. |
13 participants
STOR-2918: Rebase to upstream v1.62.0 for OCP 5.0
Issue link
https://redhat.atlassian.net/browse/STOR-2918
Diff to upstream v1.62.0
kubernetes-sigs/aws-ebs-csi-driver@v1.62.0...dfajmon:rebase-v1.62.0
Notes for reviewers
Upgrade notice (v1.49.0): The EBS CSI Driver Controller's readiness and liveness probes now periodically check AWS API access via a dry-run
DescribeAvailabilityZonescall. Clusters with broken networking/DNS or incorrectly configured IAM may fail to install.Upgrade notice (v1.59.0): The driver now calls
DescribeInstanceTypesat runtime for multi-card EBS instance handling. Custom IAM policies must addDescribeInstanceTypes. A fallback table is still present but will be removed in a future release.Summary of changes
Breaking Changes
-a1compatimage fora1.*family Amazon EC2 instances; upgrade to a more recent AWS Graviton instance type (#2836)Major Features
ebs-csi-drivercontroller readiness/liveness probes (#2590)CreateVolumedry-run to support increased IOPS limits for GP3 and other volume types (#2682)ebs.csi.aws.com/cluster-nameto support cluster-scoped IAM policies (#2899)Notable Bug Fixes
CVE Fixes
CVE-2026-33814, CVE-2026-33186, CVE-2025-61726
Cherry-picked commits
Upstream changelogs
The project uses a shared CHANGELOG.md covering all releases (no per-release changelog pages):
Full changelog
kubernetes-sigs/aws-ebs-csi-driver@v1.48.0...v1.62.0
Last rebase
#294
@openshift/storage
Co-authored by Claude
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores