Skip to content

Commit 918593d

Browse files
authored
fix(eks): remove usage of shell=True in helm commands (#35148)
Improve subprocess handling in EKS Helm custom resource handler. Same PR as: #35141 ### Reason for this change The EKS Helm custom resource handler used `shell=True` with subprocess calls, which is not aligned with security best practices. Following Python's recommended approach for subprocess execution improves code robustness and follows secure coding guidelines. ### Description of changes **Refactor subprocess execution to follow Python best practices** https://docs.python.org/3/library/subprocess.html#replacing-shell-pipeline - **Replaced shell command strings with array-based commands**: Refactored `get_oci_cmd()` to return structured command objects instead of shell strings - **Implemented proper subprocess pipelines**: Used `Popen` with `PIPE` to chain `aws ecr get-login-password` and `helm registry login` commands following Python documentation recommendations - **Removed `shell=True`**: Adopted array-based command execution as recommended by Python subprocess documentation - **Maintained functionality**: Preserved all existing behavior for private ECR, public ECR, and fallback scenarios **Files modified:** - `packages/@aws-cdk/custom-resource-handlers/lib/aws-eks/kubectl-handler/helm/__init__.py` - `packages/@aws-cdk/aws-eks-v2-alpha/lib/kubectl-handler/helm/__init__.py` - `packages/@aws-cdk/aws-eks-v2-alpha/test/integ.alb-controller.js.snapshot/asset.*/helm/__init__.py` **Technical approach:** - Commands are now built as arrays: `['helm', 'pull', repository, '--version', version, '--untar']` - Pipeline implementation follows Python subprocess best practices using `Popen` with proper `PIPE` connections - User inputs are passed as separate array elements, ensuring proper argument handling ### Describe any new or updated permissions being added No new IAM permissions required. The change maintains the same AWS API calls and functionality. ### Description of how you validated changes - **Functionality testing**: Confirmed that ECR authentication and Helm chart pulling continues to work correctly for all scenarios - **Code review**: Verified implementation follows Python subprocess best practices as documented in the Python documentation - **Compatibility testing**: Ensured backward compatibility with existing CDK Helm chart deployments ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
1 parent cd9d69c commit 918593d

File tree

341 files changed

+7316
-6132
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

341 files changed

+7316
-6132
lines changed
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
*.zip filter=lfs diff=lfs merge=lfs -text
Lines changed: 64 additions & 18 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.
Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk-testing/framework-integ/test/aws-eks/test/integ.alb-controller-authapi.js.snapshot/integ-eks-stack.assets.json

Lines changed: 17 additions & 17 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

packages/@aws-cdk-testing/framework-integ/test/aws-eks/test/integ.alb-controller-authapi.js.snapshot/integ-eks-stack.template.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -941,7 +941,7 @@
941941
{
942942
"Fn::Sub": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}"
943943
},
944-
"/9e5735ab13fb0ab98eb443d105c092608811daa53d367ae36289cca965add4c4.json"
944+
"/1cf186b4d089119320de10437dfa334d7b9f08ff99f2c87633f28d225ac8e097.json"
945945
]
946946
]
947947
}
@@ -991,7 +991,7 @@
991991
{
992992
"Fn::Sub": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}"
993993
},
994-
"/26a86baa1f7a7274bc43922caad22fa28546b8934343da72395fda351aa5f2aa.json"
994+
"/03063b33cd0e6181b1deeeb06d2eb78f33ad4f71f39310976d3a81023e3cb72d.json"
995995
]
996996
]
997997
}

packages/@aws-cdk-testing/framework-integ/test/aws-eks/test/integ.alb-controller-authapi.js.snapshot/integeksstackawscdkawseksClusterResourceProviderD385D88C.nested.template.json

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@
5050
"S3Bucket": {
5151
"Fn::Sub": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}"
5252
},
53-
"S3Key": "75f7b6a23d8f39fbba91063166ea824a6b248a2b6eb9e9f6ce75ac58d33a1941.zip"
53+
"S3Key": "fbe89680da6eb304bf1bd582d61bdb89d82c75c3b2f4c87040c9385af73b9299.zip"
5454
},
5555
"Description": "onEvent handler for EKS cluster resource provider",
5656
"Environment": {
@@ -123,7 +123,7 @@
123123
"S3Bucket": {
124124
"Fn::Sub": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}"
125125
},
126-
"S3Key": "75f7b6a23d8f39fbba91063166ea824a6b248a2b6eb9e9f6ce75ac58d33a1941.zip"
126+
"S3Key": "fbe89680da6eb304bf1bd582d61bdb89d82c75c3b2f4c87040c9385af73b9299.zip"
127127
},
128128
"Description": "isComplete handler for EKS cluster resource provider",
129129
"Environment": {

0 commit comments

Comments
 (0)