censor versioning [T971] #321
Conversation
…t-version-as-metric
| // LoadConfiguration loads configuration of AcraCensor | ||
| func (acraCensor *AcraCensor) LoadConfiguration(configuration []byte) error { | ||
| var censorConfiguration Config | ||
| err := yaml.Unmarshal(configuration, &censorConfiguration) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| configVersion, err := utils.ParseVersion(censorConfiguration.Version) |
There was a problem hiding this comment.
current censor configs don't have version, so it can be parsed. shouldn't we return ErrUnsupportedConfigVersion`?
There was a problem hiding this comment.
it will return this error and log will looks like:
[...] "Can't setup censor" error="VERSION value has incorrect format (semver 2.0.0 format expected, https://semver.org/)"
There return another error value to distinguish cases when version is missed/has incorrect value and version is outdated. Maybe better to return 3 different error values: missed version/incorrect format/outdated?
Plus I think to change "VERSION value has incorrect format (semver 2.0.0 format expected, https://semver.org/)" -> "version value has incorrect format (semver 2.0.0 format expected, https://semver.org/)". Because at start it was a reference to variable VERSION
There was a problem hiding this comment.
added a check on empty version and return ErrUnsupportedConfigVersion
| if err != nil { | ||
| return err | ||
| } | ||
| if censorVersion.Compare(configVersion) == utils.Greater { |
There was a problem hiding this comment.
| if censorVersion.Compare(configVersion) == utils.Greater { | |
| if currentlySupportedVersion.Compare(configVersion) == utils.Greater { |
| return err | ||
| } | ||
| if censorVersion.Compare(configVersion) == utils.Greater { | ||
| // censor has version newer than config |
There was a problem hiding this comment.
I think we should log error in human readable form, smth like
acraCensor.logger.
WithField(logging.FieldKeyEventCode, logging.EventCodeErrorCensorSetupError).
Errorln("AcraCensor config file version is not supported: probably AcraCensor configuration (acra-censor.yaml) is outdated, check docs for deprecation warnings https://docs.cossacklabs.com/pages/documentation-acra/#acracensor-acra-s-firewall")
| {"1.0.0", "0.0.0", Greater}, | ||
| {"0.0.0", "1.0.0", Less}, | ||
|
|
||
| {"0.1.0", "0.0.0", Greater}, |
There was a problem hiding this comment.
| {"0.1.0", "0.0.0", Greater}, | |
| {"0.1.0", "0.0.0", Greater}, | |
| {"0.84.2", "0.1.0", Greater}, | |
| {"0.84.2", "1.0.0", Less}, |
There was a problem hiding this comment.
added (but imho it test the same cases : )
| send_signal_by_process_name('acra-server', signal.SIGTERM) | ||
| send_signal_by_process_name('acra-connector', signal.SIGTERM) | ||
| send_signal_by_process_name('acra-server', signal.SIGKILL) | ||
| send_signal_by_process_name('acra-connector', signal.SIGKILL) |
There was a problem hiding this comment.
Why do we need to use SIGKILL instead of SIGTERM? Is there any problem with process termination during test?
There was a problem hiding this comment.
siterm + sigkill after timeout sent via stop_process function above that uses pid's that was returned after expected fork from our code. send_signal_by_process_name use pidof to finally kill any zombie processes that may be forked after unexpected sighup signal. because after sighup the processes that we forked from tests create own fork with new pid that we don't know from test environment and terminate itself.
in general cases send_signal_by_process_name will do nothing because all processes was terminated correctly before.
There was a problem hiding this comment.
Thanks for the explanation.
var MinimalCensorConfigVersion = "0.84.2". If version in config is older than minimal then log error message and stop acra-server. But don't know how to check it better. Compare with version in config or with version of acra-server (also hardcoded). And which version will be supported by censor, less/greater/equal. When major/patch equals and ignore patch or full version value or maybe set minimal version (as it done in PR) and update hardcoded version after censor's updates?go testrun