oc import-image: deprecate legacy path using annotations by wozniakjan · Pull Request #19673 · openshift/origin

wozniakjan · 2018-05-10T09:48:53Z

~~WATCH doesn't work with RBAC resourceNames as well as GET. Changing the waitForImport to poll a GET instead.~~

fixes #13214

NOTE: I selected both pollPeriod: 1*time.Second and pollTimeout: 60*time.Second based on what I saw when going over a couple of polls in our codebase but not sure if it is appropriate and fits here.

ptal @openshift/sig-developer-experience

wozniakjan · 2018-05-10T09:49:49Z

pkg/oc/cli/cmd/importimage.go

+		var err error
+		is, err = o.isClient.Get(o.Name, metav1.GetOptions{ResourceVersion: resourceVersion})
+		if err != nil {
+			if errors.IsNotFound(err) {


I assumed that if IS is not found, we would like to continue polling. But if error is anything else, stop polling and report the error

You don't need to. This is the old path, for which the stream should be already created for you.

mfojtik · 2018-05-10T10:25:52Z

pkg/oc/cli/cmd/importimage.go

 import (
 	"fmt"
 	"io"
+	"k8s.io/apimachinery/pkg/util/wait"


move this import down

mfojtik · 2018-05-10T10:27:06Z

pkg/oc/cli/cmd/importimage.go

-			if !ok {
-				return nil, fmt.Errorf("image stream watch ended prematurely")
+	var is *imageapi.ImageStream
+	err := wait.Poll(1*time.Second, 60*time.Second, func() (bool, error) {


wait.PollImmediate ?

during testing, it usually polled ~3 times, would we gain anything with wait.PollImmediate?

in case the server is fast, we don't have to wait 1s at least

mfojtik · 2018-05-10T10:28:12Z

pkg/oc/cli/cmd/importimage.go

-				return nil, fmt.Errorf("image stream watch ended prematurely")
+	var is *imageapi.ImageStream
+	err := wait.Poll(1*time.Second, 60*time.Second, func() (bool, error) {
+		var err error


don't shadow the outer-err ;-)

this is what I took as a reference:

origin/pkg/cmd/server/origin/openshift_apiserver.go

Lines 675 to 684 in 74a6a14

var securityClient securityclient.Interface

err := wait.Poll(1*time.Second, 30*time.Second, func() (bool, error) {

var err error

securityClient, err = securityclient.NewForConfig(context.LoopbackClientConfig)

if err != nil {

utilruntime.HandleError(fmt.Errorf("unable to initialize client: %v", err))

return false, nil

}

return true, nil

})

could you recommend what pattern should I rather follow? I would like to get the is out of the polled function and ideally name error vars as err

mfojtik · 2018-05-10T10:31:11Z

pkg/oc/cli/cmd/importimage.go

+	var is *imageapi.ImageStream
+	err := wait.Poll(1*time.Second, 60*time.Second, func() (bool, error) {
+		var err error
+		is, err = o.isClient.Get(o.Name, metav1.GetOptions{ResourceVersion: resourceVersion})


why passing the resourceVersion?

because the original code was also passing it and didn't want to cause any regression

origin/pkg/oc/cli/cmd/importimage.go

Line 302 in 74a6a14

streamWatch, err := o.isClient.Watch(metav1.ListOptions{FieldSelector: fields.OneTermEqualSelector("metadata.name", o.Name).String(), ResourceVersion: resourceVersion})

Should I remove it?

Yeah, you don't need that anymore now that you're using GET, it was there to explicitly watch newer events only.

soltysh

I wonder how long we want to keep supporting the legacy path when importing images.

soltysh · 2018-05-10T11:19:18Z

pkg/oc/cli/cmd/importimage.go

+	var is *imageapi.ImageStream
+	err := wait.Poll(1*time.Second, 60*time.Second, func() (bool, error) {
+		var err error
+		is, err = o.isClient.Get(o.Name, metav1.GetOptions{ResourceVersion: resourceVersion})


Yeah, you don't need that anymore now that you're using GET, it was there to explicitly watch newer events only.

soltysh · 2018-05-10T11:22:46Z

pkg/oc/cli/cmd/importimage.go

+		var err error
+		is, err = o.isClient.Get(o.Name, metav1.GetOptions{ResourceVersion: resourceVersion})
+		if err != nil {
+			if errors.IsNotFound(err) {


You don't need to. This is the old path, for which the stream should be already created for you.

wozniakjan · 2018-05-10T12:07:27Z

@soltysh @mfojtik @bparees quick question related to the issue. If the IS doesn't exist, oc import-image tries to create it

origin/pkg/oc/cli/cmd/importimage.go

Lines 343 to 365 in 74a6a14

    
           // no stream, try creating one 
        
           if err != nil { 
        
           	if !errors.IsNotFound(err) { 
        
           		return nil, nil, err 
        
           	} 
        
           	if !o.Confirm { 
        
           		return nil, nil, fmt.Errorf("no image stream named %q exists, pass --confirm to create and import", o.Name) 
        
           	} 
        
           	stream, isi = o.newImageStream() 
        
           	// ensure defaulting is applied by round trip converting 
        
           	// TODO: convert to using versioned types. 
        
           	external, err := legacyscheme.Scheme.ConvertToVersion(stream, imageapiv1.SchemeGroupVersion) 
        
           	if err != nil { 
        
           		return nil, nil, err 
        
           	} 
        
           	legacyscheme.Scheme.Default(external) 
        
           	internal, err := legacyscheme.Scheme.ConvertToVersion(external, imageapi.SchemeGroupVersion) 
        
           	if err != nil { 
        
           		return nil, nil, err 
        
           	} 
        
           	stream = internal.(*imageapi.ImageStream) 
        
           	return stream, isi, nil 
        
           }

but using restricted SA with resourceNames, creating IS fails on RBAC. If the IS exists, the oc import-image succeeds without an error. Is this desired behavior or am I missing something?

Command oc get image_name --as=system:serviceaccount:test:sa works well, oc create -f imagestream.yaml --as=system:serviceaccount:test:sa doesn't.

serviceaccount

oc create serviceaccount sa --namespace test

role.yaml

apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
  name: r1
  namespace: test
rules:
- apiGroups:
  - "image.openshift.io"
  resourceNames:
  - image_name
  resources:
  - imagestreams
  - imagestreams/layers
  verbs:
  - create
  - delete
  - edit
  - get
  - list
  - update
  - watch

rolebinding.yaml

apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: r1-to-sa
  namespace: test
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: Role
  name: r1
subjects:
- kind: ServiceAccount
  name: sa
  namespace: test

imagestream.yaml

kind: ImageStream
apiVersion: v1
metadata:
  name: image_name

oc create -f imagestream.yaml --as=system:serviceaccount:test:sa --loglevel=9

I0510 08:02:48.367934   10485 loader.go:357] Config loaded from file /root/.kube/config
I0510 08:02:48.369065   10485 decoder.go:224] decoding stream as YAML
I0510 08:02:48.375512   10485 request.go:890] Request Body: {"apiVersion":"image.openshift.io/v1","kind":"ImageStream","metadata":{"name":"image_name","namespace":"test"}}
I0510 08:02:48.375590   10485 round_trippers.go:386] curl -k -v -XPOST  -H "Content-Type: application/json" -H "Impersonate-User: system:serviceaccount:test:sa" -H "User-Agent: oc/v1.10.0+b81c8f8 (linux/amd64) kubernetes/b81c8f8" -H "Accept: application/json" https://127.0.0.1:8443/apis/image.openshift.io/v1/namespaces/test/imagestreams
I0510 08:02:48.385447   10485 round_trippers.go:405] POST https://127.0.0.1:8443/apis/image.openshift.io/v1/namespaces/test/imagestreams 403 Forbidden in 9 milliseconds
I0510 08:02:48.385464   10485 round_trippers.go:411] Response Headers:
I0510 08:02:48.385468   10485 round_trippers.go:414]     Date: Thu, 10 May 2018 12:02:48 GMT
I0510 08:02:48.385472   10485 round_trippers.go:414]     Cache-Control: no-store
I0510 08:02:48.385475   10485 round_trippers.go:414]     Content-Type: application/json
I0510 08:02:48.385478   10485 round_trippers.go:414]     X-Content-Type-Options: nosniff
I0510 08:02:48.385482   10485 round_trippers.go:414]     Content-Length: 439
I0510 08:02:48.385508   10485 request.go:890] Response Body: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"imagestreams.image.openshift.io is forbidden: User \"system:serviceaccount:test:sa\" cannot create imagestreams.image.openshift.io in the namespace \"test\": User \"system:serviceaccount:test:sa\" cannot create imagestreams.image.openshift.io in project \"test\"","reason":"Forbidden","details":{"group":"image.openshift.io","kind":"imagestreams"},"code":403}
I0510 08:02:48.385719   10485 helpers.go:201] server response object: [{
  "kind": "Status",
  "apiVersion": "v1",
  "metadata": {},
  "status": "Failure",
  "message": "error when creating \"imagestream.yaml\": imagestreams.image.openshift.io is forbidden: User \"system:serviceaccount:test:sa\" cannot create imagestreams.image.openshift.io in the namespace \"test\": User \"system:serviceaccount:test:sa\" cannot create imagestreams.image.openshift.io in project \"test\"",
  "reason": "Forbidden",
  "details": {
    "group": "image.openshift.io",
    "kind": "imagestreams"
  },
  "code": 403
}]
F0510 08:02:48.385742   10485 helpers.go:119] Error from server (Forbidden): error when creating "imagestream.yaml": imagestreams.image.openshift.io is forbidden: User "system:serviceaccount:test:sa" cannot create imagestreams.image.openshift.io in the namespace "test": User "system:serviceaccount:test:sa" cannot create imagestreams.image.openshift.io in project "test"

