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

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

Merged
merged 9 commits into from
Feb 4, 2024
Merged

Conversation

xw-g
Copy link
Contributor

@xw-g xw-g commented Dec 29, 2023

This API is required by the The Bootz solution

Copy link

github-actions bot commented Dec 29, 2023

Pull Request Test Coverage Report for Build 7496564776

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 0.0%

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

💛 - Coveralls

@robshakir
Copy link
Contributor

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 xw-g marked this pull request as ready for review January 5, 2024 19:42
@xw-g xw-g requested a review from marcushines January 5, 2024 19:43
@xw-g
Copy link
Contributor Author

xw-g commented Jan 5, 2024

@gmacf to help review too.

@pranav-jnpr
Copy link

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
Copy link

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 xw-g changed the title Add gNOI.Bootz API Add gNOI API to access Boot config namespace in Bootz Feb 1, 2024
@xw-g
Copy link
Contributor Author

xw-g commented Feb 1, 2024

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
Copy link
Contributor Author

xw-g commented Feb 1, 2024

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.

@xw-g xw-g merged commit 9708792 into main Feb 4, 2024
7 of 10 checks passed
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.

4 participants