-
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
remove user specification via request parameter for TSB #16434
remove user specification via request parameter for TSB #16434
Conversation
ah ... was about to mention @spadgett but the robot beat me to it :-) |
/lgtm |
/unassign @mfojtik |
Hmm ... import verifier is flagging an import I'm not immediately seeing:
investigating ... |
f7ca9bf
to
c835658
Compare
the user validate import we now no longer require needed to be removed from hack/import-restrictions.json ... change pushed |
/lgtm |
integration tests saw an apparent reincarnation of #14020 update / correction: with an assist from Mo, it was actually a validation error wrt the request user name no longer being there. The error:
and in particular something is still trying to validate on the spec.requester.username .... need to investigate |
@spadgett and I gave this a go and realized we didn't enable the orig id hdr feature via the new controller manager mechanism. We didn't see a way to actually do that yet via a @pmorie @staebler - are we correct in that there is not a way to do that yet? Or if there is, please provide us the details? thanks |
@gabemontero @spadgett You are correct that there is not currently a way to enable the feature. That was missed but has been checked in. There will be a new release of service catalog tonight that will include the flag to enabled feature gates in controller manager. When it is ready, you will be able to enable the feature with the argument |
I believe I sorted out the integration test issue ... We discussed this some before during the original pull ... I believe the net was long term, those two username's are valid. Bottom line, I believe our validation should stop prior to sorting out the specifics of the username, and let the lower level auth stuff manage it. |
On Tue, Sep 19, 2017 at 4:14 PM, Matthew Staebler ***@***.***> wrote:
@gabemontero <https://github.com/gabemontero> @spadgett
<https://github.com/spadgett> You are correct that there is not currently
a way to enable the feature. That was missed but has been checked in. There
will be a new release of service catalog tonight that will include the flag
to enabled feature gates in controller manager. When it is ready, you will
be able to enable the feature with the argument --feature-gates
OriginatingIdentity=true.
Thanks for the confirmation and update @staebler.
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16434 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADbadKDvqUJFyNiAKiUk1g2nId_keAAFks5skCCogaJpZM4PcmQg>
.
|
c835658
to
aab6564
Compare
ok integration tests are now passing, but somewhere along the way since first starting with this some of the template related extended tests are now failing; need to circle back and clean that back up; |
@@ -52,14 +52,9 @@ func (templateInstanceStrategy) Canonicalize(obj runtime.Object) { | |||
func (templateInstanceStrategy) PrepareForCreate(ctx apirequest.Context, obj runtime.Object) { | |||
templateInstance := obj.(*templateapi.TemplateInstance) | |||
|
|||
//TODO - when https://github.com/kubernetes-incubator/service-catalog/pull/939 sufficiently progresses, remove the if nil check and always | |||
// Clear any user-specified info, then seed it from the context | |||
if templateInstance.Spec.Requester == 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.
Could you remind me on what basis this is being removed? Does it affect the TSB setting the templateinstance requester on behalf of the TSB caller? If so, a test should be tripped?
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.
Actually @jim-minter perhaps my TODO comment here was erroneous, given the test failure noted at https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/16434/test_pull_request_origin_extended_conformance_gce/8233/
Or maybe an associated change elsewhere is needed.
I've started investigating what I currently have here, and cycling through the original PR. Will report back when I have discoveries of note.
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 it stemmed from a discussion thread that culminated at #15719 (comment)
But upon revisit/review, I believe my conclusion was incorrect. The user obj is in fact passed into Provision, etc., based on the service catalog auth path, and set on the requester.
And hence the extended test failures I noted.
Reverting the change momentarily ... we'll see how the extended test does. I'll then pull in the requisite SMEs for final sign off.
aab6564
to
df9ffc7
Compare
ok ... all happy tests .... the strategy.go change was the last bit will ask for a quick re-review in code |
// Clear any user-specified info, then seed it from the context | ||
// if request not set, pull from context; note: the requester can be set via the service catalog | ||
// propagating the user information via the openservicebroker origination api header on | ||
// calls to the TSB endpoints (i.e. the Provision call) | ||
if templateInstance.Spec.Requester == 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.
@liggitt @pmorie - if you all could, please re-review / take a quick peek at PrepareForCreate
and see if you agree. I tried to capture in the comment above what we think is different from "typical" PrepareForCreate
. Namely:
- the entry points like
Provision
into the TSB will provide the user.Info (as the service catalog propagates it in the orig identity header) and we set the user onto thetemplateInstance.Spec.Requester
there - so if it is set, we don't nil out
- if it is not set, we pull from the context
Ultimately, we test this behavior in various places, most notably our impersonation tests: https://github.com/openshift/origin/blob/master/test/extended/templates/templateinstance_impersonation.go#L288
I believe prior agreement was obtained on all this, but after some discussions with @jim-minter it seemed prudent to double-check one more time.
thanks
@bparees fyi
This means we can never give access to this API to a non-superuser, right? |
Ability to set the username to anything that isn't your username should be being gated in |
@gabemontero #16457 should enable the originating identity header |
thanks @spadgett / yep I had been monitoring that PR, as well as #16440 before it regardless, I was interested in trying it manually anyway :-) I just rebased, and pulled down the following service catalog image:
And manually updated the template ... here is the relevant snippet:
But the service catalog reports this error:
@staebler @jboyd01 @pmorie - any insight if I did something wrong? Or are there additional changes I need to wait on? thanks |
@gabemontero My understanding with the helm chart is that you need
|
@gabemontero I don't think that image has the changes in 0.0.21. You need to build a new service catalog image from #16457 |
Thanks for the quick resposnes @staebler and @spadgett And yep,
I'm going to stop here for now (move on to other to-do's) and wait for #16457 to progress vs. checking out #16457 and trying to build the service catalog image. |
@gabemontero I think the next step that you are missing is that the OriginatingIdentity feature needs to be enabled for service-catalog apiserver as well as controller-manager. Re-reading my previous comments, I see where it can be interpreted that it is only needed for controller-manager. To clarify my previous comment, the bug in the v0.0.20 release of service-catalog was that there was no way to enable the OriginatingIdentity feature in the controller-manager. It was possible to enable it in the apiserver. |
OK, I was able to do some basic provision / bind flows rebased on b3912c0 @bparees - if still inclined please re-add you good to merge tag from #16434 (comment) |
@gabemontero has a service catalog image that provides originating identity been published to dockerhub? we can't merge this until that has happened right? |
On Fri, Sep 22, 2017 at 6:15 PM Ben Parees ***@***.***> wrote:
@gabemontero <https://github.com/gabemontero> has a service catalog image
that provides originating identity been published to dockerhub?
Yes it has and that is what I tested with. I monitored the
push-origin-images job and pulled the image once the job with the
appropriate commit finished.
we can't merge this until that has happened right?
Correct but per above we are good there
… —
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#16434 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADbadApXYi17QM80418lFcJTMNB-g9Atks5slDFhgaJpZM4PcmQg>
.
|
/lgtm |
@crossonrose: changing LGTM is restricted to assignees, and only openshift org members may be assigned issues. In response to this:
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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bparees, crossonrose, gabemontero 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 |
Automatic merge from submit-queue |
@openshift/devex ptal
@pmorie @staebler fyi