Yyamout/public bootz server updates#314
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for GNSI configurations, including Pathz, Credentials, and CertzProfiles, within the Bootz server and entity manager. It adds logic to populate these configurations from files or direct proto messages and updates the bootstrap data response to include them. Additionally, a new DisableBootstrapStream option is introduced to deprecate the streaming RPC, and certificate generation is updated to support IP SANs. Review feedback identifies that the new population methods for Pathz and Credentials lack fallbacks to global configurations, which is inconsistent with existing implementations.
| gnsiPathzReqFile := gnsiConf.GetPathzUploadFile() | ||
| if gnsiPathzReq.GetVersion() != "" && gnsiPathzReq.GetPolicy() != nil { | ||
| return gnsiPathzReq, nil | ||
| } | ||
| if gnsiPathzReqFile == "" { |
There was a problem hiding this comment.
The populatePathzConfig function is missing a fallback to the global Pathz configuration if the chassis-specific configuration file is not provided. This is inconsistent with how populateAuthzConfig is implemented and will prevent global Pathz policies from being applied.
gnsiPathzReqFile := gnsiConf.GetPathzUploadFile()
if gnsiPathzReqFile == "" && m.defaults != nil && m.defaults.GnsiGlobalConfig != nil {
gnsiPathzReqFile = m.defaults.GnsiGlobalConfig.GetPathzUploadFile()
}
if gnsiPathzReq.GetVersion() != "" && gnsiPathzReq.GetPolicy() != nil {
return gnsiPathzReq, nil
}
if gnsiPathzReqFile == "" {| gnsiCredzReqFile := gnsiConf.GetCredentialsFile() | ||
| if len(gnsiCredzReq.GetCredentials()) > 0 || len(gnsiCredzReq.GetUsers()) > 0 || len(gnsiCredzReq.GetPasswords()) > 0 { | ||
| return gnsiCredzReq, nil | ||
| } | ||
| if gnsiCredzReqFile == "" { |
There was a problem hiding this comment.
Similar to populatePathzConfig, populateCredentialsConfig should also fallback to the global credentials configuration if the chassis-specific file is missing.
gnsiCredzReqFile := gnsiConf.GetCredentialsFile()
if gnsiCredzReqFile == "" && m.defaults != nil && m.defaults.GnsiGlobalConfig != nil {
gnsiCredzReqFile = m.defaults.GnsiGlobalConfig.GetCredentialsFile()
}
if len(gnsiCredzReq.GetCredentials()) > 0 || len(gnsiCredzReq.GetUsers()) > 0 || len(gnsiCredzReq.GetPasswords()) > 0 {
return gnsiCredzReq, nil
}
if gnsiCredzReqFile == "" {
There was a problem hiding this comment.
Could you sync your branch to the latest of main branch?
You change was done on a stale version.
|
|
||
| // DisableBootstrapStream disables the deprecated BootstrapStream RPC while | ||
| // keeping unary RPCs available. | ||
| type DisableBootstrapStream struct{} |
There was a problem hiding this comment.
We don't need to disable Streaming Bootz on the global sever level.
This is already implemented on a per chassis level.
bootz/server/service/service.go
Line 693 in 02faee8
bootz/server/service/service.go
Line 919 in 02faee8
There was a problem hiding this comment.
We are going to rewrite the entire EntityManager soon to utlize the "ChassisManager" interface.
bootz/server/service/service.go
Line 82 in 02faee8
It probably doesn't affect your PR, only as a heads-up.
gmacf
left a comment
gmacf
left a comment
There was a problem hiding this comment.
Please address merge conflicts and Michael's comments, otherwise LGTM.
3 participants
This PR adds the public Bootz server support needed for the first featureprofiles Bootz FNT case.
feature profile PR: openconfig/featureprofiles#5016
Included in here:
BootstrapStreamso tests can use the unary flowThis is intended to support the initial public featureprofiles Bootz test flow and close some gaps between the current public server behaviour and the test environment.
A couple of notes:
So this PR gets the public server into the right shape for the first test. More tests will follow up validation, and we will have medications here as needed!
Adding for reference: openconfig/featureprofiles#5016