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

model consumption in path expansion #776

Merged
merged 2 commits into from
Nov 20, 2023
Merged

model consumption in path expansion #776

merged 2 commits into from
Nov 20, 2023

Conversation

jhsinger-klotho
Copy link
Contributor

Adds as source to cloudront and as dest to rest api for correct resolution and topology yaml

visualization fixes to only consider parent(groups) for namespace resources and clusters/network resources


Introduces emitter and consumer for construct expansion and path selection

kb now stores models to be used for emissions

during path expansion we consume anything from the target which is available and wanted by the src

example for emissions

sample inputs

constraints:
  - node: aws:lambda_function:pets
    operator: add
    scope: application 
  - node: aws:ecs_service:pets2
    operator: add
    scope: application 
  - node: kubernetes:pod:pets3
    operator: add
    scope: application 
  - node: aws:dynamodb_table:store
    operator: add
    scope: application
  - scope: edge
    operator: must_exist
    target:
      source: aws:ecs_service:pets2
      target: aws:dynamodb_table:store
  - scope: edge
    operator: must_exist
    target:
      source: aws:lambda_function:pets
      target: aws:dynamodb_table:store
  - scope: edge
    operator: must_exist
    target:
      source: kubernetes:pod:pets3
      target: aws:dynamodb_table:store

sample outputs for relevant consumers

    kubernetes:pod:eks_cluster-0:pets3:
        Cluster: aws:eks_cluster:eks_cluster-0
        Object:
            apiVersion: v1
            kind: Pod
            metadata:
                labels:
                    KLOTHO_ID_LABEL: pets3
                name: pets3
            spec:
                automountServiceAccountToken: true
                containers:
                    - env:
                        - name: store_TABLE_NAME
                          value: aws:dynamodb_table:store#Name
                      image: aws:ecr_image:ecr_image-pets3-2#ImageName
                      name: pets3
                      ports:
                        - containerPort: 80
                          hostPort: 80
                          name: default-tcp
                          protocol: TCP
                serviceAccountName: kubernetes:service_account:eks_cluster-0:pets3
    aws:lambda_function:pets:
        EnvironmentVariables:
            store_TABLE_NAME: aws:dynamodb_table:store#Name
        ExecutionRole: aws:iam_role:pets-ExecutionRole
        Image: aws:ecr_image:pets-image
        LogGroup: aws:log_group:pets-log-group
        MemorySize: 512
        SecurityGroups:
            - aws:security_group:vpc-0:security_group-pets-3
        Subnets:
            - aws:subnet:vpc-0:subnet-1
            - aws:subnet:vpc-0:subnet-0
        Timeout: 180
 aws:ecs_task_definition:pets2:
        Cpu: "256"
        EnvironmentVariables:
            store_TABLE_NAME: aws:dynamodb_table:store#Name
        ExecutionRole: aws:iam_role:pets2-execution-role
        Image: aws:ecr_image:pets2-image
        LogGroup: aws:log_group:pets2-log-group
        Memory: "512"
        NetworkMode: awsvpc
        PortMappings:
            - ContainerPort: 80
              HostPort: 80
              Protocol: TCP
        Region: aws:region:region-0
        RequiresCompatibilities:
            - FARGATE
        TaskRole: aws:iam_role:pets2-execution-role

Standard checks

  • Unit tests: Any special considerations?
  • Docs: Do we need to update any docs, internal or public?
  • Backwards compatibility: Will this break existing apps? If so, what would be the extra work required to keep them working?

@@ -64,6 +65,11 @@ func checkProperties(ctx solution_context.SolutionContext, resource, toCheck *co
if err != nil || template == nil {
return false, fmt.Errorf("error getting resource template for resource %s: %w", resource.ID, err)
}

if strings.Contains(resource.ID.Name, PHANTOM_PREFIX) {
Copy link
Contributor

Choose a reason for hiding this comment

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

HasPrefix is technically more correct, right?

Comment on lines +164 to +166
func checkIfLoneDependency(ctx solution_context.SolutionContext,
resource, toCheck construct.ResourceId, direction knowledgebase.Direction,
selector knowledgebase.ResourceSelector) (bool, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func checkIfLoneDependency(ctx solution_context.SolutionContext,
resource, toCheck construct.ResourceId, direction knowledgebase.Direction,
selector knowledgebase.ResourceSelector) (bool, error) {
func checkIfLoneDependency(
ctx solution_context.SolutionContext,
resource, toCheck construct.ResourceId,
direction knowledgebase.Direction,
selector knowledgebase.ResourceSelector
) (bool, error) {

nit: I prefer this style of multi-line definition, but we haven't standardized anything

if err != nil {
errs = errors.Join(errs, err)
}
if node.Parent.IsZero() && hasPath {
Copy link
Contributor

Choose a reason for hiding this comment

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

node.Parent.IsZero() is always true because it's checked in the outer if, right?

Comment on lines +39 to +42
consumption:
consumed:
- model: EnvironmentVariables
property_path: Object.spec.containers[0].env
Copy link
Contributor

Choose a reason for hiding this comment

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

This kinda feels like it should be just a property as the model, with the converter being the default value of property_path. Fine for now, but feels like it could be unified.

@jhsinger-klotho jhsinger-klotho merged commit cac1103 into main Nov 20, 2023
79 of 85 checks passed
@jhsinger-klotho jhsinger-klotho deleted the consumption branch November 20, 2023 22:01
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