allow imported resources to be path expanded and more state reading #979
Conversation
| @@ -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) { | |||
There was a problem hiding this comment.
nit: If neither of these are template resource ids (ones without names), then IMO == is much clearer.
There was a problem hiding this comment.
the selector may not have a name
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
ah youre right neither of these is the selector, i can change it to ==
| @@ -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( | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
i can change the naming and make it return the inverse then
| errs = errors.Join(errs, err) | ||
| return | ||
| } | ||
| if !doesNotModifyImport { |
There was a problem hiding this comment.
nit continued: this further leans me towards the function should be checkDoesModify... (positive) so it doesn't need to be negated
| return nil | ||
| } | ||
|
|
||
| func (p propertyCorrelator) checkValue( |
There was a problem hiding this comment.
nit: this function could use either a better name or some documentation on what it does. Seems like it does more than checking, since I see a SetProperty and AddVertex (write operations).
| AsSource []PathSatisfactionRoute `json:"as_source" yaml:"as_source"` | ||
| AsTarget []PathSatisfactionRoute `json:"as_target" yaml:"as_target"` | ||
| AsSource []PathSatisfactionRoute `json:"as_source" yaml:"as_source"` | ||
| DenyClassifications []string `yaml:"deny_classifications"` |
There was a problem hiding this comment.
I'm not sure I understand what DenyClassifications is used for / what problems it solves. Could you document and add a test case (either in path_selection or as a functional test) demonstrating it its use-case?
There was a problem hiding this comment.
was just carry over from the other stuff so can remove. But it was a way of making sure subnets arent used in permissions path solving and iam roles arent used in network path solving since they were getting connected by the ec2 launch templates security group. Since this isnt tackling dynamic port mappings anymore i can remove it
There was a problem hiding this comment.
Up to you, but the choices IMO are: (1) keep it in and add tests/docs for it, (2) split it out (either to be reimplemented later or keep around as an incomplete PR)
| AsSource []PathSatisfactionRoute `json:"as_source" yaml:"as_source"` | ||
| AsTarget []PathSatisfactionRoute `json:"as_target" yaml:"as_target"` | ||
| AsSource []PathSatisfactionRoute `json:"as_source" yaml:"as_source"` | ||
| DenyClassifications []string `yaml:"deny_classifications"` |
There was a problem hiding this comment.
Up to you, but the choices IMO are: (1) keep it in and add tests/docs for it, (2) split it out (either to be reimplemented later or keep around as an incomplete PR)
adds checks to ensure we are choosing paths which dont change properties of imported resources
ensures that we dont only consider direct edges for imported resource path expansion
allows resources to deny themselves from being used in specific path selection classifications
adds the ability to read state for fields that need to be translated to field Refs or resource ids
making sure imports work with more resources
Standard checks