wilfred-s commented on a change in pull request #207:
URL:
https://github.com/apache/incubator-yunikorn-core/pull/207#discussion_r543366570
##########
File path: pkg/common/configs/config.go
##########
@@ -35,7 +36,7 @@ import (
// set of scheduler resources.
type SchedulerConfig struct {
Partitions []PartitionConfig
- Checksum [32]byte `yaml:"-" json:"-"`
+ Checksum string `yaml:",omitempty" json:",omitempty"`
Review comment:
I am wondering if we should split this in two. Checksum is always the
current config checksum and "UpdateChecksum" is the checksum of the config we
are updating. The reuse of the same field for two purposes is a bit strange and
looks really weird in the code. What do you think?
##########
File path: pkg/webservice/handlers.go
##########
@@ -467,16 +463,21 @@ func updateConfig(w http.ResponseWriter, r *http.Request)
{
return
}
// validation is already called when loading the config
- var newConf *configs.SchedulerConfig
- newConf, err = configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ newConf, err := configs.ParseAndValidateConfig(requestBytes)
Review comment:
the comment does not seem to cover the call anymore.
##########
File path: pkg/webservice/handlers.go
##########
@@ -495,6 +496,11 @@ func updateConfig(w http.ResponseWriter, r *http.Request) {
buildUpdateResponse(nil, w)
}
+func isChecksumValid(checksum string) bool {
+ actualConf :=
configs.ConfigContext.Get(schedulerContext.GetPolicyGroup())
+ return actualConf.Checksum == checksum
Review comment:
Is this all safe, can actualConf be nil or not?
If not we can directly get the current Checksum from it without assigning
the actualConf and return the comparison in one statement.
##########
File path: pkg/webservice/handlers.go
##########
@@ -495,6 +496,11 @@ func updateConfig(w http.ResponseWriter, r *http.Request) {
buildUpdateResponse(nil, w)
}
+func isChecksumValid(checksum string) bool {
Review comment:
this should be called `isChecksumEqual()`, we compare two checksums we
do not validate that the checksum is valid for the data given.
##########
File path: pkg/webservice/handlers.go
##########
@@ -467,16 +463,21 @@ func updateConfig(w http.ResponseWriter, r *http.Request)
{
return
}
// validation is already called when loading the config
- var newConf *configs.SchedulerConfig
- newConf, err = configs.LoadSchedulerConfigFromByteArray(requestBytes)
+ newConf, err := configs.ParseAndValidateConfig(requestBytes)
if err != nil {
buildUpdateResponse(err, w)
return
}
+ baseChecksum := newConf.Checksum
+ if !isChecksumValid(baseChecksum) {
Review comment:
This can be one call and not stored in `baseChecksum`
##########
File path: pkg/common/configs/config.go
##########
@@ -191,6 +204,13 @@ func resolveConfigurationFileFunc(policyGroup string)
string {
}
return filePath
}
+func GetConfigurationString(requestBytes []byte) string {
+ conf := string(requestBytes)
+ if strings.Contains(conf, "checksum") {
+ conf = strings.Split(conf, "checksum: ")[0]
Review comment:
Can we rely on the fact that the checksum will always be the last part
of the marshalled request?
The go yaml or json code might do that but there is no guarantee that it
will happen for all requests that come in. We need to be really careful here.
##########
File path: pkg/common/configs/config.go
##########
@@ -191,6 +204,13 @@ func resolveConfigurationFileFunc(policyGroup string)
string {
}
return filePath
}
+func GetConfigurationString(requestBytes []byte) string {
Review comment:
Missing empty line before function start
Codecov also says that we never test this call with a checksum as part of
the call. We need to cover that to make sure it works.
##########
File path: pkg/common/configs/config.go
##########
@@ -142,6 +143,21 @@ type LoadSchedulerConfigFunc func(policyGroup string)
(*SchedulerConfig, error)
// Visible by tests
func LoadSchedulerConfigFromByteArray(content []byte) (*SchedulerConfig,
error) {
+ conf, err := ParseAndValidateConfig(content)
+ if err != nil {
+ return nil, err
+ }
+ // Create a sha256 checksum for this validated config
+ UpdateChecksum(content, conf)
+ return conf, err
+}
+
+func UpdateChecksum(content []byte, conf *SchedulerConfig) {
Review comment:
`SetChecksum()` seems more logical.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
