Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gnoi: add service for extension management #160

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

brianneville
Copy link
Contributor

Add the Extension service to allow for network operators to manage extensions on the system, where an extension is some arbitrary patch or upgrade which can be applied to services running on the system.

These extensions can have signatures which may be verified (e.g. a signature may identify an extension as originating from the vendor).

The extension.proto file details proposed workflows for managing extensions (including tranferring those extensions onto the system, configuring signature verification options, and install/uninstall workflow)

Add the Extension service to allow for network operators
to manage extensions on the system, where an extension is
some arbitrary patch or upgrade which can be applied to
services running on the system.

These extensions can have signatures which may be verified
(e.g. a signature may identify an extension as originating from
the vendor).

The extension.proto file details proposed workflows for managing
extensions (including tranferring those extensions onto the system,
configuring signature verification options, and install/uninstall
workflow)
Comment on lines 11 to 15
// Extension is a service for managing extensions on the system.
// Only one RPC (SetSignatureVerification, FinalizeExtensions,
// InstallExtension, UninstallExtension)
// may be in progress at any given time
service Extension{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is meant by an extension? Is it a software bundle that runs on a NOS?
The term seems to be not so well known...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an extension here is just an bundle of arbitrary files (e.g. RPMs) which can be installed and applied over the system to modify the behaviour of some services on the system.
This is very generic, theres nothing specific here about what an extension must do or any requirements for the extension file itself.
I've added a comment in the next commit ea29e01

As an example of this:
Some network operator experiences a bug with BGP services, and a vendor may provide that network operator with an extension which can be installed to apply a fix for that bug. The network operator transfers the extension (verifying that it was signed by the device manufacturer if desired) and then activates it, which applies the fix to BGP.

Network operators may prefer doing this rather than performing a full upgrade to a new OS version using gnoi OS, as an extension can be applied without installing a new software image, and it doesnt affect any unrelated services

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hellt -

the notion of extension installation is pretty common across NOS. this might take the form of vendor-provided (and signed) extensions or the installation of common linux utilities that extend the base capabilities of the NOS.

examples include:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sulrich yes, the concept is clear, it is just that "extension" seems to be a peculiar choice for a package.

But maybe it is just me :)

