Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Goal Seeking Keyvaults #3357

Merged
merged 28 commits into from
Oct 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
9fbdebf
Add new options to CreateMode
theunrepentantgeek Sep 28, 2023
a6f79c2
Improve readability of test logs
theunrepentantgeek Sep 28, 2023
2004c47
Create test to reveal existing bug
theunrepentantgeek Sep 28, 2023
94a6794
Detect soft-deleted keyvault
theunrepentantgeek Sep 28, 2023
485c3d9
Update generated files
theunrepentantgeek Sep 28, 2023
aeeb26a
Fix bug in EnumType Transformer
theunrepentantgeek Sep 28, 2023
6729916
Update extension
theunrepentantgeek Oct 1, 2023
aabd6e6
Update recording
theunrepentantgeek Oct 1, 2023
5918560
Update test
theunrepentantgeek Oct 1, 2023
953cd07
Update test recording
theunrepentantgeek Oct 1, 2023
2345e98
Convert the value if needed
theunrepentantgeek Oct 2, 2023
820fa47
Add support for purge
theunrepentantgeek Oct 2, 2023
437a4bb
Update test for generating string values
theunrepentantgeek Oct 2, 2023
705eb7f
Fixup error
theunrepentantgeek Oct 2, 2023
e4e5cf6
Improve logging
theunrepentantgeek Oct 3, 2023
7ccb70a
Switch to using the SDK to detect and purge
theunrepentantgeek Oct 9, 2023
a4fc4b0
Fix rebase issues
theunrepentantgeek Oct 9, 2023
2b5a5da
Update based on PR feedback
theunrepentantgeek Oct 9, 2023
6aa38c8
Move imports
theunrepentantgeek Oct 11, 2023
7b76148
Tweak test headers
theunrepentantgeek Oct 11, 2023
e5878e8
Remove unused methods
theunrepentantgeek Oct 11, 2023
77ee23c
Merge branch 'main' into feature/goal-seeking-keyvaults
theunrepentantgeek Oct 11, 2023
bc1438e
Merge branch 'main' into feature/goal-seeking-keyvaults
theunrepentantgeek Oct 11, 2023
c99f538
Update asoctl dependencies
theunrepentantgeek Oct 12, 2023
eb58067
Merge branch 'main' into feature/goal-seeking-keyvaults
theunrepentantgeek Oct 12, 2023
eac9be1
Update v2/api/keyvault/customizations/vault_extensions.go
theunrepentantgeek Oct 12, 2023
2901cfd
Merge main
theunrepentantgeek Oct 12, 2023
6a96978
Merge branch 'main' into feature/goal-seeking-keyvaults
theunrepentantgeek Oct 12, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
289 changes: 289 additions & 0 deletions v2/api/keyvault/customizations/vault_extensions.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,289 @@
/*
Copyright (c) Microsoft Corporation.
Licensed under the MIT license.
*/

package customizations

import (
"context"
"net/http"
"time"

"github.com/Azure/azure-sdk-for-go/sdk/azcore"
"github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime"
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/keyvault/armkeyvault"
"github.com/go-logr/logr"
"github.com/pkg/errors"
"sigs.k8s.io/controller-runtime/pkg/conversion"

keyvault "github.com/Azure/azure-service-operator/v2/api/keyvault/v1api20210401previewstorage"
resources "github.com/Azure/azure-service-operator/v2/api/resources/v1api20200601storage"

"github.com/Azure/azure-service-operator/v2/internal/genericarmclient"
"github.com/Azure/azure-service-operator/v2/internal/reflecthelpers"
"github.com/Azure/azure-service-operator/v2/internal/resolver"
"github.com/Azure/azure-service-operator/v2/internal/util/kubeclient"
"github.com/Azure/azure-service-operator/v2/pkg/genruntime"
"github.com/Azure/azure-service-operator/v2/pkg/genruntime/extensions"
)

var _ extensions.ARMResourceModifier = &VaultExtension{}

