Capture information about the user that created an instance or binding

[pmorie]

First part of #462

Essentially this adds a non-user-settable field in Instance and Binding that captures the userinfo from the authorizer. This is a pattern that has precedent in k8s in the certificate API: https://github.com/kubernetes/kubernetes/blob/master/pkg/registry/certificates/certificates/strategy.go#L57

[MHBauer]

Thanks for linking to precedent.

[pmorie]

Np, this is yet another thing that i would like to get written up

[pmorie]

Related: #934

[jwforres]

Curious how this maps when the "user" is a service account?

[duglin]

rebase needed but beyond that, is this supposed to be merged or it is a WIP? If so, can you add WIP to the title and some label to indicate its not ready?

[pmorie]

This is still WIP but basically representative of the scope I would like for this PR; labeling as in-progress.

[pmorie]

Here's an example of what the authenticator yields on a local-up cluster under system:admin:

$ k get instances -n test-ns ups-instance -o yaml
apiVersion: servicecatalog.k8s.io/v1alpha1
kind: Instance
metadata:
  creationTimestamp: 2017-08-09T02:34:54Z
  finalizers:
  - kubernetes-incubator/service-catalog
  name: ups-instance
  namespace: test-ns
  resourceVersion: "6"
  selfLink: /apis/servicecatalog.k8s.io/v1alpha1/namespaces/test-ns/instances/ups-instance
  uid: 53dddacd-7cab-11e7-b0f6-0242ac110005
spec:
  user:
    groups:
    - system:masters
    - system:authenticated
    uid: ""
    username: system:admin
  externalID: 239cec20-c123-49c4-9ffb-41e6472cf254
  parameters:
    credentials:
      name: root
      password: letmein
  planName: default
  serviceClassName: user-provided-service
status:
  asyncOpInProgress: false
  checksum: 349cd95f4d7e6d3ca360b1a8b5e2626763c230e52c6fd2e15b34be55864b381a
  conditions:
  - lastTransitionTime: 2017-08-09T02:34:54Z
    message: The instance was provisioned successfully
    reason: ProvisionedSuccessfully
    status: "True"
    type: Ready

$ k get bindings.v1alpha1.servicecatalog.k8s.io -n test-ns ups-binding -o yaml
apiVersion: servicecatalog.k8s.io/v1alpha1
kind: Binding
metadata:
  creationTimestamp: 2017-08-09T02:39:13Z
  finalizers:
  - kubernetes-incubator/service-catalog
  name: ups-binding
  namespace: test-ns
  resourceVersion: "9"
  selfLink: /apis/servicecatalog.k8s.io/v1alpha1/namespaces/test-ns/bindings/ups-binding
  uid: edf8ed73-7cab-11e7-b0f6-0242ac110005
spec:
  user:
    groups:
    - system:masters
    - system:authenticated
    uid: ""
    username: system:admin
  externalID: fada0ada-31bf-4c6a-bdca-ae613fe2f4b3
  instanceRef:
    name: ups-instance
  secretName: ups-binding
status:
  checksum: 304390737544622e98c1609494ebdaaa3828ba941c3cef6d56d5302f843593c8
  conditions:
  - lastTransitionTime: 2017-08-09T02:39:13Z
    message: Injected bind result
    reason: InjectedBindResult
    status: "True"
    type: Ready

[duglin]

Why do we need this info? Is it just for auditing purposes? Is there not some other auditing info kept (perhaps at the apiserver level) of who did what to which resource?

[duglin]

I disagree. Whether we're updating a plan or deleting a binding, they're both OSB actions. It would be a bit odd if we had totally different solutions for them. Besides, I think what I proposed above works for both.

[pmorie]

I disagree. Whether we're updating a plan or deleting a binding, they're both OSB actions. It would be a bit odd if we had totally different solutions for them. Besides, I think what I proposed above works for both.

Did we have a mismatch? I thought you were referring to the 'generic actions' proposal contained in openservicebrokerapi/servicebroker#114.

[duglin]

yea, I mis-typed when I put "updating a plan", I meant backing-up a service. Either way, we'll see how the talks go and we'll work it out...

[pmorie]

