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.

Sign up for GitHub

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 bugs around property loading, naming, and temp graph replacement #821

Merged
merged 2 commits into from
Dec 12, 2023
Merged

fix bugs around property loading, naming, and temp graph replacement #821

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bd59830
fix bugs around property loading, naming, and temp graph replacement
Dec 12, 2023
8f3f1fc
remove break
Dec 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions pkg/engine2/operational_eval/evaluator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ func (eval *Evaluator) addEdge(source, target Key) error {
ep.Attributes[attribAddedBy] = eval.currentKey.String()
}
})

if err != nil {
if errors.Is(err, graph.ErrEdgeCreatesCycle) {
path, _ := graph.ShortestPath(eval.graph, target, source)
Expand Down Expand Up @@ -452,12 +453,17 @@ func (eval *Evaluator) UpdateId(oldId, newId construct.ResourceId) error {
vertex.Edge = UpdateEdgeId(vertex.Edge, oldId, newId)
replaceVertex(key, vertex)
// because the temp graph contains the src and target as nodes, we need to update it if it exists
if vertex.TempGraph != nil {
err := construct.ReplaceResource(vertex.TempGraph, oldId, &construct.Resource{ID: newId})
if err != nil {
errs = errors.Join(errs, err)
continue
}
}
if vertex.TempGraph != nil {
_, err := vertex.TempGraph.Vertex(oldId)
switch {
case errors.Is(err, graph.ErrVertexNotFound):
// do nothing
break
Copy link
Contributor

Choose a reason for hiding this comment

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

break? that'd break out of the for _, key := range topo loop, is that what you intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope definitely didnt mean for that to be there, removing

case err != nil:
errs = errors.Join(errs, err)
default:
errs = errors.Join(errs, construct.ReplaceResource(vertex.TempGraph, oldId, &construct.Resource{ID: newId}))
}
}
}
Expand Down
1 change: 1 addition & 0 deletions pkg/engine2/operational_eval/vertex_path_expand.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ func (v *pathExpandVertex) runExpansion(eval *Evaluator, expansion path_selectio

for _, satisfication := range pathSatisfications {
if satisfication.Classification == v.Satisfication.Classification {
// we cannot evaluate these vertices immediately because we are unsure if their dependencies have settled
changes.addNode(&pathExpandVertex{
Edge: construct.SimpleEdge{Source: subExpand.Source, Target: subExpand.Target},
TempGraph: expansion.TempGraph,
Expand Down
6 changes: 5 additions & 1 deletion pkg/engine2/operational_rule/operational_action.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,8 +404,12 @@ func (action *operationalResourceAction) generateResourceName(resourceToSet *con
resourceToSet.Name = fmt.Sprintf("%s-%s%s", resource.Name, resourceToSet.Type, suffix)
return nil
}
return generateResourceName(action.ruleCtx.Solution, resourceToSet, resource)
}

func generateResourceName(sol solution_context.SolutionContext, resourceToSet *construct.ResourceId, resource construct.ResourceId) error {
numResources := 0
ids, err := construct.ToplogicalSort(action.ruleCtx.Solution.DataflowGraph())
ids, err := construct.ToplogicalSort(sol.DataflowGraph())
if err != nil {
return err
}
Expand Down
29 changes: 24 additions & 5 deletions pkg/engine2/path_selection/path_expansion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,15 @@ func renameAndReplaceInTempGraph(
if strings.HasPrefix(id.Name, PHANTOM_PREFIX) {
oldId := id
id.Name = name
// because certain resources may be namespaced, we will check against all resource type names
currNames, err := getCurrNames(ctx, &id)
if err != nil {
errs = errors.Join(errs, err)
continue
}
for suffix := 0; suffix < 1000; suffix++ {
_, err := ctx.RawView().Vertex(id)
if err != nil && errors.Is(err, graph.ErrVertexNotFound) {
if !currNames.Contains(id.Name) {
break
} else if err != nil {
errs = errors.Join(errs, err)
continue
}
id.Name = fmt.Sprintf("%s-%d", name, suffix)
}
Expand All @@ -146,6 +148,23 @@ func renameAndReplaceInTempGraph(
return resultResources, errs
}

func getCurrNames(sol solution_context.SolutionContext, resourceToSet *construct.ResourceId) (set.Set[string], error) {
currNames := make(set.Set[string])
ids, err := construct.ToplogicalSort(sol.DataflowGraph())
if err != nil {
return currNames, err
}
// we cannot consider things only in the namespace because when creating a resource for an operational action
// it likely has not been namespaced yet and we dont know where it will be namespaced to
matcher := construct.ResourceId{Provider: resourceToSet.Provider, Type: resourceToSet.Type}
for _, id := range ids {
if matcher.Matches(id) {
currNames.Add(id.Name)
}
}
return currNames, nil
}

func findSubExpansionsToRun(
result []*construct.Resource,
ctx solution_context.SolutionContext,
Expand Down
3 changes: 2 additions & 1 deletion pkg/engine2/solution_context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,11 @@ func (ctx solutionContext) LoadGraph(graph construct.Graph) error {
return err
}
op := ctx.OperationalView()
raw := ctx.RawView()
if err := op.AddVerticesFrom(graph); err != nil {
return err
}
return op.AddEdgesFrom(graph)
return raw.AddEdgesFrom(graph)
}

func (c solutionContext) GetDecisions() solution_context.DecisionRecords {
Expand Down
10 changes: 5 additions & 5 deletions pkg/engine2/testdata/k8s_api.dataflow-viz.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ resources:
parent: eks_cluster/eks_cluster-0


load_balancer/rest-api-4-integ6897f0b9:
load_balancer/rest-api-4-integbcc77100:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this non-deterministic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i dont think so, the ordering did change, which i assumed was from the way we load the graph, but i ran the same architecture 10 times and always saw the same result

Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if the UUIDs weren't deterministic, do we set the seed somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dont think we do, we could probably look into that

parent: vpc/vpc-0

load_balancer/rest-api-4-integ6897f0b9 -> kubernetes:pod:eks_cluster-0/pod2:
path: aws:load_balancer_listener:rest_api_4_integration_0-eks_cluster-0,aws:target_group:rest-api-4-integ6897f0b9,kubernetes:target_group_binding:restapi4integration0-ekscluster-0,kubernetes:service:restapi4integration0-pod2
load_balancer/rest-api-4-integbcc77100 -> kubernetes:pod:eks_cluster-0/pod2:
path: aws:load_balancer_listener:rest_api_4_integration_0-pod2,aws:target_group:rest-api-4-integbcc77100,kubernetes:target_group_binding:restapi4integration0-pod2,kubernetes:service:restapi4integration0-pod2


kubernetes:helm_chart:eks_cluster-0/metricsserver:
Expand All @@ -31,7 +31,7 @@ resources:
aws:api_integration:rest_api_4/rest_api_4_integration_0:
parent: rest_api/rest_api_4

aws:api_integration:rest_api_4/rest_api_4_integration_0 -> load_balancer/rest-api-4-integ6897f0b9:
path: aws:vpc_link:rest_api_4_integration_0-eks_cluster-0
aws:api_integration:rest_api_4/rest_api_4_integration_0 -> load_balancer/rest-api-4-integbcc77100:
path: aws:vpc_link:rest_api_4_integration_0-pod2


49 changes: 24 additions & 25 deletions pkg/engine2/testdata/k8s_api.expect.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,26 +111,26 @@ resources:
Resource: aws:api_resource:rest_api_4:api_resource-0
RestApi: aws:rest_api:rest_api_4
Route: /{proxy+}
Target: aws:load_balancer:rest-api-4-integ6897f0b9
Target: aws:load_balancer:rest-api-4-integbcc77100
Type: HTTP_PROXY
Uri: aws:api_integration:rest_api_4:rest_api_4_integration_0#LbUri
VpcLink: aws:vpc_link:rest_api_4_integration_0-eks_cluster-0
aws:vpc_link:rest_api_4_integration_0-eks_cluster-0:
Target: aws:load_balancer:rest-api-4-integ6897f0b9
aws:load_balancer:rest-api-4-integ6897f0b9:
VpcLink: aws:vpc_link:rest_api_4_integration_0-pod2
aws:vpc_link:rest_api_4_integration_0-pod2:
Target: aws:load_balancer:rest-api-4-integbcc77100
aws:load_balancer:rest-api-4-integbcc77100:
Scheme: internal
Subnets:
- aws:subnet:vpc-0:subnet-0
- aws:subnet:vpc-0:subnet-1
Type: network
aws:load_balancer_listener:rest_api_4_integration_0-eks_cluster-0:
aws:load_balancer_listener:rest_api_4_integration_0-pod2:
DefaultActions:
- TargetGroup: aws:target_group:rest-api-4-integ6897f0b9
- TargetGroup: aws:target_group:rest-api-4-integbcc77100
Type: forward
LoadBalancer: aws:load_balancer:rest-api-4-integ6897f0b9
LoadBalancer: aws:load_balancer:rest-api-4-integbcc77100
Port: 80
Protocol: TCP
aws:target_group:rest-api-4-integ6897f0b9:
aws:target_group:rest-api-4-integbcc77100:
HealthCheck:
Enabled: true
HealthyThreshold: 5
Expand All @@ -143,19 +143,19 @@ resources:
Protocol: TCP
TargetType: ip
Vpc: aws:vpc:vpc-0
kubernetes:target_group_binding:restapi4integration0-ekscluster-0:
kubernetes:target_group_binding:restapi4integration0-pod2:
Object:
apiVersion: elbv2.k8s.aws/v1beta1
kind: TargetGroupBinding
metadata:
labels:
KLOTHO_ID_LABEL: restapi4integration0-ekscluster-0
name: restapi4integration0-ekscluster-0
KLOTHO_ID_LABEL: restapi4integration0-pod2
name: restapi4integration0-pod2
spec:
serviceRef:
name: restapi4integration0-pod2
port: 80
targetGroupARN: aws:target_group:rest-api-4-integ6897f0b9#Arn
targetGroupARN: aws:target_group:rest-api-4-integbcc77100#Arn
kubernetes:helm_chart:eks_cluster-0:aws-load-balancer-controller:
Chart: aws-load-balancer-controller
Cluster: aws:eks_cluster:eks_cluster-0
Expand Down Expand Up @@ -285,12 +285,12 @@ resources:
- ec2.amazonaws.com
Version: "2012-10-17"
ManagedPolicies:
- arn:aws:iam::aws:policy/AWSCloudMapFullAccess
- arn:aws:iam::aws:policy/CloudWatchAgentServerPolicy
- arn:aws:iam::aws:policy/AmazonSSMManagedInstanceCore
- arn:aws:iam::aws:policy/AmazonEKSWorkerNodePolicy
- arn:aws:iam::aws:policy/AmazonEC2ContainerRegistryReadOnly
- arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy
- arn:aws:iam::aws:policy/AWSCloudMapFullAccess
aws:iam_role:pod2:
AssumeRolePolicyDoc:
Statement:
Expand Down Expand Up @@ -609,17 +609,16 @@ edges:
aws:api_resource:rest_api_4:api_resource-0 -> aws:api_integration:rest_api_4:rest_api_4_integration_0:
aws:api_resource:rest_api_4:api_resource-0 -> aws:api_method:rest_api_4:rest_api_4_integration_0_method:
aws:api_method:rest_api_4:rest_api_4_integration_0_method -> aws:api_integration:rest_api_4:rest_api_4_integration_0:
aws:api_integration:rest_api_4:rest_api_4_integration_0 -> aws:vpc_link:rest_api_4_integration_0-eks_cluster-0:
aws:vpc_link:rest_api_4_integration_0-eks_cluster-0 -> aws:load_balancer:rest-api-4-integ6897f0b9:
aws:load_balancer:rest-api-4-integ6897f0b9 -> aws:load_balancer_listener:rest_api_4_integration_0-eks_cluster-0:
aws:load_balancer:rest-api-4-integ6897f0b9 -> aws:subnet:vpc-0:subnet-0:
aws:load_balancer:rest-api-4-integ6897f0b9 -> aws:subnet:vpc-0:subnet-1:
aws:load_balancer_listener:rest_api_4_integration_0-eks_cluster-0 -> aws:target_group:rest-api-4-integ6897f0b9:
aws:target_group:rest-api-4-integ6897f0b9 -> kubernetes:target_group_binding:restapi4integration0-ekscluster-0:
kubernetes:target_group_binding:restapi4integration0-ekscluster-0 -> aws:eks_cluster:eks_cluster-0:
? kubernetes:target_group_binding:restapi4integration0-ekscluster-0 -> kubernetes:helm_chart:eks_cluster-0:aws-load-balancer-controller
:
kubernetes:target_group_binding:restapi4integration0-ekscluster-0 -> kubernetes:service:restapi4integration0-pod2:
aws:api_integration:rest_api_4:rest_api_4_integration_0 -> aws:vpc_link:rest_api_4_integration_0-pod2:
aws:vpc_link:rest_api_4_integration_0-pod2 -> aws:load_balancer:rest-api-4-integbcc77100:
aws:load_balancer:rest-api-4-integbcc77100 -> aws:load_balancer_listener:rest_api_4_integration_0-pod2:
aws:load_balancer:rest-api-4-integbcc77100 -> aws:subnet:vpc-0:subnet-0:
aws:load_balancer:rest-api-4-integbcc77100 -> aws:subnet:vpc-0:subnet-1:
aws:load_balancer_listener:rest_api_4_integration_0-pod2 -> aws:target_group:rest-api-4-integbcc77100:
aws:target_group:rest-api-4-integbcc77100 -> kubernetes:target_group_binding:restapi4integration0-pod2:
kubernetes:target_group_binding:restapi4integration0-pod2 -> aws:eks_cluster:eks_cluster-0:
kubernetes:target_group_binding:restapi4integration0-pod2 -> kubernetes:helm_chart:eks_cluster-0:aws-load-balancer-controller:
kubernetes:target_group_binding:restapi4integration0-pod2 -> kubernetes:service:restapi4integration0-pod2:
kubernetes:helm_chart:eks_cluster-0:aws-load-balancer-controller -> aws:eks_cluster:eks_cluster-0:
kubernetes:helm_chart:eks_cluster-0:aws-load-balancer-controller -> aws:region:region-0:
? kubernetes:helm_chart:eks_cluster-0:aws-load-balancer-controller -> kubernetes:service_account:eks_cluster-0:aws-load-balancer-controller
Expand Down
30 changes: 15 additions & 15 deletions pkg/engine2/testdata/k8s_api.iac-viz.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ resources:
aws:api_resource:rest_api_4/api_resource-0:
aws:api_resource:rest_api_4/api_resource-0 -> rest_api/rest_api_4:

load_balancer/rest-api-4-integ6897f0b9:
load_balancer/rest-api-4-integ6897f0b9 -> aws:subnet:vpc-0/subnet-0:
load_balancer/rest-api-4-integ6897f0b9 -> aws:subnet:vpc-0/subnet-1:
load_balancer/rest-api-4-integbcc77100:
load_balancer/rest-api-4-integbcc77100 -> aws:subnet:vpc-0/subnet-0:
load_balancer/rest-api-4-integbcc77100 -> aws:subnet:vpc-0/subnet-1:

iam_role/aws-load-balancer-controller:
iam_role/aws-load-balancer-controller -> iam_oidc_provider/eks_cluster-0:
Expand Down Expand Up @@ -84,8 +84,8 @@ resources:
aws:api_method:rest_api_4/rest_api_4_integration_0_method -> aws:api_resource:rest_api_4/api_resource-0:
aws:api_method:rest_api_4/rest_api_4_integration_0_method -> rest_api/rest_api_4:

vpc_link/rest_api_4_integration_0-eks_cluster-0:
vpc_link/rest_api_4_integration_0-eks_cluster-0 -> load_balancer/rest-api-4-integ6897f0b9:
vpc_link/rest_api_4_integration_0-pod2:
vpc_link/rest_api_4_integration_0-pod2 -> load_balancer/rest-api-4-integbcc77100:

kubernetes:service_account:eks_cluster-0/aws-load-balancer-controller:
kubernetes:service_account:eks_cluster-0/aws-load-balancer-controller -> eks_cluster/eks_cluster-0:
Expand Down Expand Up @@ -115,9 +115,9 @@ resources:
aws:api_integration:rest_api_4/rest_api_4_integration_0 -> aws:api_method:rest_api_4/rest_api_4_integration_0_method:
aws:api_integration:rest_api_4/rest_api_4_integration_0 -> aws:api_resource:rest_api_4/api_resource-0:
aws:api_integration:rest_api_4/rest_api_4_integration_0 -> rest_api/rest_api_4:
aws:api_integration:rest_api_4/rest_api_4_integration_0 -> vpc_link/rest_api_4_integration_0-eks_cluster-0:
aws:api_integration:rest_api_4/rest_api_4_integration_0 -> vpc_link/rest_api_4_integration_0-pod2:

target_group/rest-api-4-integ6897f0b9:
target_group/rest-api-4-integbcc77100:

kubernetes:helm_chart:eks_cluster-0/aws-load-balancer-controller:
kubernetes:helm_chart:eks_cluster-0/aws-load-balancer-controller -> eks_cluster/eks_cluster-0:
Expand Down Expand Up @@ -155,11 +155,11 @@ resources:
aws:api_deployment:rest_api_4/api_deployment-0 -> aws:api_method:rest_api_4/rest_api_4_integration_0_method:
aws:api_deployment:rest_api_4/api_deployment-0 -> rest_api/rest_api_4:

kubernetes:target_group_binding/restapi4integration0-ekscluster-0:
kubernetes:target_group_binding/restapi4integration0-ekscluster-0 -> eks_cluster/eks_cluster-0:
kubernetes:target_group_binding/restapi4integration0-ekscluster-0 -> target_group/rest-api-4-integ6897f0b9:
kubernetes:target_group_binding/restapi4integration0-ekscluster-0 -> kubernetes:helm_chart:eks_cluster-0/aws-load-balancer-controller:
kubernetes:target_group_binding/restapi4integration0-ekscluster-0 -> kubernetes:service/restapi4integration0-pod2:
kubernetes:target_group_binding/restapi4integration0-pod2:
kubernetes:target_group_binding/restapi4integration0-pod2 -> eks_cluster/eks_cluster-0:
kubernetes:target_group_binding/restapi4integration0-pod2 -> target_group/rest-api-4-integbcc77100:
kubernetes:target_group_binding/restapi4integration0-pod2 -> kubernetes:helm_chart:eks_cluster-0/aws-load-balancer-controller:
kubernetes:target_group_binding/restapi4integration0-pod2 -> kubernetes:service/restapi4integration0-pod2:

kubernetes:manifest/fluent-bit:
kubernetes:manifest/fluent-bit -> eks_cluster/eks_cluster-0:
Expand Down Expand Up @@ -191,9 +191,9 @@ resources:
route_table_association/subnet-0-subnet-0-route_table -> route_table/subnet-0-route_table:
route_table_association/subnet-0-subnet-0-route_table -> aws:subnet:vpc-0/subnet-0:

load_balancer_listener/rest_api_4_integration_0-eks_cluster-0:
load_balancer_listener/rest_api_4_integration_0-eks_cluster-0 -> load_balancer/rest-api-4-integ6897f0b9:
load_balancer_listener/rest_api_4_integration_0-eks_cluster-0 -> target_group/rest-api-4-integ6897f0b9:
load_balancer_listener/rest_api_4_integration_0-pod2:
load_balancer_listener/rest_api_4_integration_0-pod2 -> load_balancer/rest-api-4-integbcc77100:
load_balancer_listener/rest_api_4_integration_0-pod2 -> target_group/rest-api-4-integbcc77100:

iam_role_policy_attachment/aws-load-balancer-controller-iam_policy-0:
iam_role_policy_attachment/aws-load-balancer-controller-iam_policy-0 -> iam_policy/iam_policy-0:
Expand Down
3 changes: 3 additions & 0 deletions pkg/knowledge_base2/reader/properties.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ func (p *Property) Convert() (knowledgebase.Property, error) {
// Skip nil pointers
if (srcField.Kind() == reflect.Ptr || srcField.Kind() == reflect.Interface) && srcField.IsNil() {
continue
// skip empty arrays and slices
} else if (srcField.Kind() == reflect.Array || srcField.Kind() == reflect.Slice) && srcField.Len() == 0 {
continue
}
// Handle sub properties so we can recurse down the tree
if fieldName == "Properties" {
Expand Down
2 changes: 1 addition & 1 deletion pkg/templates/aws/resources/ecs_task_definition.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ properties:
default_value: awsvpc
description: The Docker networking mode to use for the containers in the task
PortMappings:
type: set
type: list
default_value:
- ContainerPort: 80
HostPort: 80
Expand Down
Loading