const (
CreateMode_Default = "default"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use the code-gen enums instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

CreateMode_Recover = "recover"
CreateMode_CreateOrRecover = "createOrRecover"
CreateMode_PurgeThenCreate = "purgeThenCreate"
)

// ModifyARMResource implements extensions.ARMResourceModifier.
func (ex *VaultExtension) ModifyARMResource(
ctx context.Context,
armClient *genericarmclient.GenericClient,
armObj genruntime.ARMResource,
obj genruntime.ARMMetaObject,
kubeClient kubeclient.Client,
resolver *resolver.Resolver,
log logr.Logger,
) (genruntime.ARMResource, error) {

kv, ok := obj.(*keyvault.Vault)
if !ok {
return nil, errors.Errorf(
"Cannot run VaultExtension.ModifyARMResource() with unexpected resource type %T",
obj)
}

// Type assert that we are the hub type. This will fail to compile if
// the hub type has been changed but this extension has not been updated to match
var _ conversion.Hub = kv

// If createMode is nil, nothing for us to do
// (This shouldn't be possible, but better to hedge against it)
if kv.Spec.Properties == nil || kv.Spec.Properties.CreateMode == nil {
return armObj, nil
}

// Get the owner of the KeyVault, we need this resource group to determine the subscription
owner, ownerErr := ex.getOwner(ctx, kv, resolver, log)
if ownerErr != nil {
return nil, errors.Wrapf(ownerErr, "unable to find owner of KeyVault %s", kv.Name)
}

// Parse the ID of the owner
// (Can't use the KeyVault as we do this before the KV exists)
id, err := genruntime.GetAndParseResourceID(owner)
if err != nil {
return nil, errors.Wrap(err, "failed to get and parse resource ID from KeyVault owner")
}

vc, err := armkeyvault.NewVaultsClient(id.SubscriptionID, armClient.Creds(), armClient.ClientOptions())
if err != nil {
return nil, errors.Wrap(err, "failed to create new VaultsClient")
}

createMode := *kv.Spec.Properties.CreateMode
if createMode == CreateMode_CreateOrRecover {
createMode, err = ex.handleCreateOrRecover(ctx, kv, vc, resolver, log)
if err != nil {
return nil, errors.Wrapf(err, "error checking for existence of soft-deleted KeyVault")
}
}

if createMode == CreateMode_PurgeThenCreate {
err = ex.handlePurgeThenCreate(ctx, kv, vc, resolver, log)
if err != nil {
return nil, errors.Wrapf(err, "error purging soft-deleted KeyVault")
}

createMode = CreateMode_Default
}

// Modify the payload as necessary
spec := armObj.Spec()
err = reflecthelpers.SetProperty(spec, "Properties.CreateMode", &createMode)
if err != nil {
return nil, errors.Wrapf(err, "error setting CreateMode to %s", createMode)
}

return armObj, nil
}

func (ex *VaultExtension) handleCreateOrRecover(
ctx context.Context,
kv *keyvault.Vault,
vc *armkeyvault.VaultsClient,
resolver *resolver.Resolver,
log logr.Logger,
) (string, error) {
exists, err := ex.checkForExistenceOfDeletedKeyVault(ctx, kv, resolver, vc, log)
if err != nil {
return "", errors.Wrapf(err, "error checking for existence of soft-deleted KeyVault %s", kv.Name)
}

result := CreateMode_Default
if exists {
result = CreateMode_Recover
}

log.Info(
"KeyVault reconciliation requested CreateOrRecover",
"KeyVault", kv.Name,
theunrepentantgeek marked this conversation as resolved.
Show resolved Hide resolved
"softDeletedKeyvaultExists", exists,
"createMode", result)

return result, err
}

func (ex *VaultExtension) handlePurgeThenCreate(
ctx context.Context,
kv *keyvault.Vault,
vc *armkeyvault.VaultsClient,
resolver *resolver.Resolver,
log logr.Logger,
) 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 {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Could not determine whether a soft-deleted keyvault exists in the same subscription, assume it doesn't

log.Error(err, "error checking for existence of soft-deleted KeyVault")
return nil
}

