operator-sdk: run bundle{-upgrade} support insecure registry server #4816

avalluri · 2021-04-20T20:56:16Z

Supporting insecure registry is useful for testing operator installation
from a bundle image stored at the local registry.

Closes #4815

Description of the change:

Add a new boolean command-line option(defaults to false) named --allow-insecure for both operator-sdk run bundle and operator-sdk run bundle-upgrade. When enabled this argument the commands ignores the SSL errors that occurred while communicating the docker registry. This allows pulling the bundle images from insecure(usually local) docker registries.

Motivation for the change:

Using a local insecure registry is a possible case for storing the generated bundle image, and test the operator installation.

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
Add or update relevant sections of the docs website in website/content/en/docs

estroz · 2021-04-20T23:37:32Z

internal/olm/operator/bundle/install.go

@@ -51,6 +51,7 @@ func (i *Install) BindFlags(fs *pflag.FlagSet) {

 	// --mode is hidden so only users who know what they're doing can alter add mode.
 	fs.StringVar((*string)(&i.BundleAddMode), "mode", "", "mode to use for adding bundle to index")
+	fs.BoolVar(&i.IndexImageCatalogCreator.AllowInsecure, "allow-insecure", false, "allows pulling bundle image from an insecure registry server")


How about mirroring opm and naming this

Suggested change

fs.BoolVar(&i.IndexImageCatalogCreator.AllowInsecure, "allow-insecure", false, "allows pulling bundle image from an insecure registry server")

fs.BoolVar(&i.IndexImageCatalogCreator.SkipTLS, "skip-tls", false, "skip authentication of image registry TLS certificate when pulling a bundle image in-cluster")

How about mirroring opm and naming this

Sounds good, i will update the PR.

Renamed the option as suggested. Also moved the flag to IndexImageCatalogCreator in internal/olm/operator/registry/index_image.go so the option will be available to both run bundle and run bundle-upgrade.

Released operator-sdk binary does not support running bundle image from local insecure registry. The fix has submitted to upstream: operator-framework/operator-sdk#4816 Till this fix is available in release binary we have to build from source.

estroz · 2021-04-23T18:23:43Z

internal/olm/operator/registry/index/registry_pod.go

@@ -50,6 +50,8 @@ type BundleItem struct {
 	ImageTag string `json:"imageTag"`
 	// AddMode describes how the bundle should be added to an index image.
 	AddMode BundleAddMode `json:"mode"`
+	// SkipTLS controls wether to ignore SSL errors while pulling bundle image from registry server.
+	SkipTLS bool `json:"SkipTLS"`


This field should be added to RegistryPod not BundleItem since it's globally configured and not on a per-bundle basis.

that's also where the preexisting cert stuff is anyway 👍

Done. Changed as @estroz suggested.

Released operator-sdk binary does not support running bundle image from local insecure registry. The fix has submitted to upstream: operator-framework/operator-sdk#4816 Till this fix is available in release binary we have to build from source.

Venkat19967 · 2021-04-27T16:25:54Z

/lgtm

jmrodri

All in all a GREAT PR. I have 2 items:

the custom DB path should be a separate PR
the SkipTLS field should be on RegistryPod like others mentioned.

jmrodri · 2021-04-27T19:37:37Z

internal/olm/operator/registry/index/registry_pod.go

@@ -50,6 +50,8 @@ type BundleItem struct {
 	ImageTag string `json:"imageTag"`
 	// AddMode describes how the bundle should be added to an index image.
 	AddMode BundleAddMode `json:"mode"`
+	// SkipTLS controls wether to ignore SSL errors while pulling bundle image from registry server.
+	SkipTLS bool `json:"SkipTLS"`


jmrodri · 2021-04-27T19:39:23Z

internal/olm/operator/registry/index/registry_pod.go

@@ -302,7 +304,7 @@ func newBool(b bool) *bool {

 const cmdTemplate = `/bin/mkdir -p {{ dirname .DBPath }} && \
 {{- range $i, $item := .BundleItems }}
-/bin/opm registry add -d {{ $.DBPath }} -b {{ $item.ImageTag }} --mode={{ $item.AddMode }}{{ if $.CASecretName }} --ca-file=/certs/cert.pem{{ end }} && \
+/bin/opm registry add -d {{ $.DBPath }} -b {{ $item.ImageTag }} --mode={{ $item.AddMode }}{{ if $.CASecretName }} --ca-file=/certs/cert.pem{{ end }} --skip-tls={{ $item.SkipTLS }} && \


👍 Great catch. When I was playing around to add this locally I didn't notice having to do this.

jmrodri · 2021-04-27T19:40:35Z

internal/olm/operator/registry/index/registry_pod_test.go

+					BundleItem{
+						ImageTag: "localhost/example-operator-bundle:1.0.1",
+						AddMode:  SemverBundleAddMode,
+						SkipTLS:  true,


Like was said earlier, this field should be on RegistryPod.

jmrodri · 2021-04-27T19:41:19Z

internal/olm/operator/registry/index/registry_pod_test.go

@@ -91,6 +92,14 @@ var _ = Describe("RegistryPod", func() {
 				Expect(rp.DBPath).To(Equal("/database/index.db"))
 			})

+			It("should create a registry with custom database path", func() {
+				customDBPath := "/foo/bar/database.db"


Do we allow a custom database? This seems like we're mixing 2 different items.

Do we allow a custom database?

I am not sure, in case, we don't support then we could remove dbPath parameter from containerCommandFor in the unit tests.

At the same time, I don't see any logical reason for not supporting the custom database path.

jmrodri · 2021-04-27T19:42:54Z

internal/olm/operator/registry/index/registry_pod_test.go

 	}
-	return fmt.Sprintf("/bin/mkdir -p /database && \\\n%s/bin/opm registry serve -d /database/index.db -p 50051\n", additions.String())
+	return fmt.Sprintf("/bin/mkdir -p %s && \\\n%s/bin/opm registry serve -d %s -p 50051\n", filepath.Dir(dbPath), additions.String(), dbPath)


I didn't think we were allowing custom database path. I think that should be a separate PR.

I added this custom database change in this PR as the CI reported a failure:

go vet ./... tools/scripts/fetch golangci-lint 1.31.0 && tools/bin/golangci-lint run golangci-lint missing or not version '1.31.0', downloading... golangci/golangci-lint info checking GitHub for tag 'v1.31.0' golangci/golangci-lint info found version: 1.31.0 for v1.31.0/linux/amd64 golangci/golangci-lint info installed /home/runner/work/operator-sdk/operator-sdk/tools/bin/golangci-lint internal/olm/operator/registry/index/registry_pod_test.go:234:26: `containerCommandFor` - `dbPath` always receives `defaultDBPath` (`"/database/index.db"`) (unparam) func containerCommandFor(dbPath string, items []BundleItem, hasCA bool) string { ^ Makefile:122: recipe for target 'test-sanity' failed make: *** [test-sanity] Error 1 Error: Process completed with exit code 2.

Released operator-sdk binary does not support running bundle image from local insecure registry. The fix has submitted to upstream: operator-framework/operator-sdk#4816 Till this fix is available in release binary we have to build from source.

avalluri · 2021-04-30T16:25:04Z

All in all a GREAT PR.

Thanks! for your review time. Sorry for late responce.
I have 2 items:

the custom DB path should be a separate PR

I removed this change from this PR. I will try to send it as a separate PR.

the SkipTLS field should be on RegistryPod like others mentioned.

Changed as suggested.

estroz

/lgtm

Supporting insecure registry is useful for testing operator installation from bundle image stored at local registry. Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>

jmrodri

/lgtm

Released operator-sdk binary does not support running bundle image from local insecure registry. The fix has submitted to upstream: operator-framework/operator-sdk#4816 Till this fix is available in release binary we have to build from source.

openshift-ci-robot requested review from jberkhahn and theishshah April 20, 2021 20:57

avalluri force-pushed the insecure-registry branch 2 times, most recently from e78295e to 07bd720 Compare April 20, 2021 21:26

estroz reviewed Apr 20, 2021

View reviewed changes

avalluri force-pushed the insecure-registry branch from 07bd720 to caf2c69 Compare April 21, 2021 07:01

avalluri requested a review from estroz April 21, 2021 19:45

avalluri force-pushed the insecure-registry branch from 7d1fdc2 to dc4c035 Compare April 22, 2021 09:25

estroz reviewed Apr 23, 2021

View reviewed changes

openshift-ci-robot assigned Venkat19967 Apr 27, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2021

jmrodri requested changes Apr 27, 2021

View reviewed changes

avalluri force-pushed the insecure-registry branch from dc4c035 to e6ce4a3 Compare April 30, 2021 16:23

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2021

avalluri force-pushed the insecure-registry branch from e6ce4a3 to 76d64ad Compare April 30, 2021 16:34

estroz approved these changes Apr 30, 2021

View reviewed changes

openshift-ci-robot assigned estroz Apr 30, 2021

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

estroz requested a review from jmrodri April 30, 2021 16:36

operator-sdk: run bundle{-upgrade} support insecure registry server

76d64ad

Supporting insecure registry is useful for testing operator installation from bundle image stored at local registry. Signed-off-by: Amarnath Valluri <amarnath.valluri@intel.com>

jmrodri approved these changes May 4, 2021

View reviewed changes

openshift-ci-robot assigned jmrodri May 4, 2021

jmrodri merged commit 1be35be into operator-framework:master May 4, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

operator-sdk: run bundle{-upgrade} support insecure registry server #4816

operator-sdk: run bundle{-upgrade} support insecure registry server #4816

avalluri commented Apr 20, 2021

estroz Apr 20, 2021

avalluri Apr 21, 2021

avalluri Apr 21, 2021

estroz Apr 23, 2021

jberkhahn Apr 26, 2021

jmrodri Apr 27, 2021

avalluri Apr 30, 2021

Venkat19967 commented Apr 27, 2021

jmrodri left a comment

jmrodri Apr 27, 2021

jmrodri Apr 27, 2021

jmrodri Apr 27, 2021

avalluri Apr 30, 2021

jmrodri Apr 27, 2021

avalluri Apr 30, 2021

avalluri Apr 30, 2021

jmrodri Apr 27, 2021

avalluri Apr 30, 2021

avalluri commented Apr 30, 2021

estroz left a comment

jmrodri left a comment

	fs.BoolVar(&i.IndexImageCatalogCreator.AllowInsecure, "allow-insecure", false, "allows pulling bundle image from an insecure registry server")
	fs.BoolVar(&i.IndexImageCatalogCreator.SkipTLS, "skip-tls", false, "skip authentication of image registry TLS certificate when pulling a bundle image in-cluster")

operator-sdk: run bundle{-upgrade} support insecure registry server #4816

operator-sdk: run bundle{-upgrade} support insecure registry server #4816

Conversation

avalluri commented Apr 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Venkat19967 commented Apr 27, 2021

jmrodri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avalluri commented Apr 30, 2021

estroz left a comment

Choose a reason for hiding this comment

jmrodri left a comment

Choose a reason for hiding this comment