I think we should put this in as an alpha field with the semantics I suggested earlier. There are many reasons to make this available and no reason that I can think of not to, especially if we qualify the field as alpha and let users know that it's subject to change.

[arschles]

LGTM. I see no problem with this since all fields are marked alpha.

[duglin]

Let's add this to the design session today

[liggitt]

I would strongly recommend against naming the field with an alpha name. Doing so requires breaking all users of the field when the field progresses to stable status. See discussion in kubernetes/community#869

[liggitt]

If you want to position field as alpha, and require a feature gate to enable it in the server, that can work. Also, if the thing you are capturing is the creating user's info, I'd name the field to reflect that

[pmorie]

If @liggitt or someone else familiar with identity in k8s can be present, feel free to discuss otherwise I would prefer to be present when we discuss it.

@liggitt is the feature gate finctionality available on 1.7 machinery?

[duglin]

My proposal:

• rather than saving just the userInfo for the person who created the resource, we save the userInfo for of the current user as each action is taken on the resource that we know will result in an OSB API call.
• the resource will have something that maps each possible OSB API call to a UserInfo struct, that can be filled in as we detect changes (just a simple map[string]userInfo, where "string" == action, is sufficient). You can see a very rough start of this idea here: https://github.com/duglin/service-catalog/tree/pr939
• then the controller will use this info, if a value is available, to populate the OriginatingIdentity header on the OSB API call. If no data for that action is available then the header will be blank.
• as the OSB API action completes successfully, or if we determine that we will no longer retry that action, we will erase the userInfo data for that action.
• if someone else tries to perform the same action as someone else, e.g. two people tried to delete the same binding, then the 2nd+ actions will have no effect and the userInfo will not be updated. Note, this only works for cases where there is no data associated with the action because the first person who asked for this action should be the one who wins.
• if someone else tries to perform the same action as someone else and there is data associated with it (e.g. updating a Plan twice), if we accept the 2nd+ actions then the 2nd+ userInfo will be saved instead of the 1st userInfo. This assume only a single action is taken to the broker and the last request (and its data) wins.
• future: if we get to the point where we need to queue up actions then we'll need to create some mechanism to store not just a list of userInfos but also the data associated with each action. But that's for later.

[pmorie]

the resource will have something that maps each possible OSB API call to a UserInfo struct, that can be filled in as we detect changes (just a simple map[string]userInfo, where "string" == action, is sufficient).

See the guidance on maps in the API conventions.

[pmorie]

It sounds to me like we have consensus on the approach in this issue, based on the Tues, Aug 15th call.

[duglin]

re: maps - that's interesting and a bit unclear. I understand the recommendation of using properties when the "keys" are well-defined and known in advance. But then the text goes on to talk about the valid cases where maps are ok - like labels. So, does that mean that for cases where we don't know the keys in advance maps are ok?

[duglin]

On yesterday's call I was asked to provide more background on the need for the OriginatingIdentity header and how it might be used, here we go....

Cases we've mentioned before:

• brokers may need the info for auditing purposes to track who did what
• brokers may need talk to the platform to provision additional resources on the user's behalf

Some additional cases:

• we need the user performing the current action because the person who created the resource might not be valid any more (they might have left the company)
• due to limitations in platforms we've found the need to create "proxy brokers". These sit between the platform and the broker, and are actually the ones registered with the platform. In order for these proxies to do their job they need the info provided by the Header. These proxy brokers serve several purposes... (following bullets)
• access control. Some platforms do not provide the proper IAM (identity & access management) hooks to allow custom (e.g fine grained) checks - e.g. CF does not. We plan on providing OSB API support across Kube, CF, Docker and IaaS. Not all support pluggable IAM, and so having a single centralized point of enforcement ensures consistency.
• business logic enforcement. Neither platforms nor brokers know what business rules need to be applied. For example, while a user might be able to see a service they may not be allowed to provision it if they don't have the proper "credit" on their account? Or based on the type of account they may be restricted in what actions they can take. These are not codified into the platforms or brokers, so the proxies manage this.
• they allow for additional features that brokers may not support. For example, "undelete". We have the need to not actually delete the service when the user asks us to, rather we "fake it" so that they have a "grace period" during which they can choose to "undelete" it if they need to. Brokers are unaware of this going on.

