@pmuens @eahefnawy @mthenw I updated this logic as it seemed to be broken, or my brain didn't compute 😊

This code block must be at the very beginning of the merge() method, otherwise bug #3174 will resurface. In other words, now that we always use log group resources, they must be merged regardless of whether we're using custom or default roles.

I'm not sure if I completely follow because of all the prior checks:

// resolve early if no functions are provided
// resolve early if provider level role is provided
// resolve early if all functions contain a custom role

If we're talking purely about Log groups and assuming the user is providing his own IAM statement the create logs code:

this.serverless.service.getAllFunctions().forEach((functionName) => {
      const functionObject = this.serverless.service.getFunction(functionName);
      const logGroupLogicalId = this.provider.naming
        .getLogGroupLogicalId(functionName);
      const newLogGroup = {
        [logGroupLogicalId]: {
          Type: 'AWS::Logs::LogGroup',
          Properties: {
            LogGroupName: this.provider.naming.getLogGroupName(functionObject.name),
          },
        },
      };
      _.merge(this.serverless.service.provider.compiledCloudFormationTemplate.Resources,
        newLogGroup);
});

might come between:

// resolve early if no functions are provided
// HEREEEEEE
// resolve early if provider level role is provided

@nicka yep exactly ... what we're trying to avoid is to resolving early before getting the chance to merge the LogGroup, which must be merged regardless of the what role we're using.

Ok, I'll update the flow 👍

This is probably taken from the legacy policy json file, but do you see any reason why we have two seperate statements? we can just have 1 statement for all 3 permissions (CreateLogGroup, CreateLogStream & PutLogEvents), since after all they're all affecting the same resources/functions. Actually, I don't think we need the logs:CreateLogGroup permission anymore since we're now explicitly creating it via CF.

This has probably should've been done earlier and not really critical, but do you see any downside to that? if not let's make those changes 😊

Makes total sense to update, especially if were ALWAYS creating the log groups. Will only keep CreateLogStream and PutLogEvents then.

Might still need two because of /* within IAM. What do you think?

aaah right I think you're right! 😊

Sorry for the delay, switched MacBook's etc. Will work on the updates.

@eahefnawy using two was important because the use of star would give creation rights to any log group with the same prefix. but as you note, it is legacy that can be removed now that explicit log group creation happens.

There's a bit more paranoia not in there though... Here is our latest:

                - Effect: Allow
                  Action:
                    - logs:CreateLogStream
                  Resource:
                    - Fn::GetAtt:
                      - TheLogGroup
                      - Arn
                - Effect: Allow
                  Action:
                    - logs:PutLogEvents
                  Resource:
                    - Fn::Join:
                      - ":"
                      - - Fn::GetAtt:
                          - TheLogGroup
                          - Arn
                        - "*"

This way, the lambda can't create log streams in any similarly-prefixed-but-not-the-log-group-that-has-been-created while still being able to create in the created log group and then write to any such log stream.

small surface space but since we're choosing this for all users and we can do it once and be done... seems worth the energy.

and... pretty sure it's my fault it's not already correct. sorry for that