CBG-3867 ban empty replications or replication id on REST calls #6895
Conversation
rest/admin_api.go
Outdated
| @@ -60,7 +60,8 @@ func (h *handler) handleCreateDB() error { | |||
| return base.HTTPErrorf(http.StatusBadRequest, err.Error()) | |||
| } | |||
|
|
|||
| if err := config.validate(h.ctx(), validateOIDC); err != nil { | |||
| isUpsert := true | |||
There was a problem hiding this comment.
Create db shouldn't be considered an upsert - are we using the wrong terminology for the legacy case?
rest/config.go
Outdated
| @@ -676,7 +676,8 @@ func (dbConfig *DbConfig) validatePersistentDbConfig() (errorMessages error) { | |||
|
|
|||
| // validateConfigUpdate combines the results of validate and validateChanges. | |||
| func (dbConfig *DbConfig) validateConfigUpdate(ctx context.Context, old DbConfig, validateOIDCConfig bool) error { | |||
| err := dbConfig.validate(ctx, validateOIDCConfig) | |||
| isUpsert := true | |||
| err := dbConfig.validate(ctx, validateOIDCConfig, isUpsert) | |||
There was a problem hiding this comment.
At this point for a POST _config we've already performed a merge between the incoming request and the existing db config. It seems like if there's an existing replication in the config with id="", and we're making an upsert unrelated to replications, we're going to reject that update here. Given that, do we think this is the right place to be performing the empty replication ID check? Or is there something in the handling for existing replications I'm overlooking.
rest/config.go
Outdated
| func (dbConfig *DbConfig) validate(ctx context.Context, validateOIDCConfig bool) error { | ||
| return dbConfig.validateVersion(ctx, base.IsEnterpriseEdition(), validateOIDCConfig) | ||
| // validate checks the DbConfig for any invalid or unsupported values and return a http error. If isUpsert is true, add any checks that might be upposrte | ||
| func (dbConfig *DbConfig) validate(ctx context.Context, validateOIDCConfig bool, isUpsert bool) error { |
There was a problem hiding this comment.
I don't like adding the conditional 'isUpsert' parameter here - previously this function's usage was clear - pass a fully populated dbconfig, and find out if it's valid. Conditional handling based on the type of update makes that quite a bit harder to reason about for me.
If the more correct name for the parameter is 'isLegacyConfig' that might be an improvement?
There was a problem hiding this comment.
I changed the name of this to be validateReplications to indicate whether we want to validateReplications. It does make it narrower but hopefully easier to understand when we want to do it.
allow them for legacy config reading, but print a warning
|
Added a feature to not overwrite the replications from a dbConfig, since overwriting them would mean that the replications in CfgSG get updated every time the database gets reloaded, despite updates to /db/_replication/. I added warning code to |
db/sg_replicate_cfg.go
Outdated
| base.WarnfCtx(ctx, "An ISGR replication with an empty key exists. To fix this, this replication should be removed from database config and directly from edit the database config and the %q document. %s", | ||
| m.dbContext.MetadataKeys.SGCfgPrefix(m.dbContext.Options.GroupID), resetCheckpointMsg) | ||
| } else if replication.ID == "" { | ||
| base.WarnfCtx(ctx, "An ISGR replication with an empty `id` exists. To fix this, update the replication using PUT /%s/_replication/%s. %s", m.dbContext.Name, replicationID, resetCheckpointMsg) |
There was a problem hiding this comment.
I think avoiding back-ticks in log lines would be preferred, and we decided at one point to try to standardize on ID.
| base.WarnfCtx(ctx, "An ISGR replication with an empty `id` exists. To fix this, update the replication using PUT /%s/_replication/%s. %s", m.dbContext.Name, replicationID, resetCheckpointMsg) | |
| base.WarnfCtx(ctx, "An ISGR replication with an empty ID exists. To fix this, update the replication using PUT /%s/_replication/%s. %s", m.dbContext.Name, replicationID, resetCheckpointMsg) |
* CBG-3867 ban empty replications or replication id on REST calls allow them for legacy config reading, but print a warning * add test * don't overwrite replications * Add check to validate replications in sgReplicateManager when they are started * Add test to ensure we do not overwrite replications from the database config * Skip test if not EE * provide credentials for connecting * Clarify error text
allow them for legacy config reading, but print a warning.
This is a follow on to 84997a0
I could potentially remove the call in
PutReplicationsbecause that will now be hit earlierdbConfig.validatePre-review checklist
fmt.Print,log.Print, ...)base.UD(docID),base.MD(dbName))docs/apiIntegration Tests
GSI=true,xattrs=truehttps://jenkins.sgwdev.com/job/SyncGateway-Integration/2523/ (unrelated failure)