Bug 1924955: Fix that image containers are fetched from external container registries (which doesn't work for private image containers) #8046

jerolimov · 2021-02-04T01:39:22Z

Fixes:
https://issues.redhat.com/browse/ODC-3062
https://issues.redhat.com/browse/OCPBUGSM-24359
https://bugzilla.redhat.com/show_bug.cgi?id=1924955

Analysis / Root cause:
When the user provides a Image Pull Secret (a Secret with type kubernetes.io/dockerconfigjson) to access private image registry the Add/Import page validates the image name but the Pod doesn't start later.

The "Deploy Image" pages makes an API call to .../$namespace/imagestreamimports to verify if the cluster could connect to the external image registry and which image tags are available. The form could be only submit if this was successful. The imagestreamimports endpoint uses the shared Image Pull Secrets of the namespace.

But after creating this Deployment (or DeploymentConfig) the container image could not be pulled from the private registry. All Pods are failing with ErrImagePull and ImagePullBackOff errors. The reason is that the cluster tries to load the container image directly from the external URL without the provided Secret.

As workaround it was possible to assign the Image Pull Secret as imagePullSecrets: in the "default" ServiceAccount of the name or provide it (also as imagePullSecrets: in the Deployment/DeploymentConfig template section for the Pod.

Solution Description:
Because the imagestreamimports endpoint uses all provided Secrets we decided not to update the page (yet) to select a Secret and assign it to the Deployment.

Instead of that, this PR changes the URL of a Deployment from the external image registry to the internal image registry. We already created a ImageStream which pulls the container image successfully.

For this the console needs just to set the image container name (as it does already). Whenever this image: was changed in a Deployment it was automatically overridden by an ImageStream admission webhook. This webhook supports a referencePolicy which is Source by default. This PR changes this value to Local so that the cluster internal image registry was used instead of the external image registry. Whenever the Deployment or ImageStream was changed or triggered, the Deployment contains now a link to the internal image registry.

See also:

Screen shots / Gifs for design review:
None. UI is not changed.

Unit test coverage report:
Added some tests for createOrUpdateImageStream

Test setup:

You need a private container image on an external registry like Docker Hub or Quay with credentials to read the image. (Feel free to ask me in Slack for some credentials for a code review.)
Open the developer perspective
Select Secrets > Create > Image Pull Secret and add enter your credentials. Use docker.io for the Docker Hub and quay.io for Quay as Registry Server Address.
Go to Add > Container Image and import your private container image as Deployment or DeploymentConfig. (If you have the Serverless Operator installed you can also verify that the Knative Services works already before this change.)
Check in Topology that the Pods can start successful.
I also verified that an updated image in the external image registry could be updated via oc import-image nodeinfo-private.

Browser conformance:

Chrome
Firefox
Safari
Edge

/kind bug

openshift-ci-robot · 2021-02-04T01:39:28Z

@jerolimov: This pull request references Bugzilla bug 1924955, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1924955: Fix that image containers are fetched from external container registries (which doesn't work for private image containers)

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 · 2021-02-04T01:40:19Z

@jerolimov: This pull request references Bugzilla bug 1924955, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1924955: Fix that image containers are fetched from external container registries (which doesn't work for private image containers)

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.

jerolimov · 2021-02-04T01:42:22Z

/assign @christianvogt
/cc @invincibleJai

jerolimov · 2021-02-04T11:02:59Z

/retest

openshift-ci-robot · 2021-02-04T11:23:15Z

@jerolimov: This pull request references Bugzilla bug 1924955, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1924955: Fix that image containers are fetched from external container registries (which doesn't work for private image containers)

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 · 2021-02-04T11:24:06Z

@jerolimov: This pull request references Bugzilla bug 1924955, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1924955: Fix that image containers are fetched from external container registries (which doesn't work for private image containers)

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 · 2021-02-04T12:11:34Z

@jerolimov: This pull request references Bugzilla bug 1924955, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1924955: Fix that image containers are fetched from external container registries (which doesn't work for private image containers)

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.

rottencandy

Tested. Works fine
/lgtm

invincibleJai

Thanks @jerolimov 🎉

Looks good and have verified works as expected, one nit see if it make sense

invincibleJai · 2021-02-04T15:08:06Z

frontend/packages/dev-console/src/components/import/__tests__/deployImage-submit-utils.spec.ts

+      const mockImageStreamData = {
+        apiVersion: 'image.openshift.io/v1',
+        kind: 'ImageStream',
+        metadata: {
+          labels: {
+            app: 'test-app',
+            'app.kubernetes.io/component': 'test-app',
+            'app.kubernetes.io/instance': 'test-app',
+            'app.kubernetes.io/part-of': 'mock-app',
+          },
+          name: 'test-app',
+          namespace: 'mock-project',
+        },
+        spec: {
+          tags: [
+            {
+              name: 'latest',
+              annotations: {
+                'openshift.io/generated-by': 'OpenShiftWebConsole',
+                'openshift.io/imported-from': 'myimage',
+              },
+              from: { kind: 'DockerImage', name: 'myimage' },
+              importPolicy: { insecure: false },
+              referencePolicy: { type: 'Local' },
+            },
+          ],
+        },
+      };


nit: this data is used even in below it blocks , we can have it in separate mock data file or before each and use it (update only particular spec field if needed) WDYT?

+:100: Absolutely. Extracted this into the mock file which contains also the form data. PTAL

With this option (Local) the ImageStream admission webhook creates a cluster internal URL for the Deployment container template. So images will be downloaded from the internal registry (similar to Knative Services) instead of from the external Image registry. This fixes an issue that external private images could not be downloaded from the Pod with additional changes on the Deployment (define imagePullSecrets) or the ServiceAccount (shared imagePullSecrets)

christianvogt · 2021-02-04T16:11:21Z

/approve

invincibleJai · 2021-02-04T16:47:43Z

/lgtm

openshift-ci-robot · 2021-02-04T16:47:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, invincibleJai, jerolimov, rottencandy

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:

~~frontend/packages/dev-console/OWNERS~~ [christianvogt,invincibleJai]

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

openshift-ci-robot · 2021-02-04T22:11:09Z

@jerolimov: All pull requests linked via external trackers have merged:

openshift/console#8046

Bugzilla bug 1924955 has been moved to the MODIFIED state.

In response to this:

Bug 1924955: Fix that image containers are fetched from external container registries (which doesn't work for private image containers)

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.

jerolimov · 2021-02-08T16:47:31Z

/cherry-pick release-4.6

openshift-cherrypick-robot · 2021-02-08T16:48:18Z

@jerolimov: new pull request created: #8098

In response to this:

/cherry-pick release-4.6

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 component/dev-console Related to dev-console label Feb 4, 2021

openshift-ci-robot requested review from karthikjeeyar and nemesis09 February 4, 2021 01:39

openshift-ci-robot assigned christianvogt Feb 4, 2021

jerolimov mentioned this pull request Feb 4, 2021

Bug 1925087: Add i18n support to the Secret modals and add new hint for the servername #8047

Merged

4 tasks

rottencandy approved these changes Feb 4, 2021

View reviewed changes

openshift-ci-robot assigned rottencandy Feb 4, 2021

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

invincibleJai reviewed Feb 4, 2021

View reviewed changes

jerolimov added 2 commits February 4, 2021 17:08

Add unit test for createOrUpdateImageStream

9c2c452

jerolimov force-pushed the odc-3062-referencepolicy branch from 0a0790c to 1c60af2 Compare February 4, 2021 16:08

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

jerolimov requested review from rottencandy and invincibleJai February 4, 2021 16:11

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 4, 2021

openshift-ci-robot assigned invincibleJai Feb 4, 2021

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

openshift-merge-robot merged commit 809e2da into openshift:master Feb 4, 2021

spadgett added this to the v4.7 milestone Feb 5, 2021

openshift-cherrypick-robot mentioned this pull request Feb 8, 2021

[release-4.6] Bug 1926340: Fix that image containers are fetched from external container registries (which doesn't work for private image containers) #8098

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1924955: Fix that image containers are fetched from external container registries (which doesn't work for private image containers) #8046

Bug 1924955: Fix that image containers are fetched from external container registries (which doesn't work for private image containers) #8046

jerolimov commented Feb 4, 2021 •

edited

Loading

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

jerolimov commented Feb 4, 2021

jerolimov commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

rottencandy left a comment

invincibleJai left a comment

invincibleJai Feb 4, 2021

jerolimov Feb 4, 2021

christianvogt commented Feb 4, 2021

invincibleJai commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

jerolimov commented Feb 8, 2021

openshift-cherrypick-robot commented Feb 8, 2021

Bug 1924955: Fix that image containers are fetched from external container registries (which doesn't work for private image containers) #8046

Bug 1924955: Fix that image containers are fetched from external container registries (which doesn't work for private image containers) #8046

Conversation

jerolimov commented Feb 4, 2021 • edited Loading

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

jerolimov commented Feb 4, 2021

jerolimov commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

rottencandy left a comment

Choose a reason for hiding this comment

invincibleJai left a comment

Choose a reason for hiding this comment

invincibleJai Feb 4, 2021

Choose a reason for hiding this comment

jerolimov Feb 4, 2021

Choose a reason for hiding this comment

christianvogt commented Feb 4, 2021

invincibleJai commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

openshift-ci-robot commented Feb 4, 2021

jerolimov commented Feb 8, 2021

openshift-cherrypick-robot commented Feb 8, 2021

jerolimov commented Feb 4, 2021 •

edited

Loading