Default values of 'enable' are ignored when its mapped in vars_map but not defined

[Lagertonne]

• [x] I have read and understood docs/Issues.md.

Describe the problem The default values in the config are ignored if enable is specified in the vars_map of a jsonfile, even if the specific device does not have an enable-key specified. I can only test it with Cisco ASA devices, but I guess it should also appear with other models.

Expected behavior If the "enable" field is empty for a device in the json-file, the default values specified in the config should be used

Configuration

source:
  default: jsonfile
  jsonfile:
    file: "/home/oxidized/.config/oxidized/router.json"
    map:
      name: name
      model: model
      username: username
      password: password
    vars_map:
      cumulus_use_nvue: cumulus_use_nvue
      cumulus_use_nclu: cumulus_use_nclu
      enable: enable # This is the line that makes oxidized ignore all other enable definitions
[...]
models:
  asa:
    vars:
      enable: "ASAenablePW"

The router.json:

[
    {
        "name": "cisco-asa1.example.com",
        "model": "asa",
        "state": "up"
    },
    {
        "name": "cisco-asa2.example.com",
        "model": "asa",
        "enable": "enablePW2",
        "state": "up"
    }
]

Running environment (please complete the following information):

• OS: Container version oxidized/oxidized:master-39aff85
• oxidized version: see container version
• oxidized-web version: see container version
• Manufacturer model an software version: Cisco ASA, but should not matter
• oxidized model name: asa

[ytti]

With debug on, what does the key resolution tell for the unexpected case.

[ytti]

Actually never mind, we don't have debugging information for this.

https://github.com/ytti/oxidized/blob/39aff85bdfcec93ee45e83dfe7303937b8a5fe5b/lib/oxidized/config/vars.rb#L5

Is your json example correct, if we did have 'enable' key, without value, we'd still stop there.

[ytti]

At any rate, we probably should use same method in node and for model vars to find the values, at least for the applicable part the code should be refactored to be shared as we expect them to behave same.

[Lagertonne]

Is your json example correct, if we did have 'enable' key, without value, we'd still stop there.

Yes, the example is correct.

I just did a little testing, I can mitigate the issue with the following change: https://github.com/Lagertonne/oxidized/commit/6e5557892cd1c98f3a2c13a29a41cb66f31052e5

If I omit this, I see a :enable => nil for devices which haven't an enable password specified, I guess this prevents any further definitions of enable

[ytti]

There are three cases here

a) node returns string "nil" which we interpolate as object nil b) node returns object nil c) node does not have the key

Your proposal breaks a+b in favour of fixing c.

         @cfg.vars_map.each do |key, want_position|
            val = string_navigate_object(node, want_position)
            vars[key.to_sym] = node_var_interpolate(val) if val
          end

Above change would break b in favour of fixing c.

To have a+b+c work, we'd probably need to have string_navigate_object raise an error when key is missing. And code should be:

         @cfg.vars_map.each do |key, want_position|
            val = string_navigate_object(node, want_position) rescue next
            vars[key.to_sym] = node_var_interpolate(val)
          end

Other potential solutions a) we only consider 'false' as negative to keep, 'nil' value is considered key-missing b) we invent magic string which causes node_var_interpolate to tell us that we don't want the value

I'm not entirely sure which direction to take, and this should be combined with refactoring the value traversal to share code between model vars and node.

[ytti]

Actually we need both nil and false literal values. So I'm not yet seeing really good solution that allows 'false, nil, fallback'.

I'm not sure if we can even rely on key not existing, since I think it's fair to assume some backends will emit key due to schema always., despite behaviourally user wanting to fall-back to next level for some value.

[ytti]

I think I know what we can do. Let's consider enable use case:

1. We do not want to enable - false
2. We want to enable, but we expect no enable password prompt - true
3. We want to enable, and we expect enable prompt, but do not have password - ""
4. We want to enable, and we expect enable prompt, and you do have password - "something"

nil is not used explicitly, so I believe we can reconsider nil but not false to mean 'no value here, look at the next level'.

Which we can achieve by two ways in ytti/oxidized/lib/oxidized/config/vars.rb

1. we #compact the hashes before we use them, removing all keys with nil values
2. we explicitly check if the key value is nil hash[key].nil? and fall to the next level if so (this is true for missing key as well as explicit nil. so has_key? is not needed, so we change has_key? to nil? and add negation)

I think this cover all cases and is also backwards compatible.

But also this is too close to node.rb #resolve_key we need to refactor to share these two codes to ensure behaviour is same.

[ytti]

I wonder if this works

      def vars(name)
        model_name = @node.model.class.name.to_s.downcase
        group = Oxidized.config.groups[@node.group]
        scopes = {
          :node => @node.vars,
          :group_model => group&.models[model_name]&.vars,
          :group => group&.vars,
          :model => Oxidized.config.models[model_name]&.vars,
          :vars => Oxidized.config.vars
        }

        scopes.each do |scope_name, scope|
          next unless scope&.has_key?(name.to_s)
          val = scope[name.to_s]

          if val.nil?
            Oxidized.logger.debug "vars.rb: scope #{scope_name} has key #{name} with value nil, ignoring scope"
          else
            Oxidized.logger.debug "vars.rb: scope #{scope_name} has key #{name} with value #{val}, using scope"
            return val
          end
        end

[ytti]

Actually above won't work, as I'm assuming ConfigStruct returns nil on missing key, I have to check them like originally.

[ytti]

