Resolving review comments

apis/kms/v1alpha1/kmsautokeyconfig_types.go

@@ -17,34 +17,26 @@ package v1alpha1
 import (
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/apis/k8s/v1alpha1"
 
-	//folderref "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct/kms/folderref"
 	refs "github.com/GoogleCloudPlatform/k8s-config-connector/apis/refs/v1beta1"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 var KMSAutokeyConfigGVK = GroupVersion.WithKind("KMSAutokeyConfig")
 
-// EDIT THIS FILE!  THIS IS SCAFFOLDING FOR YOU TO OWN!
 // NOTE: json tags are required.  Any new fields you add must have json tags for the fields to be serialized.
 
 // KMSAutokeyConfigSpec defines the desired state of KMSAutokeyConfig
 // +kcc:proto=google.cloud.kms.v1.AutokeyConfig
 type KMSAutokeyConfigSpec struct {
-	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="ResourceID field is immutable"
-	// Immutable.
-	// The KMSAutokeyConfig name. If not given, the metadata.name will be used.
-	// + optional
-	ResourceID *string `json:"resourceID,omitempty"`
+
+	// NOTE: ResourceID field is not required for AutokeyConfig as its ID has the format folders/<folderID>/autokeyConfig i.e., it doesnt have any unique ID of its own and relies on folderID for uniqueness.
 
 	// Immutable. The folder that this resource belongs to.
 	FolderRef *refs.FolderRef `json:"folderRef"`
 
 	// +optional
 	KeyProject *refs.ProjectRef `json:"keyProject,omitempty"`
-
-	// +optional
-	State *string `json:"state,omitempty"`
 }
 
 // KMSAutokeyConfigStatus defines the config connector machine state of KMSAutokeyConfig

mockgcp/mockkms/service.go

@@ -16,7 +16,6 @@ package mockkms
 
 import (
 	"context"
-	"fmt"
 	"net/http"
 	"strings"
 
@@ -65,13 +64,11 @@ func (s *MockService) NewHTTPMux(ctx context.Context, conn *grpc.ClientConn) (ht
 		//	s.operations.RegisterOperationsPath("/v1/{prefix=**}/operations/{name}"),
 	)
 	if err != nil {
-		fmt.Printf("Error occurred %v", err)
 		return nil, err
 	}
 
 	// Returns slightly non-standard errors
 	mux.RewriteError = func(ctx context.Context, error *httpmux.ErrorResponse) {
-		fmt.Printf("KMS error :%v", error)
 		if error.Code == 404 && (strings.Contains(error.Message, "KeyRing") || strings.Contains(error.Message, "CryptoKey")) {
 			error.Errors = nil
 		}

pkg/controller/direct/kms/autokeyconfig_controller.go

@@ -19,21 +19,16 @@ import (
 	"fmt"
 	"reflect"
 
-	//"reflect"
-
 	krm "github.com/GoogleCloudPlatform/k8s-config-connector/apis/kms/v1alpha1"
 	refs "github.com/GoogleCloudPlatform/k8s-config-connector/apis/refs/v1beta1"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/config"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct/directbase"
 
-	//folderref "github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct/kms/folderref"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct/registry"
 
-	// TODO(user): Update the import with the google cloud client
 	gcp "cloud.google.com/go/kms/apiv1"
 
-	// TODO(user): Update the import with the google cloud client api protobuf
 	kmspb "cloud.google.com/go/kms/apiv1/kmspb"
 	"google.golang.org/api/option"
 	"google.golang.org/protobuf/types/known/fieldmaskpb"
@@ -45,8 +40,7 @@ import (
 )
 
 const (
-	ctrlName = "kms-controller"
-	// TODO(user): Confirm service domain
+	ctrlName      = "kms-controller"
 	serviceDomain = "//cloudkms.googleapis.com"
 )
 
@@ -83,13 +77,6 @@ func (m *model) AdapterForObject(ctx context.Context, reader client.Reader, u *u
 		return nil, fmt.Errorf("error converting to %T: %w", obj, err)
 	}
 
-	resourceID := direct.ValueOf(obj.Spec.ResourceID)
-	if resourceID == "" {
-		resourceID = obj.GetName()
-	}
-	if resourceID == "" {
-		return nil, fmt.Errorf("cannot resolve resource ID")
-	}
 	var folder *refs.Folder
 	var err error
 	folder, err = refs.ResolveFolderFromAnnotation(ctx, reader, obj)
@@ -113,10 +100,6 @@ func (m *model) AdapterForObject(ctx context.Context, reader client.Reader, u *u
 			return nil, fmt.Errorf("KMSAutokeyConfig %s/%s has spec.folderRef changed, expect %s, got %s",
 				u.GetNamespace(), u.GetName(), id.Parent.FolderID, folder.FolderID)
 		}
-		if id.FullyQualifiedName() != resourceID {
-			return nil, fmt.Errorf("KMSAutokeyConfig  %s/%s has metadata.name or spec.resourceID changed, expect %s, got %s",
-				u.GetNamespace(), u.GetName(), id.FullyQualifiedName(), resourceID)
-		}
 	}
 
 	gcpClient, err := m.client(ctx)
@@ -211,7 +194,7 @@ func (a *Adapter) Update(ctx context.Context, updateOp *directbase.UpdateOperati
 
 func (a *Adapter) updateAutokeyConfig(ctx context.Context, resource *kmspb.AutokeyConfig) (*kmspb.AutokeyConfig, error) {
 	log := klog.FromContext(ctx).WithName(ctrlName)
-	// to populate a.actual
+	// To populate a.actual calling a.Find()
 	isExist, err := a.Find(ctx)
 	if !isExist {
 		return nil, fmt.Errorf("updateAutokeyConfig failed as AutokeyConfig does not exist, name: %s", a.id.FullyQualifiedName())
@@ -266,18 +249,8 @@ func (a *Adapter) Delete(ctx context.Context, deleteOp *directbase.DeleteOperati
 	log := klog.FromContext(ctx).WithName(ctrlName)
 	log.V(2).Info("deleting AutokeyConfig", "name", a.id.FullyQualifiedName())
 
-	/*req := &kmspb.DeleteAutokeyConfigRequest{Name: a.id.FullyQualifiedName()}
-	op, err := a.gcpClient.DeleteAutokeyConfig(ctx, req)
-	if err != nil {
-		return false, fmt.Errorf("deleting AutokeyConfig %s: %w", a.id.FullyQualifiedName(), err)
-	}
-	*/
 	log.V(2).Info("no-op, cannot deleted AutokeyConfig", "name", a.id.FullyQualifiedName())
 
-	/*err = op.Wait(ctx)
-	if err != nil {
-		return false, fmt.Errorf("waiting delete AutokeyConfig %s: %w", a.id.FullyQualifiedName(), err)
-	}*/
 	return true, nil
 }
 

pkg/test/resourcefixture/contexts/kms_context.go

@@ -31,12 +31,7 @@ func init() {
 	}
 	resourceContextMap["kmsautokeyconfig"] = ResourceContext{
 		ResourceKind: "KMSAutokeyConfig",
-		// TestCreateNoChangeUpdateDelete/basic-kmscryptokey: dynamic_controller_integration_test.go:149: value
-		//   mismatch for label with key 'key-one': got 'value-two', want 'value-one'
-		// TestCreateNoChangeUpdateDelete/basic-kmscryptokey: dynamic_controller_integration_test.go:282: reconcile
-		//   returned unexpected error: Delete call failed: error deleting resource: googleapi: Error 400: The request
-		//   cannot be fulfilled. Resource projects/cnrm-test-tgooo56g38yqbn3k/locations/us-central1/keyRings/kmscryptokey-i94182u4ku0un1f0nsna/cryptoKeys/kmscryptokey-i94182u4ku0un1f0nsna/cryptoKeyVersions/1
-		//   has value DESTROY_SCHEDULED in field crypto_key_version.state., failedPrecondition
+		// The AutokeyConfig resource does not support delete operation.
 		SkipDriftDetection: true,
 		SkipDelete:         true,
 	}