soltysh · 2018-05-10T13:56:32Z

but using restricted SA with resourceNames, creating IS fails on RBAC.

Don't use resourceName, there's on other way around it.

wozniakjan · 2018-05-10T14:11:26Z

Don't use resourceName, there's on other way around it.

@soltysh I thought we wanted to fix oc import-image so that it works with SA resourceName #13214, should I just close that issue and this PR instead?

bparees · 2018-05-10T14:13:04Z

Don't use resourceName, there's on other way around it.

@soltysh why doesn't resourceName work?

If the IS doesn't exist, oc import-image tries to create it

I don't know what purpose this serves? you're going to end up w/ an empty imagestream that has no import source, so why bother? there are other/more appropriate ways to create an imagestream if that is your goal. I would love to deprecate this behavior unless someone can tell me its use case (@mfojtik @soltysh ?)

bparees · 2018-05-10T14:14:43Z

@soltysh I thought we wanted to fix oc import-image so that it works with SA resourceName #13214, should I just close that issue and this PR instead?

This PR still addresses the scenario where the imagestream already exists, right? So it would seem to still have value.

wozniakjan · 2018-05-10T14:28:57Z

This PR still addresses the scenario where the imagestream already exists, right? So it would seem to still have value.

correct

wozniakjan · 2018-05-11T11:28:08Z

