-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[v2] [customization] Create/delete role associations for EMR on EKS #9254
base: v2
Are you sure you want to change the base?
Conversation
…ounts and provided IAM role so it can be used in Amazon EMR on EKS with EKS pod identity.
@@ -0,0 +1,5 @@ | |||
{ | |||
"type": "api-change", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it count as an api change when we're adding commands via customization?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm yeah it looks like new customizations are introduced as features. I'll update.
@@ -0,0 +1,298 @@ | |||
# Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2024 -> 2025
{ | ||
"name": "type", | ||
"help_text": ( | ||
"Specify the EMR on EKS submission model and choose service accounts that you want to associate " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EMR -> Amazon EMR
"name": "type", | ||
"help_text": ( | ||
"Specify the EMR on EKS submission model and choose service accounts that you want to associate " | ||
"with Amazon EMR on EKS. By default is start_job_run. Supported Types: start_job_run, " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking nit:
By default -> The default
Types -> types
"Specify the namespace that you want to associate the operator service account " | ||
"with the IAM role. Default to the job/application namespace specified. Note: " | ||
"If jobs are running in a different namespace than the operator installation namespace, " | ||
"this parameter needs to be set as where the operator is running on." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
non-blocking nit:
where -> the namespace that
@@ -0,0 +1,177 @@ | |||
import pytest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add copyright
cli_runner.add_response( | ||
HTTPResponse(body=create_pod_identity_association_response) | ||
) | ||
result = cli_runner.run( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests seem like functional tests rather than unit tests. Can we update their directory locations accordingly?
@@ -0,0 +1,578 @@ | |||
# Copyright 2024 Amazon.com, Inc. or its affiliates. All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2024 -> 2025
) | ||
request_idx += 1 | ||
self.assert_delete_call_matches( | ||
result.aws_requests[request_idx], cluster_name, str(i + 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be worth adding an assertion that there are no more than the expected number of requests in result.aws_requests list
|
||
# Use case: Expect to do nothing on deletion when resource not found but print warning message | ||
# Expected results: Association deletions are skipped | ||
def test_create_role_associations_already_exists_for_start_job_run( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
create -> delete
Description
EKS Pod Identity requires users to create pod identity associations for cluster service accounts that the pod container is going to use with a configured job execution IAM role. This requires users to provide service account names and the IAM role arn. However, service account information is opaque in the EMR on EKS service so the
--create-role-associations
and--delete-role-associations
customizations simplify management by resolving the correct service accounts based on the input cluster and role names.Manual testing
Create an IAM role with the following assume role policy document:
Create an EKS cluster.
Run the create command, replacing the
my-cluster
andmy-role
values:Run the delete command, replacing the
my-cluster
andmy-role
values: