-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Update etcd stores to use DefaultQualifiedResource #15721
Update etcd stores to use DefaultQualifiedResource #15721
Conversation
/unassign |
1527c37
to
b226b76
Compare
@mfojtik can you help me debug the Locally it fails in the same way:
|
…ect APIVersion Signed-off-by: Monis Khan <mkhan@redhat.com>
This field replaces QualifiedResource and serves as the default group resource info when the request does not provide that data. Signed-off-by: Monis Khan <mkhan@redhat.com>
b226b76
to
e8e4e46
Compare
status.Details.Name != m.repo.name { | ||
kind := strings.ToLower(status.Details.Kind) | ||
isValidKind := kind == "imagestream" /*pre-1.2*/ || kind == "imagestreams" /*1.2 to 1.6*/ || kind == "imagestreammappings" /*1.7+*/ | ||
if !isValidKind || status.Code != http.StatusNotFound || status.Details.Name != m.repo.name { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is required because in pkg/image/registry/imagestreammapping/rest.go
:
// findStreamForMapping retrieves an ImageStream whose DockerImageRepository matches dockerRepo.
func (s *REST) findStreamForMapping(ctx apirequest.Context, mapping *imageapi.ImageStreamMapping) (*imageapi.ImageStream, error) {
if len(mapping.Name) > 0 {
// Since ctx is passed through here, this returns a NotFound error for imagestreammappings now
return s.imageStreamRegistry.GetImageStream(ctx, mapping.Name, &metav1.GetOptions{})
}
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@smarterclayton @mfojtik all green now, just waiting on your approval. |
/approve |
1 similar comment
/approve |
@@ -153,7 +153,7 @@ os::test::junit::declare_suite_start "cmd/deployments/setdeploymenthook" | |||
arg="-f test/integration/testdata/test-deployment-config.yaml" | |||
os::cmd::expect_failure_and_text "oc set deployment-hook" "error: one or more deployment configs" | |||
os::cmd::expect_failure_and_text "oc set deployment-hook ${arg}" "error: you must specify one of --pre, --mid, or --post" | |||
os::cmd::expect_failure_and_text "oc set deployment-hook ${arg} -o yaml --pre -- mycmd" 'deploymentconfigs.apps.openshift.io "test-deployment-config" not found' | |||
os::cmd::expect_failure_and_text "oc set deployment-hook ${arg} -o yaml --pre -- mycmd" 'deploymentconfigs "test-deployment-config" not found' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
os::cmd::expect_failure_and_text 'oadm groups new errorgroup -o blah' 'error: output format "blah" not recognized' | ||
os::cmd::expect_failure_and_text 'oc get groups/errorgroup' 'groups.user.openshift.io "errorgroup" not found' | ||
os::cmd::expect_failure_and_text 'oc get groups/errorgroup' 'groups "errorgroup" not found' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -116,9 +116,9 @@ os::test::junit::declare_suite_end | |||
|
|||
os::test::junit::declare_suite_start "cmd/admin/groups" | |||
os::cmd::expect_success_and_text 'oadm groups new shortoutputgroup -o name' 'groups/shortoutputgroup' | |||
os::cmd::expect_failure_and_text 'oadm groups new shortoutputgroup' 'groups.user.openshift.io "shortoutputgroup" already exists' | |||
os::cmd::expect_failure_and_text 'oadm groups new shortoutputgroup' 'groups "shortoutputgroup" already exists' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those changes look to be due to the preference for oapi. I think its fine. /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, enj, legionus The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
/test all [submit-queue is verifying that this PR is safe to merge] |
/retest Please review the full test history for this PR and help us cut down flakes. |
/retest |
2 similar comments
/retest |
/retest |
/test all [submit-queue is verifying that this PR is safe to merge] |
@enj: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
Automatic merge from submit-queue (batch tested with PRs 15581, 15721) |
Automatic merge from submit-queue Migrate to Kubernetes RBAC Trello xref: https://trello.com/c/n3bR3Ys9 Fixes #12303 Fixes #13549 Fixes #13432 Fixes #15338 Fixes #14168 Fixes #10056 Need to investigate: - [x] ... Dependencies: - [x] Prerequisite #15342 - [x] Requires openshift/openshift-ansible/pull/4933 @sdodson - [x] Blocked on openshift/openshift-ansible/issues/4967 - [x] Prerequisite kubernetes/kubernetes#50639 Followups: - [ ] #15412 - [ ] #13316 - [ ] #13156 - [ ] #13430 - [ ] Should delete with proxy return details? - [ ] Make project creation use RBAC instead of proxy endpoints? - [ ] Remove policy objects from bootstrap roles - [ ] Check if delegated_test.go can be revived - [ ] Check to see if the deleted unit tests are reflected upstream and fix gaps - [ ] Open issue to remove `openshiftSubjectLocator` - [ ] Open issue to revisit forbidden message maker - [ ] Update upstream `subject_locator_test` with origin's extensive testing - [ ] Fix proxied create: ` _ bool is includeUnintialized, which we should really be passing through to the underlying API... it's odd there's not a CreateOptions parameter to Create` - [ ] Fix proxied update: `if initializers use Update() to initialize objects (which I think they do), we may need to pass GetOptions{IncludeUninitialized: true} here...` - [ ] Fix panics() in Convert...OrDie() functions - [ ] glog.Fatal on post stark hook error - [ ] Remove `TestPolicyCache`? - [ ] Use discovery API based gating? - [ ] upstream rules have always required a group. followup issue to remove getAPIGroupLegacy from `pkg/authorization/authorizer/scope/converter.go` - [ ] issue to remove "normalizeResources" from `pkg/cmd/server/bootstrappolicy/policy.go` - [ ] issue to find callers of `clusterpolicyregistry "github.com/openshift/origin/pkg/authorization/registry/clusterpolicy"` and move to point of use - [ ] issue to switch our encoding to rbac in `pkg/cmd/server/admin/create_bootstrappolicy_file.go` - [ ] Exercise proxied endpoints - [ ] hack/test-cmd.sh of gated overwrite bootstrap policy - [ ] Delete unused legacy policy registry code - [ ] Make RBAC discovery rule authoritative `pkg/authorization/apis/authorization/types.go` - [ ] Fix `ignoreError` in `pkg/oc/admin/router/router.go` - [ ] Confirm changes to `TestAuthorizationResolution` and `TestAuthorizationResourceAccessReview` in `test/integration/authorization_test.go` Done: - Store ClusterRoles as native RBAC Objects via Kubernetes. - Provides backwards compatible API for the old policy based roles. - Use Kubernetes authorizer TODO: - [x] Delete policy end points - [x] Decide what to do with overwrite policy - [x] Remove or gate `oc create policybinding` - [x] Move new impersonation code to `pkg/auth/client/impersonate.go` - [x] Remove any unnecessary conversions - [x] Review new `proxy.go` files - [x] Remove reason logic `allowed by rule in ...` - [x] Add interface assertion to proxy files - [x] Confirm we need `pkg/authorization/util/convert/convert.go` - [x] Confrim we need to expose some of the private conversion functions - [x] Add protect/autoupdate annotation conversion to general conversion functions - [x] ~~Support watch on proxied endpoints~~ - [x] Cherry pick kubernetes/kubernetes#49868 -> #15721 - [x] Fix upstream commits - [x] Restore and version gate `NewCmdMigrateAuthorization` - [x] ~~Wrap other errors in proxy files?~~ Remove all error wrapping - [x] Make `NewImpersonatingRBACFromContext` more generic - [x] Kube authorizer's reason on deny contains evaluation errors - do we want to preserve those? - [x] Review `ImpersonatingRESTClient` in `pkg/auth/client/impersonate.go` - [ ] Review `pkg/project/auth/cache.go` and ` pkg/project/auth/cache_test.go` - [ ] Review ` pkg/authorization/authorizer/scope/converter_test.go` - [ ] Review `k8s.io/kubernetes/staging/src/k8s.io/client-go/rest/request.go`
This field replaces QualifiedResource and serves as the default group resource info when the request does not provide that data.
Signed-off-by: Monis Khan mkhan@redhat.com
xref: #15213 #15006 kubernetes/kubernetes/pull/49868