Ack addon must support inline policies for EKS

[jackkleeman]

Description of changes:
The EKS ack controller doesn't have a recommended managed policy, only an inline one, and the currently provided managed policy in blueprints doesn't seem to give any eks permissions, nor does any other managed policy seem to be appropriate. The best option is to support inline policies. There may be other ack controllers with similar issues, but EKS is the one I'm aware of.

Also, this file appeared to have CRLF, so I've converted it to LF to match the rest of the project - I suggest viewing the diff without whitespace

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

[elamaran11]

@jackkleeman Thankyou so much for the PR, this is a needed one. Appreciate if you can update the documentation of ACK addon to support this change with a sample so that people can start using this feature. Looking forward to merge after your update.

[elamaran11]

LGTM. @shapirov103 Please review, run e2e and merge this.

[shapirov103]

@jackkleeman thank you for your contribution, surely needed capabilities.

I added one comment, as a rule we don't apply formatting changes to enable better review experience.

I don't observe tests for this, how did you validate it? Can you share a blueprint that we can run to try it out?

[shapirov103]

Was there a chance in this file? It is very hard to see because of the formatting. Short of reapplying the changes without formatting, may you can point specific lines that were changed?
Is it only about allowing to pass inline policies along with managed?

[shapirov103]

@elamaran11 ^^ we will need functional validation before we merge. I can run e2e to make sure no regression, but new capability of passing inline policy will need a separate QA exercise.

[jackkleeman]

try viewing without whitespace. the issue is the file was encoded with CLRD line endings (windows) which differs from other files in this repo yes it is only about allowing the passing of inline policies

[jackkleeman]

I don't observe tests for this, how did you validate it?

I validated by running the EKS ACK controller, which does not work without this change as it needs eks permissions which were not present before. You can simply add the following addon to a blueprint to test:

const addOn = new blueprints.addons.AckAddOn({
  serviceName: AckServiceName.EKS,
}),

And then you can try applying a resource such as:

apiVersion: eks.services.k8s.aws/v1alpha1
kind: PodIdentityAssociation
metadata:
  name: test
  namespace: default
spec:
  clusterName: your-cluster-name
  namespace: default
  roleARN: some-role-arn-in-this-account
  serviceAccount: default

and running kubectl describe to see that the ack controller had enough permissions to attempt to create it (most likely it will fail if you didnt set the trust policy on the role appropriately, but that still proves the permissions work)

[Pjv93]

You can simply add the following addon to a blueprint to test

Thank you for your contribution, I have reviewed the changes you made and the code looks good, however I was unable to successfully replicate the creation of the inline policies for either AckServiceName.EKS or the AckServiceName.S3 pattern. The Managed policy "AmazonEKSClusterPolicy" was attached and no inline policy was integrated and the trust policy was not applied. I also attempted to exclusively reference the inline policy in the s3 pattern and the managed "AmazonS3FullAccess"policy was attached with no sight of the inline policy.

[jackkleeman]

@Pjv93 that is weird. If you put the following in examples/blueprint-construct/index.ts:

import * as cdk from 'aws-cdk-lib';
import {Construct} from "constructs";
import * as blueprints from '../../lib';

const blueprintID = 'blueprint-construct-dev';

export default class BlueprintConstruct {
  constructor(scope: Construct, props: cdk.StackProps) {
    const addOns: Array<blueprints.ClusterAddOn> = [
      new blueprints.addons.AckAddOn({
        serviceName: blueprints.AckServiceName.EKS
      }),
    ];

    blueprints.EksBlueprint.builder()
      .version("auto")
      .addOns(...addOns)
      .build(scope, blueprintID, props);
  }
}

And run npx cdk synth, you should see the inline policy as an attachment to the role:

  blueprintconstructdevekschartsaRoleB7051B6A:
    Type: AWS::IAM::Role
    Properties:
      AssumeRolePolicyDocument:
        Statement:
          - Action: sts:AssumeRoleWithWebIdentity
            Condition:
              StringEquals:
                Fn::GetAtt:
                  - blueprintconstructdevekschartsaConditionJson6E37C7DB
                  - Value
            Effect: Allow
            Principal:
              Federated:
                Ref: blueprintconstructdevOpenIdConnectProvider3733B8DD
        Version: "2012-10-17"
      ManagedPolicyArns:
        - Fn::Join:
            - ""
            - - "arn:"
              - Ref: AWS::Partition
              - :iam::aws:policy/AmazonEKSClusterPolicy
...
  inlinepolicy168DC373:
    Type: AWS::IAM::Policy
    Properties:
      PolicyDocument:
        Statement:
          - Action:
              - eks:*
              - iam:GetRole
              - iam:PassRole
            Effect: Allow
            Resource: "*"
        Version: "2012-10-17"
      PolicyName: inlinepolicy168DC373
      Roles:
        - Ref: blueprintconstructdevekschartsaRoleB7051B6A
    Metadata:
      aws:cdk:path: blueprint-construct-dev/inline-policy/Resource

And when I run npx cdk deploy --require-approval=any-change, it does show the inline policy:

So I am not sure what could be the problem

As a sidenote, I think I should remove the EksClusterPolicy for EKS as it is unnecessary and confusing

[Pjv93]

@jackkleeman Thanks for that, the mistake was actually on my end. After running npx cdk synth && npx cdk deploy --require-approval=any-change, i see the inline policy and it looks good! I can also confirm the s3 inline policy pattern is also working. Great work.

[shapirov103]

markdown links failure is unrelated.

[shapirov103]

LGTM
@jackkleeman thank you for your contribution!

[shapirov103]

@elamaran11 your review is marked as "requested changes". can't merge with that one, please review.

[elamaran11]

LGTM

[elamaran11]

aa923ee

+1. Thankyou for the Contribution