/test gcp
cluster provisioning failed

wozniakjan · 2018-05-11T11:38:37Z

This PR still addresses the scenario where the imagestream already exists, right? So it would seem to still have value.

@bparees actually, considering earlier comment from @soltysh

I wonder how long we want to keep supporting the legacy path when importing images.

maybe this PR doesn't add much value since it addresses part of the legacy path. Should I rather try to deprecate the legacy path instead?

bparees · 2018-05-11T13:54:56Z

It's not obvious to me what "// Legacy path, remove when support for older importers is removed" is referring to. What part of this code is legacy path?

I would expect any import operation to need to wait for the import to complete to report the results.

@soltysh can you elaborate on what part of this code is legacy path vs new and why?

(also still interested in why we create an imagestream if it does not exist, since nothing will be imported in that case, as i understand it)

soltysh · 2018-05-14T13:25:47Z

This PR still addresses the scenario where the imagestream already exists, right? So it would seem to still have value.

Yes, updating existing does make sense. I was talking about the UC where the IS is to be created as a result of image-import.

wozniakjan · 2018-06-25T12:35:13Z

/retest

Docker flake and rpm flake?

Trying to pull repository docker.io/openshift/origin-release ... 
Get https://registry-1.docker.io/v2/: EOF

Error downloading packages:
  origin-3.11.0-0.alpha.0.67.8183c53.x86_64: [Errno 256] No more mirrors to try.

wozniakjan · 2018-06-25T14:44:35Z

/test e2e-gcp

flake, test timed out

wozniakjan · 2018-06-25T16:23:27Z

@soltysh, @bparees: needed to rebase on top of the master which removed lgtm, could you re-approve if you find it still good to go?

bparees · 2018-06-25T17:31:19Z

/lgtm

/hold
because this touches oc and thus would impact the rebase. @deads2k please let us know when this can go in.

soltysh · 2018-06-27T11:03:05Z

@deads2k please let us know when this can go in.

post-rebase feel free to un-hold

bparees · 2018-06-28T19:28:38Z

/hold cancel

fixes openshift#13214

wozniakjan · 2018-06-29T08:02:18Z

/test cmd

Stage FORWARD PARAMETERS TO THE REMOTE HOST [00h 00m 01s] failed.

wozniakjan · 2018-06-29T11:31:38Z

/retest

flake #20149

wozniakjan · 2018-06-29T12:51:29Z

/test gcp

wozniakjan · 2018-06-29T13:35:23Z

@bparees post rebase (only conflict was new imports), good for lgtm?

bparees · 2018-06-29T13:37:11Z

/lgtm

openshift-ci-robot · 2018-06-29T13:37:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, soltysh, wozniakjan

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~hack/OWNERS~~ [bparees]
~~pkg/oc/OWNERS~~ [bparees,soltysh]

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

wozniakjan added component/cli sig/developer-experience labels May 10, 2018

wozniakjan requested review from bparees and soltysh May 10, 2018 09:48

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 10, 2018

openshift-ci-robot requested review from enj and juanvallejo May 10, 2018 09:48

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 10, 2018

wozniakjan commented May 10, 2018

View reviewed changes

mfojtik reviewed May 10, 2018

View reviewed changes

wozniakjan force-pushed the issue-13214/oc-import-image-watch branch from 43474fe to 7e44ef4 Compare May 10, 2018 11:14

soltysh reviewed May 10, 2018

View reviewed changes

wozniakjan force-pushed the issue-13214/oc-import-image-watch branch 2 times, most recently from 133b79b to 0e364d7 Compare May 10, 2018 11:34

wozniakjan force-pushed the issue-13214/oc-import-image-watch branch 2 times, most recently from 27948d3 to 2724ac5 Compare May 10, 2018 13:51

bparees self-assigned this May 10, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2018

wozniakjan force-pushed the issue-13214/oc-import-image-watch branch from 14f5fcc to afeb760 Compare June 25, 2018 10:12

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2018

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2018

wozniakjan force-pushed the issue-13214/oc-import-image-watch branch 2 times, most recently from 5b0717a to 8183c53 Compare June 25, 2018 11:18

openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Jun 25, 2018

soltysh approved these changes Jun 27, 2018

View reviewed changes

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 28, 2018

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2018

wozniakjan force-pushed the issue-13214/oc-import-image-watch branch from 8183c53 to 72300b7 Compare June 29, 2018 07:16

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2018

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2018

oc import-image: remove legacy code using annotations

1f5b299

fixes openshift#13214

wozniakjan force-pushed the issue-13214/oc-import-image-watch branch from 72300b7 to 1f5b299 Compare June 29, 2018 07:33

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2018

openshift-merge-robot merged commit b8d718c into openshift:master Jun 29, 2018

	var securityClient securityclient.Interface
	err := wait.Poll(1time.Second, 30time.Second, func() (bool, error) {
	var err error
	securityClient, err = securityclient.NewForConfig(context.LoopbackClientConfig)
	if err != nil {
	utilruntime.HandleError(fmt.Errorf("unable to initialize client: %v", err))
	return false, nil
	}
	return true, nil
	})

Conversation

wozniakjan commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wozniakjan May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wozniakjan commented May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soltysh commented May 10, 2018

Uh oh!

wozniakjan commented May 10, 2018

Uh oh!

bparees commented May 10, 2018

Uh oh!

bparees commented May 10, 2018

Uh oh!

wozniakjan commented May 10, 2018

Uh oh!

wozniakjan commented May 11, 2018

Uh oh!

wozniakjan commented May 11, 2018

Uh oh!

bparees commented May 11, 2018

Uh oh!

soltysh commented May 14, 2018

Uh oh!

wozniakjan commented Jun 25, 2018

Uh oh!

wozniakjan commented Jun 25, 2018

Uh oh!

wozniakjan commented Jun 25, 2018

Uh oh!

bparees commented Jun 25, 2018

Uh oh!

soltysh commented Jun 27, 2018

Uh oh!

bparees commented Jun 28, 2018

Uh oh!

wozniakjan commented Jun 29, 2018

Uh oh!

wozniakjan commented Jun 29, 2018

Uh oh!

wozniakjan commented Jun 29, 2018

Uh oh!

wozniakjan commented Jun 29, 2018

Uh oh!

bparees commented Jun 29, 2018

Uh oh!

openshift-ci-robot commented Jun 29, 2018

Uh oh!

wozniakjan commented May 10, 2018 •

edited

Loading

wozniakjan May 10, 2018 •

edited

Loading

wozniakjan commented May 10, 2018 •

edited

Loading