Skip to content

Commit e26e4b0

Browse files
Benjosh95Benjosh95
committed
fix waiter and tests
1 parent f6ae2d9 commit e26e4b0

File tree

2 files changed

+111
-39
lines changed

2 files changed

+111
-39
lines changed

‎services/kms/wait/wait.go‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package wait
33
import (
44
"context"
55
"errors"
6+
"fmt"
67
"net/http"
78
"time"
89

@@ -117,6 +118,12 @@ func EnableKeyVersionWaitHandler(ctx context.Context, client ApiKmsClient, proje
117118
handler := wait.New(func() (bool, *kms.Version, error) {
118119
response, err := client.GetVersionExecute(ctx, projectId, region, keyRingId, keyId, version)
119120
if err != nil {
121+
var apiErr *oapierror.GenericOpenAPIError
122+
if errors.As(err, &apiErr) {
123+
if statusCode := apiErr.StatusCode; statusCode == http.StatusNotFound || statusCode == http.StatusGone {
124+
return true, nil, fmt.Errorf("enabling failed for key %s version %d: version not found", keyId, version)
125+
}
126+
}
120127
return false, nil, err
121128
}
122129

@@ -143,16 +150,36 @@ func EnableKeyVersionWaitHandler(ctx context.Context, client ApiKmsClient, proje
143150

144151
func DisableKeyVersionWaitHandler(ctx context.Context, client ApiKmsClient, projectId, region, keyRingId, keyId string, version int64) *wait.AsyncActionHandler[kms.Version] {
145152
handler := wait.New(func() (bool, *kms.Version, error) {
146-
_, err := client.GetVersionExecute(ctx, projectId, region, keyRingId, keyId, version)
153+
response, err := client.GetVersionExecute(ctx, projectId, region, keyRingId, keyId, version)
147154
if err != nil {
148155
var apiErr *oapierror.GenericOpenAPIError
149156
if errors.As(err, &apiErr) {
150157
if statusCode := apiErr.StatusCode; statusCode == http.StatusNotFound || statusCode == http.StatusGone {
151-
return true, nil, nil
158+
return true, nil, fmt.Errorf("disabling failed for key %s version %d: version not found", keyId, version)
152159
}
153160
}
154-
return true, nil, err
161+
return false, nil, err
162+
}
163+
164+
if response.State != nil {
165+
switch *response.State {
166+
case kms.VERSIONSTATE_DISABLED:
167+
return true, response, nil
168+
case kms.VERSIONSTATE_ACTIVE:
169+
return false, nil, nil
170+
case kms.VERSIONSTATE_CREATING:
171+
return false, nil, nil
172+
case kms.VERSIONSTATE_KEY_MATERIAL_UNAVAILABLE:
173+
return false, nil, nil
174+
case kms.VERSIONSTATE_DESTROYED:
175+
return true, response, fmt.Errorf("disabling failed for key %s version %d: state %s", keyId, version, *response.State)
176+
case kms.VERSIONSTATE_KEY_MATERIAL_INVALID:
177+
return true, response, fmt.Errorf("disabling failed for key %s version %d: state %s", keyId, version, *response.State)
178+
default:
179+
return true, response, fmt.Errorf("key version %d for key %s has unexpected state %s", version, keyId, *response.State)
180+
}
155181
}
182+
156183
return false, nil, nil
157184
})
158185
handler.SetTimeout(10 * time.Minute)

‎services/kms/wait/wait_test.go‎

Lines changed: 81 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -212,7 +212,7 @@ func TestCreateKeyRingWaitHandler(t *testing.T) {
212212
}
213213

214214
if diff := cmp.Diff(tt.want, got); diff != "" {
215-
t.Errorf("differing key %s", diff)
215+
t.Errorf("differing key ring %s", diff)
216216
}
217217
})
218218
}
@@ -435,6 +435,30 @@ func TestEnableKeyVersionWaitHandler(t *testing.T) {
435435
fixtureVersion(1, false, "bogus"),
436436
false,
437437
},
438+
{
439+
"version not found (404)",
440+
[]versionResponse{
441+
{nil, oapierror.NewError(http.StatusNotFound, "not found")},
442+
},
443+
nil,
444+
true,
445+
},
446+
{
447+
"version gone (410)",
448+
[]versionResponse{
449+
{nil, oapierror.NewError(http.StatusGone, "gone")},
450+
},
451+
nil,
452+
true,
453+
},
454+
{
455+
"error fetching version",
456+
[]versionResponse{
457+
{nil, oapierror.NewError(http.StatusInternalServerError, "internal error")},
458+
},
459+
nil,
460+
true,
461+
},
438462
// no special update tests needed as the states are the same
439463
}
440464
for _, tt := range tests {
@@ -454,7 +478,7 @@ func TestEnableKeyVersionWaitHandler(t *testing.T) {
454478
}
455479

456480
if diff := cmp.Diff(tt.want, got); diff != "" {
457-
t.Errorf("differing key %s", diff)
481+
t.Errorf("differing version %s", diff)
458482
}
459483
})
460484
}
@@ -464,61 +488,84 @@ func TestDisableKeyVersionWaitHandler(t *testing.T) {
464488
tests := []struct {
465489
name string
466490
responses []versionResponse
491+
want *kms.Version
467492
wantErr bool
468493
}{
469494
{
470-
"Delete with '404' succeeded immediately",
495+
"disable succeeded immediately",
471496
[]versionResponse{
472-
{nil, oapierror.NewError(http.StatusNotFound, "not found")},
497+
{fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED), nil},
473498
},
499+
fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED),
474500
false,
475501
},
476502
{
477-
"Delete with '404' delayed",
503+
"disable succeeded delayed",
504+
[]versionResponse{
505+
{fixtureVersion(1, false, kms.VERSIONSTATE_ACTIVE), nil},
506+
{fixtureVersion(1, false, kms.VERSIONSTATE_ACTIVE), nil},
507+
{fixtureVersion(1, false, kms.VERSIONSTATE_ACTIVE), nil},
508+
{fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED), nil},
509+
},
510+
fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED),
511+
false,
512+
},
513+
{
514+
"disable succeeded from creating state",
478515
[]versionResponse{
479516
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
480517
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
481-
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
482-
{nil, oapierror.NewError(http.StatusNotFound, "not found")},
518+
{fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED), nil},
483519
},
520+
fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED),
484521
false,
485522
},
486523
{
487-
"Delete with 'gone' succeeded immediately",
524+
"version already destroyed",
488525
[]versionResponse{
489-
{nil, oapierror.NewError(http.StatusGone, "gone")},
526+
{fixtureVersion(1, false, kms.VERSIONSTATE_DESTROYED), nil},
490527
},
491-
false,
528+
fixtureVersion(1, false, kms.VERSIONSTATE_DESTROYED),
529+
true,
492530
},
493531
{
494-
"Delete with 'gone' delayed",
532+
"version key material invalid",
495533
[]versionResponse{
496-
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
497-
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
498-
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
499-
{nil, oapierror.NewError(http.StatusGone, "not found")},
534+
{fixtureVersion(1, false, kms.VERSIONSTATE_KEY_MATERIAL_INVALID), nil},
500535
},
501-
false,
536+
fixtureVersion(1, false, kms.VERSIONSTATE_KEY_MATERIAL_INVALID),
537+
true,
502538
},
503539
{
504-
"Delete with error delayed",
540+
"timeout waiting for disabled state",
505541
[]versionResponse{
506-
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
507-
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
508-
509-
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
510-
{fixtureVersion(1, false, kms.VERSIONSTATE_DESTROYED), oapierror.NewError(http.StatusInternalServerError, "kapow")},
542+
{fixtureVersion(1, false, kms.VERSIONSTATE_ACTIVE), nil},
543+
},
544+
nil,
545+
true,
546+
},
547+
{
548+
"version not found (404)",
549+
[]versionResponse{
550+
{nil, oapierror.NewError(http.StatusNotFound, "not found")},
511551
},
552+
nil,
512553
true,
513554
},
514555
{
515-
"Cannot delete",
556+
"version gone (410)",
516557
[]versionResponse{
517-
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
518-
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
519-
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
520-
{fixtureVersion(1, false, kms.VERSIONSTATE_DESTROYED), oapierror.NewError(http.StatusOK, "ok")},
558+
{nil, oapierror.NewError(http.StatusGone, "gone")},
521559
},
560+
nil,
561+
true,
562+
},
563+
{
564+
"error fetching version",
565+
[]versionResponse{
566+
{nil, oapierror.NewError(http.StatusInternalServerError, "internal error")},
567+
},
568+
nil,
522569
true,
523570
},
524571
}
@@ -529,18 +576,16 @@ func TestDisableKeyVersionWaitHandler(t *testing.T) {
529576
versionResponses: tt.responses,
530577
}
531578
handler := DisableKeyVersionWaitHandler(ctx, client, testProject, testRegion, testKeyRingId, testKeyId, 1)
532-
_, err := handler.SetTimeout(1 * time.Second).
579+
got, err := handler.SetTimeout(1 * time.Second).
533580
SetThrottle(250 * time.Millisecond).
534581
WaitWithContext(ctx)
535582

536-
if tt.wantErr != (err != nil) {
537-
t.Fatalf("wrong error result. want err: %v got %v", tt.wantErr, err)
583+
if (err != nil) != tt.wantErr {
584+
t.Fatalf("unexpected error response. want %v but got %v ", tt.wantErr, err)
538585
}
539-
if tt.wantErr {
540-
var apiErr *oapierror.GenericOpenAPIError
541-
if !errors.As(err, &apiErr) {
542-
t.Fatalf("expected openapi error, got %v", err)
543-
}
586+
587+
if diff := cmp.Diff(tt.want, got); diff != "" {
588+
t.Errorf("differing version %s", diff)
544589
}
545590
})
546591
}
@@ -618,7 +663,7 @@ func TestCreateWrappingWaitHandler(t *testing.T) {
618663
}
619664

620665
if diff := cmp.Diff(tt.want, got); diff != "" {
621-
t.Errorf("differing key %s", diff)
666+
t.Errorf("differing wrapping key %s", diff)
622667
}
623668
})
624669
}

0 commit comments

Comments
 (0)