Add Cisco FTD model

[mgrocock-cwcs]

Pre-Request Checklist

• Passes rubocop code analysis (try rubocop --auto-correct)
• Tests added or adapted (try rake test)
• Changes are reflected in the documentation
• User-visible changes appended to CHANGELOG.md

Description

Add a new model for backing up Cisco FTD firewalls via the HTTP API. Includes supporting changes to the HTTP input:

• Support overriding port
• Support DELETE method

[ytti]

I'm not familiar with the model. Why is the delete of the config needed?

Also, why are we using vars(ip) instead of node.ip? I think we have few other cases where problem is that users want to use name, that is not resolved for connecting, but files are stored at such name.

And this is valid issue, and if this is what is going on, then I think right solution is, that for the cases where people want some display_name that issued and another IP or mgmt_name that is used to connect, we need to support this on node level, not vars.
So it would be separate issue.

For example we could have node.mgmt_name, and if that is defined, we leave node.name alone, and resolve (if name) this to IP we'll connect to.

[mgrocock-cwcs]

The configexport API call saves a backup to the device as a Zip file with the specified filename (oxidized.zip in this case). This file is then downloaded, and the JSON configuration extracted from it. The first delete is to ensure that there is no existing backup with the same filename, otherwise the configexport would fail, and the second is to tidy up afterwards.

I considered including a timestamp or random component in the filename, but decided against this as it could result in a buildup of old backups on the device if jobs are interrupted for any reason, and instead took the approach of using a static filename that was unlikely to be used for anything else.

We use vars(:ip) as the devices we're backing up generally have SNMP responding on the outside interface, so this is the address we use when adding them to LibreNMS, but the HTTP API that Oxidized needs to connect to is listening on the management interface. Our approach is to define mapping rules in LibreNMS that assign each device to its own group, and then to set the ip variable for each group in /etc/oxidized/config.

If vars(:ip) is undefined, then it will default to using @node.ip.

[ytti]

Sorry I think you need to elaborate more.

why cannot you use node.ip and/or node.name?

The common reason seems to be that
a) name does not resolve, but is desirable to use as filename
b) management name does resolve, but is not desirable to use as filename

currently we are not handling that case, and that case could justify adding node.mgmt_name, which is then resolved into node.ip. But node.name still used for storage.

So I just want to understand specifics of your case, so that however we solve the existing gap also contains solution to your problem.
It almost guaranteed solution isn't models using vars(:ip). I'm struggling to see reason to do that, having node.ip that works seems to be minimum requirement. I could potentially understand if you need node.ip for some connection and you need additional ip for some other case, but that doesn't seem to be case here, you're either using node.ip or vars(:ip), at which point, why not just use node.ip.

We already today support hooks for sources, where you can pick-up e.g. node.ip or node.name conditionally from source, which could also cover your issue.

[mgrocock-cwcs]

You're right, we can simply use a mapping rule in LibreNMS to ensure that @node.ip is set appropriately. I have updated the pull request so that it no longer uses vars(:ip).

[ytti]

Can you check the Dockerfile and make a judgement call if the ruby zip should be added there or not. It'll either use distribution packaged if added there, or gem if not.
I think idea is, if distribution has it, it is preferred. But I'm not making any requirements.

[ytti]

Are you sure raising the error is prudent? I don't think this will be rescued by input during collection? That is, I think entire oxidized will crash when you raise, and I feel like you only intended the particularly run to fail, not oxidized to fail?

I think it is reasonable to raise error from class, but there probably should be some subclass which then gets :warn, instead of crash curing fetch. Like InputFailed, GetFailed, FetchFailed (I don't know if these are good names, I didn't think about it much).
Possibly inheritance such as ThisError inherits InputError, which Inherits OxidizedError. And then ThisError recovered if raised.

[ytti]

Tiny nitpick, but generally I like small methods, with focused logic and as little repetition as possible. So things you could consider (not making any asks). Are like:

def check_job_status(job)
  return if job['status'] == 'SUCCESS'

  message = case
            when job['status'] == 'FAILED'
              job['statusMessage']
            when job['error']
              job.dig('error', 'messages', 0, 'description')
            else
              'unknown error'
            end

  raise Oxidized::SomeSoftErrorThatWeCatchAndWarnInsteadOfCrash, message
end

There are some other opportunities for what I perceive as clarity and readability, but again, not making any mandates here, it'll be merged anyway once we sort the raising and docker issue.

For example / isn't valid variable name, so I think brackets are redundant and you can have just "foo/#@bar/baz" and it'll work, because / terminates the bar.

[robertcheramy]

It would also be great to update the tests for input/http for the new features (delete + custom port).

[mgrocock-cwcs]

Can you check the Dockerfile and make a judgement call if the ruby zip should be added there or not. It'll either use distribution packaged if added there, or gem if not. I think idea is, if distribution has it, it is preferred. But I'm not making any requirements.

I've added ruby-zip to the Dockerfile.

[mgrocock-cwcs]

Are you sure raising the error is prudent? I don't think this will be rescued by input during collection? That is, I think entire oxidized will crash when you raise, and I feel like you only intended the particularly run to fail, not oxidized to fail?

I think it is reasonable to raise error from class, but there probably should be some subclass which then gets :warn, instead of crash curing fetch. Like InputFailed, GetFailed, FetchFailed (I don't know if these are good names, I didn't think about it much). Possibly inheritance such as ThisError inherits InputError, which Inherits OxidizedError. And then ThisError recovered if raised.

I've now realised that I can fail the run without causing a crash by simply returning false. The approach I've taken is to create and FTDError class that inherits from OxidizedError, raise that where appropriate, and then rescue it at the end of the callback, log the message and return false.

[mgrocock-cwcs]

Tiny nitpick, but generally I like small methods, with focused logic and as little repetition as possible. So things you could consider (not making any asks). Are like:

def check_job_status(job)
  return if job['status'] == 'SUCCESS'

  message = case
            when job['status'] == 'FAILED'
              job['statusMessage']
            when job['error']
              job.dig('error', 'messages', 0, 'description')
            else
              'unknown error'
            end

  raise Oxidized::SomeSoftErrorThatWeCatchAndWarnInsteadOfCrash, message
end

There are some other opportunities for what I perceive as clarity and readability, but again, not making any mandates here, it'll be merged anyway once we sort the raising and docker issue.

For example / isn't valid variable name, so I think brackets are redundant and you can have just "foo/#@bar/baz" and it'll work, because / terminates the bar.

I've separated the code out into smaller methods, split some longer lines up to make them more readable, and amended check_job_status along the lines suggested.

The suggested change from "foo/#{@bar}/baz" to "foo/#@bar/baz" upsets rubocop, so I've left those alone.

[mgrocock-cwcs]

It would also be great to update the tests for input/http for the new features (delete + custom port).

I've added tests for the custom port and DELETE method in spec/input/http_spec.rb. I re-worked the way I'd implemented the custom port slightly to fit better with the existing tests.

[robertcheramy]

The PR ist OK for me. @ytti - for you OK?

Note: the sleep(@poll_wait) in the polling loop runs inside the global timelimit (default: 300s), which wraps the entire node run.
If a user increases values, ftd_polls × ftd_poll_wait (plus HTTP round-trips) could exceed timelimit, causing the job to be aborted with status timelimit. I'm sure if it worth noting this in FD.md, as the default values are way below the global limit.

[ytti]

From style perspective, I’m not happy. It feels strange how the methods are made, and wrapping them inside method feels ugly to me.
But these are subjective, and models arent beaty competition.

If @robertcheramy is fine by it, I’m fine by it.