Skip to content
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

Interface Definitions #36

Open
ambsw-technology opened this issue Apr 22, 2020 · 4 comments
Open

Interface Definitions #36

ambsw-technology opened this issue Apr 22, 2020 · 4 comments

Comments

@ambsw-technology
Copy link

I'd like to suggest a centralized place for interface definitions and a more use-oriented scheme for Outputs naming. I assume you were trying to simplify/standardize the interfaces when creating cfn-modules, but I've already run into a case that worries me. I bring it up since interface design gets harder and harder to fix the longer you wait.

Background: The next ECS Service I need to integrate requires an inbound TCP connection so I'm forced to implement an NLB stack in addition to our "existing" ALB stack.

Issue: This lead me to use/compare alb-listener and ecs-nlb-listener-and-target. While both expose an -Arn output,

  • In alb-listener the ARN is the Listener
  • In ecs-nlb-listener-and-target the ARN is the TargetGroup

The ambiguity is especially pronounced since ecs-nlb-listener-and-target also has a Listener with an Arn that could be exposed. Technically, the interface for the two is different (i.e. ExposeArn vs Target) so they're not "incompatible", but I don't think the ExposeArn interface is useful/scalable.

Proposal: It seems to me that interfaces should not just indicate but enforce compatibility. E.G. a parameter named TargetModule should (in theory) accept any module that implements the Target interface. Like a puzzle piece, this relationship is enforced by ensuring that exports should only be compatible with each other (e.g. TargetArn and TargetSg).

If a Resource can be used in multiple ways, it can be exposed at several endpoints. For example,

  • An ECS Service would exposed its Security Group at IngressSg to indicate where to attach an Ingress rule for access to the service.
  • The ECS Service would expose the same Security Group at EgressSg as the group that gets authorized e.g. access to the RDS database.
  • An AlbListener could expose the ALB's -LoadBalancerSg at IngressSg since this is the group that would be used to access the listener
  • An AlbTarget would expose the same -LoadBalancerSg at EgressSg since this group would be authorized to e.g. the ECS Service as the source of traffic.
  • A more complex module (e.g. ALB + ECS) could expose the ALB SG at IngressSg and the ECS SG at EgressSg

Obviously, it'd be easier to see/plan/enforce these relationships if they were listed e.g. here in the docs module.

@michaelwittig
Copy link
Contributor

The centralized place might already exist (but not easy to find): https://github.com/cfn-modules/docs/blob/master/CONTRIBUTING.md#interfaces

  • alb-listener implements the ExposeArn interface
  • ecs-nlb-listener-and-target implements the Target interface
  • ecs-alb-target implements the Target interface

The fargate-service supports the following parameter

  • TargetModule: Optional stack name of module implementing Target

The reason why the NLB listener and target are combined is this: A NLB can only have a single target. While the ALB can have multiple targets (with path/domain rules).

You can see the implemented interface of a module by looking at this comment at the top of a module.yml:

# cfn-modules:implements(Target)

Not sure if that adds some new insights.


I agree, the ExposeArn interface is vey generic. As you mentioned, that can cause confusion. We settled for this approach because we learned from aws-cf-templates that it's hard to remember the output name if sometimes it is SecurityGroupArn, SgArn, ARN, Arn, ...

@ambsw-technology
Copy link
Author

ambsw-technology commented Apr 29, 2020

Thanks. I definitely did not find those docs. Maybe each file that has an interface should list that URL above/below (e.g. "Definitions: "). Easy to bulk replace the URL if you move the docs.

I'm still extremely concerned about -Arn. This prevents a module from exposing more than one interface (that include an ARN). The VPC module is already good a example of how bad this could get, exposing multiple IDs for multiple Subnets (e.g. SubnetIdCPublic). If you wanted to refactor this into interfaces, you really have no choice but to prefix.

(edit: indicates names that more closely match the semantics of my next comment)

I'll come back to non-interface issues like Private/Public below, but each Subnet needs an interface that includes an Id field (i.e. SubnetId). The VPC contains multiple subnets (e.g. VpcASubnetId through VpcDSubnetId or F for 3az etc.). The subnet is also the piece attached to an AZ so there's also VpcASubnetAz (edit: VpcASubnetAzId or VpcASubnetAzName) and of course VpcASubnetReach. In theory this organizes the outputs into interfaces.

For practical reasons, the VPC actually defines a Private and Public interface that include one or more Subnets. So now you have VpcPrivateASubnetId. In theory you still have VpcPrivateASubnetReach, but the business rules/definition guarantee that this will always resolve to private. The Private interface also exposes an Ids field (i.e. VpcPrivateIds, edit: VpcPrivateSubnetsId) which maps to the current SubnetIdsPrivate.

Finally VPC itself has an Azs field (i.e. VpcAzs, edit: VpcSubnetsAzId or VpcSubnetsAzName) that maps to AvailabilityZones. I probably haven't exhausted all of the parameters, but it certainly shows the combinations of Interface and Field at each level of a nested Interface definition.

@ambsw-technology
Copy link
Author

ambsw-technology commented Apr 29, 2020

P.S. I doubt the Azs in VpcAzs is really enough to indicate what about the Az is exported (and the same general issue applies in other outputs like VpcASubnetAz). In reality, VpcAzs probably needs to be VpcAzsId or VpcAzsArn. The s in Azs indicates that we're collecting a list of values from each Az. The rest of the identifier tells us which value is being extracted. In this case, it'd be an Id or Arn or Name. If it wasn't obvious from the output name, you could always consult the interface definition for Az to track your way down to the identity.

The same syntax could be used to precisely specify really complex collections. For example, AccountVpcsPublicASubnetId would be a list of Ids for the first (A) Subnet in the Public group for each of the the Vpcs in the Account. By contrast AccountVpcsPublicSubnetsId indicates Ids for all of the Subnets (the nested plural) in the Public group of the Vpcs in the Account.

@ambsw-technology
Copy link
Author

ambsw-technology commented Apr 29, 2020

P.P.S. Even if you 100% agree, I don't expect you to make these changes any time soon. I just wanted to highlight the issue (and try to help solve it) as soon as possible to avoid rework/bigger problems in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants