datacenter / acitoolkit

A basic toolkit for accessing the Cisco APIC
Other
347 stars 266 forks source link

Handling the Removal of the 'ip' Key in the 'fvCEp' Object for ACI 5.1 #374

Closed timway closed 2 years ago

timway commented 3 years ago

Addresses the Dec 15, 2020 remark in https://www.cisco.com/c/en/us/td/docs/dcn/aci/apic/5x/release-notes/cisco-apic-release-notes-511.html

Let me know if there is a better way we should think about detecting and managing version incompatibilities or if we want to address handling multiple fvIps.

WIP solution for #373

JonasKs commented 3 years ago

Disclaimer: I've not worked on or with this library, but I'm working with ACI a lot so I sometimes come here to check what's up.

In ACI 5.1 the fvCEp object no longer contains a key for IP

Being deprecated shouldn't mean that it no longer exist, but I can see in #373 that it does. Bad wording on Cisco's part.

The replacement code tries the original assignment and if that fails grabs the first fvIp object and uses that 'addr'

This don't really go well with this:

This change provides better support for having multiple IP addresses on the same MAC address.

So in my opinion the ip field should either be a comma separated string (if backwards compatibility is wanted here), or a list of IPs (my preferred solution).

timway commented 3 years ago

Thank you for the reply, I left it as a single IP since that's what it would return in the older versions (and that might be depended on). I wasn't clear on the logic as far as how ACI chose the IP that was present in that field so I just picked the first valid IP I found (v4 or v6).

I agree that a larger change should be considered to include handling a fvCEp with more than 1 IP. Changing the endpoint.ip to a list (or string separated by a comma) would potentially be breaking but would maintain the way IPs are displayed in a fairly easy way. It may be better to add or change to something like endpoint.ips that is a list of all IPs on that fvCEp. The display code could be further adjusted to parse that list long term. This would leave the single endpoint.ip functionality in place.

We also could do something regarding versioning instead of a try / catch to more deliberate that we're targeting this change.

osanchez42 commented 1 year ago

not to bring this issue out of the grave, but why was this issues resolution not merged into the toolkit master.