Cannot replace variables with values of type map from another class (sometimes)

[andaryjo]

This sounds complicated, because it is. Let me explain.

Imagine you have to classes A and B:

---
a:
  location: westeurope

---
b:
  spokes:
    - location: ${a:location}

And finally a target which uses both of these classes:

---
target:
  skipper:
    use:
      - a
      - b
  spokes: ${b:spokes}

Compilation of this setup would sometimes pass and sometimes fail with this error:

unexpected node type map[string]interface {} at index 2

@alxndr13 and myself spent together around 10 hours debugging this and so far this is what we got:

• The error gets thrown by the GetPath function when invoked from the ReplaceVariables function.
• Whether the compile passes or not depends on the order in which variables get replaced (which is random, so in this case 50/50 since we only have two vars)
  • If the value of b.spokes[0].location gets replaced before the value of target.spokes, the compile would pass.
  • If the value of target.spokes gets replaced before the value of b.spokes[0].location, the compile fails.
• In case the compile passes, the variable replacement loop finishes after the first iteration, because there are no variables left to replace.
• In case the compile fails, the variable replacement loop would do a second iteration and try to replace the apparently new variable target.spokes[0].location.
• The GetPath function then throws an error because the type of the value of target.spokes[0].location is map[string]interface {} and for some reason not skipper.Data.
• We suspect that the setPath function somehow messes up the data type of the map value that gets inserted into the tree. More specifically, the YAML marshalling and unmarshalling seems to be the issue. If you remove that, the compile always passes, regardless of the order the variables get replaced in.
  • I remember the YAML marshalling and unmarshalling being added for a specific use case we had. Our custom wrapper for Skipper accessed the SetPath function to inject custom data transformations which otherwise would not get structured correctly (see https://github.com/lukasjarosch/skipper/pull/70)
• With the YAML marshalling removed, the variable replacement loop only runs one iteration, regardless of the order the variables get replaced in.
  • It is unclear to us why this affects the variable discovery. My suspicion is that the YAML marshalling somehow affects what the FindValues function determines to be a value.

We are planning to continue investigating what exactly the problem is here since we don't fully understand it yet. But maybe you have already an idea, @lukasjarosch.

[lukasjarosch]

Thank you for reporting this and huge respect for already spending so much time debugging this ❤️. I was able to reproduce the bug with your example.

You were, right about the fact that the variable replacement is random. And that is also the reason this happens. The underlying issue is that variable replacement is rather naively implemented by simply brute-forcing and replacing variables as long as variables can be found. Hence, the two cases arise.

Case 1 ✅

• ReplaceVariables is called
• Enter replacement loop
  • FindVariables is called --------------------------------------
  • 2 Variables are found:
    1. ${a:location} at spokes
    2. ${b:spokes} at b.spokes.0.location
  • replaceVariable is called for variable a
    • this ultimately calls SetPath() which in turn calls yaml.Marshal
    • So the value westeurope is yaml marshalled, which doesn't change the type (string)
  • replaceVariable is called for variable b
    • this also calls SetPath which again calls yaml.Marshal
    • the value at b.spokes is a slice of skipper.Data maps (Data{"location": "westeurope"}})
    • after marshalling the value is of type map[string]interface{} (map[string]interface{}{"location": "westeurope"}) because the yaml marshaller obviously doesnt use skipper Data (which is only a map[string]interface{} anyway).
  • FindVariables is called --------------------------------------
  • no variables found, done ✅

Case 2 💥

• ReplaceVariables is called
• Enter replacement loop
  • FindVariables is called --------------------------------------
  • 2 Variables are found (❗ note that the order changed ❗ ):
    1. ${b:spokes} at b.spokes.0.location
    2. ${a:location} at spokes
  • replaceVariable is called for variable a
    • same behavior as replacing variable b in case 1, but the content of the map is different
    • It is now: map[string]interface{}{"location": "${a:location}"} because the location variable has not yet been replaced
    • ☝🏼 This introduces a third variable which is not known at this iteration
  • replaceVariable is called for variable b
    • spokes.0.location now has the value westeurope
  • FindVariables is called --------------------------------------
  • 1 Variable is found
    1. ${a:location} at spokes.0.location
  • 💡 This is where things go bad. If you remember: spokes.0.location is a map[string]interface{} and not skipper.Data. This is significant in that neither GetPath nor SetPath support that type which in turn causes them to fall-back into the default case, which is where the error originates from.

Ultimately the issue arises from variables which point to variables are not properly resolved and replaced in order. This would've prevented that situation from happening.

I've implemented a fix here: https://github.com/lukasjarosch/skipper/pull/79 Can you verify if that fix works for you?