Skip to content

Claude.ai #39

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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
139 changes: 139 additions & 0 deletions packages/aws-cdk-lib/aws-elasticache/lib/elasticache-cluster.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,139 @@
import * as cdk from 'aws-cdk-lib';
import * as elasticache from 'aws-cdk-lib/aws-elasticache';
import * as ec2 from 'aws-cdk-lib/aws-ec2';
Comment on lines +1 to +3
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for the constructs defined in cdk-lib, we do not import it from the dependencies, but instead we depend on the modules defined in the project. For example,
import * as ec2 from '../../aws-ec2';

import { Construct } from 'constructs';

export interface ElastiCacheClusterProps {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are many missing properties:

  • clusterName
  • engine (mandatory)
  • subnetGroup
  • autoMinorVersionUpgrade
  • snapshotRetentionLimit
  • snapshotWindow
  • SnapshotName
  • PreferredAvailabilityZones
  • IpDiscovery

/**
* The VPC where the ElastiCache cluster will be deployed
*/
readonly vpc: ec2.IVpc;

/**
* The cache engine version to use
* @default - 6.x (Redis)
*/
readonly engineVersion?: string;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this property has a relation with the missed property engine .. may be the suggestion here is to replace the 2 properties (engine and version) with this property, but this is not clear from comment, and also IMOP, it should accept a limited list of values, so I believe it should be enum.


/**
* The node type for the ElastiCache cluster
* @default - cache.t3.micro
*/
readonly nodeType?: string;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is a list of allowed values, so it should be of enum type.


/**
* Number of cache nodes in the cluster
* @default 1
*/
readonly numCacheNodes?: number;

/**
* The name of the cache parameter group to associate with this cache cluster
*/
readonly parameterGroupFamily?: string;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems it is referring to this property. Since there is a list of allowed values, so it should be of enum type.


/**
* Security groups to attach to the cluster
* @default - A new security group is created
*/
readonly securityGroups?: ec2.ISecurityGroup[];
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are 2 properties that I found them similar to this property (CacheSecurityGroupNames, VpcSecurityGroupIds), so I am not sure which one it is referring to.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, not sure why this property was created as list, although in the code it uses only the first element


/**
* The weekly time range during which maintenance updates can occur.
* @default - No preference
*/
readonly preferredMaintenanceWindow?: string;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there is an allowed format for this property, in CDK we usually create a new Type that simplify it. For example this property can of a new Type which is:

interface DayTime {
     day Enum;
     time number; // we can add a validation that it is between 0-23
}
interface MaintenanceWindow {
      startMaintenanceWindow DayTime;
      endMaintenanceWindow DayTime; // we can add a validation that the end maintenance window is after the start
}


/**
* Specifies the weekly time range during which maintenance
* on the cache cluster is performed
*/
readonly port?: number;
}

export class ElastiCacheCluster extends Construct {
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this construct is missing the construct interface. You can find more details here

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This construct also some of the important patterns we usually have in CDK:

  • Imports, which means to have static methods that customer can use them to create an object from this construct for a resource that already exist in their account. Check here from more details
  • Grants, which are the methods that can grant specific role or use to do some actions on the ElastiCacheCluster. Check here for more details
  • Since this construct use the EC2 security groups, so it should implement Ec2 Connections API. Check here for details

/**
* The underlying ElastiCache cluster resource
*/
public readonly cluster: elasticache.CfnCacheCluster;

/**
* The security group associated with the cluster
*/
public readonly securityGroup: ec2.ISecurityGroup;

/**
* The subnet group where the cluster is placed
*/
public readonly subnetGroup: elasticache.CfnSubnetGroup;

constructor(scope: Construct, id: string, props: ElastiCacheClusterProps) {
super(scope, id);

// Create a subnet group for the cluster
this.subnetGroup = new elasticache.CfnSubnetGroup(this, 'SubnetGroup', {
description: 'Subnet group for ElastiCache cluster',
subnetIds: props.vpc.privateSubnets.map(subnet => subnet.subnetId),
});

// Create or use provided security group
if (props.securityGroups && props.securityGroups.length > 0) {
this.securityGroup = props.securityGroups[0];
} else {
this.securityGroup = new ec2.SecurityGroup(this, 'SecurityGroup', {
vpc: props.vpc,
description: 'Security group for ElastiCache cluster',
allowAllOutbound: true,
});
}

// Create parameter group if family is specified
let parameterGroup;
if (props.parameterGroupFamily) {
parameterGroup = new elasticache.CfnParameterGroup(this, 'ParameterGroup', {
cacheParameterGroupFamily: props.parameterGroupFamily,
description: 'Parameter group for ElastiCache cluster',
});
}

// Create the ElastiCache cluster
this.cluster = new elasticache.CfnCacheCluster(this, 'Resource', {
engine: 'redis',
Copy link
Owner Author

@moelasmar moelasmar Oct 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it totally ignore all other 2 engines

cacheNodeType: props.nodeType ?? 'cache.t3.micro',
numCacheNodes: props.numCacheNodes ?? 1,
Comment on lines +102 to +103
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these default values are not best practices, or at least in the service console, we suggest 3 predefined types of Clusters (Production, Dev/test, demo), and each type has some default values. We should replicate the same concept in CDK.

engineVersion: props.engineVersion ?? '6.x',
vpcSecurityGroupIds: [this.securityGroup.securityGroupId],
cacheSubnetGroupName: this.subnetGroup.ref,
...(parameterGroup && { cacheParameterGroupName: parameterGroup.ref }),
...(props.preferredMaintenanceWindow && {
preferredMaintenanceWindow: props.preferredMaintenanceWindow,
}),
...(props.port && { port: props.port }),
});
}

/**
* Allow incoming connections on the default Redis port
*/
public allowIngressFrom(peer: ec2.IPeer, port?: number) {
this.securityGroup.addIngressRule(
peer,
ec2.Port.tcp(port ?? 6379),
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it added the default port here, but it did not use it while creating the CfnCacheCluster resource

'Allow inbound Redis traffic'
);
}

/**
* Returns the cluster endpoint address
*/
public get clusterEndpoint(): string {
return this.cluster.attrRedisEndpointAddress;
}

/**
* Returns the cluster endpoint port
*/
public get clusterPort(): string {
return this.cluster.attrRedisEndpointPort;
}
}
3 changes: 2 additions & 1 deletion packages/aws-cdk-lib/aws-elasticache/lib/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
export * from './elasticache.generated';
export * from './elasticache.generated';
export * from './elasticache-cluster';
Loading