Abort reference resolution when a reference loop is detected #32
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
simu
force-pushed
the
feat/abort-on-ref-loop
branch
from
49f1b8f to
e6ac8d9
Compare
simu
force-pushed
the
feat/kapitan-patch
branch
from
6825201 to
39d59c3
Compare
simu
force-pushed
the
feat/abort-on-ref-loop
branch
from
e6ac8d9 to
0ab1953
Compare
We track the number of recursive calls to `Token::resolve()` for `Token::Ref` objects and abort resolution when the call depth exceeds 64. We already introduce a custom struct `ResolveState` to keep track of the call depth, so that we can easily add more state that we want to track during reference resolution in the future. We also already ensure we don't track call depth across boundaries where calls to `Token::resolve()` can't recurse into each other, e.g. for ValueList layers, Sequence elements, and Mapping key-value pairs. See the inline comments for detailed reasoning on why these recursive calls can't recurse into each other.
We add logic to track any reference paths which we've already seen while resolving references in a `Value`. Whenever we encounter a reference path which we've already seen before, we abort reference resolution and provide the list of paths which form a loop in the error message. Note that the already introduced boundaries where we don't need to track paths seen by recursive calls (across ValueList layers, between Sequence elements and between Mapping key-value pairs) still apply. We also leave the recursion depth limit in place for now as a safeguard for potential bugs in the loop detection. The existing tests are updated to reflect the changed error messages for the test cases which contain reference loops.
simu
force-pushed
the
feat/abort-on-ref-loop
branch
from
0ab1953 to
31f5ec0
Compare
bastjan
approved these changes
Sep 14, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces logic to track the number of recursive calls to
Token::resolve()forToken::Refobjects and uses that state to abort reference resolution with an error when the call depth exceeds 64.Additionally, the PR introduces logic to track any already seen "reference paths" (i.e. fully qualified colon-delimited parameter keys). All reference paths which are found during resolution are checked against the set of seen paths and if a path is already in the set we know that we're trying to resolve a loop where one or more references contain references to each other. In this case, we abort the reference resolution with an error showing the reference paths which form a loop.
The PR introduces a custom struct
ResolveStateto keep track of the call depth and seen reference paths. This design allows us to change the information which is tracked during reference resolution without having to update all call sites.The PR also carefully doesn't track state across boundaries where calls to
Token::resolve()can't recurse into each other, e.g. for ValueList layers, Sequence elements, and Mapping key-value pairs. See the inline comments for detailed reasoning on why these recursive calls can't recurse into each other.Additionally, the PR adds a number of new tests both for cases which shouldn't be detected as loops and cases which should be detected as loops.
Checklist
bug,enhancement,documentation,change,breaking,dependency,internalas they show up in the changelog