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

Don't evaluate edge rules if property is set by constraints #952

Merged
merged 6 commits into from
Mar 4, 2024

Conversation

gordon-klotho
Copy link
Contributor

@gordon-klotho gordon-klotho commented Mar 4, 2024

Edge Rules

This is required for gitea StackPack to change HealthCheck protocol on an NLB to be HTTP (and others, I don't remember).

kb

Also adds kb tool to help inspect the knowledgebase.

Example

kb --classification network aws:ecs_service aws:efs_file_system

image

kb --distance 3 aws:ecs_service

image

Support EnableExecuteCommand on ECS Service

For debugging ECS services

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?

@jhsinger-klotho
Copy link
Contributor

jhsinger-klotho commented Mar 4, 2024

we need a way to then signal if an edge rule is critical if we want to guarantee correctness, otherwise we cant make that guarantee. We could say that validation is that method also but thats a lot more work for us to bake this in then

Copy link
Contributor

@jhsinger-klotho jhsinger-klotho left a comment

Choose a reason for hiding this comment

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

nothing blocking really just a few questions

Comment on lines +290 to +291
// NOTE(gg): does operator even matter here? If it's not a collection,
// what does an 'add' mean? Should it allow edges to overwrite?
Copy link
Contributor

Choose a reason for hiding this comment

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

if its not a collection yeah im not sure that it would matter

Comment on lines +87 to +104
// Check to see if the whole path is valid before adding phantoms and edges.
// It's a miniscule performance benefit, and is mostly done for clarity in the debug graph output.
validPath := true
for i, res := range path {
if i == 0 {
continue
}
edgeTemplate := kb.GetEdgeTemplate(path[i-1].Id(), res.Id())
if edgeTemplate == nil || edgeTemplate.DirectEdgeOnly {
validPath = false
break
}
}
if !validPath {
continue
}

satisfied_paths++
Copy link
Contributor

Choose a reason for hiding this comment

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

dont we already do this when building the path selection graph? Isnt this redundant

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, yeah, I meant to move it instead of duplicate it. The main impact of this is that it removed resources used in paths that never connected to the target. Per the comment, it's mostly to support the kb tool to prevent extra irrelevant resources.

pkg/graph_addons/topology.go Outdated Show resolved Hide resolved
@gordon-klotho
Copy link
Contributor Author

we need a way to then signal if an edge rule is critical if we want to guarantee correctness, otherwise we cant make that guarantee. We could say that validation is that method also but thats a lot more work for us to bake this in then

I agree, added #953 to track

@gordon-klotho gordon-klotho merged commit 27bd3c2 into main Mar 4, 2024
6 checks passed
@gordon-klotho gordon-klotho deleted the kb_graph branch March 4, 2024 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants