Skip to content

Commit

Permalink
allow imported resources to be path expanded and more state reading (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
jhsinger-klotho authored Apr 12, 2024
1 parent f1c0e35 commit 992a83c
Show file tree
Hide file tree
Showing 51 changed files with 1,316 additions and 175 deletions.
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
2 changes: 1 addition & 1 deletion pkg/engine/operational_eval/resource_template_mock_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

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] == resource.ID {
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 (
}
)

// checkModifiesImportedResource 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 checkModifiesImportedResource(
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 false, nil
}

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

checkRules := func(resources construct.ResourceList) (bool, error) {
if len(resources) == 0 {
return false, 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 true, nil
}
}
}
return false, 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
81 changes: 81 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,94 @@ 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_checkModifiesImportedResource(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: false,
},
{
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: false,
},
{
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 }}",
},
},
},
},
},
want: true,
},
{
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: false,
},
}
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 := checkModifiesImportedResource(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
modifiesImport, err := checkModifiesImportedResource(input.SatisfactionEdge.Source.ID,
input.SatisfactionEdge.Target.ID, ctx, nil)
if err != nil {
return err
}
if modifiesImport {
// 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
}

modifiesImport, err := checkModifiesImportedResource(source.id, target.id, ctx, tmpl)
if err != nil {
errs = errors.Join(errs, err)
return
}
if modifiesImport {
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
Loading

0 comments on commit 992a83c

Please sign in to comment.