OCPBUGS-31522: Warn and allow CRD upgrade if validation fails but new CRD specifies a conversion strategy #3209
+50
−23
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
openshift-ci
bot
added
the
do-not-merge/work-in-progress
…ersion strategy Signed-off-by: everettraven <[email protected]>
everettraven
force-pushed
the
bug/crd-upgrade-conversion-warn
branch
from
4fc01b4 to
43c7bd9
Compare
everettraven
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/jira refresh |
joelanford
reviewed
Apr 30, 2024
Signed-off-by: everettraven <[email protected]>
joelanford
reviewed
Apr 30, 2024
Comment on lines
147
to
148
| if crd.Spec.Conversion == nil || crd.Spec.Conversion.Strategy == apiextensionsv1.NoneConverter || !errors.As(err, vErr) { | ||
| return fmt.Errorf("error validating existing CRs against new CRD's schema for %q: %w", step.Resource.Name, err) |
There was a problem hiding this comment.
I think we need to explicitly check for Webhook Converter to switch to the warn logic. If another converter type is added in the future, I think that should be an error, not a warning. If it turns out that new converter type is something we can't evaluate we could opt that into the warning logic at that time.
There was a problem hiding this comment.
Should be done in 209d783
joelanford
reviewed
Apr 30, 2024
| // is an error related to validation, warn that validation failed but that we are trusting | ||
| // that the conversion strategy specified by the author will successfully convert to a format | ||
| // that passes validation and allow the upgrade to continue | ||
| b.logger.Warnf("trusting conversion strategy detected in new CRD, but failed validation of existing CRs against new CRD's schema for %q: %s", |
There was a problem hiding this comment.
Two things:
- This is likely something that needs some pretty descriptive messaging. Can you try to explain a bit more about why this isn't being treated as an error.
- I think we need to use an EventRecorder to emit the warning as a kubernetes event against the InstallPlan object.
There was a problem hiding this comment.
(But keep this. Logging to the olm-operator log is helpful too)
There was a problem hiding this comment.
Should be done in 209d783
… installplan Signed-off-by: everettraven <[email protected]>
joelanford
reviewed
May 1, 2024
Comment on lines
158
to
164
| warnTempl := `validation of existing CRs against new CRD's schema failed, but a webhook conversion strategy was specified. | ||
| allowing the CRD upgrade to occur as we can't run the webhook, but is assumed that it will successfully convert existing CRs | ||
| to a format that would have passed validation. | ||
| CRD: %q | ||
| Validation Error: %s | ||
| ` |
There was a problem hiding this comment.
Suggested change
| warnTempl := `validation of existing CRs against new CRD's schema failed, but a webhook conversion strategy was specified. | |
| allowing the CRD upgrade to occur as we can't run the webhook, but is assumed that it will successfully convert existing CRs | |
| to a format that would have passed validation. | |
| CRD: %q | |
| Validation Error: %s | |
| ` | |
| warnTempl := `Validation of existing CRs against the new CRD's schema failed, but a webhook conversion strategy was specified in the new CRD. The new webhook will only start after the bundle is upgraded, so we must assume that it will successfully convert existing CRs to a format that would have passed validation. | |
| CRD: %q | |
| Validation Error: %s | |
| ` |
There was a problem hiding this comment.
Updated in 91b7f5f
Signed-off-by: everettraven <[email protected]>
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Merged
via the queue into
operator-framework:master
with commit May 2, 2024
c3e1774
14 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the change:
Motivation for the change:
Architectural changes:
Testing remarks:
Reviewer Checklist
/doc[FLAKE]are truly flaky and have an issue