Warn instead of fail on failure to validate against DB #2668
Conversation
When the database has somehow stored the empty string for a field we wish to validate against, we cannot perform any reasonable validation. Instead of making this a hard failure, print a warning that we cannot validate, and bad things may happen. Signed-off-by: Matthew Heon <matthew.heon@pm.me>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mheon The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
approved
| // If DB value is the empty string, we ought to be fine. | ||
| // (Though I'm honestly not sure how we got this far | ||
| // with an empty-string DB config) | ||
| if string(keyBytes) == "" { |
There was a problem hiding this comment.
Should we update the data with the correct value?
There was a problem hiding this comment.
The problem is that we don't really know the correct value here - the DB is supposed to know, and it doesn't, so we can only hope that we're using the right storage driver.
There was a problem hiding this comment.
I think I've figured out why we're entering "" in the first place though. Going to fix that next.
|
One question, but I am fine with this change. |
|
LGTM |
|
Added one more commit to not unconditionally overwrite storage defaults on a configuration being present when we run as root - I think this might resolve our empty-string storage driver issues |
Signed-off-by: Matthew Heon <matthew.heon@pm.me>
mheon
force-pushed
the
fix_db_validation_emptystring
branch
from
e9fb9a5 to
31b12dc
Compare
|
LGTM |
| @@ -297,6 +297,9 @@ func GetDefaultStoreOptions() (storage.StoreOptions, error) { | |||
| err error | |||
| ) | |||
| storageOpts := storage.DefaultStoreOptions | |||
| if !rootless.IsRootless() { | |||
There was a problem hiding this comment.
This code makes little sense. Above you set storageOpts to DefaultStoreOptions, then you overwrite it in both root and rootless mode.
|
@mheon Any update on this PR? |
|
@rhatdan I think we're still good to proceed, unless this code already moved out into c/storage |
|
☔ The latest upstream changes (presumably #2789) made this pull request unmergeable. Please resolve the merge conflicts. |
|
@mheon: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
openshift-ci-robot
added
the
needs-rebase
|
and you need to merge too apparently |
|
I'm having second thoughts about this one, might just close it... |
|
This isn't worth doing, I think |
github-actions
bot
added
the
locked - please file new issue/PR
When the database has somehow stored the empty string for a field we wish to validate against, we cannot perform any reasonable validation. Instead of making this a hard failure, print a warning that we cannot validate, and bad things may happen.