api: fix a panic and tweak some exported types (#16237)

This PR
 - fixes a panic in GetItems when looking up a variable that does not exist.
 - deprecates GetItems in favor of GetVariableItems which avoids returning a pointer to a map
 - deprecates ErrVariableNotFound in favor of ErrVariablePathNotFound which is an actual error type
 - does some minor code cleanup to make linters happier
This commit is contained in:
Seth Hoenig 2023-02-22 08:17:22 -06:00 committed by GitHub
parent d9587b323a
commit c9ffd1274b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
3 changed files with 198 additions and 108 deletions

11
.changelog/16237.txt Normal file
View File

@ -0,0 +1,11 @@
```release-note:bug
api: Fixed a bug where Variables.GetItems would panic if variable did not exist
```
```release-note:deprecation
api: Deprecated Variables.GetItems in favor of Variables.GetVariableItems to avoid returning a pointer to a map
```
```release-note:deprecation
api: Deprecated ErrVariableNotFound in favor of ErrVariablePathNotFound to correctly represent an error type
```

View File

@ -12,8 +12,16 @@ import (
)
const (
// ErrVariableNotFound was used as the content of an error string.
//
// Deprecated: use ErrVariablePathNotFound instead.
ErrVariableNotFound = "variable not found"
ErrVariableMissingItems = "variable missing Items field"
)
var (
// ErrVariablePathNotFound is returned when trying to read a variable that
// does not exist.
ErrVariablePathNotFound = errors.New("variable not found")
)
// Variables is used to access variables.
@ -27,11 +35,10 @@ func (c *Client) Variables() *Variables {
}
// Create is used to create a variable.
func (sv *Variables) Create(v *Variable, qo *WriteOptions) (*Variable, *WriteMeta, error) {
func (vars *Variables) Create(v *Variable, qo *WriteOptions) (*Variable, *WriteMeta, error) {
v.Path = cleanPathString(v.Path)
var out Variable
wm, err := sv.client.put("/v1/var/"+v.Path, v, &out, qo)
wm, err := vars.client.put("/v1/var/"+v.Path, v, &out, qo)
if err != nil {
return nil, wm, err
}
@ -41,54 +48,49 @@ func (sv *Variables) Create(v *Variable, qo *WriteOptions) (*Variable, *WriteMet
// CheckedCreate is used to create a variable if it doesn't exist
// already. If it does, it will return a ErrCASConflict that can be unwrapped
// for more details.
func (sv *Variables) CheckedCreate(v *Variable, qo *WriteOptions) (*Variable, *WriteMeta, error) {
func (vars *Variables) CheckedCreate(v *Variable, qo *WriteOptions) (*Variable, *WriteMeta, error) {
v.Path = cleanPathString(v.Path)
var out Variable
wm, err := sv.writeChecked("/v1/var/"+v.Path+"?cas=0", v, &out, qo)
wm, err := vars.writeChecked("/v1/var/"+v.Path+"?cas=0", v, &out, qo)
if err != nil {
return nil, wm, err
}
return &out, wm, nil
}
// Read is used to query a single variable by path. This will error
// if the variable is not found.
func (sv *Variables) Read(path string, qo *QueryOptions) (*Variable, *QueryMeta, error) {
func (vars *Variables) Read(path string, qo *QueryOptions) (*Variable, *QueryMeta, error) {
path = cleanPathString(path)
var svar = new(Variable)
qm, err := sv.readInternal("/v1/var/"+path, &svar, qo)
var v = new(Variable)
qm, err := vars.readInternal("/v1/var/"+path, &v, qo)
if err != nil {
return nil, nil, err
}
if svar == nil {
return nil, qm, errors.New(ErrVariableNotFound)
if v == nil {
return nil, qm, ErrVariablePathNotFound
}
return svar, qm, nil
return v, qm, nil
}
// Peek is used to query a single variable by path, but does not error
// when the variable is not found
func (sv *Variables) Peek(path string, qo *QueryOptions) (*Variable, *QueryMeta, error) {
func (vars *Variables) Peek(path string, qo *QueryOptions) (*Variable, *QueryMeta, error) {
path = cleanPathString(path)
var svar = new(Variable)
qm, err := sv.readInternal("/v1/var/"+path, &svar, qo)
var v = new(Variable)
qm, err := vars.readInternal("/v1/var/"+path, &v, qo)
if err != nil {
return nil, nil, err
}
return svar, qm, nil
return v, qm, nil
}
// Update is used to update a variable.
func (sv *Variables) Update(v *Variable, qo *WriteOptions) (*Variable, *WriteMeta, error) {
func (vars *Variables) Update(v *Variable, qo *WriteOptions) (*Variable, *WriteMeta, error) {
v.Path = cleanPathString(v.Path)
var out Variable
wm, err := sv.client.put("/v1/var/"+v.Path, v, &out, qo)
wm, err := vars.client.put("/v1/var/"+v.Path, v, &out, qo)
if err != nil {
return nil, wm, err
}
@ -98,23 +100,21 @@ func (sv *Variables) Update(v *Variable, qo *WriteOptions) (*Variable, *WriteMet
// CheckedUpdate is used to updated a variable if the modify index
// matches the one on the server. If it does not, it will return an
// ErrCASConflict that can be unwrapped for more details.
func (sv *Variables) CheckedUpdate(v *Variable, qo *WriteOptions) (*Variable, *WriteMeta, error) {
func (vars *Variables) CheckedUpdate(v *Variable, qo *WriteOptions) (*Variable, *WriteMeta, error) {
v.Path = cleanPathString(v.Path)
var out Variable
wm, err := sv.writeChecked("/v1/var/"+v.Path+"?cas="+fmt.Sprint(v.ModifyIndex), v, &out, qo)
wm, err := vars.writeChecked("/v1/var/"+v.Path+"?cas="+fmt.Sprint(v.ModifyIndex), v, &out, qo)
if err != nil {
return nil, wm, err
}
return &out, wm, nil
}
// Delete is used to delete a variable
func (sv *Variables) Delete(path string, qo *WriteOptions) (*WriteMeta, error) {
func (vars *Variables) Delete(path string, qo *WriteOptions) (*WriteMeta, error) {
path = cleanPathString(path)
wm, err := sv.deleteInternal(path, qo)
wm, err := vars.deleteInternal(path, qo)
if err != nil {
return nil, err
}
@ -124,23 +124,20 @@ func (sv *Variables) Delete(path string, qo *WriteOptions) (*WriteMeta, error) {
// CheckedDelete is used to conditionally delete a variable. If the
// existing variable does not match the provided checkIndex, it will return an
// ErrCASConflict that can be unwrapped for more details.
func (sv *Variables) CheckedDelete(path string, checkIndex uint64, qo *WriteOptions) (*WriteMeta, error) {
func (vars *Variables) CheckedDelete(path string, checkIndex uint64, qo *WriteOptions) (*WriteMeta, error) {
path = cleanPathString(path)
wm, err := sv.deleteChecked(path, checkIndex, qo)
wm, err := vars.deleteChecked(path, checkIndex, qo)
if err != nil {
return nil, err
}
return wm, nil
}
// List is used to dump all of the variables, can be used to pass prefix
// via QueryOptions rather than as a parameter
func (sv *Variables) List(qo *QueryOptions) ([]*VariableMetadata, *QueryMeta, error) {
func (vars *Variables) List(qo *QueryOptions) ([]*VariableMetadata, *QueryMeta, error) {
var resp []*VariableMetadata
qm, err := sv.client.query("/v1/vars", &resp, qo)
qm, err := vars.client.query("/v1/vars", &resp, qo)
if err != nil {
return nil, nil, err
}
@ -148,63 +145,77 @@ func (sv *Variables) List(qo *QueryOptions) ([]*VariableMetadata, *QueryMeta, er
}
// PrefixList is used to do a prefix List search over variables.
func (sv *Variables) PrefixList(prefix string, qo *QueryOptions) ([]*VariableMetadata, *QueryMeta, error) {
func (vars *Variables) PrefixList(prefix string, qo *QueryOptions) ([]*VariableMetadata, *QueryMeta, error) {
if qo == nil {
qo = &QueryOptions{Prefix: prefix}
} else {
qo.Prefix = prefix
}
return sv.List(qo)
return vars.List(qo)
}
// GetItems returns the inner Items collection from a variable at a
// given path
func (sv *Variables) GetItems(path string, qo *QueryOptions) (*VariableItems, *QueryMeta, error) {
// GetItems returns the inner Items collection from a variable at a given path.
//
// Deprecated: Use GetVariableItems instead.
func (vars *Variables) GetItems(path string, qo *QueryOptions) (*VariableItems, *QueryMeta, error) {
vi, qm, err := vars.GetVariableItems(path, qo)
if err != nil {
return nil, nil, err
}
return &vi, qm, nil
}
// GetVariableItems returns the inner Items collection from a variable at a given path.
func (vars *Variables) GetVariableItems(path string, qo *QueryOptions) (VariableItems, *QueryMeta, error) {
path = cleanPathString(path)
svar := new(Variable)
v := new(Variable)
qm, err := sv.readInternal("/v1/var/"+path, &svar, qo)
qm, err := vars.readInternal("/v1/var/"+path, &v, qo)
if err != nil {
return nil, nil, err
}
return &svar.Items, qm, nil
// note: readInternal will in fact turn our v into a nil if not found
if v == nil {
return nil, nil, ErrVariablePathNotFound
}
return v.Items, qm, nil
}
// readInternal exists because the API's higher-level read method requires
// the status code to be 200 (OK). For Peek(), we do not consider 403 (Permission
// Denied or 404 (Not Found) an error, this function just returns a nil in those
// cases.
func (sv *Variables) readInternal(endpoint string, out **Variable, q *QueryOptions) (*QueryMeta, error) {
func (vars *Variables) readInternal(endpoint string, out **Variable, q *QueryOptions) (*QueryMeta, error) {
// todo(shoenig): seems like this could just return a *Variable instead of taking
// in a **Variable and modifying it?
r, err := sv.client.newRequest("GET", endpoint)
r, err := vars.client.newRequest("GET", endpoint)
if err != nil {
return nil, err
}
r.setQueryOptions(q)
checkFn := requireStatusIn(http.StatusOK, http.StatusNotFound, http.StatusForbidden)
rtt, resp, err := checkFn(sv.client.doRequest(r))
rtt, resp, err := checkFn(vars.client.doRequest(r))
if err != nil {
return nil, err
}
qm := &QueryMeta{}
parseQueryMeta(resp, qm)
_ = parseQueryMeta(resp, qm)
qm.RequestTime = rtt
if resp.StatusCode == http.StatusNotFound {
*out = nil
resp.Body.Close()
_ = resp.Body.Close()
return qm, nil
}
if resp.StatusCode == http.StatusForbidden {
*out = nil
resp.Body.Close()
_ = resp.Body.Close()
// On a 403, there is no QueryMeta to parse, but consul-template--the
// main consumer of the Peek() func that calls this method needs the
// value to be non-zero; so set them to a reasonable but artificial
@ -215,8 +226,10 @@ func (sv *Variables) readInternal(endpoint string, out **Variable, q *QueryOptio
return qm, nil
}
defer resp.Body.Close()
if err := decodeBody(resp, out); err != nil {
defer func() {
_ = resp.Body.Close()
}()
if err = decodeBody(resp, out); err != nil {
return nil, err
}
@ -226,51 +239,49 @@ func (sv *Variables) readInternal(endpoint string, out **Variable, q *QueryOptio
// deleteInternal exists because the API's higher-level delete method requires
// the status code to be 200 (OK). The SV HTTP API returns a 204 (No Content)
// on success.
func (sv *Variables) deleteInternal(path string, q *WriteOptions) (*WriteMeta, error) {
r, err := sv.client.newRequest("DELETE", fmt.Sprintf("/v1/var/%s", path))
func (vars *Variables) deleteInternal(path string, q *WriteOptions) (*WriteMeta, error) {
r, err := vars.client.newRequest("DELETE", fmt.Sprintf("/v1/var/%s", path))
if err != nil {
return nil, err
}
r.setWriteOptions(q)
checkFn := requireStatusIn(http.StatusOK, http.StatusNoContent)
rtt, resp, err := checkFn(sv.client.doRequest(r))
rtt, resp, err := checkFn(vars.client.doRequest(r))
if err != nil {
return nil, err
}
wm := &WriteMeta{RequestTime: rtt}
parseWriteMeta(resp, wm)
_ = parseWriteMeta(resp, wm)
return wm, nil
}
// deleteChecked exists because the API's higher-level delete method requires
// the status code to be OK. The SV HTTP API returns a 204 (No Content) on
// success and a 409 (Conflict) on a CAS error.
func (sv *Variables) deleteChecked(path string, checkIndex uint64, q *WriteOptions) (*WriteMeta, error) {
r, err := sv.client.newRequest("DELETE", fmt.Sprintf("/v1/var/%s?cas=%v", path, checkIndex))
func (vars *Variables) deleteChecked(path string, checkIndex uint64, q *WriteOptions) (*WriteMeta, error) {
r, err := vars.client.newRequest("DELETE", fmt.Sprintf("/v1/var/%s?cas=%v", path, checkIndex))
if err != nil {
return nil, err
}
r.setWriteOptions(q)
checkFn := requireStatusIn(http.StatusOK, http.StatusNoContent, http.StatusConflict)
rtt, resp, err := checkFn(sv.client.doRequest(r))
rtt, resp, err := checkFn(vars.client.doRequest(r))
if err != nil {
return nil, err
}
wm := &WriteMeta{RequestTime: rtt}
parseWriteMeta(resp, wm)
_ = parseWriteMeta(resp, wm)
// The only reason we should decode the response body is if
// it is a conflict response. Otherwise, there won't be one.
if resp.StatusCode == http.StatusConflict {
conflict := new(Variable)
if err := decodeBody(resp, &conflict); err != nil {
if err = decodeBody(resp, &conflict); err != nil {
return nil, err
}
return nil, ErrCASConflict{
@ -284,9 +295,8 @@ func (sv *Variables) deleteChecked(path string, checkIndex uint64, q *WriteOptio
// writeChecked exists because the API's higher-level write method requires
// the status code to be OK. The SV HTTP API returns a 200 (OK) on
// success and a 409 (Conflict) on a CAS error.
func (sv *Variables) writeChecked(endpoint string, in *Variable, out *Variable, q *WriteOptions) (*WriteMeta, error) {
r, err := sv.client.newRequest("PUT", endpoint)
func (vars *Variables) writeChecked(endpoint string, in *Variable, out *Variable, q *WriteOptions) (*WriteMeta, error) {
r, err := vars.client.newRequest("PUT", endpoint)
if err != nil {
return nil, err
}
@ -294,20 +304,22 @@ func (sv *Variables) writeChecked(endpoint string, in *Variable, out *Variable,
r.obj = in
checkFn := requireStatusIn(http.StatusOK, http.StatusNoContent, http.StatusConflict)
rtt, resp, err := checkFn(sv.client.doRequest(r))
rtt, resp, err := checkFn(vars.client.doRequest(r))
if err != nil {
return nil, err
}
defer resp.Body.Close()
defer func() {
_ = resp.Body.Close()
}()
wm := &WriteMeta{RequestTime: rtt}
parseWriteMeta(resp, wm)
_ = parseWriteMeta(resp, wm)
if resp.StatusCode == http.StatusConflict {
conflict := new(Variable)
if err := decodeBody(resp, &conflict); err != nil {
if err = decodeBody(resp, &conflict); err != nil {
return nil, err
}
return nil, ErrCASConflict{
@ -316,7 +328,7 @@ func (sv *Variables) writeChecked(endpoint string, in *Variable, out *Variable,
}
}
if out != nil {
if err := decodeBody(resp, &out); err != nil {
if err = decodeBody(resp, &out); err != nil {
return nil, err
}
}
@ -328,17 +340,23 @@ func (sv *Variables) writeChecked(endpoint string, in *Variable, out *Variable,
type Variable struct {
// Namespace is the Nomad namespace associated with the variable
Namespace string `hcl:"namespace"`
// Path is the path to the variable
Path string `hcl:"path"`
// Raft indexes to track creation and modification
// CreateIndex tracks the index of creation time
CreateIndex uint64 `hcl:"create_index"`
// ModifyTime is the unix nano of the last modified time
ModifyIndex uint64 `hcl:"modify_index"`
// Times provided as a convenience for operators expressed time.UnixNanos
// CreateTime is the unix nano of the creation time
CreateTime int64 `hcl:"create_time"`
// ModifyTime is the unix nano of the last modified time
ModifyTime int64 `hcl:"modify_time"`
// Items contains the k/v variable component
Items VariableItems `hcl:"items"`
}
@ -347,24 +365,29 @@ type Variable struct {
type VariableMetadata struct {
// Namespace is the Nomad namespace associated with the variable
Namespace string `hcl:"namespace"`
// Path is the path to the variable
Path string `hcl:"path"`
// Raft indexes to track creation and modification
// CreateIndex tracks the index of creation time
CreateIndex uint64 `hcl:"create_index"`
// ModifyTime is the unix nano of the last modified time
ModifyIndex uint64 `hcl:"modify_index"`
// Times provided as a convenience for operators expressed time.UnixNanos
// CreateTime is the unix nano of the creation time
CreateTime int64 `hcl:"create_time"`
// ModifyTime is the unix nano of the last modified time
ModifyTime int64 `hcl:"modify_time"`
}
// VariableItems are the key/value pairs of a Variable.
type VariableItems map[string]string
// NewVariable is a convenience method to more easily create a
// ready-to-use variable
func NewVariable(path string) *Variable {
return &Variable{
Path: path,
Items: make(VariableItems),
@ -372,12 +395,11 @@ func NewVariable(path string) *Variable {
}
// Copy returns a new deep copy of this Variable
func (sv1 *Variable) Copy() *Variable {
var out Variable = *sv1
func (v *Variable) Copy() *Variable {
var out = *v
out.Items = make(VariableItems)
for k, v := range sv1.Items {
out.Items[k] = v
for key, value := range v.Items {
out.Items[key] = value
}
return &out
}
@ -385,22 +407,21 @@ func (sv1 *Variable) Copy() *Variable {
// Metadata returns the VariableMetadata component of
// a Variable. This can be useful for comparing against
// a List result.
func (sv *Variable) Metadata() *VariableMetadata {
func (v *Variable) Metadata() *VariableMetadata {
return &VariableMetadata{
Namespace: sv.Namespace,
Path: sv.Path,
CreateIndex: sv.CreateIndex,
ModifyIndex: sv.ModifyIndex,
CreateTime: sv.CreateTime,
ModifyTime: sv.ModifyTime,
Namespace: v.Namespace,
Path: v.Path,
CreateIndex: v.CreateIndex,
ModifyIndex: v.ModifyIndex,
CreateTime: v.CreateTime,
ModifyTime: v.ModifyTime,
}
}
// IsZeroValue can be used to test if a Variable has been changed
// from the default values it gets at creation
func (sv *Variable) IsZeroValue() bool {
return *sv.Metadata() == VariableMetadata{} && sv.Items == nil
func (v *Variable) IsZeroValue() bool {
return *v.Metadata() == VariableMetadata{} && v.Items == nil
}
// cleanPathString removes leading and trailing slashes since they
@ -411,17 +432,17 @@ func cleanPathString(path string) string {
}
// AsJSON returns the Variable as a JSON-formatted string
func (sv Variable) AsJSON() string {
func (v *Variable) AsJSON() string {
var b []byte
b, _ = json.Marshal(sv)
b, _ = json.Marshal(v)
return string(b)
}
// AsPrettyJSON returns the Variable as a JSON-formatted string with
// indentation
func (sv Variable) AsPrettyJSON() string {
func (v *Variable) AsPrettyJSON() string {
var b []byte
b, _ = json.MarshalIndent(sv, "", " ")
b, _ = json.MarshalIndent(v, "", " ")
return string(b)
}
@ -442,10 +463,9 @@ type doRequestWrapper = func(time.Duration, *http.Response, error) (time.Duratio
// response codes and validates that the received response code is among them
func requireStatusIn(statuses ...int) doRequestWrapper {
fn := func(d time.Duration, resp *http.Response, e error) (time.Duration, *http.Response, error) {
statuses := statuses
if e != nil {
if resp != nil {
resp.Body.Close()
_ = resp.Body.Close()
}
return d, nil, e
}
@ -466,7 +486,7 @@ func requireStatusIn(statuses ...int) doRequestWrapper {
// HTTP response code when accessing the variable's HTTP API
func generateUnexpectedResponseCodeError(resp *http.Response) error {
var buf bytes.Buffer
io.Copy(&buf, resp.Body)
resp.Body.Close()
_, _ = io.Copy(&buf, resp.Body)
_ = resp.Body.Close()
return fmt.Errorf("Unexpected response code: %d (%s)", resp.StatusCode, buf.Bytes())
}

View File

@ -101,7 +101,7 @@ func TestVariables_SimpleCRUD(t *testing.T) {
_, err := nsv.Delete(sv1.Path, nil)
must.NoError(t, err)
_, _, err = nsv.Read(sv1.Path, nil)
must.ErrorContains(t, err, ErrVariableNotFound)
must.ErrorIs(t, err, ErrVariablePathNotFound)
})
t.Run("7 list vars after delete", func(t *testing.T) {
@ -187,14 +187,14 @@ func TestVariables_Read(t *testing.T) {
testCases := []struct {
name string
path string
expectedError string
expectedError error
checkValue bool
expectedValue *Variable
}{
{
name: "not found",
path: tID + "/not/found",
expectedError: ErrVariableNotFound,
expectedError: ErrVariablePathNotFound,
checkValue: true,
expectedValue: nil,
},
@ -208,8 +208,67 @@ func TestVariables_Read(t *testing.T) {
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
get, _, err := nsv.Read(tc.path, nil)
if tc.expectedError != "" {
must.EqError(t, err, tc.expectedError)
if tc.expectedError != nil {
must.ErrorIs(t, err, tc.expectedError)
} else {
must.NoError(t, err)
}
if tc.checkValue {
if tc.expectedValue != nil {
must.NotNil(t, get)
must.Eq(t, tc.expectedValue, get)
} else {
must.Nil(t, get)
}
}
})
}
}
func TestVariables_GetVariableItems(t *testing.T) {
testutil.Parallel(t)
c, s := makeClient(t, nil, nil)
defer s.Stop()
nsv := c.Variables()
tID := fmt.Sprint(time.Now().UTC().UnixNano())
sv1 := Variable{
Namespace: "default",
Path: tID + "/sv1",
Items: map[string]string{
"kv1": "val1",
"kv2": "val2",
},
}
writeTestVariable(t, c, &sv1)
testCases := []struct {
name string
path string
expectedError error
checkValue bool
expectedValue VariableItems
}{
{
name: "not found",
path: tID + "/not/found",
expectedError: ErrVariablePathNotFound,
checkValue: true,
expectedValue: nil,
},
{
name: "found",
path: sv1.Path,
checkValue: true,
expectedValue: sv1.Items,
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
get, _, err := nsv.GetVariableItems(tc.path, nil)
if tc.expectedError != nil {
must.ErrorIs(t, err, tc.expectedError)
} else {
must.NoError(t, err)
}