Comment on lines +168 to +177
message TransferRequest{
oneof request {
// name of the extension (including the file extension)
string extension_name = 1;
// raw byte contents to be streamed into the extension file.
bytes contents = 2;
// hash of the file.
types.HashType hash = 3;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we want to have multiple file transfer ops spread across different services.

System.Package - https://gnxi.srlinux.dev/gnoi/system#SetPackageRequest looks to be a close match

Copy link
Contributor Author

@brianneville brianneville Jan 31, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we want to have multiple file transfer ops spread across different services.

I am ok with having multiple file transfer ops across services, mainly because:

  1. gNSI Authz is service/RPC based. If we delegate that all file transfer operations must be done by some common RPC (e.g. gNOI File/Put RPC), then this means that for a system running gNSI Authz, all clients must also be authorized to run the gNOI File/Put RPC, which would enable them to put any file on the system that they want.
    I think its better to build the file-transfer operations into the RPC itself, so that the a client created for the purpose of "transferring extensions onto the system" is only given the ability to do that, rather than general access to the whole filesystem.

  2. (smaller point) for ease of use, i like that a single client (one for only the Extension service) can be used to interact with a service, rather than creating multiple clients (one for File and another for Extension)

System.Package - https://gnxi.srlinux.dev/gnoi/system#SetPackageRequest looks to be a close match

I mention this in a separate comment here:
#160 (comment)


// SetSignatureVerification sets the methods for which the
// system should verify incoming extensions
rpc SetSignatureVerification(SigVerificationRequest)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the motivation for this being a separate RPC which then implies there's some state held across requests? It seems like it could just be a flag to Install....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was made a separate RPC as there is state being held across requests :)
the intention is that the signature verification method is global config (e.g. it may mark a single SSL profile as being the chosen profile to verify against).
It can be set once before any extensions are installed, so that the same config is applied to all extensions installed afterwards (in the case of multiple InstallExtension RPCs), rather than relying on the client to re-specify the same option in their requests.

// extension will not be applied until the affected services are reloaded.
// To reload services after installing extensions, issue a
// FinalizeExtensions RPC after finishing the InstallExtension RPC.
rpc InstallExtension(stream InstallExtensionRequest)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think someone else observed this in another comment, but there is gnoi.System.SetPackage which was designed to be a "generic" means to install a package on a system. gnoi.os.Install was created specifically because there were a lot of semantics around NOS installation that were beyond SetPackage.

Here, what's the motivation that separates this RPC from SetPackage? Do you envisage that we deprecate SetPackage if this is merged?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll confess to not knowing exactly what System.SetPackage is supposed to be used for given the current gNOI services.
from the description:

gnoi/system/system.proto

Lines 53 to 54 in f62dc1b

// SetPackage places a software package (possibly including bootable images)
// on the target. The file is sent in sequential messages, each message

It seems to be a very generic way to download packages, where i dont think its very clear what a package is (this may be intentional - perhaps a package is just any bundle of files).

The comment alludes to the RPC being potentially used to download bootable images, however this functionality was later implemented in the gNOI OS package, with this comment from the PR explaining how SetPackage is too generic to really capture the nuances with OS installation: #19 (comment)
I'd argue that the same is true here with extensions.
There's more to installing an extension than just file transfer and "activation" (which is really all that the SetPackage RPC affords).

Some examples of differences here between Extension RPCs and SetPackage:

  • SetPackage only allows for activation of a package after transfer, it has no capability to "activate" a package which is already present on the device
  • SetPackage only has one option, either to "activate" a package or not:

    gnoi/system/system.proto

    Lines 320 to 323 in f62dc1b

    // Indicates that the package should be made active after receipt on
    // the device. For system image packages, the new image is expected to
    // be active after a reboot.
    bool activate = 5;

    There are other options which are relevant to extension installation, related to signature verification, dependency checks, whether this extension should be continuously applied after reboots.
    This is specific to installing an extension and not just something that would be relevant to any generic package.
  • Extension allows for the installation/activation workflow to be spread over multiple RPCs (InstallExtension and FinalizeExtensions, which gives an opportunity for a network operator to install multiple extensions in sequence, verifying that they all succeed before finalizing and reloading the relevant services.

Here, what's the motivation that separates this RPC from SetPackage? Do you envisage that we deprecate SetPackage if this is merged?

I would be ok with deprecating it, but im not sure how widely used it is or what other vendors/network operators use it for.
Deprecating it would be a different PR/issue though, made as a followup after this PR

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The spec here doesn't seem to really capture the nuances that you mention above that mean that SetPackage isn't the right solution. Can I suggest that we start with a brief design document that describes what we're trying to solve here -- particularly covering the primitives that you propose so that other implementors and operators can comment on this?

Note all the things that you mention about SetPackage could be done by augmenting its functionality to include the specific gaps. I don't know that is the right solution, but I also don't have a good picture about what workflows you're trying to achieve here -- which the design doc would help close.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a new commit with a README.md file outlining what an extension is and how the typical extension installation workflow would proceed, please take a look.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi rob,

Can you take a look at the README.md in this PR?
It would be good to get this PR finalized

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to support List extensions here initially and perhaps later adding this to gNMI if it makes sense.

added an RPC to provide this functionality in 2365ad9

Do you think there should be a third oneof type in InstallExtensionRequest to Activate extensions? I think there is a distinction between installation and activation or is the intent to activate the extension during the call to FinalizeExtensions?

The intent is indeed to separate the "installation" stage from the "activation" stage. This means that a network operator can add multiple extensions (e.g. one extension and another extension which is a dependency) and then activate all of them at once with a single FinalizeExtensions call.
As FinalizeExtensions will reload some of the running processes on the device, this also carries some risk of reloading the process hosting the gNOI server (which would disconnect the client)
I also think it feels safer to move that risk into a dedicated RPC, so the client can understand which RPCs may result in a disconnect, rather than including that risk in InstallExtension

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah understood - thanks for the clarification.

@earies @aaronmillisor @harveyyoung Would you be able to take a look at this and comment whether this is compatible with your respective NOSs?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just trying to follow up here. Any comment on this? It'd be good if we can finalize this PR

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am also following up here, as we'd like to make progress internally. What will it take to get this over the finish line?

@alshabib if we are unable to get further input, can we look at approving this?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@robshakir , same question but directed to you I guess?

update the proto to reflect that InstallExtension is a bidirectional
streaming RPC.
As shown in case 2 - the server should be able to send back
multiple responses if necessary
specify the signature verification method in the
InstallRequest message instead.
RPC to show the state of the current
extensions on the system
the signature verification is no
longer global config, it is set for
each installation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants