-
Notifications
You must be signed in to change notification settings - Fork 19
feat: data source for vended log types #2204
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
| readonly validations?: unknown; | ||
| arnTemplate?: string; | ||
| isStateful?: boolean; | ||
| logTypes?: string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably not quite enough. Let's make this a list of a new custom type VendedLogs. See slack discussion for details on what needs to go there.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| >, | ||
| report: ProblemReport, | ||
| ) { | ||
| // clears vendedLogs property from all resources before processing - goal: ensure that logTypes and destinations are up to date and cut down on complicated deduplication code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to other ideas to handle possible duplication of logTypes (when it comes to repeated imports of data source) or removal of logTypes (if a service stops supporting a certain logType). This current implementation is a bit aggressive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You just don't need to do that at all. The DB starts out empty.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| for (const resource of db.all('resource')) { | ||
| if (resource.vendedLogs) { | ||
| delete resource.vendedLogs; | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| for (const resource of db.all('resource')) { | |
| if (resource.vendedLogs) { | |
| delete resource.vendedLogs; | |
| } | |
| } |
| /** | ||
| * What version of permissions the destination supports V1 | V2 | ||
| */ | ||
| permissionsVersion: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| permissionsVersion: string; | |
| readonly permissionsVersion: string; |
| /** | ||
| * List of the destinations the can consume those logs | ||
| */ | ||
| readonly logDestinations: LogDestination[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer to remove duplicate naming. In this context, its pretty clear we are talking about log destinations.
| readonly logDestinations: LogDestination[]; | |
| readonly destinations: LogDestination[]; |
| /** | ||
| * List of the types of logs a Cloudformation resource can produce | ||
| */ | ||
| readonly logTypes: string[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally you would consider removing the log prefix here as well (see next comment). In this case it's probably okay to keep it because A/ types is very generic (probably too generic) and B/ it's the term used here.
So I vote to keep it in this case.
| /** | ||
| * Resource type of the destination S3 | CWL | FH | XRAY | ||
| */ | ||
| readonly destinationType: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again doubling up. Here I think it's fine to remove the prefix. You could also call it something like service or not even have a separate interface.
| readonly destinationType: string; | |
| readonly type: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
currently have this as a separate interface since we may want logFormat (json, parquet, plain text, raw...) in the future and that can vary per destination service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both is fine. My usual approach is to not over-optimize for future possibilities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ended up moving this out of its separate interface since it will be easier to handle and we currently don't have plans to add logFormat or any service-specific fields that would require the separate interface
| /** | ||
| * Resource type of the destination S3 | CWL | FH | XRAY | ||
| */ | ||
| readonly destinationType: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be no reason to not strongly type this:
| readonly destinationType: string; | |
| readonly destinationType: "S3" | "CWL" | "FH" | "XRAY"; |
| let permissionValue = ''; | ||
| for (const dest of value.Destinations) { | ||
| if (permissionValue === '') { | ||
| permissionValue = dest.PermissionsVersion; | ||
| } else { | ||
| if (permissionValue !== dest.PermissionsVersion) { | ||
| report.reportFailure( | ||
| new ReportAudience('Log Source Import'), | ||
| 'interpreting', | ||
| failure.in(resourceType)( | ||
| `Resouce of type ${resourceType} has inconsistent permissions version for log of type ${value.LogType}`, | ||
| ), | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simplify this a lot along these lines
| let permissionValue = ''; | |
| for (const dest of value.Destinations) { | |
| if (permissionValue === '') { | |
| permissionValue = dest.PermissionsVersion; | |
| } else { | |
| if (permissionValue !== dest.PermissionsVersion) { | |
| report.reportFailure( | |
| new ReportAudience('Log Source Import'), | |
| 'interpreting', | |
| failure.in(resourceType)( | |
| `Resouce of type ${resourceType} has inconsistent permissions version for log of type ${value.LogType}`, | |
| ), | |
| ); | |
| } | |
| } | |
| const permissionValue = Value.Destinations[0].PermissionsVersion; | |
| if (!value.Destinations.every(v => v.PermissionsVersion === permissionValue)) { | |
| // ... report failure | |
| } |
| if (resource.vendedLogs) { | ||
| // we take whatever the newest permissions value is and assume that all logs in a resource use the same permissions | ||
| resource.vendedLogs.permissionsVersion = permissionValue; | ||
| resource.vendedLogs.logTypes.push(value.LogType); | ||
| // dedupes incoming destinations | ||
| const newDestinations = destinations.filter( | ||
| (dest) => | ||
| !resource.vendedLogs!.logDestinations.some( | ||
| (existing) => existing.destinationType === dest.destinationType, | ||
| ), | ||
| ); | ||
| resource.vendedLogs.logDestinations.push(...newDestinations); | ||
| } else { | ||
| resource.vendedLogs = { | ||
| permissionsVersion: permissionValue, | ||
| logTypes: [value.LogType], | ||
| logDestinations: destinations, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplify by flipping it around:
??= = Nullish coalescing assignment
| if (resource.vendedLogs) { | |
| // we take whatever the newest permissions value is and assume that all logs in a resource use the same permissions | |
| resource.vendedLogs.permissionsVersion = permissionValue; | |
| resource.vendedLogs.logTypes.push(value.LogType); | |
| // dedupes incoming destinations | |
| const newDestinations = destinations.filter( | |
| (dest) => | |
| !resource.vendedLogs!.logDestinations.some( | |
| (existing) => existing.destinationType === dest.destinationType, | |
| ), | |
| ); | |
| resource.vendedLogs.logDestinations.push(...newDestinations); | |
| } else { | |
| resource.vendedLogs = { | |
| permissionsVersion: permissionValue, | |
| logTypes: [value.LogType], | |
| logDestinations: destinations, | |
| }; | |
| } | |
| resource.vendedLogs ??= {}; | |
| resource.vendedLogs.logTypes ??= []; | |
| resource.vendedLogs.logDestinations ??= []; | |
| // we take whatever the newest permissions value is and assume that all logs in a resource use the same permissions | |
| resource.vendedLogs.permissionsVersion = permissionValue; | |
| // Add log types | |
| resource.vendedLogs.logTypes.push(value.LogType); | |
| // dedupes incoming destinations | |
| const newDestinations = destinations.filter( | |
| (dest) => | |
| !resource.vendedLogs!.logDestinations.some( | |
| (existing) => existing.destinationType === dest.destinationType, | |
| ), | |
| ); | |
| resource.vendedLogs.logDestinations.push(...newDestinations); |
mrgrain
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor inline comments
│ └logDestinations: [{"destinationType":"S3"}, {"destinationType":"CWL"}, {"destinationType":"FH"}]
If you keep LogDestination as an interface type, let's please simplify this diff to
│ └logDestinations: [S3, CW, FH]
If you change LogDestination to be a string (or rather S3, CW, FH, XRAY) this should happen more or less for free.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@aws-cdk/aws-service-spec: Model database diff detected |
Adds new data source to populate
logTypesattribute for Cloudformation resources. Does not add github actions to update the data source (for now).