The notion of having proxy brokers isn't new or unique to our needs. I believe we've even talked about how there may be aggregation brokers that sit between the platform and the "real" brokers that act as intermediaries - and so providing them with the user info may be critical for them to get their job done.

[pmorie]

Plane about to take off. I don't see anything in a quick scan that means we shouldn't merge the current approach. We can easily deprecate the field later and add a new set of fields if necessary to encompass more complex semantics that capture updates. In this case the current field what not be settable on new resources. Older resources would continue to work correctly, newer resources could use a new set of fields. Adding mutability adds a ton of complexity. The current approach will allow us to move the ball forward and get feedback from users. As I have said before, I would also appreciate the opportunity to be present for these design discussions.

…

On Wed, Aug 16, 2017 at 2:39 PM Doug Davis ***@***.***> wrote: On yesterday's call I was asked to provide more background on the need for the OriginatingIdentity header and how it might be used, here we go.... Cases we've mentioned before: - brokers may need the info for auditing purposes to track who did what - brokers may need talk to the platform to provision additional resources on the user's behalf Some additional cases: - we need the user performing the current action because the person who created the resource might not be valid any more (they might have left the company) - due to limitations in platforms we've found the need to create "proxy brokers". These sit between the platform and the broker, and are actually the ones registered with the platform. In order for these proxies to do their job they need the info provided by the Header. These proxy brokers serve several purposes... (following bullets) - access control. Some platforms do not provide the proper IAM (identity & access management) hooks to allow custom (e.g fine grained) checks - e.g. CF does not. We plan on providing OSB API support across Kube, CF, Docker and IaaS. Not all support pluggable IAM, and so having a single centralized point of enforcement ensures consistency. - business logic enforcement. Neither platforms nor brokers know what business rules need to be applied. For example, while a user might be able to see a service they may not be allowed to provision it if they don't have the proper "credit" on their account? Or based on the type of account they may be restricted in what actions they can take. These are not codified into the platforms or brokers, so the proxies manage this. - they allow for additional features that brokers may not support. For example, "undelete". We have the need to not actually delete the service when the user asks us to, rather we "fake it" so that they have a "grace period" during which they can choose to "undelete" it if they need to. Brokers are unaware of this going on. The notion of having proxy brokers isn't new or unique to our needs. I believe we've even talked about how there may be aggregation brokers that sit between the platform and the "real" brokers that act as intermediaries - and so providing them with the user info may be critical for them to get their job done. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#939 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAWXmKaE_axZwvhrhSfbuEO66BfCu-Fmks5sYzdwgaJpZM4N5Dvv> .

[nilebox]

@duglin thanks for sharing the use cases.
While I agree that finer grained per-action UserInfo is more powerful than what @pmorie proposes, I still see this as a non-kube way and overcomplicated feature.
As @vaikas-google mentioned at the meeting yesterday, to implement this correctly we'll need to queue all the changes to guarantee that we correctly handle every action.

I still see making creator a permanent owner of the resource as enough for majority of use cases, including the ones you listed. Even if other user updates the resource, I don't see anything wrong with impersonating other user who originally created resource. In the end the platform impersonates the user anyway, since there is no synchronous call in Service Catalog which is directly proxies to the broker.

Permissions for read/update/delete is IMO just an RBAC/namespaces story.

For the case when the creator of resource has left the company (and any other story where changing the owner is necessary) we can think of feature of "migrating" the resource to other owner user. It is much easier to cover than tracking every action. And this feature can be implemented as an extension to what is currently being proposed.

[duglin]

I think its important to first agree on what we're going to support before we decide on what code to merge. And based on today's, and yesterday's chat, I don't think we have agreement.

[pmorie]

I think its important to first agree on what we're going to support before we decide on what code to merge.

I don't think it's necessary to know all the details of what the long-term solution is here to be able to merge this code. It's also impossible to necessarily know the long-term shape of this, because this feature of the OSB API isn't finalized yet. However, there's a way to deal with this ambiguity without painting ourselves into a corner (explained fully below).

