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

Check for forbidden error on ImageStreamImport client #6837

Merged
merged 1 commit into from Jan 27, 2016

Conversation

csrwng
Copy link
Contributor

@csrwng csrwng commented Jan 26, 2016

The fallback for the ImageStreamImport searcher in new-app/new-build is not working with a server version v1.1.1 and a newer client. Because the resource exists but is not exposed through policy, the request will return a 403. Added a 403 check to return an ErrImageStreamImportUnsupported error so the fallback can occur.

Fixes #6836

@csrwng
Copy link
Contributor Author

csrwng commented Jan 26, 2016

status, ok := err.(kclient.APIStatus)
if !ok {
return ErrImageStreamImportUnsupported
}
if status.Status().Details == nil || status.Status().Details.Kind == "" {
if status.Status().Code == http.StatusForbidden || status.Status().Details == nil || status.Status().Details.Kind == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Put this into it's if block and add a comment

Copy link
Contributor

Choose a reason for hiding this comment

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

Also add a test :)

Copy link
Contributor

Choose a reason for hiding this comment

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

There is an integration test or unit test that verifies this somewhere

@csrwng
Copy link
Contributor Author

csrwng commented Jan 26, 2016

fixed and added test

// The ImageStreamImport resource exists in v1.1.1 of origin but is not yet
// enabled by policy. A create request will return a Forbidden(403) error.
// We want to return ErrImageStreamImportUnsupported to allow fallback behavior
// in clients.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, an admin may just want to forbid import just for normal use.

@smarterclayton
Copy link
Contributor

LGTM [merge] thanks

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/4750/) (Image: devenv-rhel7_3251)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 8da84a5

@openshift-bot
Copy link
Contributor

[Test]ing while waiting on the merge queue

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 8da84a5

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/469/)

openshift-bot pushed a commit that referenced this pull request Jan 27, 2016
@openshift-bot openshift-bot merged commit f138a18 into openshift:master Jan 27, 2016
@smarterclayton
Copy link
Contributor

I may end up moving this out of this method so that clients have to check it themselves - will come back to this once I sort out the outcome. We use "ErrImageStreamImportNotSupported" to indicate whether we support "scheduled" or not, so we'll probably need some story.

@csrwng csrwng deleted the imagestreamimport-fallback branch January 27, 2016 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants