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

aws_autoscaling: ssm_session_permissions attribute of AutoScalingGroup() has no effect #25904

Closed
IllarionovDimitri opened this issue Jun 8, 2023 · 10 comments · Fixed by #27220
Labels
@aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p2

Comments

@IllarionovDimitri
Copy link

IllarionovDimitri commented Jun 8, 2023

Describe the bug

i have discovered the option ssm_session_permission in aws_autoscaling.AutoScalingGroup(), which can be set to True or False (default False) to update the instance profile accordingly to be able to connect to the instance via Session Manager, but the setting has no effect whatsoever.

asg = autoscaling.AutoScalingGroup(
# many lines omitted
ssm_session_permissions=True,
)

Expected Behavior

Setting the ssm_session_permissions=True adds new SSM permissions to IAM instance profile so the instance can be managed by Systems Manager

Current Behavior

Setting the ssm_session_permissions=True or False has no effect

Reproduction Steps

Set Setting the ssm_session_permissions=True and run cdk diff in a command line or deploy the setting
of autoscaling group

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.83.0

Framework Version

No response

Node.js Version

18

OS

Ubuntu 20.04.5 LTS

Language

Python

Language Version

3.10.6

Other information

No response

@IllarionovDimitri IllarionovDimitri added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 8, 2023
@github-actions github-actions bot added the @aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling label Jun 8, 2023
@peterwoodworth
Copy link
Contributor

We add a managed policy to the Role if this is enabled, and we describe in the docstring that more configuration may be necessary.

if (props.ssmSessionPermissions) {
this.role.addManagedPolicy(iam.ManagedPolicy.fromAwsManagedPolicyName('AmazonSSMManagedInstanceCore'));
}

You may see no difference in synth enabling this prop or not if you've imported your role, or set it to be immutable.

@peterwoodworth peterwoodworth added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed needs-triage This issue or PR still needs to be triaged. labels Jun 8, 2023
@IllarionovDimitri
Copy link
Author

IllarionovDimitri commented Jun 8, 2023

i define the role with S3 access permissions for EC2 instances running in ASG in ec2.LaunchTemplate(...) and provide the launch template into autoscaling.AutoScalingGroup(...), where I also set ssm_session_permissions=True with expectation that AmazonSSMManagedInstanceCore policy will be additionally attached to the instance IAM role, but after I run I run cdk deploy {stack name} and check the instance IAM role, It was not amended. The role itself is not immutable.

If I do not provide my custom role in ec2.LaunchTemplate() (since it optional according to the docs), I get the
RuntimeError: Error: The provided launch template does not expose or does not define its role. raised by ECS in

ecs_cluster.add_asg_capacity_provider(capacity_provider)

where the capacity_provider = ecs.AsgCapacityProvider(...)

@peterwoodworth
Copy link
Contributor

peterwoodworth commented Jun 8, 2023

It will take action on the role passed in as a prop to role in the AutoScalingGroup construct

Please post your full snippet, it matters how you are defining role. I'm getting expected behavior on latest version

@peterwoodworth peterwoodworth added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jun 8, 2023
@IllarionovDimitri
Copy link
Author

IllarionovDimitri commented Jun 9, 2023

the role is defined as follows (full snippet). It also worth mentioning here that if I add AmazonSSMManagedInstanceCore
to the role's managed policies, the deployed EC2 instanced will be managed by SSM, so all additional config requirements are met.

read_from_bucket_policy = {
            "ReadFromEncryptedS3Bucket": iam.PolicyDocument(
                statements=[
                    iam.PolicyStatement(
                        effect=iam.Effect.ALLOW,
                        resources=[stage_params.kms_key_arn],
                        actions=[
                            "kms:Decrypt",
                            "kms:GenerateDataKey",
                        ],
                    ),
                ]
            )
        }

asg_role = iam.Role(
            self,
            f"{config.ID}-asg-role",
            assumed_by=iam.ServicePrincipal("ec2.amazonaws.com"),
            managed_policies=[
                iam.ManagedPolicy.from_aws_managed_policy_name("AmazonS3FullAccess"),
                iam.ManagedPolicy.from_aws_managed_policy_name(
                    "EC2InstanceProfileForImageBuilderECRContainerBuilds"
                ),
            ],
            inline_policies=read_from_bucket_policy,
            role_name=f"{config.ID}-asg-role",
        )

it then is passed to the launch template

launch_template = ec2.LaunchTemplate(
            self,
            f"{config.ID}-launch-template",
# lines omitted
            role=asg_role,
            security_group=sg_asg_ecs_import,
            user_data=user_data,
        )

the launch template to ASG (where I tried to implement the discovered ssm_session_permissions=True)

asg = autoscaling.AutoScalingGroup(
            self,
            f"{config.ID}-asg",
            vpc=vpc,
            mixed_instances_policy=autoscaling.MixedInstancesPolicy(
                launch_template=launch_template,
# many lines omitted
            )
            auto_scaling_group_name=f"{config.ID}-asg",
            min_capacity=config.MIN_EC2,
            desired_capacity=config.DES_EC2,
            max_capacity=config.MAX_EC2,
# further lines omitted
)

and ASG to ESC

        capacity_provider = ecs.AsgCapacityProvider(
            self,
            f"{config.ID}-capacity-provider",
# lines omitted
            auto_scaling_group=asg,
        )

        ecs_cluster.add_asg_capacity_provider(capacity_provider)

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 9, 2023
@peterwoodworth
Copy link
Contributor

You need to pass the role in to the ASG as well if you want to enable ssmSessionPermissions

@peterwoodworth peterwoodworth added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 9, 2023
@IllarionovDimitri
Copy link
Author

IllarionovDimitri commented Jun 9, 2023

done. with following message

RuntimeError: Setting 'role' must not be set when 'launchTemplate' or 'mixedInstancesPolicy' is set

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Jun 9, 2023
@peterwoodworth
Copy link
Contributor

Ahh I see, the block of code I referenced above

if (props.ssmSessionPermissions) {
this.role.addManagedPolicy(iam.ManagedPolicy.fromAwsManagedPolicyName('AmazonSSMManagedInstanceCore'));
}

only comes into effect when specifying a machine image and instance types. I think this line of code could move to just outside the else block it currently resides in, such that the _role which can be set either way, will attempt to add the managed policy if the ssmSessionPermissions prop is set

@peterwoodworth peterwoodworth added p2 good first issue Related to contributions. See CONTRIBUTING.md effort/small Small work item – less than a day of effort labels Jun 9, 2023
@IllarionovDimitri
Copy link
Author

great, thanks. looking forward to activate this feature. if you need further info or some tests, let me know.

@tam0ri
Copy link
Contributor

tam0ri commented Sep 20, 2023

Previous PR was closed over 2 months ago, and there has been no activity after that. So I submitted the new PR.

@mergify mergify bot closed this as completed in #27220 Sep 29, 2023
mergify bot pushed a commit that referenced this issue Sep 29, 2023
…o role specified in launch template (#27220)

When we set ssmSessionPermissions to true, CDK adds `AmazonSSMManagedInstanceCore` managed policy to role specified in AutoScalingGroup construct. However, if the role specified in not AutoScalingGroup construct's prop but launch template, ssmSessionPermissions has no effect.

This PR solves the issue by adding the managed policy even when role is specified in launch template.

Closes #25904

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-autoscaling Related to Amazon EC2 Auto Scaling bug This issue is a bug. effort/small Small work item – less than a day of effort good first issue Related to contributions. See CONTRIBUTING.md p2
Projects
None yet
3 participants