Add a factory interface for constructing a CloudWatch client

[jennings]

I would like to add an optional IAmazonCloudWatchLogsProvider to the AmazonCloudWatch extension method. This interface would have the signature:

public interface IAmazonCloudWatchLogsProvider
{
    Amazon.CloudWatchLogs.IAmazonCloudWatchLogs GetClient();
}

When the sink is configured, it will store the client returned by provider.GetClient() for later use. If no provider is given, the sink will simply create a new client with new AmazonCloudWatchLogsClient() as it does today.

This would not need to be added to CloudWatchSinkOptions. In that case, the sink is being configured from code and the app can pass in whatever IAmazonCloudWatchLogs it wants to use.

This provider would be useful in this scenario:

• The app configures the Amazon CloudWatch client via code
• The app configures Serilog via Serilog.Settings.Configuration.

In our scenario, we do not store our AWS credentials in any of the locations on disk that the .NET library can discover automatically, the only place we have them is in appsettings.json. This is where all of our apps' configurations live, we try not to spread configuration all over the place.

@wparad mentioned that one extension method accepts IAmazonCloudWatchLogs and we could implement that with a wrapper. However, this method isn't usable from Serilog.Settings.Configuration (because it takes a CloudWatchSinkOptions object). And, such a wrapper would have to implement a large interface where every every implementation is boilerplate: return _base.TheSameMethod(). Or you could derive from AmazonCloudWatchLogsClient to avoid the boilerplate, but that doesn't really model what we're trying to do. It can be done, but I think a factory does this in a much more ergonomic way.

Without this interface, I think our next-best option is to start putting the AWS client configuration in App.config. This means we'll have two places to look for configuration, and we'll have to modify our deployment scripts so they copy the right values into this location. This isn't the worst thing in the world, but a provider would make this unnecessary for us.

[thoean]

@jennings - thank you for your thorough explanation of what you intend to achieve.

While I recognize your desire, I think there are already a lot of configuration options available for the AWS .NET SDK. I want to avoid having this, and potentially other AWS SDK features get into this library over time.

Let me phrase it differently: Why do you really want to configure it like you described? I'd argue:

• For local development, you want to use the SDK Store, or even simpler just an entry in the profile.
• For production use, you want to rely on IAM Instance Profiles.

In none of those cases you need any secrets stored on disk or source control etc.

I want to ensure we're providing a feature that helps people make good choices, instead of providing workarounds for choices that aren't ideal.

That said, can you explain your choices of putting credentials into a file in your repository? I might miss a point, but it feels highly insecure. Thanks for elaborating!

[jennings]

Thanks @thoean for discussing this with me.

Today, our credentials aren't checked into source control with the app, they're actually in a separate appsettings.<environment>.json that our deployment scripts generate.

We are avoiding using IAM instance profiles because we don't want anyone on the machine to inherit the permissions. We're in the middle of a project to migrate the IAM credentials to HashiCorp Vault. When we do that, the AWS SDK definitely won't be able to find its own credentials.

I agree with wanting an API to help people make good choices. I think making the provider optional makes it easy to fall into the pit of success, because most people won't implement the interface if they don't have to. And if someone really wants to shoot themselves in the foot, they already can do that by using the AmazonCloudWatch(CloudWatchSinkOptions, IAmazonCloudWatchLogs) overload.

[wparad]

Let's do this. We'll leave this issue open, makes sense to. And we'll wait for more feedback from others. I'm hesitant to rush in, and solve this problem suboptimally and force another major upgrade.

There are at least a couple of workarounds, which totally suck granted, but at this point, I'm not sure about offering a solution which is a join of appsettings and code. A solution in this space really must support an either or and not require both. I look forward to others offering suggestions on how they solve this particular issue.