log.Info(
"KeyVault reconciliation requested PurgeThenCreate",
"KeyVault", kv.Name,
"softDeletedKeyVaultExists", exists)

if exists {
// Get the owner of the KeyVault, we need this resource group to determine the location
owner, ownerErr := ex.getOwner(ctx, kv, resolver, log)
if ownerErr != nil {
return errors.Wrapf(ownerErr, "unable to find owner of KeyVault %s", kv.Name)
}

// if a soft-deleted KeyVault exists, we need to purge it before we can create a new one
// Get the location of the KeyVault
location, locationOk := ex.getLocation(kv, owner)
if !locationOk {
return errors.Errorf("unable to determine location of KeyVault %s", kv.Name)
}

poller, err := vc.BeginPurgeDeleted(ctx, kv.Name, location, &armkeyvault.VaultsClientBeginPurgeDeletedOptions{})
if err != nil {
return errors.Wrapf(err, "failed to begin purging deleted KeyVault %s", kv.Name)
}

_, err = poller.PollUntilDone(ctx, &runtime.PollUntilDoneOptions{Frequency: 10 * time.Second})
if err != nil {
return errors.Wrapf(err, "failed to purge deleted KeyVault %s", kv.Name)
}
}

return nil
}

// checkForExistenceOfDeletedKeyVault checks to see whether there's a soft deleted KeyVault with the same name.
// This might be true if another party has deleted the KeyVault, even if we previously created it
func (ex *VaultExtension) checkForExistenceOfDeletedKeyVault(
theunrepentantgeek marked this conversation as resolved.
Show resolved Hide resolved
ctx context.Context,
kv *keyvault.Vault,
resolver *resolver.Resolver,
vaultsClient *armkeyvault.VaultsClient,
log logr.Logger,
) (bool, error) {
// Get the owner of the KeyVault, we need this resource group to determine the subscription
owner, ownerErr := ex.getOwner(ctx, kv, resolver, log)
if ownerErr != nil {
return false, errors.Wrapf(ownerErr, "unable to find owner of KeyVault %s", kv.Name)
}

// Get the location of the KeyVault
location, locationOk := ex.getLocation(kv, owner)
if !locationOk {
return false, errors.Errorf("unable to determine location of KeyVault %s", kv.Name)
}

// Get the name of the KeyVault
vaultName := kv.Spec.AzureName
if vaultName == "" {
vaultName = kv.Name
}

// Default to assuming a soft-deleted keyvault exists
exists := true

// Check to see if this is true
_, err := vaultsClient.GetDeleted(ctx, vaultName, location, &armkeyvault.VaultsClientGetDeletedOptions{})
if err != nil {
var responseError *azcore.ResponseError
if errors.As(err, &responseError) {
if responseError.StatusCode != http.StatusNotFound {
return false, errors.Wrapf(err, "failed to get deleted KeyVault %s, error %d", kv.Name, responseError.StatusCode)
}

// KeyVault doesn't exist,
exists = false
}
}

log.Info(
"Checking for existence of soft-deleted KeyVault",
"keyVault", kv.Name,
"location", location,
"softDeletedKeyvaultExists", exists,
)

return exists, nil
}

func (*VaultExtension) getOwner(
ctx context.Context,
kv *keyvault.Vault,
resolver *resolver.Resolver,
log logr.Logger,
) (*resources.ResourceGroup, error) {
owner, err := resolver.ResolveOwner(ctx, kv)
if err != nil {
return nil, errors.Wrapf(err, "unable to resolve owner of KeyVault %s", kv.Name)
}

// No need to wait for resources that don't have an owner
if !owner.FoundKubernetesOwner() {
log.Info(
"KeyVault owner is not within the cluster, cannot determine subscription",
"keyVault", kv.Name)
return nil, errors.Errorf("owner of KeyVault %s is not within the cluster", kv.Name)
theunrepentantgeek marked this conversation as resolved.
Show resolved Hide resolved
}

rg, ok := owner.Owner.(*resources.ResourceGroup)
if !ok {
return nil, errors.Errorf("expected owner of KeyVault %s to be a ResourceGroup", kv.Name)
}

// Type assert that the ResourceGroup is the hub type. This will fail to compile if
// the hub type has been changed but this extension has not been updated to match
var _ conversion.Hub = rg

return rg, nil
}