We need to be able to enable a simple version of this functionality in the short-term in order to gather feedback to be able to finalize the OSB feature. To be clear, I am in no way suggesting that we make a permanent commitment to the API described here. I am only interested in:

• A tactical way to enable users to begin using a simple version of this feature without blocking on determining all the complexities of making the information mutable or capturing information about updates
• Gathering information about cases where this mode breaks down, if it does, through applied usage

As I said here, there is a well-defined strategy to deal with the situation of having to add a new API construction in the future if we feel it is necessary:

• Deprecate the OriginatingUser field being added in this PR
• Add a new field that implements the behaviors we feel
• Add a validation to ensure that the OriginatingUser field is not set on new resources being created
• There will be two sets of resources, old resources where the old field is set, new resources where the new field is set
• The controller can implement the behaviors of the old field for resources that use it, and whatever new behaviors associated with the new fields we add later

...if we are able to figure out an alternative before the API goes GA (or even before it goes beta), we will not even have to preserve the old field in the next API version.

I think there is a large amount of analysis and design required for the flavor of this feature that captures information about updates. So, the ambiguity on any alternative to what is implemented here, and the ability to gracefully deprecate the field in the future, if necessary, means that we can merge this work with confidence and unblock people in the short term while leaving ourselves space to have a different solution in the long-term without committing to a data migration.

[duglin]

At a high level the current state of this PR does this:

• adds a UserInfo property to Instance and Binding to capture who created it

New Proposal:

• add a UserInfo property to Instance and Binding to capture who was the last person to update it, which means creating, updating and deleting of these 2 resources sets the UserInfo property.

While this doesn't address all of the usecases/issues around capturing the user info, it covers enough for now. Adopting this proposal also means the following:

• concurrent updates to Instances will not act correctly w.r.t. the user info passed in the OriginatingIdentity Header
• we will revisit this situation again before beta to address this concurrency problem

[staebler]

What is the expected behavior when the update of a ServiceInstance or ServiceInstanceCredential does not modify the user-accessible portion of a spec? Should we only change the UserInfo property when the user-accessible portion of a spec changes on an update? If we change the UserInfo property when there is no change to the user-accessible portion of the spec, then we will end up sending an update to the broker that does not make any changes to the broker resource. That does not seem appropriate to me.

Note that there are not currently any fields in the spec for either ServiceInstance or ServiceInstanceCredential that the user can update. So, if we are not changing the UserInfo field for updates unless there is a modification to a user-accessible portion of the spec, then the UserInfo field will not be changed for any updates.

[nilebox]

Note that there are not currently any fields in the spec for either ServiceInstance or ServiceInstanceCredential that the user can update

@staebler parameters and parametersFrom in both resources' spec?

[staebler]

@nilebox How does that work when the update strategy for the resource explicitly discards all changes made to the spec?

https://github.com/kubernetes-incubator/service-catalog/blob/f870baf6dca4eb37b38b00de04c927ec55eab775/pkg/registry/servicecatalog/instance/strategy.go#L125-L129

[nilebox]

How does that work when the update strategy for the resource explicitly discards all changes made to the spec?

@staebler I was unaware of this restriction in our code. OSB spec definitely allows you to update the instance parameters, see https://github.com/openservicebrokerapi/servicebroker/blob/master/spec.md#updating-a-service-instance, there is parameters block as part of the request body.

So I see this as a temporary short-term hack, and I don't think making any assumptions based on it are valid.

[duglin]

Per the f2f on Monday, we have this decision:

Consensus: we are ok with adding UserInfo to Instance and Binding, and using it to represent the user who did the last operation on the resource. Its value will be used in the OriginatingIdentity header field

[duglin]

@staebler What kind of action on the Instance/Binding would cause use to send a request to the broker w/o the user changing anything in those resources? Are you thinking about that "poke" type of action where the user touched a secret that's referenced by the parametersFrom property?

In general, we only need to update the UserInfo property when there will be a resulting OSB API request. So, if there are other updates to the Spec that don't result in an OSB API request then we don't need to set the UserInfo.

[pmorie]

Superceded by #1162