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

Constraints satisfication checks #674

Merged
merged 9 commits into from
Jun 16, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
adding satisfication methods for constraints
  • Loading branch information
Jordan Singer committed Jun 13, 2023
commit 70880706aa54a5aec09186ba50fe9d207cc860e2
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ require (
github.com/bmatcuk/doublestar/v4 v4.2.0
github.com/coreos/go-semver v0.3.0
github.com/davecgh/go-spew v1.1.1
github.com/dominikbraun/graph v0.16.1
github.com/dominikbraun/graph v0.22.2
github.com/fatih/color v1.13.0
github.com/gojek/heimdall/v7 v7.0.2
github.com/golang-jwt/jwt/v4 v4.4.3
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,8 @@ github.com/docker/libtrust v0.0.0-20150114040149-fa567046d9b1 h1:ZClxb8laGDf5arX
github.com/docopt/docopt-go v0.0.0-20180111231733-ee0de3bc6815/go.mod h1:WwZ+bS3ebgob9U8Nd0kOddGdZWjyMGR8Wziv+TBNwSE=
github.com/dominikbraun/graph v0.16.1 h1:ZtzDQFo32vxwSUsK4ioX8bfE45xoh8hQfavFMlKpaC8=
github.com/dominikbraun/graph v0.16.1/go.mod h1:yOjYyogZLY1LSG9E33JWZJiq5k83Qy2C6POAuiViluc=
github.com/dominikbraun/graph v0.22.2 h1:rFtrufgqXJy7daEMTxHIQWQc12Y1XHelXi2Heo1Bfho=
github.com/dominikbraun/graph v0.22.2/go.mod h1:yOjYyogZLY1LSG9E33JWZJiq5k83Qy2C6POAuiViluc=
github.com/dprotaso/go-yit v0.0.0-20191028211022-135eb7262960 h1:aRd8M7HJVZOqn/vhOzrGcQH0lNAMkqMn+pXUYkatmcA=
github.com/dprotaso/go-yit v0.0.0-20191028211022-135eb7262960/go.mod h1:9HQzr9D/0PGwMEbC3d5AB7oi67+h4TsQqItC1GVYG58=
github.com/dustin/go-humanize v0.0.0-20171111073723-bb3d318650d4/go.mod h1:HtrtbFcZ19U5GC7JDqmcUSB87Iq5E25KnS6fMYU6eOk=
Expand Down
12 changes: 12 additions & 0 deletions pkg/core/construct_graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,18 @@ func (cg *ConstructGraph) GetUpstreamConstructs(source BaseConstruct) []BaseCons
return cg.underlying.IncomingVertices(source)
}

func (cg *ConstructGraph) ShortestPath(source ResourceId, dest ResourceId) ([]BaseConstruct, error) {
ids, err := cg.underlying.ShortestPath(source.String(), dest.String())
if err != nil {
return []BaseConstruct{}, err
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: return nil not empty lists for idiomatic Go

}
resources := make([]BaseConstruct, len(ids))
for i, id := range ids {
resources[i] = cg.underlying.GetVertex(id)
}
return resources, nil
}