// findKeyVaultLocation determines which location we're trying to create KeyVault within
func (*VaultExtension) getLocation(
kv *keyvault.Vault,
rg *resources.ResourceGroup,
) (string, bool) {
// Prefer location on the KeyVault
if kv.Spec.Location != nil && *kv.Spec.Location != "" {
return *kv.Spec.Location, true
}

// Fallback to location on ResourceGroup
if rg.Spec.Location != nil && *rg.Spec.Location != "" {
return *rg.Spec.Location, true
}

return "", false
}
16 changes: 12 additions & 4 deletions v2/api/keyvault/v1api20210401preview/structure.txt
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,10 @@ github.com/Azure/azure-service-operator/v2/api/keyvault/v1api20210401preview
│ │ │ │ ├── TenantId: Validated<*string> (1 rule)
│ │ │ │ │ └── Rule 0: Pattern: "^[0-9a-fA-F]{8}(-[0-9a-fA-F]{4}){3}-[0-9a-fA-F]{12}$"
│ │ │ │ └── TenantIdFromConfig: *genruntime.ConfigMapReference
│ │ │ ├── CreateMode: *Enum (2 values)
│ │ │ ├── CreateMode: *Enum (4 values)
│ │ │ │ ├── "createOrRecover"
│ │ │ │ ├── "default"
│ │ │ │ ├── "purgeThenCreate"
│ │ │ │ └── "recover"
│ │ │ ├── EnablePurgeProtection: *bool
│ │ │ ├── EnableRbacAuthorization: *bool
Expand Down Expand Up @@ -185,8 +187,10 @@ github.com/Azure/azure-service-operator/v2/api/keyvault/v1api20210401preview
│ │ │ │ ├── "setsas"
│ │ │ │ └── "update"
│ │ │ └── TenantId: *string
│ │ ├── CreateMode: *Enum (2 values)
│ │ ├── CreateMode: *Enum (4 values)
│ │ │ ├── "createOrRecover"
│ │ │ ├── "default"
│ │ │ ├── "purgeThenCreate"
│ │ │ └── "recover"
│ │ ├── EnablePurgeProtection: *bool
│ │ ├── EnableRbacAuthorization: *bool
Expand Down Expand Up @@ -326,8 +330,10 @@ github.com/Azure/azure-service-operator/v2/api/keyvault/v1api20210401preview
│ │ │ │ ├── "setsas"
│ │ │ │ └── "update"
│ │ │ └── TenantId: *string
│ │ ├── CreateMode: *Enum (2 values)
│ │ ├── CreateMode: *Enum (4 values)
│ │ │ ├── "createOrRecover"
│ │ │ ├── "default"
│ │ │ ├── "purgeThenCreate"
│ │ │ └── "recover"
│ │ ├── EnablePurgeProtection: *bool
│ │ ├── EnableRbacAuthorization: *bool
Expand Down Expand Up @@ -467,8 +473,10 @@ github.com/Azure/azure-service-operator/v2/api/keyvault/v1api20210401preview
│ │ │ ├── "setsas"
│ │ │ └── "update"
│ │ └── TenantId: *string
│ ├── CreateMode: *Enum (2 values)
│ ├── CreateMode: *Enum (4 values)
│ │ ├── "createOrRecover"
│ │ ├── "default"
│ │ ├── "purgeThenCreate"
│ │ └── "recover"
│ ├── EnablePurgeProtection: *bool
│ ├── EnableRbacAuthorization: *bool
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading