handle non resource urls #998

deads2k · 2015-02-12T18:09:32Z

Depends on #991

Adds the ability to describe non-resources for policy matching. This can be used to allow access to one off endpoints like healthz and version.

You can now specify a path with an ending wildcard that will be evaluated against the remainder of a request path to determine a match or not. The evaluation takes the remainder of a URL and attempts to match it against a series of explicitly allowed steps that can end in a wildcard.

@liggitt last two commits?

liggitt · 2015-02-13T18:03:18Z

calling GetAPIRequestInfo with non-resource urls is returning flawed data... GetAPIRequestInfo("/healthz") returns verb=list, type=healthz.

liggitt · 2015-02-13T18:09:37Z

pkg/authorization/api/v1beta1/types.go

Need a clear description of how this is used in combination with the Resources/ResourceNames restrictions. Is this required to match requests in addition to Resources/ResourceNames, or instead of?

Also, URL rather than Url (everywhere)

Need to fix the godoc

deads2k · 2015-02-13T18:58:15Z

calling GetAPIRequestInfo with non-resource urls is returning flawed data... GetAPIRequestInfo("/healthz") returns verb=list, type=healthz

Yeah. Looking at kubernetes, I think that's a feature not a bug: https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/apiserver/handlers.go#L225. The comment predated me, but from what I can gather they don't want to rely upon an api/version url shape. The tests and comments make it clear that it was an explicit choice.

That means that looking a URL, you cannot logically determine whether it is a non-resource url or not. The list bit is a little weird, but it doesn't bother me too much.

If you'd prefer, we can add our own detection based on a list of known resource URL prefixes. I don't feel strongly.

smarterclayton · 2015-02-18T00:49:40Z

Let's talk about this tomorrow

deads2k · 2015-02-18T12:40:37Z

Let's talk about this tomorrow

@liggitt and I secretly agreed that this implementation was bad and we should update it actually be an either/or with any resource API. That detection is possible now that we've had the rebase and one upstream patch.

I should be in the office around 10-10:30 (thank god).

smarterclayton · 2015-02-18T22:26:43Z

Don't leave too early. But I will be in as well (thank god).

deads2k · 2015-02-19T12:31:21Z

Don't leave too early.

"You're not under arrest, but don't leave town...."

deads2k · 2015-02-19T12:40:35Z

@liggitt Updates complete

To grant non-resource permissions to role looks like this:

{
    "metadata":{
        "name":"cluster-status",
        "namespace":"master"
    },
    "rules":[
        {
            "verbs":["get"],
            "nonResourceURLs":["api","healthz","osapi","version"]
        }
    ]
}

/healthz, /version, /osapi, and /api are allowed to authenticated and unauthenticated by default.

deads2k · 2015-02-19T12:50:32Z

[test]

openshift-bot · 2015-02-19T12:52:10Z

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

smarterclayton · 2015-02-19T16:26:32Z

pkg/authorization/authorizer/authorizer.go

liggitt · 2015-02-19T16:33:46Z

"nonResourceURLs":["api","healthz","osapi","version"] feels a lot better

Should we require leading slashes to avoid the appearance of relative or remainder path matching?

liggitt · 2015-02-19T16:37:20Z

pkg/authorization/api/types.go

ok... I guess I'm forgetting what we talked about... did we end up at remainder URL matching?

Bad comment. Updated .

deads2k · 2015-02-19T16:46:26Z

Should we require leading slashes to avoid the appearance of relative or remainder path matching?

Doing

liggitt · 2015-02-19T16:49:38Z

pkg/authorization/authorizer/authorizer.go

also note in the doc for namespace/resource/resourcename that they will be "" when IsNonResourceURL is true

smarterclayton · 2015-02-19T17:57:15Z

pkg/authorization/authorizer/authorizer.go

Special case needed forever? Add todo

deads2k · 2015-02-19T18:04:57Z

I think I'm caught up with comments.

smarterclayton · 2015-02-19T18:17:48Z

pkg/authorization/authorizer/authorizer.go

Much clearer, thanks

smarterclayton · 2015-02-19T18:18:15Z

LGTM [merge]

openshift-bot · 2015-02-19T18:22:07Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_openshift3/972/) (Image: devenv-fedora_841)

openshift-bot · 2015-02-19T18:22:07Z

Evaluated for origin up to 4baffdd

Merged by openshift-bot

…service-catalog/' changes from 568a7b9..8f07b7b 8f07b7b origin: add required patches ee57bfb Cleanup of ups broker example + making controller follow the OSB API (openshift#807) 45a11ed Revert "Rename our resources to have ServiceCatalog prefix (openshift#1054)" (openshift#1061) 4e47ec1 Rename our resources to have ServiceCatalog prefix (openshift#1054) 2bb334a Rebase on 1.7 API machinery (openshift#944) 5780b59 Run broker reconciler when spec is changed. (openshift#1026) 9c22d04 Merge branch 'pr/1006' d077915 check number of expected events before dereferencing to avoid panic (openshift#1052) 90d615f Merge branch 'pr/1055' bb6d6d8 fix log output to use formatted output (openshift#1056) c7abc81 Adding examples to the README ccc93c9 Remove different-org rule for LGTM (openshift#1050) be04cd5 Allow for a period in the GUID of the External ID (openshift#1034) 8c246df Make it so that binding.spec.secretName defaults to binding name (openshift#851) 6745418 Bump OSB Client (openshift#1049) 8346a0d apiserver etcd healthcheck as suggested to address k/k#48215 (openshift#1039) 11d0d4a use GKE's latest 1.6.X cluster version for Jenkins (openshift#1036) 7d71b5b Cross-build all the things! 8ec0874 RBAC setup behind the aggregator. (openshift#936) 0864a2e Upsert retry loop for Secret, set/check ownerReference for Secret owned by Binding (openshift#979) 6be9886 add info about weekly calls (openshift#1027) a242b26 add OSB API Header version flag (openshift#1014) 66e2ce6 Update REVIEWING doc with changes to LGTM process (openshift#1016) 699e016 Writing the returned progress description from the broker (openshift#998) 02642f4 Adding target to test on the host (openshift#1020) 78ca572 v0.0.13 (openshift#1024) 9e79ec2 use GKE's default K8S version for Jenkins (openshift#1023) d3c915a Fix curl on API server start error (openshift#1015) b50be75 Merge branch 'pr/1013' 2c98ba1 Using tag URLs 687f091 Parameterizing the priority fields 34ed5cd update apiregistration yaml to v1.7 final (openshift#1011) 91fa1ad make e2e look for pods' existence before checking status (openshift#1012) 0f90705 explicitly disable leader election if it is not enabled (openshift#965) f5761e7 controller-manager health checks (openshift#694) da260f2 Add logging for normal Unbind errors (openshift#992) 4c916a5 make the apiserver test use tls (openshift#991) 1a62ecc refactor reconcileBroker (openshift#986) cc179bc Add logging for normal Bind errors (openshift#993) a1458dd add parameterization for user-broker image to e2e tests (openshift#995) fb15891 Bump OSB client (openshift#1000) 79d5206 v0.0.12 (openshift#996) 39c7407 Merge branch 'pr/975' a553b2d Merge branch 'pr/974' d573339 reconcileBinding error checking (openshift#973) 39a1061 Making events and actions checks generic (openshift#960) 73136a4 Bump osb client (openshift#971) 878a987 reconcileInstance error checking in unit-tests 4991d57 reconcileBroker error checking in unit-tests 9ed6812 Extract methods for binding test setup (openshift#961) b69a1ee Make ups-broker return valid unbind response (openshift#964) 8b37d2f Releasing 0.0.11 (openshift#962) 52fec8b Merge branch 'pr/954' d49cdeb Swap client 445fa71 Add dependency on pmorie/go-open-service-broker-client 9f743b2 Instructions for enabling API Aggregation (openshift#895) 512508d Use correct infof calls in controller_manager (openshift#950) 77943ba fix regex that determines if a tag is deployable (openshift#947) 8a226b8 Updates for v0.0.10 release (openshift#943) REVERT: 568a7b9 origin build: add origin tooling git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog git-subtree-split: 8f07b7bbf3acb2b557f23596a92b5e775ae9321c

UPSTREAM: 55826: Return nil error if checkpoint returns with KeyNotFound error

) * Writing the returned progress description from the broker * Only updating the status if the description was present * Updating tests to account for the description update action * Being clearer about setting message and reason Including protection against nil descriptions * Adding test for updating action

