Skip to content

Commit

Permalink
fix RDS Aurora enrollment security group tips (#47975)
Browse files Browse the repository at this point in the history
  • Loading branch information
GavinFrazar authored Oct 28, 2024
1 parent 7db59ae commit 13b3f5e
Show file tree
Hide file tree
Showing 7 changed files with 51 additions and 30 deletions.
11 changes: 6 additions & 5 deletions lib/srv/discovery/common/database.go
Original file line number Diff line number Diff line change
Expand Up @@ -466,11 +466,12 @@ func MetadataFromRDSV2Cluster(rdsCluster *rdstypes.DBCluster, rdsInstance *rdsty
Region: parsedARN.Region,
AccountID: parsedARN.AccountID,
RDS: types.RDS{
ClusterID: aws.StringValue(rdsCluster.DBClusterIdentifier),
ResourceID: aws.StringValue(rdsCluster.DbClusterResourceId),
IAMAuth: aws.BoolValue(rdsCluster.IAMDatabaseAuthenticationEnabled),
Subnets: subnets,
VPCID: vpcID,
ClusterID: aws.StringValue(rdsCluster.DBClusterIdentifier),
ResourceID: aws.StringValue(rdsCluster.DbClusterResourceId),
IAMAuth: aws.BoolValue(rdsCluster.IAMDatabaseAuthenticationEnabled),
Subnets: subnets,
VPCID: vpcID,
SecurityGroups: rdsSecurityGroupInfo(rdsCluster.VpcSecurityGroups),
},
}, nil
}
Expand Down
15 changes: 14 additions & 1 deletion lib/srv/discovery/common/database_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -572,6 +572,11 @@ func TestDatabaseFromRDSV2Cluster(t *testing.T) {
Endpoint: aws.String("localhost"),
ReaderEndpoint: aws.String("reader.host"),
Port: aws.Int32(3306),
VpcSecurityGroups: []rdsTypesV2.VpcSecurityGroupMembership{
{VpcSecurityGroupId: aws.String("")},
{VpcSecurityGroupId: aws.String("sg-1")},
{VpcSecurityGroupId: aws.String("sg-2")},
},
CustomEndpoints: []string{
"myendpoint1.cluster-custom-example.us-east-1.rds.amazonaws.com",
"myendpoint2.cluster-custom-example.us-east-1.rds.amazonaws.com",
Expand All @@ -589,6 +594,10 @@ func TestDatabaseFromRDSV2Cluster(t *testing.T) {
ClusterID: "cluster-1",
ResourceID: "resource-1",
IAMAuth: true,
SecurityGroups: []string{
"sg-1",
"sg-2",
},
},
}

Expand Down Expand Up @@ -673,7 +682,11 @@ func TestDatabaseFromRDSV2Cluster(t *testing.T) {
ResourceID: "resource-1",
IAMAuth: true,
Subnets: []string{"subnet-123", "subnet-456"},
VPCID: "vpc-123",
SecurityGroups: []string{
"sg-1",
"sg-2",
},
VPCID: "vpc-123",
},
},
})
Expand Down
2 changes: 2 additions & 0 deletions web/packages/shared/utils/text.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ test('pluralize', () => {
expect(pluralize(0, 'apple')).toBe('apples');
expect(pluralize(1, 'apple')).toBe('apple');
expect(pluralize(2, 'apple')).toBe('apples');
expect(pluralize(undefined, 'apple')).toBe('apples');
expect(pluralize(null, 'apple')).toBe('apples');
});

test('capitalizeFirstLetter', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -201,20 +201,17 @@ export const SelectSecurityGroups = ({

function withTips(
securityGroups: SecurityGroup[],
db: AwsRdsDatabase
db?: AwsRdsDatabase
): SecurityGroupWithRecommendation[] {
if (!db || !securityGroups) {
// return early without trying to add tips if there's no selected db or
// security groups given.
return securityGroups;
}
// if db is undefined, which is possible in the auto-discovery flow, we can
// still recommend security groups that allow outbound internet access.
const trustedGroups = getTrustedSecurityGroups(securityGroups, db);
return securityGroups.map(group => {
const isTrusted = trustedGroups.has(group.id);
const isOutboundAllowed = allowsOutbound(group);
return {
...group,
tips: getTips(db, isTrusted, isOutboundAllowed),
tips: getTips(isTrusted, isOutboundAllowed),
// we recommend when either are true because security group rules are
// additive, meaning they can select multiple groups for a combined effect
// of satisfying the database inbound rules and the ECS task outbound
Expand All @@ -224,15 +221,11 @@ function withTips(
});
}

function getTips(
db: AwsRdsDatabase,
isTrusted: boolean,
allowsOutbound: boolean
): string[] {
function getTips(isTrusted: boolean, allowsOutbound: boolean): string[] {
const result: string[] = [];
if (isTrusted) {
result.push(
`The inbound rules of the database ${pluralize(db.securityGroups.length, 'security group')} allow traffic from this security group`
'The database security group inbound rules allow traffic from this security group'
);
}
if (allowsOutbound) {
Expand All @@ -243,6 +236,9 @@ function getTips(

function allowsOutbound(sg: SecurityGroup): boolean {
return sg.outboundRules.some(rule => {
if (!rule) {
return false;
}
const havePorts = allowsOutboundToPorts(rule);
// this is a heuristic, because an exhaustive analysis is non-trivial.
return havePorts && rule.cidrs.some(cidr => cidr.cidr === '0.0.0.0/0');
Expand All @@ -266,29 +262,32 @@ function allowsOutboundToPorts(rule: SecurityGroupRule): boolean {

function getTrustedSecurityGroups(
securityGroups: SecurityGroup[],
db: AwsRdsDatabase
db?: AwsRdsDatabase
): Set<string> {
const trustedGroups = new Set<string>();
if (!db || !db.securityGroups || !db.uri) {
return trustedGroups;
}

const dbPort = getPort(db);
const securityGroupsById = new Map(
securityGroups.map(group => [group.id, group])
);

const dbPort = getPort(db);
const trustedGroups = new Set<string>();
db.securityGroups.forEach(groupId => {
const group = securityGroupsById.get(groupId);
if (!group) {
return;
}
group.inboundRules.forEach(rule => {
if (!rule.groups?.length) {
if (!rule.groups.length) {
// we only care about rules that reference other security groups.
return;
}
if (!ruleAllowsPort(rule, dbPort)) {
// a group is only trusted if it is trusted for the relevant port.
return;
}
rule.groups?.forEach(({ groupId }) => {
rule.groups.forEach(({ groupId }) => {
trustedGroups.add(groupId);
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export const SecurityGroupPicker = ({
useState<ViewRulesSelection>();

function onCloseRulesDialog() {
setViewRulesSelection(null);
setViewRulesSelection(undefined);
}

if (attempt.status === 'failed') {
Expand Down Expand Up @@ -160,7 +160,7 @@ export const SecurityGroupPicker = ({
altKey: 'tooltip',
headerText: '',
render: (sg: SecurityGroupWithRecommendation) => {
if (sg.recommended) {
if (sg.recommended && sg.tips?.length) {
return (
<Cell>
<ToolTipInfo>
Expand All @@ -173,7 +173,7 @@ export const SecurityGroupPicker = ({
</Cell>
);
}
return null;
return <Cell />;
},
},
]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ test('fetchAwsDatabases response', async () => {
accountId: 'account-id-1',
resourceId: 'resource-id-1',
vpcId: 'vpc-123',
subnets: [],
securityGroups: [],
},
{
engine: 'mysql',
Expand All @@ -131,6 +133,8 @@ test('fetchAwsDatabases response', async () => {
accountId: undefined,
resourceId: undefined,
vpcId: undefined,
subnets: [],
securityGroups: [],
},
{
engine: 'mysql',
Expand All @@ -141,6 +145,8 @@ test('fetchAwsDatabases response', async () => {
accountId: undefined,
resourceId: undefined,
vpcId: undefined,
subnets: [],
securityGroups: [],
},
],
nextToken: 'next-token',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -439,10 +439,10 @@ export function makeAwsDatabase(json: any): AwsRdsDatabase {
uri,
status: aws?.status,
labels: labels ?? [],
subnets: aws?.rds?.subnets,
subnets: aws?.rds?.subnets ?? [],
resourceId: aws?.rds?.resource_id,
vpcId: aws?.rds?.vpc_id,
securityGroups: aws?.rds?.security_groups,
securityGroups: aws?.rds?.security_groups ?? [],
accountId: aws?.account_id,
region: aws?.region,
};
Expand Down

0 comments on commit 13b3f5e

Please sign in to comment.