Goal Seeking Keyvaults #3357

theunrepentantgeek · 2023-10-02T03:50:28Z

What this PR does / why we need it:

Implements goal seeking for KeyVaults as described in our ADR.

We add two additional options to createMode:

createOrRecover - creates a new KeyVault, unless there's a soft-deleted one to recover
purgeThenCreate - any existing soft-deleted KeyVault is purged, then a new one created.

Special notes for your reviewer:

The option purgeThenCreate is dangerous because it results in ASO turning a soft-delete into a hard delete without warning and without the possibility of recovery. Our documentation will call this out.

Prerequisites

Add reflection helper SetProperty #3348

How does this PR make you feel:

If applicable:

this PR contains tests

v2/api/keyvault/customizations/vault_extensions.go

super-harsh · 2023-10-03T05:15:45Z

v2/api/keyvault/customizations/vault_extensions.go

+var _ extensions.ARMResourceModifier = &VaultExtension{}
+
+const (
+	CreateMode_Default         = "default"


Can we use the code-gen enums instead?

No, we can't.

The enums only exist in the API variants, not the storage variants, and I don't want to couple this extension to a particular API version.

super-harsh · 2023-10-03T05:28:10Z

v2/internal/testcommon/kube_per_test_context.go

 	tc.T.Log(line)
-	tc.T.Log(section)
+	tc.T.Log(msg)
 	tc.T.Log(line)
 }


I'd suggest something like below here to re-use the code:

// LogSection creates a distinctive header in the log to aid scanning func (tc *KubePerTestContext) LogSectionf(section string, args ...any) { tc.logf(subsection, "=", args) } // LogSection creates a distinctive header in the log to aid scanning func (tc *KubePerTestContext) LogSubsectionf(subsection string, args ...any) { tc.logf(subsection, "-", args) } func (tc *KubePerTestContext) logf(s string, r string, args ...any) { msg := fmt.Sprintf(s, args...) line := strings.Repeat(r, len(msg)) tc.T.Log(line) tc.T.Log(msg) tc.T.Log(line) }

v2/api/keyvault/customizations/vault_extensions.go

matthchr · 2023-10-06T23:39:00Z

v2/api/keyvault/customizations/vault_extensions.go

+	"context"
+	"strings"
+
+	keyvault "github.com/Azure/azure-service-operator/v2/api/keyvault/v1api20210401previewstorage"


minor: I think these two should go down below in the "local imports" section so that there are a total of 3 sections.

I've been deliberately separating out the named imports from the vanilla ones; still respecting the stdlib/library/local order though, so I'll move these down.

v2/api/keyvault/customizations/vault_extensions.go

v2/internal/controllers/keyvault_goalseekingcreation_test.go

matthchr · 2023-10-06T23:59:36Z

v2/internal/genericarmclient/generic_client.go

@@ -264,6 +264,38 @@ func (client *GenericClient) getByIDHandleResponse(resp *http.Response, resource
 	return nil
 }

