Constraints satisfication checks #674

jhsinger-klotho · 2023-06-13T13:32:24Z

• Does any part of it require special attention?
• Does it relate to or fix any issue?

follows #670, will be too large until that is merged

This pr shows how we check the end resource graph to see if each constraint is satisfied.
Theres also some validate methods which we can use when taking in the constraints to ensure that they are solvable

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?

ghost · 2023-06-13T13:46:33Z

pkg/engine/constraints/application_constraint.go

+		}
+		return dag.GetResource(b.Node) == nil
+	case ReplaceConstraintOperator:
+		// We should entail edges are copied from the original source to the new replacement node in the dag


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?

likely a GH autocomplete comment, will fix

ghost · 2023-06-13T13:47:36Z

pkg/engine/constraints/application_constraint.go

+		return dag.GetResource(b.Node) == nil
+	case ReplaceConstraintOperator:
+		// We should entail edges are copied from the original source to the new replacement node in the dag
+		// Ignoring for now, but will be an optimization we can make


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

ghost · 2023-06-13T13:49:45Z

pkg/engine/constraints/application_constraint.go

+	}
+)
+
+func (b *ApplicationConstraint) Scope() ConstraintScope {


out of curiosity, why is the receiver var name b? (It confused me a bit as I was reading below — I forgot that's what b was, and got a bit confused.)

likely GH just auto completing to b, ill rename them

ghost · 2023-06-13T13:51:32Z

pkg/engine/constraints/application_constraint.go

+}
+
+func (b *ApplicationConstraint) IsSatisfied(dag *core.ResourceGraph) bool {
+	switch b.Operator {


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.

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

ghost · 2023-06-13T13:53:18Z

pkg/engine/constraints/application_constraint.go

+}
+
+func (b *ApplicationConstraint) Conflict(other Constraint) bool {
+	return false


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

this is just stubbed for now, its not implemented yet

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 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.

ghost · 2023-06-13T14:00:38Z

pkg/engine/constraints/constraint.go

+				validOperators := []string{string(AddConstraintOperator), string(RemoveConstraintOperator), string(ReplaceConstraintOperator)}
+				if !slices.Contains(validOperators, string(appConstraint.Operator)) {


minor/readability: I find it a bit distracting with all the casts to string. Why not keep them as [] ConstraintOperator ?

Suggested change

validOperators := []string{string(AddConstraintOperator), string(RemoveConstraintOperator), string(ReplaceConstraintOperator)}

if !slices.Contains(validOperators, string(appConstraint.Operator)) {

validOperators := []ConstraintOperator{AddConstraintOperator, RemoveConstraintOperator, ReplaceConstraintOperator}

if !collectionutil.Contains(validOperators, appConstraint.Operator) {

(You'd need to write collectionutil.Contains, but it's trivial.)

Similar for the other cases.

makes sense, going to do that

ghost · 2023-06-13T14:04:04Z

pkg/engine/constraints/constraints_test.go

+	}{
+		{
+			name: "test",
+			path: "./samples/constraints.yaml",


Rather than hard-coding the user of the OS fs, and thus having the tests rely on a sample file, I think it'd be better to have the parse work on just bytes or an fs.FS interface (or whatever it's called, I forget exactly), with this test providing a fully in-memory option. That way you can have each case provide just a small snippet of the yaml it's interested in, rather than having to maintain a monolithic yaml file for all possible cases (and have it be harder to read this test, since the input is in a different file from the expected results here).

ghost · 2023-06-13T14:05:00Z

pkg/engine/constraints/construct_constraint.go

+}
+
+func (b *ConstructConstraint) Conflict(other Constraint) bool {
+	return false


"construct has type=lambda" and "construct has type=eks"?

just stubbed right now

ghost · 2023-06-13T14:06:32Z

pkg/engine/constraints/node_constraint.go

+type (
+	// NodeConstraint is a struct that represents constraints that can be applied on a specific node in the resource graph.
+	// NodeConstraints are used to control intrinsic properties of a node in the resource graph
+	NodeConstraint struct {


Is a construct not a kind of node? (You have a separate ConstructConstraint in this PR, and I'm not sure what the relationship is.)

i could rename this to resource constraint, but yeah the intention is this is used for end resources

ghost · 2023-06-13T14:08:18Z

pkg/engine/constraints/node_constraint.go

+	if b.Target.Provider == core.AbstractConstructProvider {
+		return errors.New("node constraint cannot be applied to an abstract construct")
+	}
+	if b.Property == "" || reflect.ValueOf(b.Value).IsZero() {


Does this imply I can't say something like "set number of replicas to 0"?

right now it does imply that, which maybe is incorrect, so maybe i should remove the check of value being a zero value

👍 Yeah, I would. If we later want to ensure some value isn't 0, a system-provided constraint can do that ("instanceCount must be > 0", which would then be in conflict with "instanceCount = 0" if the user provided that).

gordon-klotho · 2023-06-13T13:33:17Z

pkg/core/construct_graph.go

+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


nit: return nil not empty lists for idiomatic Go

gordon-klotho · 2023-06-13T13:34:08Z

pkg/core/resource_graph.go

+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


nit: same as above

gordon-klotho · 2023-06-13T13:34:33Z

pkg/core/resource_graph.go

@@ -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{}


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

gordon-klotho · 2023-06-13T13:35:58Z

pkg/core/resources.go

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

+func (s BaseConstructSet) HasId(k ResourceId) bool {


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

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

gordon-klotho · 2023-06-13T13:36:28Z

pkg/engine/constraints/application_constraint.go

+)
+
+func (b *ApplicationConstraint) Scope() ConstraintScope {
+	return EdgeConstraintScope


Suggested change

return EdgeConstraintScope

return ApplicationConstraintScope

gordon-klotho · 2023-06-13T13:59:32Z

pkg/engine/constraints/edge_constraint.go

+
+	for _, s := range src {
+		for _, d := range dst {
+			path, _ := dag.ShortestPath(s, d)


Don't ignore errors - either we need to update the interface method or log it and return false.
Also, why only check the shortest path? Shouldn't it be like "must contain X" = "any(path contains X)", or "must not contain X" = "all(path not contains X)"?

well shortest path returns an error if there is no path, which could be different based on the type of constraint err get, so i need to go modify the graph method, but i can do that.

We do want to check all i had the comment
// We will likely want to search all paths to see if ANY contain the node. There's an open issue for this https://githuconstraint.com/dominikbraun/graph/issues/82
so i was hoping to just do that once the code is supported by the library, but we can write it all if we want. Lets chat about that though

gordon-klotho · 2023-06-13T14:03:24Z

pkg/engine/constraints/edge_constraint.go

+
+func (b *EdgeConstraint) checkSatisfication(path []core.Resource) bool {
+	// Currently we only support MustContain & MustNotContainConstraintOperator searching for if the node exists in the shortest path
+	// We will likely want to search all paths to see if ANY contain the node. There's an open issue for this https://github.com/dominikbraun/graph/issues/82


I see... This is probably important enough to implement ourselves. Either outside the library or fork it and PR (I prefer the latter). If you want to leave this for now, I recommend moving this comment to where ShortestPath is being called as it is more relevant there.

oh lol i see, ok, can i split this off into another task? Just to descope this initial pr?

gordon-klotho · 2023-06-13T14:05:45Z

pkg/engine/constraints/application_constraint.go

+)
+
+type (
+	// ApplicationConstraint is a struct that represents constraints that can be applied on the entire resource graph


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

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

gordon-klotho · 2023-06-13T14:14:37Z

pkg/engine/engine.go

+	}
+}
+
+func (e *Engine) LoadContext(InitialState *core.ConstructGraph, Constraints map[constraints.ConstraintScope][]constraints.Constraint) {


nit: title case is weird for variable names

oops, will fix

gordon-klotho · 2023-06-13T14:15:45Z

pkg/graph/graph.go

@@ -79,6 +83,17 @@ func (d *Directed[V]) IncomingEdges(to V) []Edge[V] {
 	})
 }

+func (d *Directed[V]) RemoveVertex(v string) error {
+	err := d.underlying.RemoveVertex(v)
+	if err != nil && !errors.Is(err, graph.ErrVertexHasEdges) {


What's the behaviour if it does return ErrVertexHasEdges? Does it remove it and its edges? Does it not remove it? Does it leave it in a weird state where the node is removed but not the edges?

if it has edges it wont remove the node, so thats why for that error we should error. This is a bug though and i dont error for it, so good catch

gordon-klotho · 2023-06-16T15:46:08Z

pkg/engine/constraints/application_constraint.go

+			return len(dag.FindResourcesWithRef(b.Node)) == 0
+		}
+		return dag.GetResource(b.Node) == nil
+	case ReplaceConstraintOperator:


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

gordon-klotho · 2023-06-16T15:48:02Z

pkg/engine/constraints/edge_constraint.go

-	switch b.Operator {
+func (constraint *EdgeConstraint) checkSatisfication(path []core.Resource) bool {
+	// Currently we only support searching for if the node exists in the shortest path
+	// We will likely want to search all paths to see if ANY contain the node. There's an open issue for this https://githuconstraint.com/dominikbraun/graph/issues/82


Find/replace got a little extra here

Jordan Singer added 6 commits June 12, 2023 16:55

add constraint interface, structs, and parsing from a file

ba0a446

update constraint interface

cbf731b

upgrade go for vuln check

f11e3d9

fix

68dbc1d

fix

49e11db

adding satisfication methods for constraints

7088070

jhsinger-klotho changed the title ~~Constraints satisfication~~ Constraints satisfication checks Jun 13, 2023

ghost reviewed Jun 13, 2023

View reviewed changes

gordon-klotho reviewed Jun 13, 2023

View reviewed changes

jhsinger-klotho mentioned this pull request Jun 13, 2023

Write algorithm for Graph library to get all paths between two nodes #677

Closed

address comments

5bb4d36

jhsinger-klotho mentioned this pull request Jun 13, 2023

start of engine #670

Closed

Jordan Singer added 2 commits June 13, 2023 11:30

address comments

1e979f3

change to use mapped res

5dc86ec

gordon-klotho approved these changes Jun 16, 2023

View reviewed changes

jhsinger-klotho merged commit d9485c9 into main Jun 16, 2023

jhsinger-klotho deleted the constraints_satisfication branch June 16, 2023 17:46

	// 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

		validOperators := []string{string(AddConstraintOperator), string(RemoveConstraintOperator), string(ReplaceConstraintOperator)}
		if !slices.Contains(validOperators, string(appConstraint.Operator)) {

Constraints satisfication checks #674

Constraints satisfication checks #674

Conversation

jhsinger-klotho commented Jun 13, 2023

Standard checks

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment