Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

oc import-image: deprecate legacy path using annotations #19673

Conversation

wozniakjan
Copy link
Contributor

@wozniakjan wozniakjan commented May 10, 2018

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 wozniakjan requested review from soltysh and bparees May 10, 2018 09:48
@openshift-ci-robot 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 openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 10, 2018
var err error
is, err = o.isClient.Get(o.Name, metav1.GetOptions{ResourceVersion: resourceVersion})
if err != nil {
if errors.IsNotFound(err) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -3,15 +3,14 @@ package cmd
import (
"fmt"
"io"
"k8s.io/apimachinery/pkg/util/wait"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this import down

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wait.PollImmediate ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't shadow the outer-err ;-)

Copy link
Contributor Author

@wozniakjan wozniakjan May 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is what I took as a reference:

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

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})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why passing the resourceVersion?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Should I remove it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@soltysh soltysh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@wozniakjan 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
Copy link
Contributor Author

wozniakjan commented May 10, 2018

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

// 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"

@wozniakjan 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
@soltysh
Copy link
Contributor

soltysh commented May 10, 2018

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

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

@wozniakjan
Copy link
Contributor Author

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
Copy link
Contributor

bparees commented May 10, 2018

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 bparees self-assigned this May 10, 2018
@bparees
Copy link
Contributor

bparees commented May 10, 2018

@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
Copy link
Contributor Author

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

correct

@wozniakjan
Copy link
Contributor Author

/test gcp
cluster provisioning failed

@wozniakjan
Copy link
Contributor Author

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
Copy link
Contributor

bparees commented May 11, 2018

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
Copy link
Contributor

soltysh commented May 14, 2018

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.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 20, 2018
@wozniakjan wozniakjan force-pushed the issue-13214/oc-import-image-watch branch from 14f5fcc to afeb760 Compare June 25, 2018 10:12
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 25, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2018
@wozniakjan 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
@wozniakjan
Copy link
Contributor Author

/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
Copy link
Contributor Author

/test e2e-gcp

flake, test timed out

@wozniakjan
Copy link
Contributor Author

@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
Copy link
Contributor

bparees commented Jun 25, 2018

/lgtm

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

@openshift-ci-robot 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
Copy link
Contributor

soltysh commented Jun 27, 2018

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

post-rebase feel free to un-hold

@bparees
Copy link
Contributor

bparees commented Jun 28, 2018

/hold cancel

@openshift-ci-robot 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 openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2018
@wozniakjan wozniakjan force-pushed the issue-13214/oc-import-image-watch branch from 8183c53 to 72300b7 Compare June 29, 2018 07:16
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jun 29, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2018
@wozniakjan wozniakjan force-pushed the issue-13214/oc-import-image-watch branch from 72300b7 to 1f5b299 Compare June 29, 2018 07:33
@wozniakjan
Copy link
Contributor Author

/test cmd

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

@wozniakjan
Copy link
Contributor Author

/retest

flake #20149

@wozniakjan
Copy link
Contributor Author

/test gcp

@wozniakjan
Copy link
Contributor Author

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

@bparees
Copy link
Contributor

bparees commented Jun 29, 2018

/lgtm

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

[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

Needs approval from an approver in each of these files:

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

@openshift-merge-robot openshift-merge-robot merged commit b8d718c into openshift:master Jun 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/cli lgtm Indicates that a PR is ready to be merged. sig/developer-experience size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

oc import-image attempts to watch all imagestream of namespace after an import
8 participants