+func (client *GenericClient) CreatePostRequest(


Fine with these being added but may not be needed if you follow the suggestion by @super-harsh to use the SDK for Go instead.

Removed, since we don't need them.

v2/internal/reflecthelpers/reflect_helpers.go

super-harsh · 2023-10-09T04:50:38Z

v2/internal/genericarmclient/generic_client.go

@@ -264,6 +264,38 @@ func (client *GenericClient) getByIDHandleResponse(resp *http.Response, resource
 	return nil
 }

+func (client *GenericClient) CreatePostRequest(


Do we still use this method?

No, not anymore.

super-harsh · 2023-10-12T02:35:40Z

v2/api/keyvault/customizations/vault_extensions.go

+) error {
+	// Find out whether a soft-deleted KeyVault with the same name exists
+	exists, err := ex.checkForExistenceOfDeletedKeyVault(ctx, kv, resolver, vc, log)
+	if err != nil {


Since KV names should be globally unique - Not sure what happens in the case where the soft-deleted KV is in another subscription and it's just the name that is not available.

Just double-checking if the user would get the name not available event/log anyway ?

If the existing KeyVault is in a different subscription, we can't recover nor purge it - and the user gets an appropriate error in the condition of the resource; this is unchanged.

v2/api/keyvault/customizations/vault_extensions.go

Co-authored-by: Harshdeep Singh (harshdsingh) <38904804+super-harsh@users.noreply.github.com>

codecov-commenter · 2023-10-12T19:35:01Z

Codecov Report

Merging #3357 (6a96978) into main (00f2427) will decrease coverage by 0.05%.
The diff coverage is 64.23%.

@@            Coverage Diff             @@
##             main    #3357      +/-   ##
==========================================
- Coverage   54.37%   54.33%   -0.05%     
==========================================
  Files        1544     1545       +1     
  Lines      649180   649311     +131     
==========================================
- Hits       353016   352820     -196     
- Misses     238722   239035     +313     
- Partials    57442    57456      +14

Files	Coverage Δ
...t/v1api20210401preview/vault_spec_arm_types_gen.go	`33.33% <ø> (ø)`
.../v1beta20210401preview/vault_spec_arm_types_gen.go	`33.33% <ø> (ø)`
v2/internal/testcommon/kube_per_test_context.go	`79.13% <100.00%> (+0.76%)`	⬆️
...ools/generator/internal/config/type_transformer.go	`74.70% <100.00%> (+0.40%)`	⬆️
v2/api/keyvault/customizations/vault_extensions.go	`57.01% <57.01%> (ø)`

... and 36 files with indirect coverage changes

theunrepentantgeek requested review from davefellows, matthchr, babbageclunk and super-harsh as code owners October 2, 2023 03:50

theunrepentantgeek self-assigned this Oct 2, 2023

theunrepentantgeek added this to the v2.4.0 milestone Oct 2, 2023

theunrepentantgeek mentioned this pull request Oct 3, 2023

Use createOrRecover for sample keyvaults #3370

Merged

2 tasks

super-harsh reviewed Oct 3, 2023

View reviewed changes

matthchr reviewed Oct 7, 2023

View reviewed changes

theunrepentantgeek added 18 commits October 9, 2023 10:34

Add new options to CreateMode

9fbdebf

Improve readability of test logs

a6f79c2

Create test to reveal existing bug

2004c47

Detect soft-deleted keyvault

94a6794

Update generated files

485c3d9

Fix bug in EnumType Transformer

aeeb26a

Update extension

6729916

Update recording

aabd6e6

Update test

5918560

Update test recording

953cd07

Convert the value if needed

2345e98

Add support for purge

820fa47

Update test for generating string values

437a4bb

Fixup error

705eb7f

Improve logging

e4e5cf6

Switch to using the SDK to detect and purge

7ccb70a

Fix rebase issues

a4fc4b0

Update based on PR feedback

2b5a5da

theunrepentantgeek force-pushed the feature/goal-seeking-keyvaults branch from b27efb2 to 2b5a5da Compare October 9, 2023 00:58

super-harsh reviewed Oct 9, 2023

View reviewed changes

theunrepentantgeek mentioned this pull request Oct 11, 2023

KeyVault Documentation #3400

Merged

3 tasks

theunrepentantgeek added 7 commits October 11, 2023 01:16

Move imports

6aa38c8

Tweak test headers

7b76148

Remove unused methods

e5878e8

Merge branch 'main' into feature/goal-seeking-keyvaults

77ee23c

Merge branch 'main' into feature/goal-seeking-keyvaults

bc1438e

Update asoctl dependencies

c99f538

Merge branch 'main' into feature/goal-seeking-keyvaults

eb58067

super-harsh reviewed Oct 12, 2023

View reviewed changes

v2/api/keyvault/customizations/vault_extensions.go Outdated Show resolved Hide resolved

super-harsh approved these changes Oct 12, 2023

View reviewed changes

theunrepentantgeek and others added 2 commits October 12, 2023 21:31

Update v2/api/keyvault/customizations/vault_extensions.go

eac9be1

Co-authored-by: Harshdeep Singh (harshdsingh) <38904804+super-harsh@users.noreply.github.com>

Merge main

2901cfd

matthchr approved these changes Oct 12, 2023

View reviewed changes

Merge branch 'main' into feature/goal-seeking-keyvaults

6a96978

theunrepentantgeek merged commit f16cd83 into main Oct 12, 2023
8 checks passed

theunrepentantgeek deleted the feature/goal-seeking-keyvaults branch October 12, 2023 20:29

theunrepentantgeek mentioned this pull request Oct 13, 2023

APIM: Api / Product / VersionSet / Policy / PolicyFragment #3394

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Goal Seeking Keyvaults #3357

Goal Seeking Keyvaults #3357

theunrepentantgeek commented Oct 2, 2023 •

edited

Loading

super-harsh Oct 3, 2023

theunrepentantgeek Oct 9, 2023

super-harsh Oct 3, 2023

theunrepentantgeek Oct 11, 2023

matthchr Oct 6, 2023

theunrepentantgeek Oct 8, 2023

theunrepentantgeek Oct 11, 2023

matthchr Oct 6, 2023

theunrepentantgeek Oct 11, 2023

super-harsh Oct 9, 2023

theunrepentantgeek Oct 11, 2023

super-harsh Oct 12, 2023

theunrepentantgeek Oct 12, 2023

codecov-commenter commented Oct 12, 2023

Goal Seeking Keyvaults #3357

Goal Seeking Keyvaults #3357

Conversation

theunrepentantgeek commented Oct 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Oct 12, 2023

Codecov Report

theunrepentantgeek commented Oct 2, 2023 •

edited

Loading