Add HTTP PATCH support for key metadata #57
Conversation
TypeKVPairs results in using a map[string]string
whereas TypeMap results in using a map[string]interface{}.
Being able to accept null values for custom_metadata
fields is important for HTTP PATCH operations as it
signals to the handler to remove the field. A shared
parser has been added to ensure that the provided
non-nil values are indeed parsable as strings.
ccapurso
requested review from
calvn and
a team
and removed request for
calvn
go.mod
Outdated
| @@ -10,7 +10,7 @@ require ( | |||
| github.com/hashicorp/go-secure-stdlib/parseutil v0.1.2 | |||
| github.com/hashicorp/go-secure-stdlib/strutil v0.1.1 | |||
| github.com/hashicorp/vault/api v1.3.0 | |||
| github.com/hashicorp/vault/sdk v0.3.0 | |||
| github.com/hashicorp/vault/sdk v0.3.1-0.20211124134129-09816c37b82e | |||
There was a problem hiding this comment.
Could you link the relevant sdk update that is required for this PR? I noticed that this PR uses HandlePatchOperation , which is updated here: https://github.com/hashicorp/vault/pull/13191/files . Was this the change you wanted to include?
There was a problem hiding this comment.
Ah, yes. I did link the PR above but I will add a note in the description to specify why the sdk version has been updated. Thank you for calling that out!
There was a problem hiding this comment.
Awesome! In that case, I think we'd want to merge the sdk PR first, then redo the go get so we don't depend on a branch.
There was a problem hiding this comment.
Yup, that's the plan.
| // framework.PatchPreprocessorFunc handles filtering out Vault-managed fields, | ||
| // filtering out nulls, and ensuring appropriate handling of data types not | ||
| // supported directly by FieldType. | ||
| func metadataPatchPreprocessor() framework.PatchPreprocessorFunc { |
There was a problem hiding this comment.
Why do we filter out nils here? Further down in PathMetadataPatch, the parsing logic should filter out nils already. Not a problem regardless as I think having the extra lair of defensive coding is a good thing, but I was just curious if I was missing something here.
There was a problem hiding this comment.
That's a good point. We have discussed this further offline and come to the conclusion that nils provided for top-level fields will be treated as unsetting the field. The godoc for framework.HandlePatchOperation will account for this functionality.
Overview
The
custom_metadatamap field was added to key metadata in #48. Updating the field currently results in a full overwrite. HTTPPATCHwas introduced for the data endpoint in #49. Now that Vault generally supports HTTPPATCH, it seemed prudent to add aPatchOperationhandler for the metadata endpoint so thatcustom_metadatacan be patched rather than fully overwritten.Design of Change
The
custom_metadatatype has been changed fromframework.TypeKVPairstoframework.TypeMap. Theframework.TypeKVPairsrestricts values to strings but does not result in proper handling when users to submitnullvalues within the JSON request body. Anullwill indicate to the JSON merge handler to remove the field. Usingframework.TypeKVPairswill result in coercing anilinterface{}from the request data to an empty string which is not ideal. Changing the type of this field was a concern that was handled carefully. TheparseCustomMetadatafunction was added to handle the parsing of theinterface{}values into strings to create amap[string]stringin the same way that theFieldDataparsing would if it were otherwise typed asframework.TypeKVPairs. This function is then used for theCreateOperation/UpdateOperationandPatchOperationhandlers to ensure consistent request data coercion.A
PatchOperationhandler has been added to the metadata path. Similar to theCreateOperation/UpdateOperationhandler, thecas_requiredfield is treated as lower priority than the backend configuration. If thecas_requiredfield on the backend configuration is set totrueand an attempt made to set the metadata'scas_requiredtofalsewill result in a warning.The
framework.PatchPreprocessorFuncfor the metadataPatchOperationhandler is responsible for the following:max_versions,cas_required,delete_version_after,custom_metadataas all other fields are handled internally by the backendnilvalues as they will result in the JSON merge patch removing them from the resource, they are allowed to exist withincustom_metadataDurationProtovalue fordelete_version_afterRelated Issues/Pull Requests
HandlePatchOperationwhich have been included in the update ofgithub.com/hashicorp/vault/sdkto versionv0.3.1-0.20211124134129-09816c37b82ekv metadata patchCLI support and doc updates