openconfig / gnoi

gRPC Network Operations Interface (gNOI) defines a set of gRPC-based microservices for executing operational commands on network devices.
Apache License 2.0
156 stars 67 forks source link

Add gNOI API to access Boot config namespace in Bootz #156

Closed xw-g closed 5 months ago

xw-g commented 6 months ago

This API is required by the The Bootz solution

github-actions[bot] commented 6 months ago

Pull Request Test Coverage Report for Build 7496564776


Totals Coverage Status
Change from base Build 7063939200: 0.0%
Covered Lines: 0
Relevant Lines: 0

💛 - Coveralls
robshakir commented 6 months ago

Should this be called something different or in something like gnoi.System? It feels like having two things called "bootz" as a service could be quite confusing.

xw-g commented 6 months ago

@gmacf to help review too.

pranav-jnpr commented 6 months ago

Couple of suggestions here -

  1. Currently there is no option to change the boot_password_hash - https://github.com/openconfig/bootz/blob/817f8ec9e1cb141cd437e087f3f4f98e9fa76b9c/proto/bootz.proto#L125. Suggest we add a new RPC in this service to rotate the bootloader password hash. Otherwise, there is no other way to rotate this password.
  2. Second the comments around naming the service - gNOI.Bootz might be confusing - the actual Bootz service is named "Boostrap" - https://github.com/openconfig/bootz/blob/817f8ec9e1cb141cd437e087f3f4f98e9fa76b9c/proto/bootz.proto#L33. Given the intent of this service is to modify contents of the Bootz namespace, perhaps gNOI.BootzNamespace might be considered? The gNOI.BootzNamespace.GetBootConfig() and gNOI.BootzNamespace.SetBootConfig(), and potentially gNOI.BootzNamespace.SetBootloaderPasswordHash() would identify the gNOI operations.
pranav-jnpr commented 5 months ago

Additionally,

An advantage of the approach is that there is clean separation according to the operational use case described above - a gNSI namespace exists for policies on the device; the ‘boot’ namespace would exist for configuration that is immutable after device boot; and the existing configuration becomes the dynamic configuration of the device. Each namespace can have its own policies for permissions, and client service. Client services need not know about each other’s configuration since replacing a configuration replaces only the namespace of that client.

https://github.com/openconfig/bootz/blob/main/README.md#multiple-namespaces-for-the-configuration

Suggest to change the highlighted portion to "the ‘boot’ namespace would exist for configuration that is immutable by the gNMI after device boot, requiring gNOI.Bootz/gNOI.BootzNamespace to change the configuration" as mentioned in the "Proposed Solution" section - Boot configuration is expressly defined to be immutable, and provided only at the time of device boot. If the boot configuration is required to be modified, it would be changed through calling a Set RPC through the Bootz service.

xw-g commented 5 months ago

still use BootConfig

Should this be called something different or in something like gnoi.System? It feels like having two things called "bootz" as a service could be quite confusing.

updated.

xw-g commented 5 months ago

Couple of suggestions here -

  1. Currently there is no option to change the boot_password_hash - https://github.com/openconfig/bootz/blob/817f8ec9e1cb141cd437e087f3f4f98e9fa76b9c/proto/bootz.proto#L125. Suggest we add a new RPC in this service to rotate the bootloader password hash. Otherwise, there is no other way to rotate this password.
  2. Second the comments around naming the service - gNOI.Bootz might be confusing - the actual Bootz service is named "Boostrap" - https://github.com/openconfig/bootz/blob/817f8ec9e1cb141cd437e087f3f4f98e9fa76b9c/proto/bootz.proto#L33. Given the intent of this service is to modify contents of the Bootz namespace, perhaps gNOI.BootzNamespace might be considered? The gNOI.BootzNamespace.GetBootConfig() and gNOI.BootzNamespace.SetBootConfig(), and potentially gNOI.BootzNamespace.SetBootloaderPasswordHash() would identify the gNOI operations.

I'm not sure if we should add boot_password_hash here, or should think about moving boot_password_hash to bootz.BootConfig proto in the bootz.proto

But anyway it probably shouldn't stop the merging of this PR.