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

fix: Avoid Log Role Creation when roleArn is proivded #624

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Lorenzohidalgo
Copy link
Contributor

Detected Issue:

A new Log Role and policy is always created, without considering if one is provided or not.

If a roleArn is provided in the configuration, that one will be used as CloudWatchLogsRoleArn.

      set(endpointResource, 'Properties.LogConfig', {
        CloudWatchLogsRoleArn: this.config.logging.roleArn || {
          'Fn::GetAtt': [logicalIdCloudWatchLogsRole, 'Arn'],
        },
        FieldLogLevel: this.config.logging.level,
        ExcludeVerboseContent: this.config.logging.excludeVerboseContent,
      });

The current implementation of compileCloudWatchLogGroup will always create a new policy and role alongside the log group:

 return {
      [logGroupLogicalId]: {
        Type: 'AWS::Logs::LogGroup',
        ...
      },
      [policyLogicalId]: {
        Type: 'AWS::IAM::Policy',
        ...
      },
      [roleLogicalId]: {
        Type: 'AWS::IAM::Role',
        ...
      },
    };

Proposed Fix:

Update compileCloudWatchLogGroup to consider if roleArn is provided or not.

We could see two different scenarios:

  • roleArn is provided -> only the Log Group should be created
  • roleArn not provided -> Log Group, Policy and Role should be created

@Lorenzohidalgo
Copy link
Contributor Author

As additional context,

With the current version, deploying with the following configuration
image

Triggers the creation of two roles:
image
Our custom one "AppSyncLoggingServiceRole" and the default one, even though only the custom one will be used.

This fix removes the creation of the default one when a roleArn is provided:
image

@Lorenzohidalgo Lorenzohidalgo marked this pull request as ready for review December 13, 2023 16:59
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.

1 participant