deads2k force-pushed the deads-handle-non-resource-urls branch 2 times, most recently from e4a9829 to 7f1b442 Compare February 12, 2015 21:01

deads2k changed the title ~~[waiting] handle non resource urls~~ handle non resource urls Feb 12, 2015

deads2k force-pushed the deads-handle-non-resource-urls branch from 7f1b442 to 7268f27 Compare February 13, 2015 14:04

liggitt reviewed Feb 13, 2015
View reviewed changes

deads2k force-pushed the deads-handle-non-resource-urls branch 2 times, most recently from fa5e86d to 5398a50 Compare February 18, 2015 21:23

deads2k force-pushed the deads-handle-non-resource-urls branch from 5398a50 to d2ec071 Compare February 19, 2015 12:43

jwforres added this to the 0.4.0 (beta2) milestone Feb 19, 2015

smarterclayton reviewed Feb 19, 2015
View reviewed changes

pkg/authorization/authorizer/authorizer.go Outdated

Copy link

Contributor

smarterclayton Feb 19, 2015

Choose a reason for hiding this comment

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

Godoc

deads2k mentioned this pull request Feb 19, 2015

tighten authorization rules #1074

Merged

liggitt reviewed Feb 19, 2015
View reviewed changes

deads2k force-pushed the deads-handle-non-resource-urls branch from 96a63fc to 14ff408 Compare February 19, 2015 17:45

smarterclayton reviewed Feb 19, 2015
View reviewed changes

pkg/authorization/authorizer/authorizer.go Outdated

Copy link

Contributor

smarterclayton Feb 19, 2015

Choose a reason for hiding this comment

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

Special case needed forever? Add todo

smarterclayton reviewed Feb 19, 2015
View reviewed changes

pkg/authorization/authorizer/authorizer.go

Copy link

Contributor

smarterclayton Feb 19, 2015

Choose a reason for hiding this comment

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

Much clearer, thanks

authorize non-resource urls

4baffdd

deads2k force-pushed the deads-handle-non-resource-urls branch from ad0b626 to 4baffdd Compare February 19, 2015 18:20

openshift-bot pushed a commit that referenced this pull request Feb 19, 2015

Merge pull request #998 from deads2k/deads-handle-non-resource-urls

e6e2177

Merged by openshift-bot

openshift-bot merged commit e6e2177 into openshift:master Feb 19, 2015

deads2k deleted the deads-handle-non-resource-urls branch February 23, 2015 16:27

enj pushed a commit to enj/origin that referenced this pull request Jan 25, 2018

Merge pull request openshift#998 from dcbw/38-portmap-checkpoint-error

10da361

UPSTREAM: 55826: Return nil error if checkpoint returns with KeyNotFound error

handle non resource urls #998

handle non resource urls #998

Uh oh!

Conversation

deads2k commented Feb 12, 2015

Uh oh!

liggitt commented Feb 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Feb 13, 2015

Uh oh!

smarterclayton commented Feb 18, 2015

Uh oh!

deads2k commented Feb 18, 2015

Uh oh!

smarterclayton commented Feb 18, 2015

Uh oh!

deads2k commented Feb 19, 2015

Uh oh!

deads2k commented Feb 19, 2015

Uh oh!

deads2k commented Feb 19, 2015

Uh oh!

openshift-bot commented Feb 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Feb 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Feb 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

deads2k commented Feb 19, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarterclayton commented Feb 19, 2015

Uh oh!

openshift-bot commented Feb 19, 2015

Uh oh!

openshift-bot commented Feb 19, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants