-
Notifications
You must be signed in to change notification settings - Fork 91
feat(cloud-assembly-schema): add policy validation report schema types #1515
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
Changes from 1 commit
bf2cb86
45a738d
f4de61c
3f06a2d
07add62
71d7d2c
d39ba6d
7f24a6a
e0951ff
46b3a44
85fd946
ea25ab9
53b27b3
56c5fc3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,217 @@ | ||||||
| /** | ||||||
| * JSON schema for policy-validation-report.json | ||||||
| * | ||||||
| * This file is written to the cloud assembly directory by aws-cdk-lib | ||||||
| * during synthesis and consumed by the CDK CLI's validate command. | ||||||
| */ | ||||||
|
|
||||||
| /** | ||||||
| * The top-level structure of the policy validation report file. | ||||||
| */ | ||||||
| export interface PolicyValidationReportJson { | ||||||
| /** | ||||||
| * Report title. | ||||||
| */ | ||||||
| readonly title: string; | ||||||
|
|
||||||
| /** | ||||||
| * Reports from all validation plugins that ran during synthesis. | ||||||
| */ | ||||||
| readonly pluginReports: PluginReportJson[]; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * A report from a single validation plugin. | ||||||
| */ | ||||||
| export interface PluginReportJson { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we not report on suppressions in here as well? |
||||||
| /** | ||||||
| * Version of the plugin that produced this report. | ||||||
| * | ||||||
| * @default - no version | ||||||
| */ | ||||||
| readonly version?: string; | ||||||
|
|
||||||
| /** | ||||||
| * Summary of the plugin's validation run. | ||||||
| */ | ||||||
| readonly summary: PolicyValidationReportSummary; | ||||||
|
|
||||||
| /** | ||||||
| * Violations found by this plugin. | ||||||
| */ | ||||||
| readonly violations: PolicyViolationJson[]; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Summary of a plugin's validation run. | ||||||
| */ | ||||||
| export interface PolicyValidationReportSummary { | ||||||
| /** | ||||||
| * The name of the plugin that produced this report. | ||||||
| */ | ||||||
| readonly pluginName: string; | ||||||
|
|
||||||
| /** | ||||||
| * Whether the plugin's validation passed or failed. | ||||||
| */ | ||||||
| readonly status: PolicyValidationReportStatus; | ||||||
|
|
||||||
| /** | ||||||
| * Additional plugin-specific metadata. | ||||||
| * | ||||||
| * @default - no metadata | ||||||
| */ | ||||||
| readonly metadata?: { readonly [key: string]: string }; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * The final status of a validation report. | ||||||
| */ | ||||||
| export type PolicyValidationReportStatus = 'success' | 'failure'; | ||||||
|
|
||||||
| /** | ||||||
| * A single policy violation found by a validation plugin. | ||||||
| */ | ||||||
| export interface PolicyViolationJson { | ||||||
| /** | ||||||
| * The name of the rule that was violated. | ||||||
| */ | ||||||
| readonly ruleName: string; | ||||||
|
|
||||||
| /** | ||||||
| * A description of the violation. | ||||||
| */ | ||||||
| readonly description: string; | ||||||
|
|
||||||
| /** | ||||||
| * How to fix the violation. | ||||||
| * | ||||||
| * @default - no fix provided | ||||||
| */ | ||||||
| readonly fix?: string; | ||||||
|
|
||||||
| /** | ||||||
| * The severity of the violation. | ||||||
| * | ||||||
| * @default - no severity | ||||||
| */ | ||||||
| readonly severity?: string; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No enumeration here? That means there will not be an interpretation to any of the values here? No colorization? No sorting?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this will all have to be done in a bespoke way, as in we'll have to match otherwise we can't fit existing plugins into the system
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You could have |
||||||
|
|
||||||
| /** | ||||||
| * Additional rule-specific metadata. | ||||||
| * | ||||||
| * @default - no metadata | ||||||
| */ | ||||||
| readonly ruleMetadata?: { readonly [key: string]: string }; | ||||||
|
|
||||||
| /** | ||||||
| * Resources that violated the rule. | ||||||
| */ | ||||||
| readonly violatingResources: ViolatingResourceJson[]; | ||||||
|
|
||||||
| /** | ||||||
| * Constructs that violated the rule. | ||||||
| */ | ||||||
| readonly violatingConstructs: ViolatingConstructJson[]; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does that mean violating resources will never have a construct path?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. violatingResources: violation.violatingResources,
violatingConstructs: violation.violatingResources.map(resource => {
// Use constructPath from the input if provided (e.g. annotations),
// otherwise derive it from the logical ID via the construct tree.
const constructPath = resource.constructPath ?? (
resource.templatePath && resource.resourceLogicalId
? this.tree.getConstructByLogicalId(
path.basename(resource.templatePath),
resource.resourceLogicalId,
)?.node.path
: undefined
);right now the report maps resources to constructs so they will have construct path if avail
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the use though? Why not combine these?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. export interface ValidationViolatingConstruct extends report.PolicyViolatingResource {
/**
* The construct path as defined in the application.
*
* @default - construct path will be empty if the cli is not run with `--debug`
*/
readonly constructPath?: string;
/**
* A stack of constructs that lead to the violation.
*
* @default - stack will be empty if the cli is not run with `--debug`
*/
readonly constructStack?: ConstructTrace;
}they should be combined in the schema, but changing this breaks the existing schema and I'd rather not unless we have to. my thoughts are that we already have some sort of report, and if it can be repurposed into what we need, even if there's some redundancy, that's better than creating a new-but-slightly-different report that we then have to explain migration path or hide behind a feature flag.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. eh, i'll remove it |
||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * A resource that violated a policy rule. | ||||||
| */ | ||||||
| export interface ViolatingResourceJson { | ||||||
| /** | ||||||
| * The logical ID of the resource in the CloudFormation template. | ||||||
| */ | ||||||
| readonly resourceLogicalId: string; | ||||||
|
|
||||||
| /** | ||||||
| * The path to the CloudFormation template containing this resource. | ||||||
| */ | ||||||
| readonly templatePath: string; | ||||||
|
|
||||||
| /** | ||||||
| * Locations within the template that pose violations. | ||||||
| * | ||||||
| * @default - no locations | ||||||
| */ | ||||||
| readonly locations?: string[]; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * A construct that violated a policy rule. | ||||||
| */ | ||||||
| export interface ViolatingConstructJson { | ||||||
| /** | ||||||
| * The construct path as defined in the application. | ||||||
| * | ||||||
| * @default - no construct path | ||||||
| */ | ||||||
| readonly constructPath?: string; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would we ever not have a construct path? |
||||||
|
|
||||||
| /** | ||||||
| * The construct creation stack trace. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a lot of data in that struct that is not a stack trace though. |
||||||
| * | ||||||
| * @default - no stack trace | ||||||
| */ | ||||||
| readonly constructStack?: ConstructTraceJson; | ||||||
|
|
||||||
| /** | ||||||
| * Locations within the construct where the violation was detected. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the formalism here? Property names? JSON paths? What? |
||||||
| * | ||||||
| * @default - no locations | ||||||
| */ | ||||||
| readonly locations?: string[]; | ||||||
|
|
||||||
| /** | ||||||
| * The logical ID of the resource in the CloudFormation template. | ||||||
| */ | ||||||
| readonly resourceLogicalId: string; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needs to be optional if this is about constructs. (In fact: how do we have optional construct PATHS, but definitely logical IDs?) |
||||||
|
|
||||||
| /** | ||||||
| * The path to the CloudFormation template containing this resource. | ||||||
| */ | ||||||
| readonly templatePath: string; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, this cannot be required. |
||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * A node in the construct creation stack trace. | ||||||
| */ | ||||||
| export interface ConstructTraceJson { | ||||||
| /** | ||||||
| * The construct ID. | ||||||
| */ | ||||||
| readonly id: string; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ID is unnecessary, it's the last part of teh path. |
||||||
|
|
||||||
| /** | ||||||
| * The construct path. | ||||||
| */ | ||||||
| readonly path: string; | ||||||
|
|
||||||
| /** | ||||||
| * The child node in the trace (towards the leaf). | ||||||
| * | ||||||
| * @default - this is the leaf node | ||||||
| */ | ||||||
| readonly child?: ConstructTraceJson; | ||||||
|
|
||||||
| /** | ||||||
| * The fully qualified name of the construct class. | ||||||
| * | ||||||
| * @default - no construct info | ||||||
| */ | ||||||
| readonly construct?: string; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
|
||||||
| /** | ||||||
| * The version of the library that contains this construct. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Library name and version? Or just version? |
||||||
| * | ||||||
| * @default - no version info | ||||||
| */ | ||||||
| readonly libraryVersion?: string; | ||||||
|
|
||||||
| /** | ||||||
| * The source code location where this construct was created. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mean stack trace? |
||||||
| * | ||||||
| * @default - no location info | ||||||
| */ | ||||||
| readonly location?: string; | ||||||
| } | ||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Required?
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.
It was previously required, I don't think it's that problematic though
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.
But what is the "single title" of the entire report? Isn't it always going to be a constant?