From c9ffd1274b9dc7fcdceb44420cc2eedd7024376a Mon Sep 17 00:00:00 2001 From: Seth Hoenig Date: Wed, 22 Feb 2023 08:17:22 -0600 Subject: [PATCH] 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 --- .changelog/16237.txt | 11 ++ api/variables.go | 226 +++++++++++++++++++++++------------------- api/variables_test.go | 69 ++++++++++++- 3 files changed, 198 insertions(+), 108 deletions(-) create mode 100644 .changelog/16237.txt diff --git a/.changelog/16237.txt b/.changelog/16237.txt new file mode 100644 index 000000000..4dccfc47b --- /dev/null +++ b/.changelog/16237.txt @@ -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 +``` diff --git a/api/variables.go b/api/variables.go index 2a6a3ca96..4d1c8f392 100644 --- a/api/variables.go +++ b/api/variables.go @@ -12,8 +12,16 @@ import ( ) const ( - ErrVariableNotFound = "variable not found" - ErrVariableMissingItems = "variable missing Items field" + // ErrVariableNotFound was used as the content of an error string. + // + // Deprecated: use ErrVariablePathNotFound instead. + ErrVariableNotFound = "variable not found" +) + +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()) } diff --git a/api/variables_test.go b/api/variables_test.go index 8ea709d80..8bb6b8ea6 100644 --- a/api/variables_test.go +++ b/api/variables_test.go @@ -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) }