CloudWatch Logs: new library #307
Conversation
| @@ -212,6 +218,22 @@ export abstract class LambdaRef extends Construct implements IEventRuleTarget { | |||
| return this.metric('Throttles', { statistic: 'sum', ...props }); | |||
| } | |||
|
|
|||
| public get subscriptionDestinationProps(): SubscriptionDestinationProps { | |||
There was a problem hiding this comment.
How about we standardize on this idiom for this type of pattern where a construct can be used within a "mini-framework" (CloudWatch events, bucket notifications, alarm actions...):
// the Ixxx represents the interface that constructs need to implement
interface ISubscriptionDestination {
// the property (or method) has the same name
// the return type has the same name without "I" and represents the information needed in order to participate in the framework
readonly subscriptionDestination: SubscriptionDestination;
}| @@ -212,6 +218,22 @@ export abstract class LambdaRef extends Construct implements IEventRuleTarget { | |||
| return this.metric('Throttles', { statistic: 'sum', ...props }); | |||
| } | |||
|
|
|||
| public get subscriptionDestinationProps(): SubscriptionDestinationProps { | |||
| if (!this.logSubscriptionDestinationPolicyAdded) { | |||
| // FIXME: this limits to the same region, which shouldn't really be an issue. | |||
There was a problem hiding this comment.
Not before we know it's actually an issue. Should have been NOTE instead of FIXME, probably.
| // | ||
| // Whitelisting the whole of CWL is not as secure as the example in | ||
| // https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/SubscriptionFilters.html#LambdaFunctionExample | ||
| // (which also limits on source ARN) but this is far simpler and we trust CloudWatch Logs. |
There was a problem hiding this comment.
Should we get a security review for this?
There was a problem hiding this comment.
Hmm. Thinking about it a bit more, you're right, this is actually not good enough.
This allows attackers to send arbitrary log data to our Lambda from their own log groups.
| }); | ||
| this.logSubscriptionDestinationPolicyAdded = true; | ||
| } | ||
| return new SubscriptionDestinationProps(this.functionArn); |
There was a problem hiding this comment.
SubscriptionDestinationProps should be an interface (and called SubscriptionDestination)
| @@ -0,0 +1,39 @@ | |||
| import { expect, haveResource } from '@aws-cdk/assert'; | |||
| import { Stack } from '@aws-cdk/core'; | |||
| import { LogGroup, LogPattern, SubscriptionFilter } from '@aws-cdk/logs'; | |||
There was a problem hiding this comment.
Let's start moving towards import * as logs from '@aws-cdk/logs'. It will make the code much more readable
There was a problem hiding this comment.
Oh, honestly I prefer the unscoped imports.
There was a problem hiding this comment.
Why? I think the scoping make the code much more readable
There was a problem hiding this comment.
It’s also more idiomatic in node.js
There was a problem hiding this comment.
Only for resource libraries? Or also for @aws-cdk/core?
As in:
export class MyConstruct extends core.Construct {
consructor(parent: core.Construct, id: string, props: MyConstructProps) {
/// etc
}
}There was a problem hiding this comment.
Let’s use “cdk” for core (we are going to rename it to @aws-cdk/cdk soon)
| /** | ||
| * Properties returned by a Subscription destination | ||
| */ | ||
| export class SubscriptionDestinationProps { |
| /** | ||
| * The log group to create the subscription on. | ||
| */ | ||
| logGroup: LogGroup; |
There was a problem hiding this comment.
Would be useful to have logGroup.newFilter(parent, id, props) for discoverability
There was a problem hiding this comment.
Maybe. All those methods do introduce bidirectional dependencies between modules though (that TypeScript lets us get away with but aren't very nice).
There was a problem hiding this comment.
This is not between modules, it's between files in the same module, which is absolutely not an issue.
| /** | ||
| * Log events matching this pattern will be sent to the destination. | ||
| */ | ||
| logPattern: ILogPattern; |
There was a problem hiding this comment.
Should be named filterPattern (this is the pattern of the filter), does it make sense to default to allTerms? I guess not really...
There was a problem hiding this comment.
I explicitly called it logPattern so the name matches the class with the factory functions--it's easy to remember how to find it that way.
Change that class into FilterPattern as well then?
There was a problem hiding this comment.
I guess, these are the semantics of the service
There was a problem hiding this comment.
Also, I picked the original name "log pattern" because it's a pattern for what is found inside your logs.
| * | ||
| * A Kinesis stream in the same account can be subscribed directly. | ||
| */ | ||
| export class Destination extends Construct implements ISubscriptionDestination { |
There was a problem hiding this comment.
Horrible name, especially given we already have SubscriptionDestination. I wonder if this should be an abstract class and then you can create KinesisLogDestination in the kinesis L2 (I think we have one :-))
There was a problem hiding this comment.
Maybe. At the same time, the AWS::Logs::Destination resource needs to go somewhere, and it's only currently restricted to Kinesis streams. Conceivably, more resources might be addable as destinations later on.
There was a problem hiding this comment.
That's why I suggested that it will be abstract (because that's the intent of this abstract name)
| /** | ||
| * Interface for classes that can be the target of a Log Destination | ||
| */ | ||
| export interface ILogDestinationTarget { |
There was a problem hiding this comment.
It will be used by the Kinesis library once written.
Add a new library supporting CloudWatch Logs. Lambdas gain a feature so they can be used as a log subscription destination.
Implement ISubscriptionDestination for Kinesis Stream.
rix0rrr
force-pushed
the
huijbers/cloudwatch-logs
branch
from
f276c0b to
8ad77fe
Compare
| @@ -159,6 +170,45 @@ export abstract class StreamRef extends Construct { | |||
| ); | |||
| } | |||
|
|
|||
| public subscriptionDestination(sourceLogGroup: logs.LogGroup): logs.SubscriptionDestination { | |||
There was a problem hiding this comment.
I think this should be called logSubscriptionDestination (and ILogSubscriptionDestination, LogSubscriptionDescription), or maybe just logDestination or logSubscription? The word "log" is needed here because this is a method of kinesis.Stream, and people will wonder "what does stream.subscriptionDestination mean?"... We need to contextualize this to logs somehow.
| if (!this.cloudWatchLogsRole) { | ||
| // Create a role to be assumed by CWL that can write to this stream and pass itself. | ||
| this.cloudWatchLogsRole = new Role(this, 'CloudWatchLogsCanPutRecords', { | ||
| assumedBy: new ServicePrincipal(new FnSub('logs.${AWS::Region}.amazonaws.com')), |
There was a problem hiding this comment.
Should we provide a more high-level API for devising service principals? (i.e. ServicePrincipal.regional("logs"))
There was a problem hiding this comment.
Also, for some reason, I think we shouldn't use FnSub (can't remember, but I recall we it had some potential pitfalls -- sadly I don't remember what were they). Maybe just use FnConcat to help me sleep at night.
There was a problem hiding this comment.
Should we provide a more high-level API for devising service principals?
Meh. CWL is the exception here, all other service principal are region-agnostic (as they should be, I feel).
Also, for some reason, I think we shouldn't use FnSub
This is very unsatisfying :(
| // to assume we don't need to do anything special. | ||
| const sameAccount = sourceStack.env.account === thisStack.env.account; | ||
|
|
||
| if (sameAccount) { |
There was a problem hiding this comment.
Just to clarify (and maybe worth a comment in the code): will this Just Work across regions?
There was a problem hiding this comment.
It will never work across regions, I don't think. There are very few things that work across regions.
In fact, this code will also not work today--but it will magically start working once we implement cross-stack references!
| // The destination lives in the target account | ||
| const dest = new logs.CrossAccountDestination(this, 'CloudWatchCrossAccountDestination', { | ||
| // Unfortunately destinationName is required so we have to invent one that won't conflict. | ||
| destinationName: new FnConcat(sourceLogGroup.logGroupName, 'To', this.streamName) as any, |
There was a problem hiding this comment.
Perhaps use the stackname+logicalId?
There was a problem hiding this comment.
Come to think of it, this already won't work. The CrossAccountDestination should have a dynamic id as well, because the same stream could be the target for multiple log groups.
| }); | ||
| dest.addToPolicy(new PolicyStatement() | ||
| .addAction('logs:PutSubscriptionFilter') | ||
| .addAwsAccountPrincipal(sourceStack.env.account) |
There was a problem hiding this comment.
What happens if account is undefined?
There was a problem hiding this comment.
Then we assume both accounts will be undefined and the if above will have kicked in.
If one is undefined and the other is not, we're very much SOL.
|
|
||
| let retentionInDays = props.retentionDays; | ||
| if (retentionInDays === undefined) { retentionInDays = 730; } | ||
| if (retentionInDays === Infinity) { retentionInDays = undefined; } |
There was a problem hiding this comment.
How does this translate across jsii?
There was a problem hiding this comment.
All doubles in all languages can represent Infinity, so I had assumed it would Just Work(tm). But I can check.
| } | ||
|
|
||
| /** | ||
| * Create a new Log Stream for this Log Group |
| } | ||
|
|
||
| /** | ||
| * Create a new Subscription Filter on this Log Group |
| /** | ||
| * Interface for classes that can be the destination of a log Subscription | ||
| */ | ||
| export interface ISubscriptionDestination { |
There was a problem hiding this comment.
I think this should be ILogDestination
There was a problem hiding this comment.
It's the destination for a SubscriptionFilter. But I can live with LogSubscriptionDestination.
packages/@aws-cdk/logs/README.md
Outdated
| or a Kinesis stream. | ||
|
|
||
| * If the Kinesis stream lives in a different account, you have to also create a | ||
| `Destination` object in the current account which will act as a proxy for the |
- Rename SubscriptionDestionation->LogSubscriptionDestination. - Introducing Arn.parseToken() - Use FnConcat instead of FnSub to build a region-aware service principal - Add an additional safeguard in the cross-account subscription generation for if one account is unset. Don't forget to mention this fixes #174.
- Provide an example of ARNs that don't have a resourceName. - Turn order of if clauses around.
I meant about the region checking. I agree completely about the early exits (though it's arguable whether this case counts as an early exit. It's definitely not an "abnormal" case. But I'm not going to fight you on that) |
Add a new library supporting CloudWatch Logs. Lambdas gain a feature
so they can be used as a log subscription destination.
By submitting this pull request, I confirm that my contribution is made under
the terms of the beta license.