fix: correct setDeletionBillingTimeStamp trigger to set deletionTime

pkg/deploy/assets/databases-development.json

@@ -326,7 +326,7 @@
             "name": "[concat(parameters('databaseAccountName'), '/', parameters('databaseName'), '/Billing/setDeletionBillingTimeStamp')]",
             "properties": {
                 "resource": {
-                    "body": "function trigger() {\n\t\t\t\tvar request = getContext().getRequest();\n\t\t\t\tvar body = request.getBody();\n\t\t\t\tvar date = new Date();\n\t\t\t\tvar now = Math.floor(date.getTime() / 1000);\n\t\t\t\tvar billingBody = body[\"billing\"];\n\t\t\t\tif (!billingBody[\"creationTime\"]) {\n\t\t\t\t\tbillingBody[\"creationTime\"] = now;\n\t\t\t\t}\n\t\t\t\trequest.setBody(body);\n\t\t\t}",
+                    "body": "function trigger() {\n\t\t\t\tvar request = getContext().getRequest();\n\t\t\t\tvar body = request.getBody();\n\t\t\t\tvar date = new Date();\n\t\t\t\tvar now = Math.floor(date.getTime() / 1000);\n\t\t\t\tvar billingBody = body[\"billing\"];\n\t\t\t\tif (!billingBody[\"deletionTime\"]) {\n\t\t\t\t\tbillingBody[\"deletionTime\"] = now;\n\t\t\t\t}\n\t\t\t\trequest.setBody(body);\n\t\t\t}",
                     "id": "setDeletionBillingTimeStamp",
                     "triggerOperation": "Replace",
                     "triggerType": "Pre"

pkg/deploy/assets/rp-production.json

@@ -1065,7 +1065,7 @@
             "name": "[concat(parameters('databaseAccountName'), '/', 'ARO', '/Billing/setDeletionBillingTimeStamp')]",
             "properties": {
                 "resource": {
-                    "body": "function trigger() {\n\t\t\t\tvar request = getContext().getRequest();\n\t\t\t\tvar body = request.getBody();\n\t\t\t\tvar date = new Date();\n\t\t\t\tvar now = Math.floor(date.getTime() / 1000);\n\t\t\t\tvar billingBody = body[\"billing\"];\n\t\t\t\tif (!billingBody[\"creationTime\"]) {\n\t\t\t\t\tbillingBody[\"creationTime\"] = now;\n\t\t\t\t}\n\t\t\t\trequest.setBody(body);\n\t\t\t}",
+                    "body": "function trigger() {\n\t\t\t\tvar request = getContext().getRequest();\n\t\t\t\tvar body = request.getBody();\n\t\t\t\tvar date = new Date();\n\t\t\t\tvar now = Math.floor(date.getTime() / 1000);\n\t\t\t\tvar billingBody = body[\"billing\"];\n\t\t\t\tif (!billingBody[\"deletionTime\"]) {\n\t\t\t\t\tbillingBody[\"deletionTime\"] = now;\n\t\t\t\t}\n\t\t\t\trequest.setBody(body);\n\t\t\t}",
                     "id": "setDeletionBillingTimeStamp",
                     "triggerOperation": "Replace",
                     "triggerType": "Pre"

pkg/deploy/generator/const.go

@@ -33,8 +33,44 @@ const (
 	tagKeyExemptPublicBlob   = "Az.Sec.AnonymousBlobAccessEnforcement::Skip"
 	tagValueExemptPublicBlob = "PublicRelease"
 
-	renewLeaseTriggerFunction                  = "function trigger() {\n\t\t\t\tvar request = getContext().getRequest();\n\t\t\t\tvar body = request.getBody();\n\t\t\t\tvar date = new Date();\n\t\t\t\tbody[\"leaseExpires\"] = Math.floor(date.getTime() / 1000) + 60;\n\t\t\t\trequest.setBody(body);\n\t\t\t}"
-	retryLaterTriggerFunction                  = "function trigger() {\n\t\t\t\tvar request = getContext().getRequest();\n\t\t\t\tvar body = request.getBody();\n\t\t\t\tvar date = new Date();\n\t\t\t\tbody[\"leaseExpires\"] = Math.floor(date.getTime() / 1000) + 600;\n\t\t\t\trequest.setBody(body);\n\t\t\t}"
-	setCreationBillingTimeStampTriggerFunction = "function trigger() {\n\t\t\t\tvar request = getContext().getRequest();\n\t\t\t\tvar body = request.getBody();\n\t\t\t\tvar date = new Date();\n\t\t\t\tvar now = Math.floor(date.getTime() / 1000);\n\t\t\t\tvar billingBody = body[\"billing\"];\n\t\t\t\tif (!billingBody[\"creationTime\"]) {\n\t\t\t\t\tbillingBody[\"creationTime\"] = now;\n\t\t\t\t}\n\t\t\t\trequest.setBody(body);\n\t\t\t}"
-	setDeletionBillingTimeStampTriggerFunction = "function trigger() {\n\t\t\t\tvar request = getContext().getRequest();\n\t\t\t\tvar body = request.getBody();\n\t\t\t\tvar date = new Date();\n\t\t\t\tvar now = Math.floor(date.getTime() / 1000);\n\t\t\t\tvar billingBody = body[\"billing\"];\n\t\t\t\tif (!billingBody[\"creationTime\"]) {\n\t\t\t\t\tbillingBody[\"creationTime\"] = now;\n\t\t\t\t}\n\t\t\t\trequest.setBody(body);\n\t\t\t}"
+	// CosmosDB Trigger Functions
+	renewLeaseTriggerFunction = `function trigger() {
+				var request = getContext().getRequest();
+				var body = request.getBody();
+				var date = new Date();
+				body["leaseExpires"] = Math.floor(date.getTime() / 1000) + 60;
+				request.setBody(body);
+			}`
+
+	retryLaterTriggerFunction = `function trigger() {
+				var request = getContext().getRequest();
+				var body = request.getBody();
+				var date = new Date();
+				body["leaseExpires"] = Math.floor(date.getTime() / 1000) + 600;
+				request.setBody(body);
+			}`
+
+	setCreationBillingTimeStampTriggerFunction = `function trigger() {
+				var request = getContext().getRequest();
+				var body = request.getBody();
+				var date = new Date();
+				var now = Math.floor(date.getTime() / 1000);
+				var billingBody = body["billing"];
+				if (!billingBody["creationTime"]) {
+					billingBody["creationTime"] = now;
+				}
+				request.setBody(body);
+			}`
+
+	setDeletionBillingTimeStampTriggerFunction = `function trigger() {
+				var request = getContext().getRequest();
+				var body = request.getBody();
+				var date = new Date();
+				var now = Math.floor(date.getTime() / 1000);
+				var billingBody = body["billing"];
+				if (!billingBody["deletionTime"]) {
+					billingBody["deletionTime"] = now;
+				}
+				request.setBody(body);
+			}`
 )

pkg/operator/controllers/subnets/subnet_controller_test.go

@@ -6,15 +6,17 @@ package subnets
 import (
 	"context"
 	"net/http"
-	"reflect"
 	"strconv"
 	"testing"
 
+	. "github.com/onsi/gomega"
+
 	"github.com/sirupsen/logrus"
 	"go.uber.org/mock/gomock"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	armnetwork "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v6"
@@ -47,6 +49,9 @@ var (
 	subnetResourceIdMaster        = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameMaster
 	subnetResourceIdWorker        = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameWorker
 	subnetResourceIdWorkerInvalid = "/subscriptions/" + subscriptionId + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/virtualNetworks/" + vnetName + "/subnets/" + subnetNameWorker + "-invalid"
+
+	dummyAnnotationKey   = "dummy-key"
+	dummyAnnotationValue = "dummy-value"
 )
 
 func getValidClusterInstance(operatorFlagEnabled bool, operatorFlagNSG bool, operatorFlagServiceEndpoint bool) *arov1alpha1.Cluster {
@@ -123,6 +128,9 @@ func TestReconcileManager(t *testing.T) {
 				subnetObjectWorker.Properties.NetworkSecurityGroup.ID = pointerutils.ToPtr(nsgv1NodeResourceId)
 				mock.EXPECT().Get(gomock.Any(), vnetResourceGroup, vnetName, subnetNameWorker, nil).Return(armnetwork.SubnetsClientGetResponse{Subnet: *subnetObjectWorker}, nil).MaxTimes(2)
 			},
+			instance: func(instance *arov1alpha1.Cluster) {
+				instance.Spec.ArchitectureVersion = int(api.ArchitectureVersionV1)
+			},
 		},
 		{
 			name:                        "Architecture V1 - no change",
@@ -148,6 +156,9 @@ func TestReconcileManager(t *testing.T) {
 				subnetObjectWorker.Properties.NetworkSecurityGroup.ID = pointerutils.ToPtr(nsgv1NodeResourceId)
 				mock.EXPECT().Get(gomock.Any(), vnetResourceGroup, vnetName, subnetNameWorker, nil).Return(armnetwork.SubnetsClientGetResponse{Subnet: *subnetObjectWorker}, nil).MaxTimes(2)
 			},
+			instance: func(instance *arov1alpha1.Cluster) {
+				instance.Spec.ArchitectureVersion = int(api.ArchitectureVersionV1)
+			},
 		},
 		{
 			name:                        "Architecture V1 - all fixup",
@@ -183,6 +194,10 @@ func TestReconcileManager(t *testing.T) {
 				subnetObjectWorkerUpdate.Properties.NetworkSecurityGroup.ID = pointerutils.ToPtr(nsgv1NodeResourceId)
 				mock.EXPECT().CreateOrUpdateAndWait(gomock.Any(), vnetResourceGroup, vnetName, subnetNameWorker, *subnetObjectWorkerUpdate, nil).Return(nil)
 			},
+			instance: func(instance *arov1alpha1.Cluster) {
+				instance.Spec.ArchitectureVersion = int(api.ArchitectureVersionV1)
+				instance.SetAnnotations(map[string]string{dummyAnnotationKey: dummyAnnotationValue})
+			},
 		},
 		{
 			name:                        "Architecture V1 - skips invalid/not found subnets",
@@ -229,6 +244,10 @@ func TestReconcileManager(t *testing.T) {
 				mock.EXPECT().Get(gomock.Any(), vnetResourceGroup, vnetName, subnetNameWorkerInvalid, nil).Return(armnetwork.SubnetsClientGetResponse{Subnet: *subnetObjectWorkerUpdate}, notFoundErr).AnyTimes()
 				mock.EXPECT().CreateOrUpdateAndWait(gomock.Any(), vnetResourceGroup, vnetName, subnetNameWorkerInvalid, nil, gomock.Any()).Times(0)
 			},
+			instance: func(instance *arov1alpha1.Cluster) {
+				instance.Spec.ArchitectureVersion = int(api.ArchitectureVersionV1)
+				instance.SetAnnotations(map[string]string{dummyAnnotationKey: dummyAnnotationValue})
+			},
 		},
 		{
 			name:                        "Architecture V1 - node only fixup",
@@ -259,6 +278,10 @@ func TestReconcileManager(t *testing.T) {
 				subnetObjectWorkerUpdate.Properties.NetworkSecurityGroup.ID = pointerutils.ToPtr(nsgv1NodeResourceId)
 				mock.EXPECT().CreateOrUpdateAndWait(gomock.Any(), vnetResourceGroup, vnetName, subnetNameWorker, *subnetObjectWorkerUpdate, nil).Return(nil)
 			},
+			instance: func(instance *arov1alpha1.Cluster) {
+				instance.Spec.ArchitectureVersion = int(api.ArchitectureVersionV1)
+				instance.SetAnnotations(map[string]string{dummyAnnotationKey: dummyAnnotationValue})
+			},
 		},
 		{
 			name:                        "Architecture V2 - no fixups",
@@ -288,6 +311,7 @@ func TestReconcileManager(t *testing.T) {
 			},
 			instance: func(instance *arov1alpha1.Cluster) {
 				instance.Spec.ArchitectureVersion = int(api.ArchitectureVersionV2)
+				instance.SetAnnotations(map[string]string{dummyAnnotationKey: dummyAnnotationValue})
 			},
 		},
 		{
@@ -326,6 +350,7 @@ func TestReconcileManager(t *testing.T) {
 			},
 			instance: func(instance *arov1alpha1.Cluster) {
 				instance.Spec.ArchitectureVersion = int(api.ArchitectureVersionV2)
+				instance.SetAnnotations(map[string]string{dummyAnnotationKey: dummyAnnotationValue})
 			},
 		},
 		{
@@ -375,6 +400,7 @@ func TestReconcileManager(t *testing.T) {
 			},
 			instance: func(instance *arov1alpha1.Cluster) {
 				instance.Spec.ArchitectureVersion = int(api.ArchitectureVersionV2)
+				instance.SetAnnotations(map[string]string{dummyAnnotationKey: dummyAnnotationValue})
 			},
 		},
 		{
@@ -500,10 +526,12 @@ func TestReconcileManager(t *testing.T) {
 			},
 			instance: func(instance *arov1alpha1.Cluster) {
 				instance.Spec.ArchitectureVersion = int(api.ArchitectureVersionV2)
+				instance.SetAnnotations(map[string]string{dummyAnnotationKey: dummyAnnotationValue})
 			},
 		},
 	} {
 		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
 			controller := gomock.NewController(t)
 			defer controller.Finish()
 
@@ -528,7 +556,6 @@ func TestReconcileManager(t *testing.T) {
 				kubeSubnets:    kubeSubnets,
 			}
 
-			instanceCopy := *r.instance
 			err := r.reconcileSubnets(context.Background())
 			if err != nil {
 				if tt.wantErr == nil {
@@ -538,9 +565,11 @@ func TestReconcileManager(t *testing.T) {
 					t.Errorf("Expected Error %s, got %s when processing %s testcase", tt.wantErr.Error(), err.Error(), tt.name)
 				}
 			}
-
-			if tt.wantAnnotationsUpdated && reflect.DeepEqual(instanceCopy, *r.instance) {
-				t.Errorf("Expected annotations to be updated")
+			if tt.wantAnnotationsUpdated {
+				updatedCluster := &arov1alpha1.Cluster{}
+				g.Expect(clientFake.Get(context.Background(), client.ObjectKeyFromObject(instance), updatedCluster)).To(Succeed())
+				g.Expect(AnnotationTimestamp).Should(BeKeyOf(updatedCluster.Annotations))
+				g.Expect(dummyAnnotationKey).Should(BeKeyOf(updatedCluster.Annotations))
 			}
 		})
 	}

pkg/operator/controllers/subnets/subnet_nsg.go

@@ -5,22 +5,33 @@ package subnets
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"strings"
 	"time"
 
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/util/retry"
+
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
 	armnetwork "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/network/armnetwork/v6"
 
 	"github.com/Azure/ARO-RP/pkg/api"
 	"github.com/Azure/ARO-RP/pkg/api/util/subnet"
+	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
 	"github.com/Azure/ARO-RP/pkg/util/azureerrors"
 )
 
 const (
 	AnnotationTimestamp = "aro.openshift.io/lastSubnetReconcileTimestamp"
 )
 
+// ensureSubnetNSG verifies the subnet has the correct Network Security Group assigned.
+// If the NSG is missing or incorrect, it updates the subnet with the correct NSG
+// and records the reconciliation timestamp on the Cluster resource.
 func (r *reconcileManager) ensureSubnetNSG(ctx context.Context, s subnet.Subnet) error {
 	architectureVersion := api.ArchitectureVersion(r.instance.Spec.ArchitectureVersion)
 
@@ -68,10 +79,27 @@ func (r *reconcileManager) ensureSubnetNSG(ctx context.Context, s subnet.Subnet)
 	return r.updateReconcileSubnetAnnotation(ctx)
 }
 
+// updateReconcileSubnetAnnotation updates the Cluster resource with a timestamp annotation
+// indicating when the last subnet reconciliation occurred. It uses retry-on-conflict to
+// handle concurrent modifications to the Cluster resource.
 func (r *reconcileManager) updateReconcileSubnetAnnotation(ctx context.Context) error {
-	if r.instance.Annotations == nil {
-		r.instance.Annotations = make(map[string]string)
-	}
-	r.instance.Annotations[AnnotationTimestamp] = time.Now().Format(time.RFC1123)
-	return r.client.Update(ctx, r.instance)
+	return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		cluster := &arov1alpha1.Cluster{}
+		if err := r.client.Get(ctx, types.NamespacedName{Name: arov1alpha1.SingletonClusterName}, cluster); err != nil {
+			return err
+		}
+
+		if cluster.Annotations == nil {
+			cluster.Annotations = make(map[string]string)
+		}
+		cluster.Annotations[AnnotationTimestamp] = time.Now().Format(time.RFC1123)
+
+		patchPayload := &metav1.PartialObjectMetadata{
+			ObjectMeta: metav1.ObjectMeta{
+				Annotations: cluster.Annotations,
+			},
+		}
+		payloadBytes, _ := json.Marshal(patchPayload)
+		return r.client.Patch(ctx, cluster, client.RawPatch(types.MergePatchType, payloadBytes))
+	})
 }

test/e2e/operator.go

@@ -417,11 +417,16 @@ var _ = Describe("ARO Operator - Azure Subnet Reconciler", func() {
 				s, err := clients.Subnet.Get(ctx, resourceGroup, vnetName, subnet, nil)
 				g.Expect(err).NotTo(HaveOccurred())
 				g.Expect(*s.Properties.NetworkSecurityGroup.ID).To(Equal(*correctNSG))
+			}).WithContext(ctx).WithTimeout(DefaultEventuallyTimeout).Should(Succeed())
 
+			By("checking that the cluster document annotations have been patched")
+			Eventually(func(g Gomega, ctx context.Context) {
 				co, err := clients.AROClusters.AroV1alpha1().Clusters().Get(ctx, "cluster", metav1.GetOptions{})
-				g.Expect(err).NotTo(HaveOccurred())
-				g.Expect(co.Annotations).To(Satisfy(subnetReconciliationAnnotationExists))
-			}).WithContext(ctx).WithTimeout(DefaultEventuallyTimeout).Should(Succeed())
+				Expect(err).NotTo(HaveOccurred())
+				Expect(co.Annotations).To(Satisfy(subnetReconciliationAnnotationExists))
+				// Using 2 seconds because the cluster doc should be patched very quickly after the subnet itself is patched.
+				// The small eventually avoids any unfortunate timing
+			}).WithContext(ctx).WithTimeout(time.Second * 2).Should(Succeed())
 		}
 	})
 })