Status objects for 404 API errors will have the correct APIVersion #49868
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-github-robot
added
size/L
shiywang
force-pushed
the
testlog
branch
2 times, most recently
from
2d3f72e to
94eebc5
Compare
|
@sttts I just add a |
|
Will review tomorrow morning. |
| @@ -42,6 +43,7 @@ func newStorage(t *testing.T) (*REST, *StatusREST, *etcdtesting.EtcdTestServer) | |||
| // createStatefulSet is a helper function that returns a StatefulSet with the updated resource version. | |||
| func createStatefulSet(storage *REST, ps apps.StatefulSet, t *testing.T) (apps.StatefulSet, error) { | |||
| ctx := genericapirequest.WithNamespace(genericapirequest.NewContext(), ps.Namespace) | |||
| ctx = genericapirequest.WithRequestInfo(ctx, testhelper.FakeRequestInfo("", "")) | |||
There was a problem hiding this comment.
I wouldn't expect to need to do this in all the tests... I'd expect the store to use the requestinfo from the context if present, otherwise use the default qualified resource
There was a problem hiding this comment.
that's the behaviour now in qualifiedResourceFromContext (it wasn't before). @shiywang does that mean we can remove some of the WithRequestInfo calls?
shiywang
force-pushed
the
testlog
branch
5 times, most recently
from
0740a8f to
16674ed
Compare
|
/test pull-kubernetes-federation-e2e-gce |
| @@ -623,6 +642,17 @@ func (e *Store) Get(ctx genericapirequest.Context, name string, options *metav1. | |||
| return obj, nil | |||
| } | |||
|
|
|||
| // qualifiedResourceFromContext retrieves GroupResource from Context per request, if not found, return default GroupResource | |||
| func qualifiedResourceFromContext(ctx genericapirequest.Context, def schema.GroupResource) (schema.GroupResource, error) { | |||
There was a problem hiding this comment.
this no longer has the possibility of returning an error, so changing the signature would let it be used inline.
if you make it a method on Store, you also remove the need to pass in the default qualified resource.
func (e *Store) qualifiedResource(ctx genericapirequest.Context) schema.GroupResource {
if info, ok := request.RequestInfoFrom(ctx); ok {
return schema.GroupResource{Group: info.APIGroup, Resource: info.Resource}
}
return e.QualifiedResource
}
this can then be used as storeerr.InterpretListError(err, e.qualifiedResource(ctx))
I'd also consider renaming the QualifiedResource field to DefaultQualifiedResource... that would make sure this PR catches all references to it, and the field documentation could clarify that it is the qualified resource used if there is no request info present in the context.
k8s-github-robot
added
size/XL
|
@liggitt done, ptal if all tests pass |
| @@ -322,12 +327,12 @@ func (e *Store) Create(ctx genericapirequest.Context, obj runtime.Object, includ | |||
| } | |||
| } | |||
| if !includeUninitialized { | |||
| return e.WaitForInitialized(ctx, out) | |||
| return e.waitForInitialized(ctx, out, qualifiedResource) | |||
There was a problem hiding this comment.
no need to change the signature, you can just call e.qualifiedResourceFromContext(ctx) inside e.waitForInitialized
| @@ -439,23 +444,23 @@ func (e *Store) shouldDeleteForFailedInitialization(ctx genericapirequest.Contex | |||
|
|
|||
| // deleteWithoutFinalizers handles deleting an object ignoring its finalizer list. | |||
| // Used for objects that are either been finalized or have never initialized. | |||
| func (e *Store) deleteWithoutFinalizers(ctx genericapirequest.Context, name, key string, obj runtime.Object, preconditions *storage.Preconditions) (runtime.Object, bool, error) { | |||
| func (e *Store) deleteWithoutFinalizers(ctx genericapirequest.Context, name, key string, obj runtime.Object, preconditions *storage.Preconditions, qualifiedResource schema.GroupResource) (runtime.Object, bool, error) { | |||
| } | ||
| _, err := e.finalizeDelete(out, true) | ||
| _, err := e.finalizeDelete(out, true, qualifiedResource) |
There was a problem hiding this comment.
rather than plumbing the qualified resource, why not plumb the context and call e.qualifiedResource(ctx) internally?
|
@liggitt now only |
k8s-github-robot
added
the
needs-rebase
k8s-github-robot
removed
lgtm
|
/test pull-kubernetes-e2e-kops-aws |
shiywang
force-pushed
the
testlog
branch
2 times, most recently
from
d479506 to
8443c29
Compare
|
/test pull-kubernetes-e2e-gce-etcd3 |
|
@liggitt @smarterclayton could you please take a look and merge this, I already rebased twice and it would be nice if this pr could be merged soon. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enj, liggitt, shiywang, smarterclayton Associated issue: 48959 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 |
k8s-github-robot
added
the
approved
|
/test pull-kubernetes-e2e-gce-etcd3 |
1 similar comment
|
/test pull-kubernetes-e2e-gce-etcd3 |
|
/retest I read somewhere the gce bug is solved? Maybe rebasing would help. |
k8s-github-robot
removed
the
lgtm
sttts
added
the
lgtm
|
/test all [submit-queue is verifying that this PR is safe to merge] |
|
@shiywang: 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 49868, 50143, 49377, 50141, 50145) |
Automatic merge from submit-queue (batch tested with PRs 15581, 15721) Update etcd stores to use DefaultQualifiedResource 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
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`
Fixes #48959
superseded #49183