Catalog switcher #2281

jchunkins · 2021-07-22T19:13:21Z

Description of the change:

Implementation of catalog switching enhancement proposal

Quick check for e2e test:

make e2e-local TEST='adding catalog template adjusts image used'

Motivation for the change:

See summary and motivation of enhancement proposal.

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 · 2021-07-22T19:13:30Z

Hi @jchunkins. 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.

go.mod

kevinrizza · 2021-07-22T20:13:06Z

/ok-to-test

pkg/controller/operators/catalogtempate/operator.go

kevinrizza · 2021-07-22T22:53:11Z

/approve

kevinrizza · 2021-07-22T22:53:32Z

/hold

pkg/controller/operators/catalogtempate/operator.go

ecordell · 2021-07-22T23:00:47Z

pkg/lib/catalogsource/image_template.go

+// of a value in the slice means that the template was either not found in the cache or its value has not been
+// fetched yet). Providing an empty catalogImageTemplate results in empty processedCatalogImageTemplate and
+// zero length unresolvedTemplates
+func ReplaceTemplates(catalogImageTemplate string) (processedCatalogImageTemplate string, unresolvedTemplates []string) {


without the dynamic gvk templating, this could probably be simplified to a call to the stdlib template package

also: since any unresolved components should mean the image reference is invalid, I'm not sure I get the value in returning a partially-resolved reference and a list of the unresolved ones: why not just return an error with the missing values?

The unresolvedTemplates are used to inform the user in a message about exactly what template that was not resolvable. It is used in messages exposed in both the log and the status conditions.

Also the processedCatalogImageTemplate is used to inform the user of what the template looks like in the status conditions. It helps the user see what's wrong by looking at the message AND what the templating code computed.

I'd like to address the security shortcomings of the GVK feature in the future, so I'd like to keep things as close to "working" as possible.

scripts/copy_crds.sh

test/e2e/catalog_e2e_test.go

jchunkins · 2021-07-23T14:53:51Z

It looks like unit / unit (pull_request) GitHub action failed, but when I run the error test case TestSyncOperatorGroups on my machine it does not have an error. Not sure how to restart the GitHub action to prove that its a flake of some kind.

exdx · 2021-07-26T15:13:15Z

It looks like unit / unit (pull_request) GitHub action failed, but when I run the error test case TestSyncOperatorGroups on my machine it does not have an error. Not sure how to restart the GitHub action to prove that its a flake of some kind.

It's a well known flake, I restarted it

cmd/catalog/main.go

pkg/controller/operators/catalogtempate/operator.go

pkg/lib/catalogsource/catalogsource_update.go

test/e2e/catalog_e2e_test.go

pkg/lib/catalogsource/image_template.go

pkg/controller/operators/catalogtempate/operator.go

pkg/lib/catalogsource/catalogsource_update.go

pkg/lib/catalogsource/image_template.go

- implements catalog switching proposal See https://github.com/operator-framework/enhancements/blob/master/enhancements/automatic-catalog-switching-annotations.md - picks up v0.10.3 - change sed command in copy_crds.sh to be posix compliant The command + in sed is non-portable. The posix compliant version of this command is {1,} This change allows this script to run on a mac too (which uses BSD sed) See https://riptutorial.com/sed/topic/9436/bsd-macos-sed-vs--gnu-sed-vs--the-posix-sed-specification Note: GVK was implemented but disabled due to security concerns. Future versions will address these concerns Signed-off-by: John Hunkins <[email protected]>

- reorganize imports - change logging common case to debug Signed-off-by: John Hunkins <[email protected]>

- this is the commit that should be reviewed later when it comes time to re-enable the GVK code Signed-off-by: John Hunkins <[email protected]>

Signed-off-by: John Hunkins <[email protected]>

- fix names that don't match Golang convention - fix misspelled package name - return nil for sync function casting Signed-off-by: John Hunkins <[email protected]>

Signed-off-by: John Hunkins <[email protected]>

- clean up every instance of the GVK templating feature - add last minute change for RWMutex instead of Mutex - this is the second commit that should be reviewed later when it comes time to re-enable the GVK code Signed-off-by: John Hunkins <[email protected]>

pkg/controller/operators/catalogtemplate/operator.go

Signed-off-by: John Hunkins <[email protected]>

njhale

looking really good. thank you for addressing all the feedback so far! I think we're in the home stretch.

cmd/catalog/main.go

pkg/controller/operators/catalogtemplate/operator.go

pkg/lib/catalogsource/catalogsource_update.go

- update logging for clarity - clean up code blocks - refactor catalogsource helper functions: - remove mutex - remove two step get/update* in preference to update* even though this may result in more "the object has been modified" errors - rename helper functions to clarify usage - clarify the update helper function docs Signed-off-by: John Hunkins <[email protected]>

njhale

/hold cancel
/lgtm

@jchunkins thanks for sticking with this through all the feedback!

I'd be interested in any thoughts you have about your experience contributing; e.g. good, bad, suggestions. Maybe we can continue in slack.

openshift-ci · 2021-07-30T01:59:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jchunkins, kevinrizza, njhale

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:

~~OWNERS~~ [kevinrizza,njhale]

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

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2021

openshift-ci bot requested review from ecordell and timflannagan July 22, 2021 19:13

openshift-ci bot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 22, 2021

jchunkins force-pushed the catalog_switcher branch from 658646e to 295ba3e Compare July 22, 2021 19:18

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 22, 2021

timflannagan reviewed Jul 22, 2021

View reviewed changes

go.mod Outdated Show resolved Hide resolved

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 22, 2021

jchunkins force-pushed the catalog_switcher branch from dfafccd to 57ac4f4 Compare July 22, 2021 20:13

kevinrizza reviewed Jul 22, 2021

View reviewed changes

pkg/controller/operators/catalogtempate/operator.go Outdated Show resolved Hide resolved

jchunkins force-pushed the catalog_switcher branch from 57ac4f4 to 1c7258d Compare July 22, 2021 22:34

jchunkins changed the title ~~WIP Catalog switcher~~ Catalog switcher Jul 22, 2021

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 22, 2021

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2021

ecordell reviewed Jul 22, 2021

View reviewed changes

jchunkins force-pushed the catalog_switcher branch from 448a052 to 36c7f64 Compare July 23, 2021 14:33

jchunkins force-pushed the catalog_switcher branch from 5b0e169 to 25bdeb6 Compare July 23, 2021 20:51

njhale reviewed Jul 26, 2021

View reviewed changes

ecordell reviewed Jul 26, 2021

View reviewed changes

pkg/lib/catalogsource/catalogsource_update.go Outdated Show resolved Hide resolved

pkg/lib/catalogsource/image_template.go Outdated Show resolved Hide resolved

jchunkins added 2 commits July 27, 2021 17:55

Address review comments from ecordell

053c93a

- reorganize imports - change logging common case to debug Signed-off-by: John Hunkins <[email protected]>

jchunkins added 5 commits July 27, 2021 17:55

removed all watcher related code

358c357

- this is the commit that should be reviewed later when it comes time to re-enable the GVK code Signed-off-by: John Hunkins <[email protected]>

update vendor to remove dynamic lister code

f211819

Signed-off-by: John Hunkins <[email protected]>

fix incorrect names and improper error usage

11387b2

- fix names that don't match Golang convention - fix misspelled package name - return nil for sync function casting Signed-off-by: John Hunkins <[email protected]>

change code block organization and add mutex

6c2902b

Signed-off-by: John Hunkins <[email protected]>

jchunkins force-pushed the catalog_switcher branch from cc0db6b to c6b1437 Compare July 27, 2021 23:02

kevinrizza reviewed Jul 28, 2021

View reviewed changes

pkg/controller/operators/catalogtemplate/operator.go Outdated Show resolved Hide resolved

return status update errors and adjust logging

ecf1888

Signed-off-by: John Hunkins <[email protected]>

njhale reviewed Jul 29, 2021

View reviewed changes

njhale approved these changes Jul 30, 2021

View reviewed changes

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 30, 2021

openshift-ci bot assigned njhale Jul 30, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2021

openshift-merge-robot merged commit 5f6fcd1 into operator-framework:master Jul 30, 2021

timflannagan linked an issue Feb 11, 2022 that may be closed by this pull request

Need a consistent way to manage switching out a custom operator catalog as clusters are being upgraded to newer versions #1769

Closed

Catalog switcher #2281

Catalog switcher #2281

Uh oh!

Conversation

jchunkins commented Jul 22, 2021

Uh oh!

openshift-ci bot commented Jul 22, 2021

Uh oh!

Uh oh!

kevinrizza commented Jul 22, 2021

Uh oh!

Uh oh!

kevinrizza commented Jul 22, 2021

Uh oh!

kevinrizza commented Jul 22, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ecordell Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

ecordell Jul 22, 2021

Choose a reason for hiding this comment

Uh oh!

jchunkins Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

jchunkins Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

jchunkins Jul 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jchunkins commented Jul 23, 2021

Uh oh!

exdx commented Jul 26, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

njhale left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

njhale left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jul 30, 2021

Uh oh!

Uh oh!