-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
feat(ec2): add vpn metrics #1979
Conversation
Add VPN metrics (TunnelState, TunnelDataIn, TunnelDataOut) across all tunnels and per connection by adding a new augmentation. Adapt AugmentationGenerator to convert resource name to Kebab case module name and to support name overrides for class, interface and module when these cannot be directly derived from the CloudFormation resource name: no base class or resource name not really Pascal case (e.g. VPNConnection).
packages/@aws-cdk/aws-ec2/README.md
Outdated
@@ -341,3 +341,5 @@ vpc.addVpnConnection('Dynamic', { | |||
``` | |||
|
|||
Routes will be propagated on the route tables associated with the private subnets. | |||
|
|||
VPN connections expose metric across all tunnels and per connection. |
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.
example/snippet on how to use?
packages/@aws-cdk/aws-ec2/lib/vpn.ts
Outdated
@@ -98,6 +99,44 @@ export enum VpnConnectionType { | |||
} | |||
|
|||
export class VpnConnection extends cdk.Construct implements IVpnConnection { | |||
/** | |||
* Return the given named metric for all VPN connections. |
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.
"all VPC connections in the account/region"
@@ -0,0 +1,27 @@ | |||
{ | |||
"overrides": { |
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.
I wouldn't call these "overrides". We should think about them simply as configuration of the augmentation system that has some default values, so you don't have to supply them but you can, and I would avoid attaching any heuristics to them (i.e. "Base" suffix).
So, maybe:
{
"baseClass": "VpnConnectionBase",
"baseClassFile": "vpn",
"interface": "IVpnConnection"
}
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.
OK, will rename these. The current implementation doesn't attach anything to these overrides.
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 does:
baseClass
is the resource name + "Base" suffixbaseClassFile
is thel2-base
interface
isI
+ resource name
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.
Not sure I'm following here.
const baseClassName = (overrides && overrides.class) || l2ClassName + 'Base';
const interfaceName = (overrides && overrides.interface) || 'I' + l2ClassName;
const baseClassModule = `./${(overrides && overrides.module) || `${kebabL2ClassName}-base`}`;
From above, if there's an override it's taken as is, no?
Regarding the names in the JSON file:
baseClass
orclass
? for theVpnConnection
it's not a base class because there's noVpnConnectionBase
in this case...file
or...module
?
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.
Oh, yes, you are right. I guess class
makes sense (and the doc should say that by default it uses the "Base" class). I would go with file
and not module
because we mostly (in our project) "module" is interchangeable with "package" (or "npm module").
overrides?: ResourceOverrides; | ||
} | ||
|
||
export interface ResourceOverrides { |
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.
rename to AugmentationOptions
and line up with the schema above, specify the @defaults
Add VPN metrics (TunnelState, TunnelDataIn, TunnelDataOut) across all tunnels
and per connection by adding a new augmentation.
Adapt AugmentationGenerator to convert resource name to Kebab case class file
and to support options to specify class file, class and interface when these cannot
be directly derived from the CloudFormation resource name: no base class or
resource name not really Pascal case (e.g. VPNConnection).
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.