-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
added OpenAPI response objects for sys endpoints #18633
added OpenAPI response objects for sys endpoints #18633
Conversation
vault/logical_raw.go
Outdated
Callback: r.handleRawWrite, | ||
Summary: "Update the value of the key at the given path.", | ||
Responses: map[int][]framework.Response{ | ||
http.StatusOK: {{ | ||
Description: "OK", | ||
}}, | ||
}, | ||
Summary: "Update the value of the key at the given path.", | ||
}, | ||
logical.CreateOperation: &framework.PathOperation{ | ||
Callback: r.handleRawWrite, | ||
Summary: "Create a key with value at the given path.", | ||
Responses: map[int][]framework.Response{ | ||
http.StatusOK: {{ | ||
Description: "OK", | ||
}}, | ||
}, | ||
Summary: "Create a key with value at the given path.", |
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.
looks like it returns 204 no content on success
Line 225 in 4b8bd15
return nil, nil |
vault/logical_system_paths.go
Outdated
@@ -371,6 +372,7 @@ func (b *SystemBackend) rekeyPaths() []*framework.Path { | |||
}, | |||
}, | |||
|
|||
// TODO not sure what to do for this as there are no callbacks |
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.
think it's here
https://github.com/hashicorp/vault/blob/4b8bd15e28ad3194159e7601fe76a994688988d4/http/sys_rekey.go
(@AnPucel helped me last time one was missing iirc)
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.
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
"t": { | ||
Type: framework.TypeInt, | ||
}, | ||
"n": { | ||
Type: framework.TypeInt, | ||
}, |
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.
I'm not sure where these fields are defined, but should some of them be marked as required?
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.
So actually no only the three I have, this endpoint is fairly jank as it can return two totally different structures. more reading here:
Lines 198 to 220 in b741fa8
// Format the response | |
resp := &RekeyUpdateResponse{} | |
if result != nil { | |
resp.Complete = true | |
resp.Nonce = req.Nonce | |
resp.Backup = result.Backup | |
resp.PGPFingerprints = result.PGPFingerprints | |
resp.VerificationRequired = result.VerificationRequired | |
resp.VerificationNonce = result.VerificationNonce | |
// Encode the keys | |
keys := make([]string, 0, len(result.SecretShares)) | |
keysB64 := make([]string, 0, len(result.SecretShares)) | |
for _, k := range result.SecretShares { | |
keys = append(keys, hex.EncodeToString(k)) | |
keysB64 = append(keysB64, base64.StdEncoding.EncodeToString(k)) | |
} | |
resp.Keys = keys | |
resp.KeysB64 = keysB64 | |
respondOk(w, resp) | |
} else { | |
handleSysRekeyInitGet(ctx, core, recovery, w, r) | |
} |
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.
If the API is unstable (can return two different structures), it might be better to just not specify the fields at all. Not really sure, what do you think?
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.
I think it's more helpful to return all possible fields than nothing at all.
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.
so responses is map[int][]framework.Response
why not have
http.StatusOK: {
{
Description: "returned if ...",
Fields: ...
},
{
Description: "returned if ...",
Fields: ...
}
}
Co-authored-by: Anton Averchenkov <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
Co-authored-by: Daniel Huckins <[email protected]>
This contains a partial list of the sys endpoints that are laid out in VAULT-12144.