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

Fix Local layer to only look at operational resource paths #847

Merged
merged 5 commits into from
Jan 10, 2024

Conversation

gordon-klotho
Copy link
Contributor

@gordon-klotho gordon-klotho commented Jan 5, 2024

Since it's a little hard to parse the test data, here's the test yaml I was using

constraints:
  - node: aws:ecs_service:ecs_service_0
    operator: add
    scope: application
  - node: aws:load_balancer:load-balancer-2
    operator: add
    scope: application
  - operator: must_exist
    scope: edge
    target:
      source: aws:load_balancer:load-balancer-2
      target: aws:ecs_service:ecs_service_0

Which results in (annotated):

provider: aws
resources:
  ecs_service/ecs_service_0:
    children:
      # AZs added because ecs -> subnet -> az is all within the operational side effect (glue) boundary
      - aws:availability_zone:region-0:availability_zone-0
      - aws:availability_zone:region-0:availability_zone-1
      # ECR things via the task definition
      - aws:ecr_image:ecs_service_0-image
      - aws:ecr_repo:ecr_repo-0
      - aws:ecs_cluster:ecs_cluster-0
      - aws:ecs_task_definition:ecs_service_0
      - aws:iam_role:ecs_service_0-execution-role
      - aws:log_group:ecs_service_0-log-group
      - aws:region:region-0
      - aws:subnet:vpc-0:subnet-0
      - aws:subnet:vpc-0:subnet-1
      - aws:vpc:vpc-0
    parent: vpc/vpc-0
    tag: big

  vpc/vpc-0:
    children:
      # VPC includes resource for which it is the namespace
      - aws:internet_gateway:vpc-0:internet_gateway-0
      - aws:route_table:vpc-0:subnet-0-route_table
      - aws:route_table:vpc-0:subnet-1-route_table
      - aws:route_table:vpc-0:subnet-2-route_table
      - aws:route_table:vpc-0:subnet-3-route_table
      - aws:security_group:vpc-0:ecs_service_0-security_group
      - aws:subnet:vpc-0:subnet-0
      - aws:subnet:vpc-0:subnet-1
      - aws:subnet:vpc-0:subnet-2
      - aws:subnet:vpc-0:subnet-3
    tag: parent

  load_balancer/load-balancer-2:
    children:
      - aws:availability_zone:region-0:availability_zone-0
      - aws:availability_zone:region-0:availability_zone-1
      - aws:region:region-0
      - aws:subnet:vpc-0:subnet-0
      - aws:subnet:vpc-0:subnet-1
      - aws:vpc:vpc-0
    parent: vpc/vpc-0
    tag: big

  load_balancer/load-balancer-2 -> ecs_service/ecs_service_0:
    path:
      # resources included in any path from load_balancer to ecs_service
      - aws:load_balancer_listener:load-balancer-2-ecs_service_0
      - aws:security_group:vpc-0:ecs_service_0-security_group
      - aws:subnet:vpc-0:subnet-0
      - aws:subnet:vpc-0:subnet-1
      - aws:target_group:load-balancer-2-6c8c4811

image
image
image

Copy link
Contributor

@jhsinger-klotho jhsinger-klotho left a comment

Choose a reason for hiding this comment

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

Is the glue layer used in any templates today or in path selection anywhere? As afar as i can tell i see it used in determining the candidate weight

downstreams, err := solution_context.Downstream(ctx, src, knowledgebase.ResourceGlueLayer)
in which case i think we need to be able to separate these search patterns because this will definitely affect path selection.

could we add some unit tests around the glue layer search? i realize our functional tests do cover it so not blocking but would be great to have isolated tests

@gordon-klotho
Copy link
Contributor Author

in which case i think we need to be able to separate these search patterns because this will definitely affect path selection.

I intentionally changed the glue layer logic instead of implementing it just for the visualizer because I believe the old glue layer logic was incorrect and the new logic is better for what it was intended to be.

could we add some unit tests around the glue layer search? i realize our functional tests do cover it so not blocking but would be great to have isolated tests

Yeah, I was going to add some but decided to post this as soon as I had it working to get an early review. Standby for some tests

@gordon-klotho gordon-klotho changed the title Fix Glue layer to only look at operational resource paths Fix Local layer to only look at operational resource paths Jan 8, 2024
@gordon-klotho
Copy link
Contributor Author

image

@gordon-klotho gordon-klotho merged commit 4d488ed into main Jan 10, 2024
6 checks passed
@gordon-klotho gordon-klotho deleted the additional_resources_fixes branch January 10, 2024 16:31
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

Successfully merging this pull request may close these issues.

2 participants