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-apprunner-alpha: accessRole prop doesn't work #32974

Closed
1 task
garysassano opened this issue Jan 16, 2025 · 7 comments
Closed
1 task

aws-apprunner-alpha: accessRole prop doesn't work #32974

garysassano opened this issue Jan 16, 2025 · 7 comments
Labels
@aws-cdk/aws-apprunner Related to the apprunner package bug This issue is a bug. effort/small Small work item – less than a day of effort p3

Comments

@garysassano
Copy link

Describe the bug

The accessRole prop in the Service L2 construct doesn't work. If you try to set it, it gets ignored.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

No response

Expected Behavior

I expected the App Runner service to be created with the configured access role, like for this service I manually created from AWS Console:

Image

Current Behavior

The App Runner service gets deployed without the configured access role:

Image

Reproduction Steps

Deploy the following Stack:

const apprunnerInstanceRole = new Role(this, "ApprunnerInstanceRole", {
  assumedBy: new ServicePrincipal("tasks.apprunner.amazonaws.com"),
});

const apprunnerAccessRole = new Role(this, "ApprunnerAccessRole", {
  assumedBy: new ServicePrincipal("build.apprunner.amazonaws.com"),
  managedPolicies: [
    {
      managedPolicyArn: "arn:aws:iam::aws:policy/AdministratorAccess",
    },
  ],
});

new Service(this, "ApacheService", {
  accessRole: apprunnerAccessRole,
  instanceRole: apprunnerInstanceRole,
  source: Source.fromEcrPublic({
    imageIdentifier: "public.ecr.aws/docker/library/httpd:bookworm",
    imageConfiguration: {
      port: 80,
    },
  }),
});

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.175.1

Framework Version

No response

Node.js Version

22.12.0

OS

Ubuntu 24.04.1

Language

TypeScript

Language Version

No response

Other information

No response

@garysassano garysassano added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 16, 2025
@github-actions github-actions bot added the @aws-cdk/aws-apprunner Related to the apprunner package label Jan 16, 2025
@khushail khushail added needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Jan 16, 2025
@khushail khushail self-assigned this Jan 16, 2025
@khushail khushail added the p2 label Jan 16, 2025
@khushail
Copy link
Contributor

Hi @garysassano , thanks for reaching out. I am able to reproduce the scenario with the given code -

const apprunnerInstanceRole = new Role(this, "ApprunnerInstanceRole", {
      assumedBy: new ServicePrincipal("tasks.apprunner.amazonaws.com"),
    });
    
    const apprunnerAccessRole = new Role(this, "ApprunnerAccessRole", {
      assumedBy: new ServicePrincipal("build.apprunner.amazonaws.com"),
      managedPolicies: [
        {
          managedPolicyArn: "arn:aws:iam::aws:policy/AdministratorAccess",
        },
      ],
    });
    
    const servicenew = new apprunner.Service(this, "ApacheService", {
      accessRole: apprunnerAccessRole,
      instanceRole: apprunnerInstanceRole,
      source: apprunner.Source.fromEcrPublic({
        imageIdentifier: "public.ecr.aws/docker/library/httpd:bookworm",
        imageConfiguration: {
          port: 80,
        },
      }),
    });
  }

Here is the output in console when deployed -

Image

Looks like the issue exists here -

this.props.accessRole ?? this.generateDefaultRole() : undefined;

https://github.com/aws/aws-cdk/blob/899965d6147829b8f8ac52ac8c1350de50d7b6d0/packages/%40aws-cdk/aws-apprunner-alpha/lib/service.ts#L1288C1-L1290C71

if you leave it undefined, it will create the default role.

Marking it as P3 as default role creation exists but code should also take passed value into consideration.

Contribution from the community are also welcome for resolution of this bug.

@khushail khushail added p3 effort/small Small work item – less than a day of effort and removed p2 needs-reproduction This issue needs reproduction. labels Jan 16, 2025
@khushail khushail removed their assignment Jan 16, 2025
@mazyu36
Copy link
Contributor

mazyu36 commented Jan 17, 2025

Since an access role is only set when ECR is private, this seems to be the intended behavior.

* It's required for ECR image repositories (but not for ECR Public repositories).

this.accessRole = (this.source.imageRepository?.imageRepositoryType == ImageRepositoryType.ECR) ?

The docs states the following:

The access role is a role that App Runner uses for accessing images in Amazon Elastic Container Registry (Amazon ECR) in your account. It's required to access an image in Amazon ECR, and isn't required with Amazon ECR Public.

Do we need to set an access role even when using ECR Public?

@garysassano
Copy link
Author

@mazyu36 My initial implementation actually used an ECR private registry configured with a pull-through cache for Docker Hub. The outcome was identical. I created a minimal reproducible code block using the Apache image solely to illustrate the issue.

@garysassano
Copy link
Author

