Skip to content
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

Open
wants to merge 1 commit into
base: v2
Choose a base branch
from

Conversation

hssyoo
Copy link
Contributor

@hssyoo hssyoo commented Jan 24, 2025

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:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "Service": "pods.eks.amazonaws.com"
            },
            "Action": [
                "sts:AssumeRole",
                "sts:TagSession"
            ]
        }
    ]
}

Create an EKS cluster.

Run the create command, replacing the my-cluster and my-role values:

aws emr-containers create-role-associations --cluster-name my-cluster --namespace default --role-name my-role

Run the delete command, replacing the my-cluster and my-role values:

aws emr-containers delete-role-associations --cluster-name my-cluster --namespace default --role-name my-role

…ounts and provided IAM role so it can be used in Amazon EMR on EKS with EKS pod identity.
@hssyoo hssyoo self-assigned this Jan 24, 2025
@@ -0,0 +1,5 @@
{
"type": "api-change",
Copy link
Contributor

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?

Copy link
Contributor Author

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.
Copy link
Contributor

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 "
Copy link
Contributor

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, "
Copy link
Contributor

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."
Copy link
Contributor

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
Copy link
Contributor

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(
Copy link
Contributor

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.
Copy link
Contributor

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)
Copy link
Contributor

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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

create -> delete

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants