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

allow imported resources to be path expanded and more state reading #979

Merged
merged 6 commits into from
Apr 12, 2024
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
2 changes: 1 addition & 1 deletion .github/workflows/govulncheck.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:
steps:
- uses: actions/setup-go@v3
with:
go-version: '1.22.1'
go-version: '1.22.2'
- uses: actions/cache@v2
with:
path: |
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/lint.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:
steps:
- uses: actions/setup-go@v3
with:
go-version: '1.21.5'
go-version: '1.22.2'
- uses: actions/checkout@v3
- uses: actions/cache@v2
with:
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/prettier.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ jobs:
steps:
- uses: actions/setup-go@v3
with:
go-version: '1.21.5'
go-version: '1.22.2'
- uses: actions/checkout@v3
- name: List files to check
run: |
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/release.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ jobs:
- name: Setup Go
uses: actions/setup-go@v3
with:
go-version: '1.21.5'
go-version: '1.22.2'

- name: Setup Zig
uses: goto-bus-stop/setup-zig@v1
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/run-integ-tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ jobs:
uses: actions/checkout@v3
- uses: actions/setup-go@v3
with:
go-version: '1.21.5'
go-version: '1.22.2'
- name: pre-build
if: ${{ inputs.pre-build-script }}
run: ${{ inputs.pre-build-script }}
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
steps:
- uses: actions/setup-go@v3
with:
go-version: '1.21.5'
go-version: '1.22.2'
- uses: actions/checkout@v3
- uses: actions/cache@v2
with:
Expand Down
2 changes: 1 addition & 1 deletion cmd/kb/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func (args Args) Run(ctx *kong.Context) error {
return fmt.Errorf("could not parse target: %w", err)
}
edge.Target.Name = "target"
g, err := path_selection.BuildPathSelectionGraph(edge, kb, args.Classification)
g, err := path_selection.BuildPathSelectionGraph(edge, kb, args.Classification, true)
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/engine/edge_targets.go
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ func (e *Engine) EdgeCanBeExpanded(ctx *solutionContext, source construct.Resour
construct.SimpleEdge{
Source: tempSource,
Target: tempTarget,
}, ctx.KnowledgeBase(), classification)
}, ctx.KnowledgeBase(), classification, false)
if err != nil {
return false, cacheable, err
}
Expand Down
12 changes: 11 additions & 1 deletion pkg/engine/operational_eval/graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,16 @@ func (eval *Evaluator) AddEdges(es ...construct.Edge) error {
func (eval *Evaluator) pathVertices(source, target construct.ResourceId) (graphChanges, error) {
changes := newChanges()

src, err := eval.Solution.RawView().Vertex(source)
if err != nil {
return changes, fmt.Errorf("failed to get source vertex for %s: %w", source, err)
}
dst, err := eval.Solution.RawView().Vertex(target)
if err != nil {
return changes, fmt.Errorf("failed to get target vertex for %s: %w", target, err)
}
requireFullBuild := dst.Imported || src.Imported

generateAndAddVertex := func(
edge construct.SimpleEdge,
kb knowledgebase.TemplateKB,
Expand All @@ -101,7 +111,7 @@ func (eval *Evaluator) pathVertices(source, target construct.ResourceId) (graphC
var tempGraph construct.Graph
if buildTempGraph {
var err error
tempGraph, err = path_selection.BuildPathSelectionGraph(edge, kb, satisfication.Classification)
tempGraph, err = path_selection.BuildPathSelectionGraph(edge, kb, satisfication.Classification, !requireFullBuild)
if err != nil {
return fmt.Errorf("could not build temp graph for %s: %w", edge, err)
}
Expand Down
7 changes: 4 additions & 3 deletions pkg/engine/operational_eval/vertex_path_expand.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func (v *pathExpandVertex) addDepsFromEdge(
return err
}

se := construct.SimpleEdge{Source: edge.Source, Target: edge.Target}
se := construct.Edge{Source: edge.Source, Target: edge.Target}
se.Source.Name = ""
se.Target.Name = ""

Expand Down Expand Up @@ -238,7 +238,7 @@ func (v *pathExpandVertex) addDepsFromEdge(
for i, rule := range tmpl.OperationalRules {
for j, cfg := range rule.ConfigurationRules {
var err error
data := knowledgebase.DynamicValueData{Edge: &edge}
data := knowledgebase.DynamicValueData{Edge: &se}
data.Resource, err = knowledgebase.ExecuteDecodeAsResourceId(dyn, cfg.Resource, data)

// We ignore the error because it just means that we cant resolve the resource yet
Expand Down Expand Up @@ -358,6 +358,7 @@ func (runner *pathExpandVertexRunner) getExpansionsToRun(v *pathExpandVertex) ([
if err != nil {
errs = errors.Join(errs, err)
}
requireFullBuild := sourceRes.Imported || targetRes.Imported

result := make([]path_selection.ExpansionInput, len(expansions))
for i, expansion := range expansions {
Expand All @@ -369,7 +370,7 @@ func (runner *pathExpandVertexRunner) getExpansionsToRun(v *pathExpandVertex) ([
}
if expansion.SatisfactionEdge.Source != edge.Source || expansion.SatisfactionEdge.Target != edge.Target {
simple := construct.SimpleEdge{Source: expansion.SatisfactionEdge.Source.ID, Target: expansion.SatisfactionEdge.Target.ID}
tempGraph, err := path_selection.BuildPathSelectionGraph(simple, eval.Solution.KnowledgeBase(), expansion.Classification)
tempGraph, err := path_selection.BuildPathSelectionGraph(simple, eval.Solution.KnowledgeBase(), expansion.Classification, requireFullBuild)
if err != nil {
errs = errors.Join(errs, fmt.Errorf("error getting expansions to run. could not build path selection graph: %w", err))
continue
Expand Down
6 changes: 0 additions & 6 deletions pkg/engine/operational_eval/vertex_property.go
Original file line number Diff line number Diff line change
Expand Up @@ -163,12 +163,6 @@ func (v *propertyVertex) Evaluate(eval *Evaluator) error {
Data: dynData,
}

// we know we cannot change properties of imported resources only users through constraints
// we still want to be able to update ids in case they are setting the property of a namespaced resource
// so we just conditionally run the edge operational rules
//
// we still need to run the resource operational rules though,
// to make sure dependencies exist where properties have operational rules set
if err := v.evaluateResourceOperational(&opCtx); err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/engine/operational_rule/operational_action.go
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ func (action *operationalResourceAction) createUniqueResources(resource *constru
return err
}
}
if len(uids) == 1 {
if len(uids) == 1 && uids[0].Matches(resource.ID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: If neither of these are template resource ids (ones without names), then IMO == is much clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the selector may not have a name

Copy link
Contributor

Choose a reason for hiding this comment

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

Which is the selector? uids comes from downstream so they're real resources. If resource.ID is the selector, then it needs to be the receiver (per the godoc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah youre right neither of these is the selector, i can change it to ==

res, err := action.ruleCtx.Solution.RawView().Vertex(id)
if err != nil {
return err
Expand Down
53 changes: 53 additions & 0 deletions pkg/engine/path_selection/candidate_validity.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"fmt"
"strings"

"github.com/dominikbraun/graph"
"github.com/klothoplatform/klotho/pkg/collectionutil"
construct "github.com/klothoplatform/klotho/pkg/construct"
"github.com/klothoplatform/klotho/pkg/engine/solution_context"
Expand All @@ -20,6 +21,58 @@ type (
}
)

// checkDoesNotModifyImportedResource checks if there is an imported resource that would be modified due to the edge
// If there is an edge rule modifying the resource then we consider the edge to be invalid
func checkDoesNotModifyImportedResource(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: the "does not" kinda throws me for a loop - usually I like to keep functions in the positive (and properties, when the default value isn't important)

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 can change the naming and make it return the inverse then

source, target construct.ResourceId,
ctx solution_context.SolutionContext,
et *knowledgebase.EdgeTemplate,
) (bool, error) {
// see if the source resource exists in the graph
sourceResource, srcErr := ctx.RawView().Vertex(source)
// see if the target resource exists in the graph
targetResource, trgtErr := ctx.RawView().Vertex(target)
if errors.Is(srcErr, graph.ErrVertexNotFound) && errors.Is(trgtErr, graph.ErrVertexNotFound) {
return true, nil
}

if et == nil {
et = ctx.KnowledgeBase().GetEdgeTemplate(source, target)
}

checkRules := func(resources construct.ResourceList) (bool, error) {
if len(resources) == 0 {
return true, nil
}
for _, rule := range et.OperationalRules {
for _, config := range rule.ConfigurationRules {
dynamicCtx := solution_context.DynamicCtx(ctx)
id := construct.ResourceId{}
// we ignore the error since phantom resources will cause errors in the decoding of templates
_ = dynamicCtx.ExecuteDecode(config.Resource, knowledgebase.DynamicValueData{
Edge: &construct.Edge{
Source: source,
Target: target,
}}, &id)

if resources.MatchesAny(id) {
return false, nil
}
}
}
return true, nil
}

importedResources := construct.ResourceList{}
if sourceResource != nil && sourceResource.Imported {
importedResources = append(importedResources, source)
}
if targetResource != nil && targetResource.Imported {
importedResources = append(importedResources, target)
}
return checkRules(importedResources)
}

// checkCandidatesValidity checks if the candidate is valid based on the validity of its own path satisfaction rules and namespace
func checkCandidatesValidity(
ctx solution_context.SolutionContext,
Expand Down
80 changes: 80 additions & 0 deletions pkg/engine/path_selection/candidate_validity_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,93 @@ package path_selection
import (
"testing"

"github.com/klothoplatform/klotho/pkg/construct"
"github.com/klothoplatform/klotho/pkg/construct/graphtest"
"github.com/klothoplatform/klotho/pkg/engine/enginetesting"
knowledgebase "github.com/klothoplatform/klotho/pkg/knowledgebase"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
)

func Test_checkDoesNotModifyImportedResource(t *testing.T) {
tests := []struct {
name string
source *construct.Resource
target *construct.Resource
et *knowledgebase.EdgeTemplate
mocks func(*enginetesting.MockKB)
want bool
}{
{
name: "no imported resource returns true",
source: &construct.Resource{ID: graphtest.ParseId(t, "p:a:a")},
target: &construct.Resource{ID: graphtest.ParseId(t, "p:b:b")},
et: &knowledgebase.EdgeTemplate{},
want: true,
},
{
name: "imported resource with no modifications returns true",
source: &construct.Resource{ID: graphtest.ParseId(t, "p:a:a"), Imported: true},
target: &construct.Resource{ID: graphtest.ParseId(t, "p:b:b")},
et: &knowledgebase.EdgeTemplate{
OperationalRules: []knowledgebase.OperationalRule{
{
ConfigurationRules: []knowledgebase.ConfigurationRule{
{
Resource: "{{ .Target }}",
},
},
},
},
},
want: true,
},
{
name: "imported resource with modifications returns false",
source: &construct.Resource{ID: graphtest.ParseId(t, "p:a:a"), Imported: true},
target: &construct.Resource{ID: graphtest.ParseId(t, "p:b:b")},
et: &knowledgebase.EdgeTemplate{
OperationalRules: []knowledgebase.OperationalRule{
{
ConfigurationRules: []knowledgebase.ConfigurationRule{
{
Resource: "{{ .Source }}",
},
},
},
},
},
},
{
name: "gets edge template if not provided",
source: &construct.Resource{ID: graphtest.ParseId(t, "p:a:a"), Imported: true},
target: &construct.Resource{ID: graphtest.ParseId(t, "p:b:b")},
mocks: func(kb *enginetesting.MockKB) {
kb.On("GetEdgeTemplate", graphtest.ParseId(t, "p:a:a"), graphtest.ParseId(t, "p:b:b")).Return(&knowledgebase.EdgeTemplate{})
},
want: true,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
sol := enginetesting.NewTestSolution()
sol.KB.On("GetResourceTemplate", mock.Anything).Return(&knowledgebase.ResourceTemplate{}, nil)
if tt.mocks != nil {
tt.mocks(&sol.KB)
}
err := sol.RawView().AddVertex(tt.source)
require.NoError(t, err)
err = sol.RawView().AddVertex(tt.target)
require.NoError(t, err)

got, err := checkDoesNotModifyImportedResource(tt.source.ID, tt.target.ID, sol, tt.et)
require.NoError(t, err)
require.Equal(t, tt.want, got)
sol.KB.AssertExpectations(t)
})
}
}

func Test_checkAsTargetValidity(t *testing.T) {
type testResource struct {
id string
Expand Down
25 changes: 21 additions & 4 deletions pkg/engine/path_selection/path_expansion.go
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,18 @@ func expandPath(
path construct.Path,
resultGraph construct.Graph,
) error {

if len(path) == 2 {
return nil
doesNotModifyImport, err := checkDoesNotModifyImportedResource(input.SatisfactionEdge.Source.ID,
input.SatisfactionEdge.Target.ID, ctx, nil)
if err != nil {
return err
}
if !doesNotModifyImport {
// Because the direct edge will cause modifications to an imported resource, we need to remove the direct edge
return input.TempGraph.RemoveEdge(input.SatisfactionEdge.Source.ID,
input.SatisfactionEdge.Target.ID)
}
}
zap.S().Debugf("Resolving path %s", path)

Expand Down Expand Up @@ -434,10 +444,17 @@ func expandPath(
if !tmpl.Unique.CanAdd(edges, source.id, target.id) {
return
}

doesNotModifyImport, err := checkDoesNotModifyImportedResource(source.id, target.id, ctx, tmpl)
if err != nil {
errs = errors.Join(errs, err)
return
}
if !doesNotModifyImport {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit continued: this further leans me towards the function should be checkDoesModify... (positive) so it doesn't need to be negated

return
}
// if the edge doesnt exist in the actual graph and there is any uniqueness constraint,
// then we need to check uniqueness validity
_, err := ctx.RawView().Edge(source.id, target.id)
_, err = ctx.RawView().Edge(source.id, target.id)
if errors.Is(err, graph.ErrEdgeNotFound) {
if tmpl.Unique.Source || tmpl.Unique.Target {
valid, err := checkUniquenessValidity(ctx, source.id, target.id)
Expand Down Expand Up @@ -527,7 +544,7 @@ func connectThroughNamespace(src, target *construct.Resource, ctx solution_conte
continue
}
// if we have a namespace resource that is not the same as the target namespace resource
tg, err := BuildPathSelectionGraph(construct.SimpleEdge{Source: res, Target: target.ID}, kb, "")
tg, err := BuildPathSelectionGraph(construct.SimpleEdge{Source: res, Target: target.ID}, kb, "", true)
if err != nil {
continue
}
Expand Down
7 changes: 5 additions & 2 deletions pkg/engine/path_selection/path_selection.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package path_selection
import (
"errors"
"fmt"
"slices"

"github.com/dominikbraun/graph"
"github.com/klothoplatform/klotho/pkg/collectionutil"
Expand All @@ -21,12 +22,14 @@ func BuildPathSelectionGraph(
dep construct.SimpleEdge,
kb knowledgebase.TemplateKB,
classification string,
ignoreDirectEdge bool,
) (construct.Graph, error) {
zap.S().Debugf("Building path selection graph for %s", dep)
tempGraph := construct.NewAcyclicGraph(graph.Weighted())

// Check to see if there is a direct edge which satisfies the classification and if so short circuit in building the temp graph
if et := kb.GetEdgeTemplate(dep.Source, dep.Target); et != nil && dep.Source.Namespace == dep.Target.Namespace {
et := kb.GetEdgeTemplate(dep.Source, dep.Target)
if !ignoreDirectEdge && et != nil && dep.Source.Namespace == dep.Target.Namespace {
directEdgeSatisfies := collectionutil.Contains(et.Classification, classification)

if !directEdgeSatisfies {
Expand Down Expand Up @@ -146,7 +149,7 @@ func PathSatisfiesClassification(
}
for i, res := range path {
resTemplate, err := kb.GetResourceTemplate(res)
if err != nil {
if err != nil || slices.Contains(resTemplate.PathSatisfaction.DenyClassifications, classification) {
return false
}
if collectionutil.Contains(resTemplate.Classification.Is, classification) {
Expand Down
Loading
Loading