first attempt to add cross account permissions to aws#24
Conversation
This separates calls bwtween vpcs / enis and ec2 / instances attachments
|
High-level design feedback 1. Consider generalizing to N accounts instead of 2 The hard-coded local/remote split is specific to Roblox's topology and may be too narrow to get accepted upstream. A map-based multi-account design would be more general and easier for the community to adopt and maintain. One approach: type MultiAccountEC2Client struct {
logger *slog.Logger
clients map[AccountRole]EC2API
localAccountID string
}
type AccountRole string
const (
RoleNetwork AccountRole = "network" // VPC, subnets, ENIs
RoleCompute AccountRole = "compute" // EC2 instances, instance types
RoleEIP AccountRole = "eip" // Elastic IPs
)This makes the account-to-operation mapping explicit and extensible, rather than baking in a two-client assumption. 2. If keeping the two-account design, minor nit:
|
| } | ||
|
|
||
| func (c *CrossAccountEC2Client) AssociateEIP(ctx context.Context, eniID string, eipTags ipamTypes.Tags) (string, error) { | ||
| return c.local.AssociateEIP(ctx, eniID, eipTags) |
There was a problem hiding this comment.
eip's can be either local or remote for the avg user. we don't allow them at all in rbx, but if we did, we would want them in the clients local account. Otherwise we'd have every customer eip in network and it would get messy
| return c.remote.GetRouteTables(ctx, vpcID) | ||
| } | ||
|
|
||
| //TODO: not sure if I need this |
There was a problem hiding this comment.
do we need this comment? it seems correct to me.
| logfields.Error, permErr, | ||
| ) | ||
| if delErr := c.remote.DeleteNetworkInterface(ctx, eniID); delErr != nil { | ||
| //TODO: maybe make a bigger deal of this |
There was a problem hiding this comment.
A Warn log is insufficient here. A leaked ENI in the network account causes lasting cost and attachment-slot exhaustion. Emit a metric and consider logging at Error level so it surfaces in alerting.
| @@ -36,6 +36,7 @@ type EC2API interface { | |||
|
|
|||
| GetDetachedNetworkInterfaces(ctx context.Context, tags ipamTypes.Tags, maxResults int32) ([]string, error) | |||
| CreateNetworkInterface(ctx context.Context, toAllocate int32, subnetID, desc string, groups []string, allocatePrefixes bool) (string, *eniTypes.ENI, error) | |||
There was a problem hiding this comment.
CreateNetworkInterfacePermission is only called from within CrossAccountEC2Client.CreateNetworkInterface, yet adding it to EC2API forces every implementation to carry this cross-account detail. Consider keeping it out of the shared interface and calling it via a narrower sub-interface or directly on the concrete type inside crossaccount.go.
| {ID: "vpc-local", PrimaryCIDR: "192.168.0.0/16"}, | ||
| } | ||
| noRouteTables = []*ipamTypes.RouteTable{} | ||
| ) |
There was a problem hiding this comment.
Remove the // MADE BY AI. REVIEWED annotation before merge -- it carries no information for future readers.
| @@ -0,0 +1,48 @@ | |||
| { | |||
There was a problem hiding this comment.
we don't need flake.nix and flake.lock in this PR.
2 participants
This separates calls bwtween vpcs / enis and ec2 / instances attachments
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXXline if the commit addresses a particularGitHub issue.
Fixes: <commit-id>tag, thenplease add the commit author[s] as reviewer[s] to this issue.
Fixes: #issue-number