Address review feedback for MonitoringAlertPolicy direct controller

Author: codebot-robot

.gemini/skills/kcc-direct-controller-implementer/SKILL.md

@@ -43,9 +43,29 @@ This skill guides the implementation of the controller, mappers, and fuzzer for
       ```
     - **Diff Comparison & Structured Diff (`tags.DiffForTopLevelFields`)**: Always prefer using top-level field tags-based diff comparison via `tags.DiffForTopLevelFields` over recursive/magical comparison functions (such as `common.CompareProtoMessageStructuredDiff` or `common.CompareProtoMessage` which are deprecated/discouraged due to unpredictable behaviors in `BasicDiff`).
       ```go
-      diffs, updateMask, err := tags.DiffForTopLevelFields(ctx, clonedDesired.ProtoReflect(), maskedActual.ProtoReflect())
-      if err != nil {
-          return nil, nil, err
+      func compareResource(ctx context.Context, actual, desired *pb.MyResource) (*structuredreporting.Diff, *fieldmaskpb.FieldMask, error) {
+          maskedActual, err := mappers.OnlySpecFields(actual, MyResourceSpec_FromProto, MyResourceSpec_ToProto)
+          if err != nil {
+              return nil, nil, err
+          }
+          maskedActual.Name = desired.Name // Restore any non-spec identifier fields if needed
+
+          clonedDesired := proto.CloneOf(desired)
+
+          populateDefaults := func(obj *pb.MyResource) {
+              // Even if empty, it's a good pattern to define and populate GCP/server defaults here
+              if obj.SomeDefaultedField == nil {
+                  obj.SomeDefaultedField = ...
+              }
+          }
+          populateDefaults(maskedActual)
+          populateDefaults(clonedDesired)
+
+          diffs, updateMask, err := tags.DiffForTopLevelFields(ctx, clonedDesired.ProtoReflect(), maskedActual.ProtoReflect())
+          if err != nil {
+              return nil, nil, err
+          }
+          return diffs, updateMask, nil
       }
       ```
     - **Reconciling Empty or Incomplete LRO Responses**: Many GCP APIs (such as Dataproc's `UpdateCluster` LRO) return an empty response (`google.protobuf.Empty`), or do not fully populate read-only status fields (such as state, metrics, or instance names) during resource creation. If you map status directly from such incomplete/empty LRO responses, you will inadvertently clear status fields in Kubernetes.

apis/monitoring/v1beta1/monitoringalertpolicy_identity.go

@@ -19,15 +19,16 @@ import (
 	"fmt"
 	"strings"
 
+	"github.com/GoogleCloudPlatform/k8s-config-connector/apis/common"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/apis/common/identity"
 	refs "github.com/GoogleCloudPlatform/k8s-config-connector/apis/refs/v1beta1"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/gcpurls"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 var (
-	_ identity.IdentityV2 = &MonitoringAlertPolicyIdentity{}
-	_ identity.Resource   = &MonitoringAlertPolicy{}
+	_ identity.ServerGeneratedIdentity = &MonitoringAlertPolicyIdentity{}
+	_ identity.Resource                = &MonitoringAlertPolicy{}
 )
 
 var MonitoringAlertPolicyIdentityFormat = gcpurls.Template[MonitoringAlertPolicyIdentity]("monitoring.googleapis.com", "projects/{project}/alertPolicies/{alertpolicy}")
@@ -48,24 +49,58 @@ func (i *MonitoringAlertPolicyIdentity) ParentString() string {
 }
 
 func (obj *MonitoringAlertPolicy) GetIdentity(ctx context.Context, reader client.Reader) (identity.Identity, error) {
-	return getIdentityFromMonitoringAlertPolicySpec(ctx, reader, obj)
+	specIdentity, err := getIdentityFromMonitoringAlertPolicySpec(ctx, reader, obj)
+	if err != nil {
+		return nil, err
+	}
+
+	// Cross-check the identity against the status value, if present.
+	statusName := common.ValueOf(obj.Status.Name)
+	if statusName != "" {
+		statusIdentity := &MonitoringAlertPolicyIdentity{}
+		if err := statusIdentity.FromExternal(statusName); err != nil {
+			return nil, err
+		}
+
+		if specIdentity.AlertPolicy == "" {
+			specIdentity.AlertPolicy = statusIdentity.AlertPolicy
+		}
+
+		if statusIdentity.String() != specIdentity.String() {
+			return nil, fmt.Errorf("cannot change MonitoringAlertPolicy identity (old=%q, new=%q)", statusIdentity.String(), specIdentity.String())
+		}
+	}
+
+	return specIdentity, nil
 }
 
 func (i *MonitoringAlertPolicyIdentity) FromExternal(ref string) error {
+	normalized := ref
+	normalized = strings.TrimPrefix(normalized, "https:")
+	normalized = strings.TrimPrefix(normalized, "http:")
+	normalized = strings.TrimPrefix(normalized, "//")
+	normalized = strings.TrimPrefix(normalized, "monitoring.googleapis.com/")
+	normalized = strings.TrimPrefix(normalized, "v3/")
+
 	// This relative format is a very special case and unusual for a GCP API.
 	// For example, the IncidentList resource under Monitoring Dashboard explicitly
 	// asks that we do NOT pass the project in the policy name (e.g. use alertPolicies/utilization).
 	// Reference: https://docs.cloud.google.com/monitoring/api/ref_v3/rest/v1/projects.dashboards#incidentlist
-	if strings.HasPrefix(ref, "alertPolicies/") {
-		parts := strings.Split(ref, "/")
+	//
+	// NOTE: This is a design mistake in the KCC API, but we have to support it for legacy compatibility.
+	// The mistake is this: Just because the GCP API requires a particular format, that does not mean
+	// that we should mirror that format in the Ref. KCC is supposed to be consistent, regardless of
+	// the underlying GCP API.
+	if strings.HasPrefix(normalized, "alertPolicies/") {
+		parts := strings.Split(normalized, "/")
 		if len(parts) == 2 && parts[1] != "" {
 			i.Project = ""
 			i.AlertPolicy = parts[1]
 			return nil
 		}
 	}
 
-	parsed, match, err := MonitoringAlertPolicyIdentityFormat.Parse(ref)
+	parsed, match, err := MonitoringAlertPolicyIdentityFormat.Parse(normalized)
 	if err != nil {
 		return fmt.Errorf("format of MonitoringAlertPolicy external=%q was not known (use %s): %w", ref, MonitoringAlertPolicyIdentityFormat.CanonicalForm(), err)
 	}
@@ -81,11 +116,12 @@ func (i *MonitoringAlertPolicyIdentity) Host() string {
 	return MonitoringAlertPolicyIdentityFormat.Host()
 }
 
-func getIdentityFromMonitoringAlertPolicySpec(ctx context.Context, reader client.Reader, obj client.Object) (*MonitoringAlertPolicyIdentity, error) {
-	resourceID, err := refs.GetResourceID(obj)
-	if err != nil {
-		return nil, fmt.Errorf("cannot resolve resource ID: %w", err)
-	}
+func (i *MonitoringAlertPolicyIdentity) HasIdentitySpecified() bool {
+	return i.AlertPolicy != ""
+}
+
+func getIdentityFromMonitoringAlertPolicySpec(ctx context.Context, reader client.Reader, obj *MonitoringAlertPolicy) (*MonitoringAlertPolicyIdentity, error) {
+	resourceID := common.ValueOf(obj.Spec.ResourceID)
 
 	projectID, err := refs.ResolveProjectID(ctx, reader, obj)
 	if err != nil {

apis/monitoring/v1beta1/monitoringalertpolicy_reference.go

@@ -17,6 +17,7 @@ package v1beta1
 import (
 	"context"
 
+	"github.com/GoogleCloudPlatform/k8s-config-connector/apis/common"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/apis/common/identity"
 	refs "github.com/GoogleCloudPlatform/k8s-config-connector/apis/refs/v1beta1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -41,7 +42,7 @@ type MonitoringAlertPolicyRef struct {
 }
 
 func init() {
-	refs.Register(&MonitoringAlertPolicyRef{}, nil)
+	refs.Register(&MonitoringAlertPolicyRef{}, &MonitoringAlertPolicy{})
 }
 
 func (r *MonitoringAlertPolicyRef) GetGVK() schema.GroupVersionKind {
@@ -83,7 +84,11 @@ func (r *MonitoringAlertPolicyRef) ParseExternalToIdentity() (identity.Identity,
 
 func (r *MonitoringAlertPolicyRef) Normalize(ctx context.Context, reader client.Reader, defaultNamespace string) error {
 	fallback := func(u *unstructured.Unstructured) string {
-		identity, err := getIdentityFromMonitoringAlertPolicySpec(ctx, reader, u)
+		typedObj, err := common.ToStructuredType[*MonitoringAlertPolicy](u)
+		if err != nil {
+			return ""
+		}
+		identity, err := getIdentityFromMonitoringAlertPolicySpec(ctx, reader, typedObj)
 		if err != nil {
 			return ""
 		}

pkg/cais/caistesting/testing.go

@@ -108,6 +108,10 @@ func NormalizeDynamicIDs(s string) string {
 		if idx := strings.Index(line, "/budgets/"); idx != -1 {
 			lines[i] = line[:idx+len("/budgets/")] + "billingbudgetsbudget-${uniqueId}"
 		}
+		// Normalize Monitoring Alert Policy numeric IDs
+		if idx := strings.Index(line, "/alertPolicies/"); idx != -1 {
+			lines[i] = line[:idx+len("/alertPolicies/")]
+		}
 		// Normalize reCAPTCHA Enterprise Key IDs: projects/.../keys/<keyId>
 		if idx := strings.Index(line, "/keys/"); idx != -1 && strings.Contains(line, "recaptchaenterprise") {
 			lines[i] = line[:idx+len("/keys/")] + "${keyID}"

pkg/cli/cmd/export/parameters/parameters.go

@@ -44,14 +44,15 @@ type Parameters struct {
 	// DisableDirectExport can be set to true to bypass direct-reconciliation exporters.
 	DisableDirectExport bool
 
-	// GRPCUnaryClientInterceptor is the GRPC interceptor for use in tests.
+	// GRPCUnaryClientInterceptor allows for overriding the default GRPC Unary Interceptor
 	GRPCUnaryClientInterceptor grpc.UnaryClientInterceptor
 }
 
 func (p *Parameters) NewControllerConfig(ctx context.Context) (*config.ControllerConfig, error) {
 	c := &config.ControllerConfig{
-		HTTPClient: p.HTTPClient,
-		UserAgent:  gcp.KCCUserAgent(),
+		HTTPClient:                 p.HTTPClient,
+		UserAgent:                  gcp.KCCUserAgent(),
+		GRPCUnaryClientInterceptor: p.GRPCUnaryClientInterceptor,
 	}
 	if p.GCPAccessToken != "" {
 		c.GCPTokenSource = oauth2.StaticTokenSource(

pkg/cli/powertools/cais/cmd_test.go

@@ -268,10 +268,17 @@ func TestGoldenIdentitiesYamlFiles(t *testing.T) {
 						tempReader := cais.NewInMemoryReader(scheme, []*unstructured.Unstructured{u})
 						if id, err := resource.GetIdentity(ctx, tempReader); err == nil && id != nil {
 							if sgId, ok := id.(identity.ServerGeneratedIdentity); ok {
-								if !sgId.HasIdentitySpecified() && gk.Kind == "KMSKeyHandle" {
-									placeholder := "${keyHandleID}"
-									if err := unstructured.SetNestedField(u.Object, placeholder, "spec", "resourceID"); err != nil {
-										t.Fatalf("failed to set spec.resourceID for %s: %v", u.GetName(), err)
+								if !sgId.HasIdentitySpecified() {
+									if gk.Kind == "KMSKeyHandle" {
+										placeholder := "${keyHandleID}"
+										if err := unstructured.SetNestedField(u.Object, placeholder, "spec", "resourceID"); err != nil {
+											t.Fatalf("failed to set spec.resourceID for %s: %v", u.GetName(), err)
+										}
+									} else if gk.Kind == "MonitoringAlertPolicy" {
+										placeholder := "alertpolicy-placeholder"
+										if err := unstructured.SetNestedField(u.Object, placeholder, "spec", "resourceID"); err != nil {
+											t.Fatalf("failed to set spec.resourceID for %s: %v", u.GetName(), err)
+										}
 									}
 								}
 							}

pkg/controller/direct/monitoring/monitoringalertpolicy_controller.go

@@ -23,6 +23,7 @@ import (
 	pb "cloud.google.com/go/monitoring/apiv3/v2/monitoringpb"
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/types/known/fieldmaskpb"
+	"google.golang.org/protobuf/types/known/wrapperspb"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/klog/v2"
@@ -34,6 +35,8 @@ import (
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct/directbase"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct/registry"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/controller/direct/tags"
+	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/export"
+	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/label"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/mappers"
 	"github.com/GoogleCloudPlatform/k8s-config-connector/pkg/structuredreporting"
 )
@@ -84,12 +87,12 @@ func (m *alertPolicyModel) AdapterForObject(ctx context.Context, op *directbase.
 		return nil, fmt.Errorf("error converting to %T: %w", obj, err)
 	}
 
-	id, err := obj.GetIdentity(ctx, kube)
-	if err != nil {
+	if err := common.NormalizeReferences(ctx, kube, obj, nil); err != nil {
 		return nil, err
 	}
 
-	if err := common.NormalizeReferences(ctx, kube, obj, nil); err != nil {
+	id, err := obj.GetIdentity(ctx, kube)
+	if err != nil {
 		return nil, err
 	}
 
@@ -99,6 +102,8 @@ func (m *alertPolicyModel) AdapterForObject(ctx context.Context, op *directbase.
 		return nil, mapCtx.Err()
 	}
 
+	desiredProto.UserLabels = label.NewGCPLabelsFromK8sLabels(u.GetLabels())
+
 	return &alertPolicyAdapter{
 		id:                id.(*krm.MonitoringAlertPolicyIdentity),
 		desired:           desiredProto,
@@ -113,7 +118,11 @@ func (m *alertPolicyModel) AdapterForURL(ctx context.Context, url string) (direc
 	}
 
 	id := &krm.MonitoringAlertPolicyIdentity{}
-	if err := id.FromExternal(strings.TrimPrefix(url, "//monitoring.googleapis.com/")); err != nil {
+	if err := id.FromExternal(url); err != nil {
+		return nil, nil
+	}
+
+	if !id.HasIdentitySpecified() {
 		return nil, nil
 	}
 
@@ -135,7 +144,7 @@ func (m *alertPolicyModel) AdapterForURL(ctx context.Context, url string) (direc
 
 // Find implements the Adapter interface.
 func (a *alertPolicyAdapter) Find(ctx context.Context) (bool, error) {
-	if a.id.AlertPolicy == "" {
+	if !a.id.HasIdentitySpecified() {
 		return false, nil
 	}
 
@@ -157,11 +166,7 @@ func (a *alertPolicyAdapter) Find(ctx context.Context) (bool, error) {
 
 // Delete implements the Adapter interface.
 func (a *alertPolicyAdapter) Delete(ctx context.Context, deleteOp *directbase.DeleteOperation) (bool, error) {
-	exists, err := a.Find(ctx)
-	if err != nil {
-		return false, err
-	}
-	if !exists {
+	if !a.id.HasIdentitySpecified() {
 		return false, nil
 	}
 
@@ -181,6 +186,10 @@ func (a *alertPolicyAdapter) Delete(ctx context.Context, deleteOp *directbase.De
 
 // Create implements the Adapter interface.
 func (a *alertPolicyAdapter) Create(ctx context.Context, createOp *directbase.CreateOperation) error {
+	if a.id.HasIdentitySpecified() {
+		return fmt.Errorf("cannot create alert policy %q: server-generated identity is already specified", a.id.String())
+	}
+
 	u := createOp.GetUnstructured()
 
 	log := klog.FromContext(ctx)
@@ -200,11 +209,6 @@ func (a *alertPolicyAdapter) Create(ctx context.Context, createOp *directbase.Cr
 	}
 	log.V(2).Info("created alert policy", "alertPolicy", created)
 
-	resourceID := lastComponent(created.Name)
-	if err := unstructured.SetNestedField(u.Object, resourceID, "spec", "resourceID"); err != nil {
-		return fmt.Errorf("setting spec.resourceID: %w", err)
-	}
-
 	return a.updateStatus(ctx, createOp, created)
 }
 
@@ -256,6 +260,8 @@ func (a *alertPolicyAdapter) Export(ctx context.Context) (*unstructured.Unstruct
 		return nil, fmt.Errorf("error converting alertPolicy from API %w", err)
 	}
 
+	obj.Spec.ResourceID = direct.LazyPtr(a.id.AlertPolicy)
+
 	uObj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(obj)
 	if err != nil {
 		return nil, err
@@ -265,6 +271,9 @@ func (a *alertPolicyAdapter) Export(ctx context.Context) (*unstructured.Unstruct
 	u.SetName(a.id.AlertPolicy)
 	u.SetGroupVersionKind(krm.MonitoringAlertPolicyGVK)
 
+	export.SetProjectID(u, a.id.Project)
+	export.SetLabels(u, a.actual.UserLabels)
+
 	return u, nil
 }
 
@@ -287,12 +296,15 @@ func compareAlertPolicy(ctx context.Context, actual, desired *pb.AlertPolicy) (*
 		return nil, nil, err
 	}
 	maskedActual.Name = actual.Name
+	maskedActual.UserLabels = actual.UserLabels
 
 	populateDefaults := func(obj *pb.AlertPolicy) {
-		// Even if empty, it's a good pattern to define and populate GCP/server defaults here
+		if obj.Enabled == nil {
+			obj.Enabled = &wrapperspb.BoolValue{Value: true}
+		}
 	}
 
-	clonedDesired := proto.Clone(desired).(*pb.AlertPolicy)
+	clonedDesired := proto.CloneOf(desired)
 	clonedDesired.Name = actual.Name
 
 	populateDefaults(clonedDesired)

pkg/test/resourcefixture/testdata/basic/monitoring/v1beta1/monitoringalertpolicy/monitoringalertpolicy/_exported.yaml

@@ -0,0 +1,46 @@
+apiVersion: monitoring.cnrm.cloud.google.com/v1beta1
+kind: MonitoringAlertPolicy
+metadata:
+  annotations:
+    cnrm.cloud.google.com/project-id: ${projectId}
+  labels:
+    cnrm-test: "true"
+    managed-by-cnrm: "true"
+  name: ${alertPolicyId}
+spec:
+  combiner: OR
+  conditions:
+  - conditionThreshold:
+      aggregations:
+      - alignmentPeriod: 1m0s
+        crossSeriesReducer: REDUCE_MEAN
+        groupByFields:
+        - project
+        - resource.label.instance_id
+        - resource.label.zone
+        perSeriesAligner: ALIGN_MAX
+      comparison: COMPARISON_LT
+      duration: 15m0s
+      filter: metric.type="compute.googleapis.com/instance/cpu/utilization" AND resource.type="gce_instance"
+      thresholdValue: 0.1
+      trigger:
+        count: 3
+    displayName: Very low CPU usage
+  displayName: Updated Test Alert Policy
+  documentation:
+    content: |-
+      “Just the place for a Snark!” the Bellman cried,
+      As he monitored his resources with care;
+      Supporting each metric on the top of the tide
+      By a finger entwined in his hair.
+
+      “Just the place for a Snark! I have measured it twice:
+      That alone should discourage the crew.
+      Just the place for a Snark! I have measured it thrice:
+      What I measure three times is true.”
+  enabled: false
+  notificationChannels:
+  - external: projects/${projectId}/notificationChannels/${notificationChannelID}
+  - external: projects/${projectId}/notificationChannels/${notificationChannelID}
+  resourceID: ${alertPolicyId}
+  severity: ERROR