aws-appconfig: SourcedConfiguration doesn't use retrievalRole

[maikbasel]

Describe the bug

The SourcedConfiguration construct does not make use of it's retrievalRole property regardless of wether is defined or not. It will always create a new IAM Role (except when it's location type is ConfigurationSourceType.CODE_PIPELINE then it will default to undefined).

I encountered this behavior while trying to use role of mine created in a different stack and encountered an error that the role used was not allowed to retrieve the secret my configuration profile should reference.

Expected Behavior

The SourcedConfiguration should make use of the role passed to it via the retrievalRole property.

Current Behavior

The SourcedConfiguration will always create a new IAM Role instead of using the role passed into it via the retrievalRole property.

Reproduction Steps

• Create a secretmanager.Secret in Stack A.
• Create a iam.Role in Stack A and grant it read access rights to the secret.
• Deploy Stack A.
• Import the Secret from Stack A via Secret.fromSecretNameV2 function into Stack B.
• Import the Role from Stack A via Role.fromRoleName function into Stack B.
• Create a appconfig.SourcedConfiguration in Stack B referencing the imported Secret as location and the imported Role as retrievalRole.
• Try Deploy Stack B. You should experience and access denied error. The logs will also reference a different iam.Role. You can also debug the SourcedConfiguration initialization and see that a new role will be created even though a retrievalRole was passed into the construct.

Here a simplyfied version of my code:

StackA:

import {Stack, StackProps} from "aws-cdk-lib";
import {Construct} from "constructs";
import {Role, ServicePrincipal} from "aws-cdk-lib/aws-iam";
import {Secret} from "aws-cdk-lib/aws-secretsmanager";

export class StackA extends Stack {

    constructor(scope: Construct, id: string, props: StackProps) {
        super(scope, id, props);

        const appConfigRetrievalRole = new Role(this, 'AppConfigRetrieveRole', {
            assumedBy: new ServicePrincipal('appconfig.amazonaws.com'),
            roleName: 'AppConfigRetrieveRole',
        });

        const mySecret = new Secret(this, 'MySecret', {
            secretName: 'MySecret',
            generateSecretString: {
                secretStringTemplate: JSON.stringify({
                    username: 'admin',
                    password: '<PASSWORD>'
                }),
                generateStringKey: 'password'
            }
        });

        mySecret.grantRead(appConfigRetrievalRole);
    }
}

StackB:

import {Construct} from "constructs";
import {Stack, StackProps} from "aws-cdk-lib";
import {Secret} from "aws-cdk-lib/aws-secretsmanager";
import {Application, ConfigurationSource, Environment, SourcedConfiguration} from "aws-cdk-lib/aws-appconfig";
import {Role} from "aws-cdk-lib/aws-iam";

export class StackB extends Stack {
    constructor(scope: Construct, id: string, props: StackProps) {
        super(scope, id, props);

        const myAppConfigApp = new Application(this, 'MyAppConfigApp', {
            applicationName: 'my-app',
        })

        const devEnv = new Environment(this, 'DevEnv', {
            application: myAppConfigApp,
            environmentName: 'dev',
        });

        const mySecret = Secret.fromSecretNameV2(this, 'MySecret', 'MySecret');

        const appConfigRetrievalRole = Role.fromRoleName(this, 'AppConfigRetrieveRole', 'AppConfigRetrieveRole');

        new SourcedConfiguration(this, 'MySecretConfig', {
            name: 'mySecretConfig',
            application: myAppConfigApp,
            location: ConfigurationSource.fromSecret(mySecret),
            retrievalRole: appConfigRetrievalRole,
        });
    }
}

Possible Solution