To clarify, the default access role cannot be used because you need to create an ECR registry permission that enables the IAM role used by App Runner to pull images via the pull-through cache. Here's an example implementation:

apprunnerInstanceRole.addToPrincipalPolicy(
  new PolicyStatement({
    actions: ["ecr:*"],
    resources: [
      `arn:aws:ecr:${this.region}:${this.account}:repository/docker-hub/*`,
    ],
  }),
);

const registryPolicy = new CfnRegistryPolicy(this, "EcrRegistryPolicy", {
  policyText: {
    Version: "2012-10-17",
    Statement: [
      {
        Sid: "AllowPullThroughCache",
        Effect: "Allow",
        Principal: {
          AWS: apprunnerAccessRole.roleArn,
        },
        Action: ["ecr:*"],
        Resource:
          "arn:aws:ecr:${this.region}:${this.account}:repository/docker-hub/",
      },
    ],
  },
});

Unfortunately, there's no built-in method to modify the access role policy directly. Currently, you only have the option to use Service.addToRolePolicy() to modify the instance role.

Image

It would be much better to have more idiomatic and self-explanatory methods like Service.addToAccessRolePolicy() and Service.addToInstanceRolePolicy(). These would provide clearer semantics and finer-grained control.

@garysassano
Copy link
Author

I was able to fix the issue. The problem is in the really bad design of the Service construct. It allows you to do things that you shouldn't be allowed to do, so the developer has to figure out he wasn't supposed to do something since there's no error being thrown.

This is how I fixed my code:

const ecr = Repository.fromRepositoryArn(
  this,
  "DockerHubRepo",
  `arn:aws:ecr:${this.region}:${this.account}:repository/docker-hub/otel/opentelemetry-collector-contrib`,
);

const otelCollectorService = new Service(this, "OtelCollectorService", {
  accessRole: apprunnerAccessRole,
  instanceRole: apprunnerInstanceRole,
  source: Source.fromEcr({
    repository: ecr,
    imageConfiguration: {
      port: 4318,
      startCommand: `--config https://${confmapBucket.bucketName}.s3.${this.region}.amazonaws.com/collector-confmap.yaml`,
    },
  }),
});

So, what are the many issues a developer has to face? First and foremost, if you specify the accessRole but you use a Source.fromEcrPublic, that accessRole will ALWAYS be evaluated to undefined. You won't realize that because the construct doesn't throw any error though.

The reason is in this code that performs what I think is a dumb check:

this.accessRole = (this.source.imageRepository?.imageRepositoryType == ImageRepositoryType.ECR) ?

It doesn't actually check if your repository is an ECR_PUBLIC or an ECR type by using the regex defined at:

* `([0-9]{12}.dkr.ecr.[a-z\-]+-[0-9]{1}.amazonaws.com\/.*)`.

Instead, it simply verifies whether you used Source.fromEcr or Source.fromEcrPublic. This means that if you use Source.fromEcrPublic while providing a URI for a private ECR repository, it fails to function as expected. What’s the point of having two from methods if they don’t even validate whether the provided string conforms to the expected regex?

accessRole shouldn't even be available for a public ECR then... You could have Service.fromEcrPublic and completely get rid of the accessRole if you truly want to make it work that way, which I believe is bad.

But this is not the only bad design of the Service construct.

Another really bad thing is that things work completely differently based on if you use Source.fromEcr or Source.fromEcrPublic.

If you use Source.fromEcr, you specify the repository URI using a repository prop which is of type IRepository:

Image

If you use Source.fromEcrPublic, you specify the repository URI using an imageIdentifier prop that is of type string:

Image

This inconsistency forces developers to deal with both a different property name and type for the same underlying task of specifying a repository. It's a frustrating developer experience.

In my opinion, the Service construct needs a complete redesign to unify these discrepancies. Instead of having separate Source.fromEcr and Source.fromEcrPublic methods, they could be merged into a single, streamlined interface. The repository type (public or private) could be determined dynamically using regex, such as:

  • Private repositories: ([0-9]{12}.dkr.ecr.[a-z\-]+-[0-9]{1}.amazonaws.com\/.*)
  • Public repositories: (public\.ecr\.aws\/.*)

Developers should also have the flexibility to specify an accessRole if needed. For public repositories, a warning could be displayed at deploy time, such as:

You have specified an accessRole while using a public ECR repository. This is not required and can be omitted if desired.

By eliminating unnecessary complexity and providing clear feedback, the construct could offer a much better developer experience.

@garysassano
Copy link
Author

To clarify, my issue stemmed from using Source.fromEcrPublic() with a private repository URI in the imageIdentifier. Surprisingly, this is allowed due to the flawed design of the Service construct, as described above.

Copy link

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 19, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@aws-cdk/aws-apprunner Related to the apprunner package bug This issue is a bug. effort/small Small work item – less than a day of effort p3
Projects
None yet
Development

No branches or pull requests

3 participants