(For completeness i'll reference the Readme which explains the AWS suggestion for credentials management in C#.

[jennings]

I suppose another way would be to accept an IAmazonCloudWatchLogs and use the static property syntax to assign it:

{
  "Serilog": {
    "WriteTo": [
      {
        "Name": "AmazonCloudWatch",
        "Args": {
          "cloudWatchClient": "CoolStuff.MyContainingType::MyCloudWatchLogsClient, CoolStuff.Assembly"
        }
      }
    ]
  }
}

That property could construct a client however it wanted.

This would still require the parameter be added to a configuration method, but it wouldn't require an extra interface be defined.

[thoean]

Interesting. I find it interesting that this type of configuration is preferred over a 1-liner in code.

What do you think of @wparad's suggestion in https://github.com/Cimpress-MCP/serilog-sinks-awscloudwatch/pull/68, notably the new extension methods in AwsCloudWatchConfigurationExtension.cs? It would have only 2 extensions to configure the sink, one with a client to inject, and the other one constructing the client in the library.

I want to ensure a version upgrade would work for your needs, and we can provide a well-documented solution for using the serilog-configuration package.

[jennings]

After experimenting a bit, we discovered we can just define our own extension methods that Serilog.Settings.Configuration will find. This means we can construct our own AWS client and then delegate to the "real" AmazonCloudWatch(CloudWatchSinkOptions, IAmazonCloudWatchLogs) method. So we can implement what we want with no changes to this library.

I still think this would be a nice feature for those who want it, but if it you feel this is a poor fit for your library, I understand and wouldn't mind closing this ticket.

To answer @thoean's question: I do think there are more extension methods than necessary. I agree we can probably get away with only three (the third one seems necessary so non-code sources can specify either a constant string or ILogStreamNameProvider):

// This one is easiest to use from code
public static LoggerConfiguration AmazonCloudWatch(
    this LoggerSinkConfiguration loggerConfiguration,
    CloudWatchSinkOptions cloudWatchSinkOptions,
    IAmazonCloudWatchLogs cloudWatchClient);

// This one is used from non-code config sources, and allows specifying a logStreamNameProvider
public static LoggerConfiguration AmazonCloudWatch(
    this LoggerSinkConfiguration loggerConfiguration,
    string logGroupName,
    ILogStreamNameProvider logStreamNameProvider = null,
    ITextFormatter textFormatter = null,
    LogEventLevel minimumLogEventLevel = CloudWatchSinkOptions.DefaultMinimumLogEventLevel,
    int batchSizeLimit = CloudWatchSinkOptions.DefaultBatchSizeLimit,
    TimeSpan? period = null,
    byte retryAttempts = CloudWatchSinkOptions.DefaultRetryAttempts,
    bool createLogGroup = CloudWatchSinkOptions.DefaultCreateLogGroup);

// This one is used from non-code config sources, and allows specifying a constant logStreamName
public static LoggerConfiguration AmazonCloudWatch(
    this LoggerSinkConfiguration loggerConfiguration,
    string logGroupName,
    string logStreamName,
    ITextFormatter textFormatter = null,
    LogEventLevel minimumLogEventLevel = CloudWatchSinkOptions.DefaultMinimumLogEventLevel,
    int batchSizeLimit = CloudWatchSinkOptions.DefaultBatchSizeLimit,
    TimeSpan? period = null,
    byte retryAttempts = CloudWatchSinkOptions.DefaultRetryAttempts,
    bool createLogGroup = CloudWatchSinkOptions.DefaultCreateLogGroup);

I have a branch where I started doing this to see what it would look like. I think I remember seeing @wparad also doing this in another branch somewhere.

[thoean]

Thank you so much!

I think it'd be worthwhile adding the code to get form configuration to a client to the readme file. Can you share it (or update the readme, whatever you prefer).

Yes, Warren started simplifying it here: https://github.com/Cimpress-MCP/serilog-sinks-awscloudwatch/pull/68

[jennings]

@thoean I can do that. Just so I understand, you want the solution we used to pass in a custom Amazon client while using a config file?

[thoean]

I think we're good now. Warren simplified the whole set of extension methods to a single one, and improved the readme a lot. See https://github.com/Cimpress-MCP/serilog-sinks-awscloudwatch/pull/68 (which is now in master).

It's a major version bump (v4.0.119). Your feedback is highly appreciated.