The condition in the SourcedConfigurations constructor checking wether the retrievalRole is defined is wrong. As I already wrote, regardless of wether retrievalRole is defined or not. It will always create a new IAM Role (except when it's location type is ConfigurationSourceType.CODE_PIPELINE then it will default to undefined). This makes no sense. If the retrievalRole is defined it should use this role.

Additional Information/Context

No response

CDK CLI Version

2.146.0

Framework Version

No response

Node.js Version

20.14.0

OS

Windows 10

Language

TypeScript

Language Version

No response

Other information

No response

[ashishdhingra]

@maikbasel Good afternoon. Thanks for opening the issue. Could you please share minimal self-contained code to troubleshoot the issue? This would ensure quick reproduction and mimic the same scenario.

Thanks,
Ashish

[github-actions]

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

[maikbasel]

@ashishdhingra I updated the issue with a simplyfied code example.

[ashishdhingra]

Reproducible. Appears to be an issue at this code, where it is using ternary operator that always creates a new role if the condition is met. (for comparison, refer code for CodeBuild). The mentioned code for AppConfig should instead be written as below to achieve the desired behavior (notice the added parenthesis to avoid the 2nd condition to be part of 1st check props.retrievalRole):

this.retrievalRole = props.retrievalRole || (this.location.type != ConfigurationSourceType.CODE_PIPELINE
      ? new iam.Role(this, 'Role', {
        roleName: PhysicalName.GENERATE_IF_NEEDED,
        assumedBy: new iam.ServicePrincipal('appconfig.amazonaws.com'),
        inlinePolicies: {
          ['AllowAppConfigReadFromSourcePolicy']: this.getPolicyForRole(),
        },
      })
      : undefined);

---

As a simple test, use the following JavaScript code below:

var x = "foo";
var condition = 'test';
var somevalue = x || condition != 'test1' ? 'Hi' : undefined; // the value of somevalue will be 'Hi'
var somevalueCorrect = x || (condition != 'test1' ? 'Hi' : undefined); // the value of somevalueCorrect will be 'foo'

[pahud]

Agree and the doc string for this prop is incorrect. This means it would only generate a new one if undefined.

aws-cdk/packages/aws-cdk-lib/aws-appconfig/lib/configuration.ts

Lines 487 to 492 in 7f5ce4b

	/**
	* The IAM role to retrieve the configuration.
	*
	* @default - A role is generated.
	*/
	readonly retrievalRole?: iam.IRole;

But actually it's not

aws-cdk/packages/aws-cdk-lib/aws-appconfig/lib/configuration.ts

Lines 567 to 575 in 7f5ce4b

	this.retrievalRole = props.retrievalRole || this.location.type != ConfigurationSourceType.CODE_PIPELINE
	? new iam.Role(this, 'Role', {
	roleName: PhysicalName.GENERATE_IF_NEEDED,
	assumedBy: new iam.ServicePrincipal('appconfig.amazonaws.com'),
	inlinePolicies: {
	['AllowAppConfigReadFromSourcePolicy']: this.getPolicyForRole(),
	},
	})
	: undefined;

I would suggest change to code to

this.retrievalRole = props.retrievalRole ?? this.createAppConfigRetrievalRole();

And implement a private createAppConfigRetrievalRole() construct method in the bottom of the constructor, that would be easier to read.

For example

private createAppConfigRetrievalRole(): iam.Role | undefined {
  // Check if a custom retrieval role is provided
  if (this.props.retrievalRole) {
    return this.props.retrievalRole;
  }

  // Check if the configuration source is not from CodePipeline
  if (this.location.type !== ConfigurationSourceType.CODE_PIPELINE) {
    return new iam.Role(this, 'AppConfigRetrievalRole', {
      roleName: 'AppConfigRetrievalRole',
      assumedBy: new iam.ServicePrincipal('appconfig.amazonaws.com'),
      managedPolicies: [
        iam.ManagedPolicy.fromAwsManagedPolicyName('AWSAppConfigFullAccess'),
      ],
    });
  }

  // No role is needed if the configuration source is from CodePipeline
  return undefined;
}

Also, I am not sure why it only creates a new role when this.location.type !== ConfigurationSourceType.CODE_PIPELINE. Probably need more investigation on it.

If you believe it's not required and should be fixed, please share relevant doc in this thread and we welcome the pull requests.

[github-actions]

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.