Infer/Set Blank APIVersion - Bug 1982737

[nsapse]

Description of the change:
This commit modifies creation of the CSV passed to ownerReferences by setting it to v1alpha1 (inferred from the fact that it is being used for all other purposes) in the case that it has been left blank in the original bundle.

Motivation for the change:
In previous versions of OLM is a user passed in a bundle missing an APIVersion the install plan would fail with a difficult to decipher message stating it had failed to create a service account for the metadata.ownerReferences.apiVersion: Invalid value: "": version must not be empty'. The original bug report asked for the following in response:

1. For OPM index add to fail if an APIVersion had not been set in the bundle.

To enforce separation of concerns the proper place for this to fail is with Operator SDK Validate. This error is it is in fact caught properly by the SDK and fails at that stage as expected.

2. For the InstallPlan to fail with a message pointing the user to an error in the bundle's CSV.

Rather than update the error message this change fixes the underlying issue preventing the InstallPlan from failing.

Reviewer Checklist

• Implementation matches the proposed design, or proposal is updated to match implementation
• Sufficient unit test coverage
• Sufficient end-to-end test coverage
• Docs updated or added to /doc
• Commit messages sensible and descriptive

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: nsapse
To complete the pull request process, please assign ecordell after the PR has been reviewed.
You can assign the PR to them by writing /assign @ecordell in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci]

Hi @nsapse. Thanks for your PR.

I'm waiting for a operator-framework member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[tylerslaton]

/ok-to-test

[benluddy]

What happens if the manifest was intended to represent a v2beta3 CSV? Say there were some new fields added -- wouldn't they have been lost during unmarshaling into the v1alpha1 struct? An error is desirable here.

[njhale]

/hold

Just had a long conversation with @benluddy about this and I agree that we shouldn't infer the version is v1alpha1.

I originally suggested inference on the false assumption that a bundle could be installed today w/o the CSV version field. As it turns out, most -- I really think all -- bundles that don't specify the CSV version would cause the InstallPlan to fail on the same ownerreference issue. This means backwards compatibility is no longer a problem for solutions that don't support unspecified versions.

That being said, instead of inferring the version, I think we should fail InstallPlans earlier in their execution and output a more helpful message to their status when we notice an unspecified version. We should already be doing this for resources that use the dynamic (untyped) client (so switching to that for POSTing CSV could be helpful too).

[nsapse]

Per conversation above PR has been closed in favor of working on an alternate solution in PR#2673