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.

Sign up for GitHub

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 config to skip DHCP boot to gnoi.FactoryReset. #204

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

Add config to skip DHCP boot to gnoi.FactoryReset. #204

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1b3b706
Add config to skip DHCP boot to gnoi.FactoryReset.
daymare Jun 26, 2024
828c553
Merge branch 'openconfig:main' into FactoryReset
daymare Jul 2, 2024
b083fc5
Remove mention of Bootz from the ManagementConfig message.
daymare Jul 2, 2024
a38626e
Merge branch 'openconfig:main' into FactoryReset
daymare Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion factory_reset/factory_reset.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,9 @@ service FactoryReset {
// Optionally allows for the Target to zero-fill permanent storage where state
// data is stored.
//
// Optionally allows for the Target to skip DHCP boot and populate with the
// provided management configuration.
//
// If any of the optional flags is set but not supported, a gRPC Status with
// code INVALID_ARGUMENT must be returned with the details value set to a
// properly populated ResetError message.
Expand All @@ -34,8 +37,20 @@ message StartRequest {
bool factory_os = 1;
// Instructs the Target to zero fill persistent storage state data.
bool zero_fill = 2;
// Instructs the Target to retain certificates
// Instructs the Target to retain certificates.
bool retain_certs = 3;
// Instructs the Target to populate with the provided Ip/Routing information
// and skip DHCP boot.
ManagementConfig mgmt_config = 4;
}

message ManagementConfig {
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to use https://github.com/openconfig/bootz/blob/main/proto/bootz.proto#L225-L236 here? The idea being:

  • factory reset the config
  • apply the attached config. (which might include disabling dhcp and anything else you want to override)
  • reboot

Copy link

Choose a reason for hiding this comment

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

The problem is that this is not a traditional config.

This is a special config, that should be consumed at the start of the bootz process (which is typically invoked before any startup/running-config is involved) and by the bootz daemon only (it has no meaning to the other system components).

Copy link
Member

Choose a reason for hiding this comment

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

The bootconfig message contains a parameter bootloader_config which is intended to contain items that are invoked/processed before any startup/running config is involved. If an implementation requires bootloader_config to contain some data to enable/disable the DHCP client used during bootz, that is fine. If the implementation uses OC config to do this, that is also fine.

If my comment is not on-track with your concerns, maybe you can share more details about the implementation so we can try and map it to bootz. Happy to do this offline with you if necessary and then we can follow up with some proposal here.

message BootConfig {
  // Proprietary key-value parameters that are required as part of boot
  // configuration (e.g., feature flags, or vendor-specific hardware knobs).
  google.protobuf.Struct metadata = 1;
  // Native format vendor configuration.
  bytes vendor_config = 2;
  // JSON rendered OC configuration.
  bytes oc_config = 3;
  // Bootloader key-value parameters that are required as part of boot
  // configuration.
  google.protobuf.Struct bootloader_config = 4;
}

Copy link
Member

Choose a reason for hiding this comment

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

@LimeHat let me know what you think. Would like to keep this PR moving.

Copy link

Choose a reason for hiding this comment

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

If an implementation requires bootloader_config to contain some data to enable/disable the DHCP client used during bootz, that is fine. If the implementation uses OC config to do this, that is also fine.

There's a bit more to this proposal than a simple enablement/disablement of DHCP.
The core part of the bootz protocol is a DHCP response. It includes not only information about IP config, but also the bootstrap URL(s).

This proposal, in essence, is a way to substitute a real DHCP response with a pre-defined static message received via gNOI (pseudo-DHCP).

There are different ways to store this data. It can be stored in bootloader parameters (as you indicated, but this will be more than a flag), in can be stored in persistent memory (e.g. as a file or another implementation-dependent data structure). Probably it can't be stored as OpenConfig data (for instance, I don't think you can specify bootstrap URLs; and if the author wants to preserve compatibility with FactoryReset, it cannot be stored as a regular bootstrap cfg which is supposed to be erased during the reset). As a user, you probably don't to be concerned with implementation-specific details in order to use this.

I also don't want to couple this data with BootConfig message, for a number of reasons, including

  • in my opinion, these functions are logically independent, and,
  • overloading this structure will require the user to know and submit unneeded data to simply enable this pseudo-dhcp thing (e.g. the bytes vendor_config field will have to be present),
  • the whole SetBootConfigRequest RPC is designed to replace the data that was received from a bootstrap server, and that data only.

Personally, I still think that a new service is the best approach. If you'd prefer to extend the existing BootConfig service, I would suggest to create a new RPC for this feature, that seems like a reasonable middle ground.

Feel free to ping me in chat if you want to discuss further offline.

repeated string boot_server_uris = 1;
string device_ip = 2;
string mask_length = 3;
string gateway_ip = 4;
repeated string management_routes = 5;
repeated string dns_servers = 6;
}

message ResetSuccess {
Expand All @@ -47,6 +62,8 @@ message ResetError {
bool factory_os_unsupported = 1;
// Zero fill is not supported.
bool zero_fill_unsupported = 2;
// Management config is not supported.
bool mgmt_config_unsupported = 5;
Copy link

Choose a reason for hiding this comment

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

I know this has been flagged previously by Marcus, but I'll mention it again here in the interest of having the discussion more broadly. This field won't be a reliable indicator of what's going to happen.

Old devices that aren't aware of the mgmt_config field in the request also won't be aware of the mgmt_config_unsupported field in the response and therefore won't be able to set it to indicate their lack of awareness. Instead, they'll just silently ignore mgmt_config altogether. You won't be able to distinguish between an old device that ignored this part of your request and a new device that processed it like you requested.

Splitting the boot parameters from the FactoryReset service (like was proposed in another comment on the pull request) would make it easier to know what's going on. If you sent an old device something like SetBootstrapServerList in a BootParameters service, it would respond with an unimplemented error.

Copy link
Author

Choose a reason for hiding this comment

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

I covered this in my comment above...
Again, I don't want to make a new service and clutter the namespace when this is fundamentally tied to FactoryReset. I think adding a new RPC to factory reset would be a better way to solve this problem.

Copy link
Member

Choose a reason for hiding this comment

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

Bump the version of the service. Clients then have the option to ensure the service implements the new message.

Copy link

Choose a reason for hiding this comment

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

Bump the version of the service. Clients then have the option to ensure the service implements the new message.

gnoi package names don't include versions, hence it will not be visible

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if we are talking about the same thing, but you should be able to query service options via file descriptors via reflection if the server implementes it. Its convoluted, but its there.

gnoi/types/types.proto

Line 26 in bae5fb4

extend google.protobuf.FileOptions {

Copy link

Choose a reason for hiding this comment

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

I see what you mean. Indeed, I was thinking about a different thing (traditional grpc versioning, where the version is part of the namespace/package name). Thx for clarifying.

// Unspecified error, must provide detail message.
bool other = 3;
string detail = 4;
Expand Down