Exa-Networks / exabgp

The BGP swiss army knife of networking
Other
2.07k stars 443 forks source link

Implement software version capability for BGP #1223

Closed ton31337 closed 1 month ago

ton31337 commented 2 months ago

https://datatracker.ietf.org/doc/html/draft-abraitis-bgp-version-capability

This is already implemented by FRR, GoBGP, freertr, Wireshark, some more.

E.g.:

$ vtysh -c 'show bgp neighbor 127.0.0.1 json' | jq '."127.0.0.1".neighborCapabilities.softwareVersion'
{
  "advertisedSoftwareVersion": "FRRouting/10.2-dev-MyOwnFRRVersion-g8ca262943f",
  "receivedSoftwareVersion": "ExaBGP/5.0.0"
}
thomas-mangin commented 2 months ago

Hi @ton31337 - thank you for this work.

Many of the tests on Python 3.7+ are now failing, and it needs investigating. I also provided two feedbacks on the code.

If we are going to provide a version, we can also not keep the 5.0.0 version as a default, as it is not right. This code is not 5.0.0 yet.

So we probably need to change 5.0.0 to something like main-{tag} or something which can be supported.

I would also say ideally documented but I never did myself so I think it would be a bit harsh to ask you to do it :-)

ton31337 commented 2 months ago

I replaced using version, which is technically OK with a normal release (installed from the package managers, etc.).

thomas-mangin commented 1 month ago

Using the version.py file is right; modifying its content for the main branch is what is required as it is already modified on release, with the exact release version.

It could be main-COMMIT-YYMMDD, which could have COMMIT as unknown when the commit is unknown and YYMMDD as the build date if the commit is unset. Otherwise, it could be the date of the exabgp.

thomas-mangin commented 1 month ago

The version is taken out of the changelog: https://github.com/Exa-Networks/exabgp/blob/main/release#L199

Therefore the github release has it. https://github.com/Exa-Networks/exabgp/blob/main/release#L57

Probably need to update the PyPI release to the same version https://github.com/Exa-Networks/exabgp/blob/main/release#L281

ton31337 commented 1 month ago

So, maybe it's just fine using version overall? It will use EXABGP_VERSION (numeric) for released packages, while branch + sha for git-based installations.

thomas-mangin commented 1 month ago

The release code needs to update how PYPI is released to match GitHub. I will deal with it. As the code could be run within a folder where the .git folder has been removed, it needs to provide a consistent output, hence my suggestion of tag and date.

ton31337 commented 1 month ago

I changed some bits, is this what we were talking about?

thomas-mangin commented 1 month ago

I can later look in the exabgp version string issue, but before I can merge, I will need a test for the OPEN.

ton31337 commented 1 month ago

Sure, will do this.

ton31337 commented 1 month ago

Seems that the version to test is non-deterministic. Will think on how to overcome this for CI/QA.

thomas-mangin commented 1 month ago

I am fixing other things so please wait for my patch:

    def json(self):
        return '{ "software-version": "%s" }' % self.software_version

is what you need to fix as {} in a fstring does not work well.

thomas-mangin commented 1 month ago

Do you want to implement https://www.rfc-editor.org/rfc/rfc9072.html ?

ton31337 commented 1 month ago

Do you want to implement https://www.rfc-editor.org/rfc/rfc9072.html ?

Sure, will check later, now traveling a bit.

thomas-mangin commented 1 month ago

The work on the software version is now completed. Thank you.