[docs] allow patching of roles

[f4z3r]

Currently it is not possible to modify roles once created.

We are encountering a use-case where we need to modify the policies attached to the tokens created by a role. A workaround would be to read the role, delete it, and re-create a modified version of it. This is not very nice.

Ideally we would want to be able to send a patch request to modify some parameters from the role configuration, in order to avoid the deletion of the role.

It should be noted that this would not modify the policies of the currently outstanding tokens that were created from that role before it got patched.

FYI: @zkck

[f4z3r]

Nevermind, we looked at the code and a patch seems to be actually supported. This is mostly a documentation issue then I guess.

[maxcoulombe]

Hey, thanks for bringing this to our attention. I agree with you it should be more obvious that the create command or request can also be used to update an existing role.

Would it be enough to simply change Create Role to Create or Update Role in the API doc page for the K8s auth plugin? I see from other plugin documentation pages like Kerberos that this is usually how it's done.

When you were looking through the existing docs are there other places where you would have expected this behaviour to be explained that we can update as well?

[f4z3r]

Hey, seems enough to me. I mostly use the API reference, I have not looked through documentation or tutorials in details. But flying over them, it seems there are no changes required there.

[maxcoulombe]

Should help clarify the behaviour so people do not have to waste time going through the source code in the future:
hashicorp/vault#17371

[cipherboy]

@maxcoulombe One word of caution -- Create/Update role is not a PATCH operation, but a POST -- this means all modified values need to be re-specified on the subsequent update. To my knowledge, outside of KVv2 and PKI's roles, no other auth/secrets engines have yet adopted PATCH support for resources, and the CLI lacks generalized PATCH support right now.

This is why we have the warning in the role about POST: https://www.vaultproject.io/api-docs/secret/pki#create-update-role

[f4z3r]

Ah true, this means that the implementation conflicts with Restful definitions then. Because the implementation replaces the provided fields, but as far as I remember it looked like it kept the previously configured values for all fields that were not provided. If it would behave properly like a POST, I guess it would need to complain about mandatory fields being omitted, or update them to the default value if the field has one.

[cipherboy]

@f4z3r Correct, when fields are omitted on the POST, they are treated as if the defaults had been specified again. This confused a lot of people, hence why PKI has gone and started adding proper PATCH support.

(E.g., you could set allow_localhost=false and update the role via a say, vault write pki/roles/existing allowed_domains="example.com" and Vault would reset it to the default allow_localhost=true).

[f4z3r]

So what is your suggestion? The callback under which the current "update" is registered is logical.UpdateOperation, which is a POST I would guess. Preventing an update via POST and registering the function under a PATCH would not be backwards compatible though. What is your approach regarding such changes, considering there was no documentation about the update capabilities of the endpoint?

I could find some time in the coming days to fix the implementation to properly behave like a POST, and do something inline with the PKI secret engine to also have a PATCH with the current implementation if you guys want. Just let me know if such a change would be fine in terms of compatibility in your eyes.

[cipherboy]

@f4z3r I'd recommend adding a logical.PatchOperation handler, leaving the existing logical.UpdateOperation alone and suggest people use logical.PatchOperation when possible to update roles in your roles. And on new ones, support both from the start. Note that with the framework.FieldSchema stuff, any omitted parameters will take their defaults, so the expected handling will have the desired POST behavior (RESTful).

There'll be some subset of users without Patch capable clients who will still have to rely on updates via the existing UpdateOperation handler. Might be using Vault CLI, Vault UI, &c. But for users who do have patch support (and when Vault CLI gets it, which I don't know when that'll be), they'll be able to do the safer version.

Also, it is handy to have an Update that nukes everything for when you do want to start fresh with the role...

You can see e.g., what builtin/logical/pki/path_roles.go does to handle this in Vault itself.

My 2c. tho, I'd be curious to see what others have to say :-)

[f4z3r]

I see your points. But when you say:

Also, it is handy to have an Update that nukes everything for when you do want to start fresh with the role.

We would not have that though if leaving the existing logical.UpdateOperation though right? Since the role takes existing field values and updates them when set. I get that those might be overwritten in case there are defaults due to the framework.FieldSchema stuff, but fields such as audience can be left unset. And in the code we have:

if audience, ok := data.GetOk("audience"); ok {
	role.Audience = audience.(string)
}

so a previously set audience would be kept if the field is omitted in the subsequent POST. So this is not a desired POST behaviour. Or did I misunderstand your comment?

[cipherboy]

I think you'd do what path_roles.go does and use role.Audience = data.Get("audience").(string) instead, unconditionally. You won't get a nil return from data.Get (it sets a zero value that you can safely cast) and if you define a Default on the FieldSchema and the user elides the value, you'll get the default value returned instead. See sdk/framework/backend.go, especially DefaultOrZero which is called by FieldData.Get(...).

This gives you the create and update semantics, without the pseudo-patch-like behavior you're attempting presently, and aligns you with the Hashicorp-provided plugin base as well, IMO.

[f4z3r]

Ah ok, then I misunderstood your previous comment sorry. I understood it as leaving the implementation of the callback for logical.UpdateOperation as is.

[cipherboy]

Ahhh sorry. I hadn't seen your implementation, so I assumed you were just calling data.Get rather than GetOk as that's what most of our plugins do.

This makes Update behave like a proper POST I believe, with the risk of unintended side-effects. But that's why there's now a separate PatchOperation handler that's more surgical in nature. --- Obviously in the Patch handler, you'd do this GetOk type construct and leave the existing value alone if it isn't provided in the request.

HTH :-)

[cipherboy]

@f4z3r Now I realized I'm the dense one -- I had seen this from @maxcoulombe's docs PR on the main Vault repo and had clicked through without realizing where this discussion came from (on one of our plugin repos -- I thought this was about the problem in general on the Vault repo about how to write plugins in general). :D

I'd be curious as to why we opted for GetOk for these optional fields in this plugin, the default empty string should still be correct especially as they're strings and not pointers where we care about the semantic differences between "" and nil... But I think that'd be for Max to discuss.