Skip to content

Commit 1039c19

Browse files
xia0pin9xia0pin9
authored
Merge pull request #116 from xia0pin9/refactor/log-standardization
refactor: standardize error logging across codebase
2 parents 350b973 + d3d5c37 commit 1039c19

24 files changed

+109
-103
lines changed

‎client/add.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ var addTinkKeyset = cmdAdd.Flag.String("key-template", "", "name of a knox-suppo
3535

3636
func runAdd(cmd *Command, args []string) *ErrorStatus {
3737
if len(args) != 1 {
38-
return &ErrorStatus{fmt.Errorf("add takes only one argument. See 'knox help add'"), false}
38+
return &ErrorStatus{fmt.Errorf("add takes only one argument; see 'knox help add'"), false}
3939
}
4040
keyID := args[0]
4141
var data []byte
@@ -50,7 +50,7 @@ func runAdd(cmd *Command, args []string) *ErrorStatus {
5050
}
5151
versionID, err := cli.AddVersion(keyID, data)
5252
if err != nil {
53-
return &ErrorStatus{fmt.Errorf("Error adding version: %s", err.Error()), true}
53+
return &ErrorStatus{fmt.Errorf("error adding version: %w", err), true}
5454
}
5555
fmt.Printf("Added key version %d\n", versionID)
5656
return nil
@@ -65,7 +65,7 @@ func getDataWithTemplate(templateName string, keyID string) ([]byte, error) {
6565
// get all versions (primary, active, inactive) of this knox identifier
6666
allVersions, err := cli.NetworkGetKeyWithStatus(keyID, knox.Inactive)
6767
if err != nil {
68-
return nil, fmt.Errorf("error getting key: %s", err.Error())
68+
return nil, fmt.Errorf("error getting key: %w", err)
6969
}
7070
return addNewTinkKeyset(tinkKeyTemplates[templateName].templateFunc, allVersions.VersionList)
7171
}

‎client/create.go‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ var createTinkKeyset = cmdCreate.Flag.String("key-template", "", "name of a knox
3737

3838
func runCreate(cmd *Command, args []string) *ErrorStatus {
3939
if len(args) != 1 {
40-
return &ErrorStatus{fmt.Errorf("create takes exactly one argument. See 'knox help create'"), false}
40+
return &ErrorStatus{fmt.Errorf("create takes exactly one argument; see 'knox help create'"), false}
4141
}
4242
keyID := args[0]
4343
var data []byte
@@ -59,7 +59,7 @@ func runCreate(cmd *Command, args []string) *ErrorStatus {
5959
acl := knox.ACL{}
6060
versionID, err := cli.CreateKey(keyID, data, acl)
6161
if err != nil {
62-
return &ErrorStatus{fmt.Errorf("Error adding version: %s", err.Error()), true}
62+
return &ErrorStatus{fmt.Errorf("error adding version: %w", err), true}
6363
}
6464
fmt.Printf("Created key with initial version %d\n", versionID)
6565
return nil
@@ -69,7 +69,7 @@ func readDataFromStdin() ([]byte, error) {
6969
fmt.Println("Reading from stdin...")
7070
data, err := io.ReadAll(os.Stdin)
7171
if err != nil {
72-
return data, fmt.Errorf("problem reading key data: %s", err.Error())
72+
return data, fmt.Errorf("problem reading key data: %w", err)
7373
}
7474
return data, nil
7575
}

‎client/daemon.go‎

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -55,7 +55,7 @@ func runDaemon(cmd *Command, args []string) *ErrorStatus {
5555
if os.Getenv("KNOX_MACHINE_AUTH") == "" {
5656
hostname, err := os.Hostname()
5757
if err != nil {
58-
return &ErrorStatus{fmt.Errorf("You're on a host with no name: %s", err.Error()), false}
58+
return &ErrorStatus{fmt.Errorf("you're on a host with no name: %w", err), false}
5959
}
6060
os.Setenv("KNOX_MACHINE_AUTH", hostname)
6161
}
@@ -90,7 +90,7 @@ func (d *daemon) loop(refresh time.Duration) {
9090

9191
watcher, err := fsnotify.NewWatcher()
9292
if err != nil {
93-
fatalf("Unable to watch files: %s", err.Error())
93+
fatalf("unable to watch files: %v", err)
9494
}
9595
watcher.Add(d.registerFilename())
9696

@@ -100,7 +100,7 @@ func (d *daemon) loop(refresh time.Duration) {
100100
err := d.update()
101101
if err != nil {
102102
d.updateErrCount++
103-
logf("Failed to update keys: %s", err.Error())
103+
logf("failed to update keys: %v", err)
104104
} else {
105105
d.successCount++
106106
}
@@ -125,29 +125,29 @@ func (d *daemon) loop(refresh time.Duration) {
125125
func (d *daemon) initialize() error {
126126
err := os.MkdirAll(d.dir, defaultDirPermission)
127127
if err != nil {
128-
return fmt.Errorf("Failed to initialize /var/lib/knox (run 'sudo mkdir /var/lib/knox'?): %s", err.Error())
128+
return fmt.Errorf("failed to initialize /var/lib/knox (run 'sudo mkdir /var/lib/knox'?): %w", err)
129129
}
130130

131131
// Need to chmod due to a umask set on masterless puppet machines
132132
err = os.Chmod(d.dir, defaultDirPermission)
133133
if err != nil {
134-
return fmt.Errorf("Failed to open up directory permissions: %s", err.Error())
134+
return fmt.Errorf("failed to open up directory permissions: %w", err)
135135
}
136136
err = os.MkdirAll(d.keyDir(), defaultDirPermission)
137137
if err != nil {
138-
return fmt.Errorf("Failed to make key folders: %s", err.Error())
138+
return fmt.Errorf("failed to make key folders: %w", err)
139139
}
140140

141141
// Need to chmod due to a umask set on masterless puppet machines
142142
err = os.Chmod(d.keyDir(), defaultDirPermission)
143143
if err != nil {
144-
return fmt.Errorf("Failed to open up directory permissions: %s", err.Error())
144+
return fmt.Errorf("failed to open up directory permissions: %w", err)
145145
}
146146
_, err = os.Stat(d.registerFilename())
147147
if os.IsNotExist(err) {
148148
err := os.WriteFile(d.registerFilename(), []byte{}, defaultFilePermission)
149149
if err != nil {
150-
return fmt.Errorf("Failed to initialize registered key file: %s", err.Error())
150+
return fmt.Errorf("failed to initialize registered key file: %w", err)
151151
}
152152
} else if err != nil {
153153
return err
@@ -156,7 +156,7 @@ func (d *daemon) initialize() error {
156156
// Need to chmod due to a umask set on masterless puppet machines
157157
err = os.Chmod(d.registerFilename(), defaultFilePermission)
158158
if err != nil {
159-
return fmt.Errorf("Failed to open up register file permissions: %s", err.Error())
159+
return fmt.Errorf("failed to open up register file permissions: %w", err)
160160
}
161161
d.registerKeyFile = NewKeysFile(d.registerFilename())
162162
return nil
@@ -196,7 +196,7 @@ func (d *daemon) update() error {
196196
key, err := d.cli.CacheGetKey(keyID)
197197
if err != nil {
198198
// Keep going in spite of failure
199-
logf("error getting cache key: %s", err)
199+
logf("error getting cache key: %v", err)
200200
// Remove existing cached key with invalid format (saved with previous version clients)
201201
if _, err = os.Stat(d.keyFilename(keyID)); err == nil {
202202
d.deleteKey(keyID)
@@ -222,7 +222,7 @@ func (d *daemon) update() error {
222222
if err != nil {
223223
// Keep going in spite of failure
224224
d.getKeyErrCount++
225-
logf("error processing key: %s", err)
225+
logf("error processing key: %v", err)
226226
}
227227
}
228228
}
@@ -270,11 +270,13 @@ func (d daemon) keyFilename(id string) string {
270270
func (d daemon) processKey(keyID string) error {
271271
key, err := d.cli.NetworkGetKey(keyID)
272272
if err != nil {
273-
if err.Error() == "User or machine not authorized" || err.Error() == "Key identifer does not exist" {
273+
errMsg := err.Error()
274+
// Check for authorization or key not found errors (using contains for more robust matching)
275+
if strings.Contains(errMsg, "User or machine not authorized") || strings.Contains(errMsg, "Key identifier does not exist") {
274276
// This removes keys that do not exist or the machine is unauthorized to access
275277
d.registerKeyFile.Remove([]string{keyID})
276278
}
277-
return fmt.Errorf("Error getting key %s: %s", keyID, err.Error())
279+
return fmt.Errorf("error getting key %s: %w", keyID, err)
278280
}
279281
// Do not cache any new keys if they have invalid content
280282
if key.ID == "" || key.ACL == nil || key.VersionList == nil || key.VersionHash == "" {
@@ -284,42 +286,42 @@ func (d daemon) processKey(keyID string) error {
284286
if strings.HasPrefix(keyID, tinkPrefix) {
285287
keysetHandle, _, err := getTinkKeysetHandleFromKnoxVersionList(key.VersionList)
286288
if err != nil {
287-
return fmt.Errorf("Error fetching keyset handle for this tink key %s: %s", keyID, err.Error())
289+
return fmt.Errorf("error fetching keyset handle for this tink key %s: %w", keyID, err)
288290
}
289291
tinkKeyset, err := convertTinkKeysetHandleToBytes(keysetHandle)
290292
if err != nil {
291-
return fmt.Errorf("Error converting tink keyset handle to bytes %s: %s", keyID, err.Error())
293+
return fmt.Errorf("error converting tink keyset handle to bytes %s: %w", keyID, err)
292294
}
293295
key.TinkKeyset = base64.StdEncoding.EncodeToString(tinkKeyset)
294296
}
295297

296298
b, err := json.Marshal(key)
297299
if err != nil {
298-
return fmt.Errorf("Error marshalling key %s: %s", keyID, err.Error())
300+
return fmt.Errorf("error marshalling key %s: %w", keyID, err)
299301
}
300302
// Write to tmpfile, mv to normal location. Close + rm on failures
301303
tmpFile, err := os.CreateTemp(d.dir, fmt.Sprintf(".*.%s.tmp", keyID))
302304
if err != nil {
303-
return fmt.Errorf("Error opening tmp file for key %s: %s", keyID, err.Error())
305+
return fmt.Errorf("error opening tmp file for key %s: %w", keyID, err)
304306
}
305307
_, err = tmpFile.Write(b)
306308
if err != nil {
307309
tmpFile.Close()
308310
os.Remove(tmpFile.Name())
309-
return fmt.Errorf("Error writing key %s to file: %s", keyID, err.Error())
311+
return fmt.Errorf("error writing key %s to file: %w", keyID, err)
310312
}
311313
// Done writing
312314
tmpFile.Close()
313315

314316
err = os.Rename(tmpFile.Name(), d.keyFilename(keyID))
315317
if err != nil {
316318
os.Remove(tmpFile.Name())
317-
return fmt.Errorf("Error renaming key %s temporary file: %s", keyID, err.Error())
319+
return fmt.Errorf("error renaming key %s temporary file: %w", keyID, err)
318320
}
319321

320322
err = os.Chmod(d.keyFilename(keyID), defaultFilePermission)
321323
if err != nil {
322-
return fmt.Errorf("Failed to open up key file permissions: %s", err.Error())
324+
return fmt.Errorf("failed to open up key file permissions: %w", err)
323325
}
324326
return nil
325327
}
@@ -360,7 +362,7 @@ func (k *KeysFile) Lock() error {
360362

361363
// Annotate error with path to file to make debugging easier
362364
if err != nil {
363-
return fmt.Errorf("unable to obtain lock on file '%s': %s", k.fn, err.Error())
365+
return fmt.Errorf("unable to obtain lock on file '%s': %w", k.fn, err)
364366
}
365367
return nil
366368
}
@@ -371,7 +373,7 @@ func (k *KeysFile) Unlock() error {
371373

372374
// Annotate error with path to file to make debugging easier
373375
if err != nil {
374-
return fmt.Errorf("unable to release lock on file '%s': %s", k.fn, err.Error())
376+
return fmt.Errorf("unable to release lock on file '%s': %w", k.fn, err)
375377
}
376378
return nil
377379
}
@@ -474,7 +476,7 @@ func identifyLockHolders(filename string) (string, error) {
474476
cmd := exec.Command("lsof", filename)
475477
out, err := cmd.CombinedOutput()
476478
if err != nil {
477-
return string(out), fmt.Errorf("error identifying lock holder: %s", err.Error())
479+
return string(out), fmt.Errorf("error identifying lock holder: %w", err)
478480
}
479481

480482
return string(out), nil

‎client/deactivate.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,14 +27,14 @@ See also: knox reactivate, knox promote
2727

2828
func runDeactivate(cmd *Command, args []string) *ErrorStatus {
2929
if len(args) != 2 {
30-
return &ErrorStatus{fmt.Errorf("deactivate takes exactly two argument. See 'knox help deactivate'"), false}
30+
return &ErrorStatus{fmt.Errorf("deactivate takes exactly two arguments; see 'knox help deactivate'"), false}
3131
}
3232
keyID := args[0]
3333
keyVersion := args[1]
3434

3535
err := cli.UpdateVersion(keyID, keyVersion, knox.Inactive)
3636
if err != nil {
37-
return &ErrorStatus{fmt.Errorf("Error updating version: %s", err.Error()), true}
37+
return &ErrorStatus{fmt.Errorf("error updating version: %w", err), true}
3838
}
3939
fmt.Printf("Deactivated %s successfully.\n", keyVersion)
4040
return nil

‎client/delete.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,12 @@ See also: knox create
1919

2020
func runDelete(cmd *Command, args []string) *ErrorStatus {
2121
if len(args) != 1 {
22-
return &ErrorStatus{fmt.Errorf("create takes exactly one argument. See 'knox help delete'"), false}
22+
return &ErrorStatus{fmt.Errorf("delete takes exactly one argument; see 'knox help delete'"), false}
2323
}
2424

2525
err := cli.DeleteKey(args[0])
2626
if err != nil {
27-
return &ErrorStatus{fmt.Errorf("Error deleting key: %s", err.Error()), true}
27+
return &ErrorStatus{fmt.Errorf("error deleting key: %w", err), true}
2828
}
2929
fmt.Printf("Successfully deleted key\n")
3030
return nil

‎client/get.go‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ func failureGetKeyMetric(keyID string, err error) {
5858

5959
func runGet(cmd *Command, args []string) *ErrorStatus {
6060
if len(args) != 1 {
61-
return &ErrorStatus{fmt.Errorf("get takes only one argument. See 'knox help get'"), false}
61+
return &ErrorStatus{fmt.Errorf("get takes only one argument; see 'knox help get'"), false}
6262
}
6363
keyID := args[0]
6464

@@ -101,7 +101,7 @@ func runGet(cmd *Command, args []string) *ErrorStatus {
101101
}
102102
if err != nil {
103103
failureGetKeyMetric(keyID, err)
104-
return &ErrorStatus{fmt.Errorf("Error getting key: %s", err.Error()), true}
104+
return &ErrorStatus{fmt.Errorf("error getting key: %w", err), true}
105105
}
106106
if *getJSON {
107107
data, err := json.Marshal(key)
@@ -128,12 +128,12 @@ func runGet(cmd *Command, args []string) *ErrorStatus {
128128
}
129129
}
130130
failureGetKeyMetric(keyID, errors.New("key version not found"))
131-
return &ErrorStatus{fmt.Errorf("%s", "Key version not found."), false}
131+
return &ErrorStatus{fmt.Errorf("key version not found"), false}
132132
}
133133

134134
func retrieveTinkKeyset(keyID string, getFromNetwork bool) ([]byte, *ErrorStatus) {
135135
if !isIDforTinkKeyset(keyID) {
136-
return nil, &ErrorStatus{fmt.Errorf("this knox identifier is not for tink keyset"), false}
136+
return nil, &ErrorStatus{fmt.Errorf("this knox identifier is not for a tink keyset"), false}
137137
}
138138
// get the primary and all active versions of this knox identifier.
139139
var primaryAndActiveVersions *knox.Key
@@ -144,7 +144,7 @@ func retrieveTinkKeyset(keyID string, getFromNetwork bool) ([]byte, *ErrorStatus
144144
primaryAndActiveVersions, err = cli.GetKey(keyID)
145145
}
146146
if err != nil {
147-
return nil, &ErrorStatus{fmt.Errorf("error getting key: %s", err.Error()), true}
147+
return nil, &ErrorStatus{fmt.Errorf("error getting key: %w", err), true}
148148
}
149149
keysetHandle, _, err := getTinkKeysetHandleFromKnoxVersionList(primaryAndActiveVersions.VersionList)
150150
if err != nil {
@@ -159,7 +159,7 @@ func retrieveTinkKeyset(keyID string, getFromNetwork bool) ([]byte, *ErrorStatus
159159

160160
func retrieveTinkKeysetInfo(keyID string, getFromNetwork bool) (string, *ErrorStatus) {
161161
if !isIDforTinkKeyset(keyID) {
162-
return "", &ErrorStatus{fmt.Errorf("this knox identifier is not for tink keyset"), false}
162+
return "", &ErrorStatus{fmt.Errorf("this knox identifier is not for a tink keyset"), false}
163163
}
164164
// get the primary and all active versions of this knox identifier.
165165
var primaryAndActiveVersions *knox.Key
@@ -170,7 +170,7 @@ func retrieveTinkKeysetInfo(keyID string, getFromNetwork bool) (string, *ErrorSt
170170
primaryAndActiveVersions, err = cli.GetKey(keyID)
171171
}
172172
if err != nil {
173-
return "", &ErrorStatus{fmt.Errorf("error getting key: %s", err.Error()), true}
173+
return "", &ErrorStatus{fmt.Errorf("error getting key: %w", err), true}
174174
}
175175
keysetHandle, tinkKeyIDToKnoxVersionID, err := getTinkKeysetHandleFromKnoxVersionList(primaryAndActiveVersions.VersionList)
176176
if err != nil {

‎client/getacl.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,20 +29,20 @@ var getACLJSON = cmdGetACL.Flag.Bool("json", false, "")
2929

3030
func runGetACL(cmd *Command, args []string) *ErrorStatus {
3131
if len(args) != 1 {
32-
return &ErrorStatus{fmt.Errorf("acl takes only one argument. See 'knox help acl'"), false}
32+
return &ErrorStatus{fmt.Errorf("acl takes only one argument; see 'knox help acl'"), false}
3333
}
3434

3535
keyID := args[0]
3636
acl, err := cli.GetACL(keyID)
3737
if err != nil {
38-
return &ErrorStatus{fmt.Errorf("Error getting key ACL: %s", err.Error()), true}
38+
return &ErrorStatus{fmt.Errorf("error getting key ACL: %w", err), true}
3939
}
4040

4141
if *getACLJSON {
4242
aclEnc, err := json.Marshal(acl)
4343
if err != nil {
4444
// malformated ACL considered as knox server side error
45-
return &ErrorStatus{fmt.Errorf("Could not marshal ACL: %v", acl), true}
45+
return &ErrorStatus{fmt.Errorf("could not marshal ACL: %v", acl), true}
4646
}
4747
fmt.Println(string(aclEnc))
4848
return nil
@@ -52,7 +52,7 @@ func runGetACL(cmd *Command, args []string) *ErrorStatus {
5252
aEnc, err := json.Marshal(a)
5353
if err != nil {
5454
// malformated ACL entry considered as knox server side error
55-
return &ErrorStatus{fmt.Errorf("Could not marshal entry: %v", a), true}
55+
return &ErrorStatus{fmt.Errorf("could not marshal entry: %v", a), true}
5656
}
5757
fmt.Println(string(aEnc))
5858
}

‎client/getkeys.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ func runGetKeys(cmd *Command, args []string) *ErrorStatus {
2828
}
2929
l, err := cli.GetKeys(m)
3030
if err != nil {
31-
return &ErrorStatus{fmt.Errorf("Error getting keys: %s", err.Error()), true}
31+
return &ErrorStatus{fmt.Errorf("error getting keys: %w", err), true}
3232
}
3333
for _, k := range l {
3434
fmt.Println(k)

‎client/getversions.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ var verboseOutput = cmdGetVersions.Flag.Bool("v", false, "verbose")
3333

3434
func runGetVersions(cmd *Command, args []string) *ErrorStatus {
3535
if len(args) != 1 {
36-
return &ErrorStatus{fmt.Errorf("get takes only one argument. See 'knox help versions'"), false}
36+
return &ErrorStatus{fmt.Errorf("versions takes only one argument; see 'knox help versions'"), false}
3737
}
3838

3939
var status knox.VersionStatus
@@ -49,7 +49,7 @@ func runGetVersions(cmd *Command, args []string) *ErrorStatus {
4949
keyID := args[0]
5050
key, err := cli.GetKeyWithStatus(keyID, status)
5151
if err != nil {
52-
return &ErrorStatus{fmt.Errorf("Error getting key: %s", err.Error()), true}
52+
return &ErrorStatus{fmt.Errorf("error getting key: %w", err), true}
5353
}
5454
kvl := key.VersionList
5555
for _, v := range kvl {

0 commit comments

Comments
 (0)