-
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
Enable authorization.k8s.io API and update integration tests #13128
Enable authorization.k8s.io API and update integration tests #13128
Conversation
[test] |
probably missing kubernetes/kubernetes#40935, part of the pick list for 1.5 |
@@ -196,7 +196,7 @@ func BuildDefaultAPIServer(options configapi.MasterConfig) (*apiserveroptions.Se | |||
return server, storageFactory, nil | |||
} | |||
|
|||
func BuildKubernetesMasterConfig(options configapi.MasterConfig, requestContextMapper kapi.RequestContextMapper, kubeClient kclientset.Interface, informers shared.InformerFactory, admissionControl admission.Interface, originAuthenticator authenticator.Request) (*MasterConfig, error) { | |||
func BuildKubernetesMasterConfig(options configapi.MasterConfig, requestContextMapper kapi.RequestContextMapper, kubeClient kclientset.Interface, informers shared.InformerFactory, admissionControl admission.Interface, originAuthenticator authenticator.Request, kubeAuthorizer authorizer.Authorizer) (*MasterConfig, error) { |
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.
Add a TODO to rethink this. Seven arguments is a bit much.
@@ -357,8 +358,19 @@ func BuildKubernetesMasterConfig(openshiftConfig *origin.MasterConfig) (*kuberne | |||
if openshiftConfig.Options.KubernetesMasterConfig == nil { | |||
return nil, nil | |||
} | |||
kubeConfig, err := kubernetes.BuildKubernetesMasterConfig(openshiftConfig.Options, openshiftConfig.RequestContextMapper, openshiftConfig.KubeClientset(), openshiftConfig.Informers, openshiftConfig.KubeAdmissionControl, openshiftConfig.Authenticator) | |||
return kubeConfig, err | |||
kubeAuthorizer, err := adapter.NewAuthorizer(openshiftConfig.Authorizer) |
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.
Find the issue to collapse these. @soltysh did you have a pull that did that?
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.
I don't recall anything like that. I think sttts has/had a todo for that.
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.
@sttts any comments?
test/cmd/authentication.sh
Outdated
@@ -74,7 +74,7 @@ os::cmd::expect_success_and_text "curl -k -XPOST -H 'Content-Type: application/j | |||
os::cmd::expect_success_and_text "oc policy can-i create pods --token='${accesstoken}' -n '${project}' --ignore-scopes" 'yes' | |||
os::cmd::expect_success_and_text "oc policy can-i create pods --token='${accesstoken}' -n '${project}'" 'no' | |||
os::cmd::expect_success_and_text "oc policy can-i create subjectaccessreviews --token='${accesstoken}' -n '${project}'" 'no' | |||
os::cmd::expect_success_and_text "oc policy can-i create subjectaccessreviews --token='${accesstoken}' -n '${project}' --ignore-scopes" 'yes' | |||
os::cmd::expect_success_and_text "oc policy can-i create subjectaccessreviews.v1. --token='${accesstoken}' -n '${project}' --ignore-scopes" 'yes' |
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 should not have needed tweaking. Why did it change?
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.
subjectaccessreviews
without group info is ambiguous now since it could refer to either the kube or origin resource.
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.
the resolution should treat the one without the group (the origin one) as an exact match and not fail. if it fails, we should fix that.
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.
subjectaccessreviews without group info is ambiguous now since it could refer to either the kube or origin resource.
Are you getting an error? It should be ambiguous with a preference.
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.
Added some logging to debug better:
With subjectaccessreviews
we get unversioned.GroupResource{Group:"", Resource:"subjectaccessreviews"}
+ .WithVersion("")
+ mapper.ResourceFor
-> unversioned.GroupVersionResource{Group:"authorization.k8s.io", Version:"v1beta1", Resource:"subjectaccessreviews"}
With subjectaccessreviews.v1.
we get unversioned.GroupVersionResource{Group:"", Version:"v1", Resource:"subjectaccessreviews"}
directly.
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.
Sounds like the priority in the discovery doc is wrong.
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.
Mapper's priority seems OK:
kubectl.OutputVersionMapper {
RESTMapper: util.ShortcutExpander {
RESTMapper: meta.FirstHitRESTMapper {
MultiRESTMapper: meta.MultiRESTMapper {
( * discovery.DeferredDiscoveryRESTMapper)(0xc420be2d80), meta.PriorityRESTMapper {
Delegate: meta.MultiRESTMapper {
( * meta.DefaultRESTMapper)(0xc4201a3770), ( * meta.DefaultRESTMapper)(0xc420016410), ( * meta.DefaultRESTMapper)(0xc420016780), meta.MultiRESTMapper {
meta.MultiRESTMapper {
meta.MultiRESTMapper {
meta.MultiRESTMapper {
meta.MultiRESTMapper {
meta.MultiRESTMapper {
meta.MultiRESTMapper {
meta.MultiRESTMapper {
meta.MultiRESTMapper {
meta.MultiRESTMapper {
meta.MultiRESTMapper {
meta.MultiRESTMapper {
( * meta.DefaultRESTMapper)(0xc4201a3450), ( * meta.DefaultRESTMapper)(0xc42021f770)
}, ( * meta.DefaultRESTMapper)(0xc42021f860)
}, ( * meta.DefaultRESTMapper)(0xc42021f9f0)
}, ( * meta.DefaultRESTMapper)(0xc42021fc20)
}, ( * meta.DefaultRESTMapper)(0xc42021fc70)
}, ( * meta.DefaultRESTMapper)(0xc42021fcc0)
}, ( * meta.DefaultRESTMapper)(0xc42021fd10)
}, ( * meta.DefaultRESTMapper)(0xc42021fe00)
}, ( * meta.DefaultRESTMapper)(0xc42021fe50)
}, ( * meta.DefaultRESTMapper)(0xc42021fea0)
}, ( * meta.DefaultRESTMapper)(0xc42021fef0)
}, ( * meta.DefaultRESTMapper)(0xc42021ff40)
}, ( * meta.DefaultRESTMapper)(0xc4201a3d10), ( * meta.DefaultRESTMapper)(0xc420016320), ( * meta.DefaultRESTMapper)(0xc4201a3590), ( * meta.DefaultRESTMapper)(0xc4201a3e00), ( * meta.DefaultRESTMapper)(0xc4201a2b40), ( * meta.DefaultRESTMapper)(0xc4201a39a0), ( * meta.DefaultRESTMapper)(0xc4200164b0), ( * meta.DefaultRESTMapper)(0xc4200168c0), ( * meta.DefaultRESTMapper)(0xc4201a3630)
},
ResourcePriority: [] unversioned.GroupVersionResource {
unversioned.GroupVersionResource {
Group: "",
Version: "v1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "extensions",
Version: "v1beta1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "extensions",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "metrics",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "apps",
Version: "v1beta1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "authentication.k8s.io",
Version: "v1beta1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "authorization.k8s.io",
Version: "v1beta1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "autoscaling",
Version: "v1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "batch",
Version: "v1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "certificates.k8s.io",
Version: "v1alpha1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "componentconfig",
Version: "v1alpha1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "federation",
Version: "v1beta1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "policy",
Version: "v1beta1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "rbac.authorization.k8s.io",
Version: "v1alpha1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "storage.k8s.io",
Version: "v1beta1",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "apps",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "authentication.k8s.io",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "authorization.k8s.io",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "autoscaling",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "batch",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "certificates.k8s.io",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "componentconfig",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "federation",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "policy",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "rbac.authorization.k8s.io",
Version: "*",
Resource: "*"
}, unversioned.GroupVersionResource {
Group: "storage.k8s.io",
Version: "*",
Resource: "*"
}
},
KindPriority: [] unversioned.GroupVersionKind {
unversioned.GroupVersionKind {
Group: "",
Version: "v1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "extensions",
Version: "v1beta1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "extensions",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "metrics",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "apps",
Version: "v1beta1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "authentication.k8s.io",
Version: "v1beta1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "authorization.k8s.io",
Version: "v1beta1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "autoscaling",
Version: "v1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "batch",
Version: "v1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "certificates.k8s.io",
Version: "v1alpha1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "componentconfig",
Version: "v1alpha1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "federation",
Version: "v1beta1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "policy",
Version: "v1beta1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "rbac.authorization.k8s.io",
Version: "v1alpha1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "storage.k8s.io",
Version: "v1beta1",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "apps",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "authentication.k8s.io",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "authorization.k8s.io",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "autoscaling",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "batch",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "certificates.k8s.io",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "componentconfig",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "federation",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "policy",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "rbac.authorization.k8s.io",
Version: "*",
Kind: "*"
}, unversioned.GroupVersionKind {
Group: "storage.k8s.io",
Version: "*",
Kind: "*"
}
}
}
}
},
All: [] unversioned.GroupResource {
unversioned.GroupResource {
Group: "",
Resource: "buildconfigs"
}, unversioned.GroupResource {
Group: "",
Resource: "builds"
}, unversioned.GroupResource {
Group: "",
Resource: "imagestreams"
}, unversioned.GroupResource {
Group: "",
Resource: "deploymentconfigs"
}, unversioned.GroupResource {
Group: "extensions",
Resource: "deployments"
}, unversioned.GroupResource {
Group: "autoscaling",
Resource: "horizontalpodautoscalers"
}, unversioned.GroupResource {
Group: "",
Resource: "replicationcontrollers"
}, unversioned.GroupResource {
Group: "",
Resource: "routes"
}, unversioned.GroupResource {
Group: "",
Resource: "services"
}, unversioned.GroupResource {
Group: "apps",
Resource: "statefulsets"
}, unversioned.GroupResource {
Group: "extensions",
Resource: "jobs"
}, unversioned.GroupResource {
Group: "extensions",
Resource: "replicasets"
}, unversioned.GroupResource {
Group: "",
Resource: "pods"
}
},
discoveryClient: ( * util.CachedDiscoveryClient)(0xc420be65c0)
},
OutputVersions: [] unversioned.GroupVersion {
unversioned.GroupVersion {
Group: "",
Version: "v1"
}
}
}
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.
The rest mapper making the choice doesn't seem to agree. It looks be doing an alpha sort.
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.
Yeah, where did you get yours? The discovery restmappers is in discovery order.
} | ||
} | ||
|
||
if test.kubeAuthInterface != nil { |
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.
Why would this ever be nil? Seems like I'd always want to do this check and rather than accidentally skip it, we should fail.
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.
Does the kube clientset have a way to handle impersonation? We have tests that check SAR functionality while impersonating a different user. If we cannot test that with kube then I can explicitly whitelist those and make kubeAuthInterface
required otherwise.
if test.kubeAuthInterface != nil { | ||
var testNS string | ||
if test.localReview != nil { | ||
if len(test.localReview.Namespace) > 0 { |
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.
Why are there so many different signals?
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.
Basically the original tests do not provide namespace info directly because it is either inferred or is part of the auth interface that is passed in. I wanted to avoid changing the old test data to prevent me from accidentally invalidating it.
if test.kubeAuthInterface != nil { | ||
var testNS string | ||
if test.localReview != nil { | ||
if len(test.localReview.Namespace) > 0 { |
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.
switch/case please.
failMessage = "" | ||
return true, nil | ||
}) | ||
func toKubeSelfSAR(testNS string, sar *authorizationapi.LocalSubjectAccessReview) *kubeauthorizationapi.SelfSubjectAccessReview { |
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.
Are these fully convertible to and from? I'll give this to you for now, but if they are convertible, you should write a real conversion and fuzz it.
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.
I do not think they are fully convertible. Our SAR stuff defaults the namespace and user to the current if not given, kube requires it. Kube SAR also has NonResourceAttributes
and Subresource
which we do not seem to have. I am also not sure if kube's Extra
and our Scopes
can be round tripped without losing data.
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.
I do not think they are fully convertible. Our SAR stuff defaults the namespace and user to the current if not given, kube requires it. Kube SAR also has NonResourceAttributes and Subresource which we do not seem to have. I am also not sure if kube's Extra and our Scopes can be round tripped without losing data.
ok.
//Subresource :sar.Subreouse,?? TODO | ||
Name: sar.ResourceName, | ||
}, | ||
//NonResourceAttributes: &kubeauthorizationapi.NonResourceAttributes{ |
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.
Why are these commented out?
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.
I planned on simply removing it but I was not sure if the origin SAR stuff even handled NonResourceAttributes
. While both ResourceAttributes
and NonResourceAttributes
cannot be specified together, the origin stuff does have a .Path
so I feel like that should be used at some point.
Spec: kubeauthorizationapi.SubjectAccessReviewSpec{ | ||
User: sar.User, | ||
Groups: sar.Groups.List(), | ||
//Extra: scope?? TODO |
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.
scopes can be translated to extras. Issue.
clusterReview: askCanDannyGetProject, | ||
kubeAuthInterface: dannySARGetter, | ||
err: `User "danny" cannot create subjectaccessreviews at the cluster scope`, | ||
kubeErr: `User "danny" cannot create authorization.k8s.io.subjectaccessreviews at the cluster scope`, |
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.
Issue, that message looks backwards. I expected subjectaccessreviews.authorization.k8s.io.
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.
The authorizer's messages put the group first (and none of its unit tests provide group info).
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.
The authorizer's messages put the group first (and none of its unit tests provide group info).
Write an issue. It's a human readable message, so we should be able to change it.
unversioned.GroupResource{Group: "", Resource: "localresourceaccessreviews"}: true, | ||
unversioned.GroupResource{Group: "", Resource: "selfsubjectrulesreviews"}: true, | ||
unversioned.GroupResource{Group: "", Resource: "subjectrulesreviews"}: true, | ||
unversioned.GroupResource{Group: "authorization.k8s.io", Resource: "selfsubjectaccessreviews"}: true, // TODO import group 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.
don't import group name. This is correct.
Minor comments. You need a separate commit for your upstream patch. It should be an UPSTREAM: : remove post 1.7 rebase. Make the pick Jordan requested and this will be about ready. |
pick is open at #13086 |
53d4e5a
to
083a442
Compare
@deads2k comments addressed, issues opened, squashed, ready for merge if tests pass / you are good. |
Signed-off-by: Monis Khan <[email protected]>
083a442
to
b5fce7e
Compare
Waiting for #13085 . Also, renamed your commit |
… rebase Signed-off-by: Monis Khan <[email protected]>
b5fce7e
to
87fa28d
Compare
Evaluated for origin test up to 87fa28d |
lgtm [merge] |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/676/) (Base Commit: 1f33720) |
re[merge] |
Evaluated for origin merge up to 87fa28d |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/760/) (Base Commit: b6847b3) (Image: devenv-rhel7_6019) |
Supersedes #13021
Integration tests are a bit hacky to avoid having to copy/paste all of them.
Things I have not figured out how to do:
Extra
andSubresource
not populated in kube SARsNonResourceAttributes
not populated in kube SARsxref issues:
subjectaccessreviews.v1.
hacks fromtest/cmd/authentication.sh
#13158Signed-off-by: Monis Khan [email protected]