func (cg *ConstructGraph) GetResourcesOfCapability(capability string) (filtered []Construct) {
vertices := cg.underlying.GetAllVertices()
for _, v := range vertices {
Expand Down
22 changes: 22 additions & 0 deletions pkg/core/resource_graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,18 @@ func NewResourceGraph() *ResourceGraph {
}
}

func (rg *ResourceGraph) ShortestPath(source ResourceId, dest ResourceId) ([]Resource, error) {
ids, err := rg.underlying.ShortestPath(source.String(), dest.String())
if err != nil {
return []Resource{}, err
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: same as above

}
resources := make([]Resource, len(ids))
for i, id := range ids {
resources[i] = rg.underlying.GetVertex(id)
}
return resources, nil
}

func (rg *ResourceGraph) AddResource(resource Resource) {
if rg.GetResource(resource.Id()) == nil {
rg.underlying.AddVertex(resource)
Expand Down Expand Up @@ -84,6 +96,16 @@ func GetResource[T Resource](g *ResourceGraph, id ResourceId) (resource T, ok bo
return
}

func (rg *ResourceGraph) FindResourcesWithRef(id ResourceId) []Resource {
result := []Resource{}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use var result []Resource which also leaves result as nil if there are no resources.

for _, resource := range rg.ListResources() {
if resource.BaseConstructsRef().HasId(id) {
result = append(result, resource)
}
}
return result
}

func (rg *ResourceGraph) GetDependency(source ResourceId, target ResourceId) *graph.Edge[Resource] {
return rg.underlying.GetEdge(source.String(), target.String())
}
Expand Down
11 changes: 11 additions & 0 deletions pkg/core/resources.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ type (
HasLocalOutput interface {
OutputTo(dest string) error
}

Capability string
)

const (
Expand Down Expand Up @@ -164,6 +166,15 @@ func (s BaseConstructSet) Has(k BaseConstruct) bool {
return ok
}

func (s BaseConstructSet) HasId(k ResourceId) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thee feels awkward, like BaseConstructSet should be a map[ResourceId]BaseConstruct insead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, this is going to make the pr much much larger if thats ok

for c := range s {
if c.Id() == k {
return true
}
}
return false
}

func (s BaseConstructSet) Delete(k BaseConstruct) {
delete(s, k)
}
Expand Down
55 changes: 49 additions & 6 deletions pkg/engine/constraints/application_constraint.go
Original file line number Diff line number Diff line change
@@ -1,14 +1,17 @@
package constraints

import "github.com/klothoplatform/klotho/pkg/core"
import (
"errors"

"github.com/klothoplatform/klotho/pkg/core"
)

type (
// ApplicationConstraint is a struct that represents constraints that can be applied on the entire resource graph
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see some examples with descriptions of what they mean for all these constraints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

im going to do a large documentation pass towards the end of this if thats ok, but can toss in some samples quick for now.

Just avoiding doing too much in case it changes

ApplicationConstraint struct {
Operator ConstraintOperator `yaml:"operator"`
Node core.ResourceId `yaml:"node"`
ReplacementNode core.ResourceId `yaml:"replacement_node"`
Edge Edge `yaml:"edge"`
}
)

Expand All @@ -17,13 +20,53 @@ func (b *ApplicationConstraint) Scope() ConstraintScope {
}

func (b *ApplicationConstraint) IsSatisfied(dag *core.ResourceGraph) bool {
return false
}
switch b.Operator {
Copy link

Choose a reason for hiding this comment

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

This is basically hand-rolled polymorphism. Any reason not to just use different structs?

For example, ReplacementNode only gets used in one of these cases; that suggests to me that the other cases shouldn't even have that field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

was opting for flat objects which can have validation to ensure that they are correctly filled in, but the other option would be to have many different constraint types based on scope + operator

case AddConstraintOperator:
// If the add was for a construct, we need to check if any resource references the construct
if b.Node.Provider == core.AbstractConstructProvider {
return len(dag.FindResourcesWithRef(b.Node)) > 0
}
return dag.GetResource(b.Node) != nil
case RemoveConstraintOperator:
// If the remove was for a construct, we need to check if any resource references the construct
if b.Node.Provider == core.AbstractConstructProvider {
return len(dag.FindResourcesWithRef(b.Node)) == 0
}
return dag.GetResource(b.Node) == nil
case ReplaceConstraintOperator:
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't IsSatisfied for replace just be Remove + Add? Since you don't get a before/after to check edge updating you can only check the nodes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well for replace my thought is that we will want to check that we copied the edges over soon, so thats why it is separate. If we recieved a remove + add we wouldnt copy edges

Copy link
Contributor

Choose a reason for hiding this comment

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

I just figured that'd be a node or construct constraint not an application one.

// We should entail edges are copied from the original source to the new replacement node in the dag
Copy link

Choose a reason for hiding this comment

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

Suggested change
// We should entail edges are copied from the original source to the new replacement node in the dag
// We should ensure edges are copied from the original source to the new replacement node in the dag

Is this what you meant?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

likely a GH autocomplete comment, will fix

// Ignoring for now, but will be an optimization we can make
Copy link

Choose a reason for hiding this comment

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

What is? Is it the line above (ensuring edges get copied)? If so, is that really an optimization?


func (b *ApplicationConstraint) Apply(dag *core.ResourceGraph) error {
return nil
// If any of the nodes are abstract constructs, we need to check if any resource references the construct
if b.Node.Provider == core.AbstractConstructProvider && b.ReplacementNode.Provider == core.AbstractConstructProvider {
return len(dag.FindResourcesWithRef(b.Node)) == 0 && len(dag.FindResourcesWithRef(b.ReplacementNode)) > 0
} else if b.Node.Provider == core.AbstractConstructProvider && b.ReplacementNode.Provider != core.AbstractConstructProvider {
return len(dag.FindResourcesWithRef(b.Node)) == 0 && dag.GetResource(b.ReplacementNode) != nil
} else if b.Node.Provider != core.AbstractConstructProvider && b.ReplacementNode.Provider == core.AbstractConstructProvider {
return dag.GetResource(b.Node) == nil && len(dag.FindResourcesWithRef(b.Node)) > 0
}
return dag.GetResource(b.Node) == nil && dag.GetResource(b.ReplacementNode) != nil
}
return false
}

func (b *ApplicationConstraint) Conflict(other Constraint) bool {
return false
Copy link
Contributor

Choose a reason for hiding this comment

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

Not entirely sure what Confict is used for, but surely this isn't correct right? Is this unimplemented or is it actually correct and I'm mistaken?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed for now

Copy link

Choose a reason for hiding this comment

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

"add Foo" and "remove Foo" don't conflict? or "remove Foo" and "replace Foo with Bar"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is just stubbed for now, its not implemented yet

Copy link

Choose a reason for hiding this comment

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

I would just remove the method then, until we actually have use for it.

Too many stubs make a PR hard to review, imo, because it means I'm always wondering "is this incomplete in a way that I should comment on, or because it's unused?"

Copy link
Contributor

Choose a reason for hiding this comment

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

I would just remove the method then, until we actually have use for it.

Too many stubs make a PR hard to review, imo, because it means I'm always wondering "is this incomplete in a way that I should comment on, or because it's unused?"

☝️ I'm in the same boat. We could just remove it from the interface until we need it.

}

func (b *ApplicationConstraint) Validate() error {
if b.Operator == ReplaceConstraintOperator && (b.Node == core.ResourceId{} || b.ReplacementNode == core.ResourceId{}) {
return errors.New("replace constraint must have a node and replacement node defined")
}
if b.Operator == ReplaceConstraintOperator && b.Node.Provider != core.AbstractConstructProvider && b.ReplacementNode.Provider == core.AbstractConstructProvider {
return errors.New("replace constraint cannot replace a resource with an abstract construct")
}
if b.Operator == AddConstraintOperator && (b.Node == core.ResourceId{}) {
return errors.New("add constraint must have a node defined")
}

if b.Operator == RemoveConstraintOperator && (b.Node == core.ResourceId{}) {
return errors.New("remove constraint must have a node defined")
}
return nil
}
182 changes: 182 additions & 0 deletions pkg/engine/constraints/application_constraint_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
package constraints

import (
"testing"

"github.com/klothoplatform/klotho/pkg/core"
"github.com/klothoplatform/klotho/pkg/provider/aws/resources"
"github.com/stretchr/testify/assert"
)

func Test_ApplicationConstraint_IsSatisfied(t *testing.T) {
eu := &core.ExecutionUnit{Name: "compute"}
eu2 := &core.ExecutionUnit{Name: "compute2"}

tests := []struct {
name string
constraint []ApplicationConstraint
resources []core.Resource
want bool
}{
{
name: "Add is satisfied",
constraint: []ApplicationConstraint{
{
Operator: AddConstraintOperator,
Node: core.ResourceId{Provider: core.AbstractConstructProvider, Type: core.EXECUTION_UNIT_TYPE, Name: "compute"},
},
{
Operator: AddConstraintOperator,
Node: core.ResourceId{Provider: "aws", Type: "lambda_function", Name: "my_function_also"},
},
},
resources: []core.Resource{
&resources.LambdaFunction{
Name: "my_function_also",
},
&resources.LambdaFunction{
Name: "my_function",
ConstructsRef: core.BaseConstructSetOf(eu),
},
},
want: true,
},
{
name: "Add is not satisfied",
constraint: []ApplicationConstraint{
{
Operator: AddConstraintOperator,
Node: core.ResourceId{Provider: core.AbstractConstructProvider, Type: core.EXECUTION_UNIT_TYPE, Name: "compute"},
},
{
Operator: AddConstraintOperator,
Node: core.ResourceId{Provider: "aws", Type: "lambda_function", Name: "my_function_also"},
},
},
resources: []core.Resource{
&resources.LambdaFunction{
Name: "my_function",
},
},
want: false,
},
{
name: "remove is satisfied",
constraint: []ApplicationConstraint{
{
Operator: RemoveConstraintOperator,
Node: core.ResourceId{Provider: core.AbstractConstructProvider, Type: core.EXECUTION_UNIT_TYPE, Name: "compute"},
},
{
Operator: RemoveConstraintOperator,
Node: core.ResourceId{Provider: "aws", Type: "lambda_function", Name: "my_function_also"},
},
},
resources: []core.Resource{
&resources.LambdaFunction{
Name: "my_function",
},
},
want: true,
},
{
name: "remove is not satisfied",
constraint: []ApplicationConstraint{
{
Operator: RemoveConstraintOperator,
Node: core.ResourceId{Provider: core.AbstractConstructProvider, Type: core.EXECUTION_UNIT_TYPE, Name: "compute"},
},
{
Operator: RemoveConstraintOperator,
Node: core.ResourceId{Provider: "aws", Type: "lambda_function", Name: "my_function_also"},
},
},
resources: []core.Resource{
&resources.LambdaFunction{
Name: "my_function_also",
},
&resources.LambdaFunction{
Name: "my_function",
ConstructsRef: core.BaseConstructSetOf(eu),
},
},
want: false,
},
{
name: "replace is satisfied",
constraint: []ApplicationConstraint{
{
Operator: ReplaceConstraintOperator,
Node: core.ResourceId{Provider: core.AbstractConstructProvider, Type: core.EXECUTION_UNIT_TYPE, Name: "compute"},
ReplacementNode: core.ResourceId{Provider: core.AbstractConstructProvider, Type: core.EXECUTION_UNIT_TYPE, Name: "compute2"},
},
{
Operator: ReplaceConstraintOperator,
Node: core.ResourceId{Provider: core.AbstractConstructProvider, Type: core.EXECUTION_UNIT_TYPE, Name: "compute"},
ReplacementNode: core.ResourceId{Provider: "aws", Type: "lambda_function", Name: "lambda_compute"},
},
{
Operator: ReplaceConstraintOperator,
Node: core.ResourceId{Provider: "aws", Type: "lambda_function", Name: "my_function_also"},
ReplacementNode: core.ResourceId{Provider: "aws", Type: "lambda_function", Name: "my_function_also2"},
},
},
resources: []core.Resource{
&resources.LambdaFunction{
Name: "lambda_compute",
},
&resources.LambdaFunction{
Name: "my_function_also2",
},
&resources.LambdaFunction{
Name: "my_function",
ConstructsRef: core.BaseConstructSetOf(eu2),
},
},
want: true,
},
{
name: "replace is not satisfied",
constraint: []ApplicationConstraint{
{
Operator: ReplaceConstraintOperator,
Node: core.ResourceId{Provider: core.AbstractConstructProvider, Type: core.EXECUTION_UNIT_TYPE, Name: "compute"},
ReplacementNode: core.ResourceId{Provider: core.AbstractConstructProvider, Type: core.EXECUTION_UNIT_TYPE, Name: "compute2"},
},
{
Operator: ReplaceConstraintOperator,
Node: core.ResourceId{Provider: core.AbstractConstructProvider, Type: core.EXECUTION_UNIT_TYPE, Name: "compute"},
ReplacementNode: core.ResourceId{Provider: "aws", Type: "lambda_function", Name: "lambda_compute"},
},
{
Operator: ReplaceConstraintOperator,
Node: core.ResourceId{Provider: "aws", Type: "lambda_function", Name: "my_function_also"},
ReplacementNode: core.ResourceId{Provider: "aws", Type: "lambda_function", Name: "my_function_also2"},
},
},
resources: []core.Resource{
&resources.LambdaFunction{
Name: "my_function_also",
},
&resources.LambdaFunction{
Name: "my_function",
ConstructsRef: core.BaseConstructSetOf(eu),
},
},
want: false,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
assert := assert.New(t)
dag := core.NewResourceGraph()
for _, res := range tt.resources {
dag.AddResource(res)
}
for _, constraint := range tt.constraint {
result := constraint.IsSatisfied(dag)
assert.Equalf(tt.want, result, "constraint %s is not satisfied", constraint)
}
})
}
}
Loading