Skip to content

Commit 85a20da

Browse files
Benjosh95Benjosh95
authored
fix(kms): fix disableKeyVersionWaitHandler and enableKeyVersionWaitHandler to check for proper state (#3699)
1 parent 7dd87f7 commit 85a20da

File tree

5 files changed

+147
-54
lines changed

5 files changed

+147
-54
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@
77
- **Breaking Change:** Removal of unused model structs: `Area`, `AreaConfig`, `AreaPrefixConfigIPv4`, `UpdateAreaIPv4`, `NetworkAreaIPv4`, `CreateAreaAddressFamily`, `CreateAreaIPv4`, `CreateNetworkAddressFamily`, `CreateNetworkIPv4Body`, `CreateNetworkIPv6Body`, `CreateServerPayloadBootVolume`, `CreateServerPayloadNetworking`, `NullableUpdateAreaAddressFamily`, `CreateServerPayloadNetworking`, `UpdateNetworkAddressFamily`, `CreateServerPayloadNetworking`, `CreateServerPayloadNetworking`
88
- `cdn`: [v2.1.0](services/cdn/CHANGELOG.md#v210)
99
- **Breaking change:** Removal of unused model structs: `GetLogsSearchFiltersResponse`, `PatchLokiLogSink`
10+
- `kms`: [v1.1.0](services/kms/CHANGELOG.md#v110)
11+
- **Bugfix:** Ensure correct state checking in `DisableKeyVersionWaitHandler` and `EnableKeyVersionWaitHandler`
1012

1113
## Release (2025-10-29)
1214
- `stackitmarketplace`: [v1.15.0](services/stackitmarketplace/CHANGELOG.md#v1150)

‎services/kms/CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
## v1.1.0
2+
- **Bugfix:** Ensure correct state checking in `DisableKeyVersionWaitHandler` and `EnableKeyVersionWaitHandler`
3+
14
## v1.0.0
25
- Switch to API version `v1` of STACKIT KMS service (previously `v1beta`)
36
- **Breaking Change:** Removal of deprecated `Backend` model

‎services/kms/VERSION‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
v1.0.0
1+
v1.1.0

‎services/kms/wait/wait.go‎

Lines changed: 33 additions & 14 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

@@ -58,7 +59,7 @@ func CreateKeyRingWaitHandler(ctx context.Context, client ApiKmsClient, projectI
5859
return false, nil, err
5960
}
6061

61-
if response.State != nil {
62+
if response != nil && response.State != nil {
6263
switch *response.State {
6364
case kms.KEYRINGSTATE_CREATING:
6465
return false, nil, nil
@@ -80,7 +81,7 @@ func CreateOrUpdateKeyWaitHandler(ctx context.Context, client ApiKmsClient, proj
8081
return false, nil, err
8182
}
8283

83-
if response.State != nil {
84+
if response != nil && response.State != nil {
8485
switch *response.State {
8586
case kms.KEYSTATE_CREATING:
8687
return false, nil, nil
@@ -117,21 +118,25 @@ 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 or key not found", keyId, version)
125+
}
126+
}
120127
return false, nil, err
121128
}
122129

123-
if response.State != nil {
130+
if response != nil && response.State != nil {
124131
switch *response.State {
125-
case kms.VERSIONSTATE_DESTROYED:
126-
return true, response, nil
127-
case kms.VERSIONSTATE_KEY_MATERIAL_INVALID:
128-
return true, response, nil
129-
case kms.VERSIONSTATE_DISABLED:
132+
case kms.VERSIONSTATE_ACTIVE:
130133
return true, response, nil
131134
case kms.VERSIONSTATE_CREATING:
132135
return false, nil, nil
136+
case kms.VERSIONSTATE_DESTROYED, kms.VERSIONSTATE_KEY_MATERIAL_INVALID:
137+
return true, response, fmt.Errorf("enabling failed for key %s version %d: state %s", keyId, version, *response.State)
133138
default:
134-
return true, response, nil
139+
return true, response, fmt.Errorf("key version %d for key %s has unexpected state %s", version, keyId, *response.State)
135140
}
136141
}
137142

@@ -143,16 +148,30 @@ func EnableKeyVersionWaitHandler(ctx context.Context, client ApiKmsClient, proje
143148

144149
func DisableKeyVersionWaitHandler(ctx context.Context, client ApiKmsClient, projectId, region, keyRingId, keyId string, version int64) *wait.AsyncActionHandler[kms.Version] {
145150
handler := wait.New(func() (bool, *kms.Version, error) {
146-
_, err := client.GetVersionExecute(ctx, projectId, region, keyRingId, keyId, version)
151+
response, err := client.GetVersionExecute(ctx, projectId, region, keyRingId, keyId, version)
147152
if err != nil {
148153
var apiErr *oapierror.GenericOpenAPIError
149154
if errors.As(err, &apiErr) {
150155
if statusCode := apiErr.StatusCode; statusCode == http.StatusNotFound || statusCode == http.StatusGone {
151-
return true, nil, nil
156+
return true, nil, fmt.Errorf("disabling failed for key %s version %d: version or key not found", keyId, version)
152157
}
153158
}
154-
return true, nil, err
159+
return false, nil, err
155160
}
161+
162+
if response != nil && response.State != nil {
163+
switch *response.State {
164+
case kms.VERSIONSTATE_DISABLED:
165+
return true, response, nil
166+
case kms.VERSIONSTATE_ACTIVE, kms.VERSIONSTATE_CREATING, kms.VERSIONSTATE_KEY_MATERIAL_UNAVAILABLE:
167+
return false, nil, nil
168+
case kms.VERSIONSTATE_DESTROYED, kms.VERSIONSTATE_KEY_MATERIAL_INVALID:
169+
return true, response, fmt.Errorf("disabling failed for key %s version %d: state %s", keyId, version, *response.State)
170+
default:
171+
return true, response, fmt.Errorf("key version %d for key %s has unexpected state %s", version, keyId, *response.State)
172+
}
173+
}
174+
156175
return false, nil, nil
157176
})
158177
handler.SetTimeout(10 * time.Minute)
@@ -166,8 +185,8 @@ func CreateWrappingKeyWaitHandler(ctx context.Context, client ApiKmsClient, proj
166185
return false, nil, err
167186
}
168187

169-
if state := response.State; state != nil {
170-
switch *state {
188+
if response != nil && response.State != nil {
189+
switch *response.State {
171190
case kms.WRAPPINGKEYSTATE_CREATING:
172191
return false, nil, nil
173192
default:

‎services/kms/wait/wait_test.go‎

Lines changed: 108 additions & 39 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
}
@@ -409,15 +409,15 @@ func TestEnableKeyVersionWaitHandler(t *testing.T) {
409409
false,
410410
},
411411
{
412-
"create failed delayed",
412+
"create failed with invalid key material",
413413
[]versionResponse{
414414
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
415415
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
416416
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
417417
{fixtureVersion(1, false, kms.VERSIONSTATE_KEY_MATERIAL_INVALID), nil},
418418
},
419419
fixtureVersion(1, false, kms.VERSIONSTATE_KEY_MATERIAL_INVALID),
420-
false,
420+
true,
421421
},
422422
{
423423
"timeout",
@@ -433,7 +433,55 @@ func TestEnableKeyVersionWaitHandler(t *testing.T) {
433433
{fixtureVersion(1, false, "bogus"), nil},
434434
},
435435
fixtureVersion(1, false, "bogus"),
436-
false,
436+
true,
437+
},
438+
{
439+
"version destroyed",
440+
[]versionResponse{
441+
{fixtureVersion(1, false, kms.VERSIONSTATE_DESTROYED), nil},
442+
},
443+
fixtureVersion(1, false, kms.VERSIONSTATE_DESTROYED),
444+
true,
445+
},
446+
{
447+
"version disabled - unexpected state",
448+
[]versionResponse{
449+
{fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED), nil},
450+
},
451+
fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED),
452+
true,
453+
},
454+
{
455+
"version key material unavailable - unexpected state",
456+
[]versionResponse{
457+
{fixtureVersion(1, false, kms.VERSIONSTATE_KEY_MATERIAL_UNAVAILABLE), nil},
458+
},
459+
fixtureVersion(1, false, kms.VERSIONSTATE_KEY_MATERIAL_UNAVAILABLE),
460+
true,
461+
},
462+
{
463+
"version not found (404)",
464+
[]versionResponse{
465+
{nil, oapierror.NewError(http.StatusNotFound, "not found")},
466+
},
467+
nil,
468+
true,
469+
},
470+
{
471+
"version gone (410)",
472+
[]versionResponse{
473+
{nil, oapierror.NewError(http.StatusGone, "gone")},
474+
},
475+
nil,
476+
true,
477+
},
478+
{
479+
"error fetching version",
480+
[]versionResponse{
481+
{nil, oapierror.NewError(http.StatusInternalServerError, "internal error")},
482+
},
483+
nil,
484+
true,
437485
},
438486
// no special update tests needed as the states are the same
439487
}
@@ -454,7 +502,7 @@ func TestEnableKeyVersionWaitHandler(t *testing.T) {
454502
}
455503

456504
if diff := cmp.Diff(tt.want, got); diff != "" {
457-
t.Errorf("differing key %s", diff)
505+
t.Errorf("differing version %s", diff)
458506
}
459507
})
460508
}
@@ -464,61 +512,84 @@ func TestDisableKeyVersionWaitHandler(t *testing.T) {
464512
tests := []struct {
465513
name string
466514
responses []versionResponse
515+
want *kms.Version
467516
wantErr bool
468517
}{
469518
{
470-
"Delete with '404' succeeded immediately",
519+
"disable succeeded immediately",
471520
[]versionResponse{
472-
{nil, oapierror.NewError(http.StatusNotFound, "not found")},
521+
{fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED), nil},
473522
},
523+
fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED),
474524
false,
475525
},
476526
{
477-
"Delete with '404' delayed",
527+
"disable succeeded delayed",
528+
[]versionResponse{
529+
{fixtureVersion(1, false, kms.VERSIONSTATE_ACTIVE), nil},
530+
{fixtureVersion(1, false, kms.VERSIONSTATE_ACTIVE), nil},
531+
{fixtureVersion(1, false, kms.VERSIONSTATE_ACTIVE), nil},
532+
{fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED), nil},
533+
},
534+
fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED),
535+
false,
536+
},
537+
{
538+
"disable succeeded from creating state",
478539
[]versionResponse{
479540
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
480541
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
481-
{fixtureVersion(1, false, kms.VERSIONSTATE_CREATING), nil},
482-
{nil, oapierror.NewError(http.StatusNotFound, "not found")},
542+
{fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED), nil},
483543
},
544+
fixtureVersion(1, true, kms.VERSIONSTATE_DISABLED),
484545
false,
485546
},
486547
{
487-
"Delete with 'gone' succeeded immediately",
548+
"version already destroyed",
488549
[]versionResponse{
489-
{nil, oapierror.NewError(http.StatusGone, "gone")},
550+
{fixtureVersion(1, false, kms.VERSIONSTATE_DESTROYED), nil},
490551
},
491-
false,
552+
fixtureVersion(1, false, kms.VERSIONSTATE_DESTROYED),
553+
true,
492554
},
493555
{
494-
"Delete with 'gone' delayed",
556+
"version key material invalid",
495557
[]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")},
558+
{fixtureVersion(1, false, kms.VERSIONSTATE_KEY_MATERIAL_INVALID), nil},
500559
},
501-
false,
560+
fixtureVersion(1, false, kms.VERSIONSTATE_KEY_MATERIAL_INVALID),
561+
true,
502562
},
503563
{
504-
"Delete with error delayed",
564+
"timeout waiting for disabled state",
505565
[]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")},
566+
{fixtureVersion(1, false, kms.VERSIONSTATE_ACTIVE), nil},
511567
},
568+
nil,
512569
true,
513570
},
514571
{
515-
"Cannot delete",
572+
"version not found (404)",
516573
[]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")},
574+
{nil, oapierror.NewError(http.StatusNotFound, "not found")},
575+
},
576+
nil,
577+
true,
578+
},
579+
{
580+
"version gone (410)",
581+
[]versionResponse{
582+
{nil, oapierror.NewError(http.StatusGone, "gone")},
583+
},
584+
nil,
585+
true,
586+
},
587+
{
588+
"error fetching version",
589+
[]versionResponse{
590+
{nil, oapierror.NewError(http.StatusInternalServerError, "internal error")},
521591
},
592+
nil,
522593
true,
523594
},
524595
}
@@ -529,18 +600,16 @@ func TestDisableKeyVersionWaitHandler(t *testing.T) {
529600
versionResponses: tt.responses,
530601
}
531602
handler := DisableKeyVersionWaitHandler(ctx, client, testProject, testRegion, testKeyRingId, testKeyId, 1)
532-
_, err := handler.SetTimeout(1 * time.Second).
603+
got, err := handler.SetTimeout(1 * time.Second).
533604
SetThrottle(250 * time.Millisecond).
534605
WaitWithContext(ctx)
535606

536-
if tt.wantErr != (err != nil) {
537-
t.Fatalf("wrong error result. want err: %v got %v", tt.wantErr, err)
607+
if (err != nil) != tt.wantErr {
608+
t.Fatalf("unexpected error response. want %v but got %v ", tt.wantErr, err)
538609
}
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-
}
610+
611+
if diff := cmp.Diff(tt.want, got); diff != "" {
612+
t.Errorf("differing version %s", diff)
544613
}
545614
})
546615
}
@@ -618,7 +687,7 @@ func TestCreateWrappingWaitHandler(t *testing.T) {
618687
}
619688

620689
if diff := cmp.Diff(tt.want, got); diff != "" {
621-
t.Errorf("differing key %s", diff)
690+
t.Errorf("differing wrapping key %s", diff)
622691
}
623692
})
624693
}

0 commit comments

Comments
 (0)