sid88in / serverless-appsync-plugin

serverless plugin for appsync
MIT License
950 stars 186 forks source link

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

Open Lorenzohidalgo opened 9 months ago

Lorenzohidalgo commented 9 months ago

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:

Lorenzohidalgo commented 9 months ago

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