Maybe this:

      def vars(name)
        model_name = @node.model.class.name.to_s.downcase
        groups = Oxidized.config.groups
        models = Oxidized.config.models
        group = groups[@node.group] if groups.has_key?(@node.group)
        model = models[model_name] if models.has_key?(model_name)
        group_model = group.models[model_name] if group&.models&.has_key?(model_name)

        scopes = {
          :node => @node.vars,
          :group_model => group_model&.vars,
          :group => group&.vars,
          :model => model&.vars,
          :vars => Oxidized.config.vars
        }

        scopes.each do |scope_name, scope|
          next unless scope&.has_key?(name.to_s)
          val = scope[name.to_s]

          if val.nil?
            Oxidized.logger.debug "vars.rb: scope #{scope_name} has key #{name} with value nil, ignoring scope"
          else
            Oxidized.logger.debug "vars.rb: scope #{scope_name} has key #{name} with value #{val}, using scope"
            return val
          end
        end
        nil
      end

Not sure I like it now tho, I'm wondering if it would be best to add new method to Asetus to get nil or key value, like:

    def get(key)
      if @cfg.has_key? key
        @cfg[key]
      else
        nil
      end
    end

Which would make above more like the original option, instead of [x] .get(x).

[ytti]

Can you test this?

https://github.com/ytti/oxidized/compare/master...refactor-vars

I'm not super happy about the 2nd proposal for refactor, compared to my 1st proposal. But it seems to work and I think is much cleaner while not as clean as it should be.

[Lagertonne]

sorry, it took some time to test this. I think this is not doing what at least I would expect. Here is the log when trying your patch. I'm using the router.json I have posted in my initial description, which means the cisco-asa2 has the variable "enable" definde in the router.json

D, [2025-06-19T10:30:03.211277 #30] DEBUG -- : lib/oxidized/input/cli.rb: Running post_login commands at cisco-asa2.example.com
D, [2025-06-19T10:30:03.211501 #30] DEBUG -- : lib/oxidized/input/cli.rb: Running post_login command: nil, block: #<Proc:0x00007c47dbf97e88 /var/lib/gems/3.2.0/gems/oxidized-0.33.0/lib/oxidized/model/asa.rb:58> at cisco-asa2.example.com
D, [2025-06-19T10:30:03.211796 #30] DEBUG -- : vars.rb: scope model has key enable with value ASAenablePW, using scope
D, [2025-06-19T10:30:03.211909 #30] DEBUG -- : lib/oxidized/input/ssh.rb "ASAenablePW" @ cisco-asa2.example.com with expect: /^\r*([\w.@()-\/]+[#>]\s?)$/
D, [2025-06-19T10:30:03.213718 #30] DEBUG -- socket[e4c]: queueing packet nr 12 type 94 len 44
D, [2025-06-19T10:30:03.214086 #30] DEBUG -- : lib/oxidized/input/ssh.rb: expecting [/^\r*([\w.@()-\/]+[#>]\s?)$/] at cisco-asa2.example.com
D, [2025-06-19T10:30:03.223727 #30] DEBUG -- : lib/oxidized/worker.rb: Jobs running: 1 of 2 - ended: 0 of 2
D, [2025-06-19T10:30:03.223901 #30] DEBUG -- : lib/oxidized/worker.rb: 1 jobs running in parallel

I would expect that the device-specific variable is used before the model one.

[ytti]

Yes as the code is written, the device (node) is first we check. Why this is not happening in your case, I don't know. I did write test cases for it, and test cases pass, but obviously I'm not covering your case.

For some reason the code is now not seeing the device variable at all. I don't have good idea yet why this might happen.

[ytti]

https://github.com/ytti/oxidized/blob/master/lib/oxidized/source/jsonfile.rb#L53

Can you change this line to:

vars[key.to_s] = node_var_interpolate string_navigate_object(node, want_position)

If that works, we need to normalise all vars to strings, but I think they are everywhere else but node.

[Lagertonne]

That fixes the issue for me!

[ytti]

Thanks for reporting back, I'll update the branch after I go through code and see where ever we populate various vars and see that we populate them all as strings.

If you could then test again that would be much appreciated.

I'm not sure if I'll get to it before early next week.

[ytti]

I've updated the branch. I have some discomfort if I've actually considered all possible cases here. And if normalisation towards strings is right solution from option space of:

1. normalize to strings
2. normalize to symbols
3. fetch both "string" and :string and prefer X

@robertcheramy what do you think about this?

I think fix is definitely needed, because use case @Lagertonne is legitimate and I'm sure other people have been disappointed by this before and just given full coverage in node level, because fallback doesn't work.

[robertcheramy]

I love the way you refactored the code, and I also think we should do something about Node#resolve_key, which contains a very similar code.

We need to be careful with the change from symbols to strings as the vars are used as symbols in the models (Example: cumulus.rb ). The CI on the branch failing on the cumulus model is probably not a coincidence.

If we choose to switch to strings, we should also switch to strings in the models, because it would be somewhat strange to work with symbols in the models and with strings under the hood.

As Asetus already works with strings [a get method that doesn't create a key when there is none would be great!], my guts say - work with strings everywhere.

I don't see an added value for option 3. Symbols are an internal thing in ruby, most people have no idea how it works under the hood and I don't think we need to expose symbols outside the code here.

I would need some more time to overview the implications in the whole code, but maybe this first comments helps you to choose the right direction.

[ytti]

It doesn't matter how we call the vars, strings or symbols, as we convert the key to string before look-up.

If I understood you correctly, you would be favour in what is currently done. That is, change source to populate node with strings, instead of symbols. And rest of them already are strings.

[robertcheramy]

It doesn't matter how we call the vars, strings or symbols, as we convert the key to string before look-up.

Agreed.

If I understood you correctly, you would be favour in what is currently done. That is, change source to populate node with strings, instead of symbols. And rest of them already are strings.

Yes (currently = commit 998d3675b58502bb32c3e3ad95d2f0f08267ff94 in branch refactor-vars). I've take some more time to look into the code. This way, we would have a consistent way of handling vars (the key is always a string).