Repository: Azure/ARO-RP
Pull request #2343 (2022-08-23 03:21:41)

Fix OpenShiftCcluster PUTs causing static validation errors due to ARM metadata
### Which issue this PR addresses:
Fixes a bug in production.

### What this PR does / why we need it:

When a PUT is done, copy over the SystemData so that it does not fail static validation. This would cause PUTs to fail on API v2022-04-01 and above.

### Test plan for issue:

Has unit tests.

### Is there any documentation that needs to be updated for this PR?
No, bug fix



Patch:
diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS
index c33a4b88d..f8065f19a 100644
--- a/.github/CODEOWNERS
+++ b/.github/CODEOWNERS
@@ -1,1 +1,1 @@
-* @jewzaam @m1kola @bennerv @hawkowl @rogbas @petrkotas @ross-bryan @darthhexx
+* @jewzaam @m1kola @bennerv @hawkowl @rogbas @petrkotas @ross-bryan @darthhexx @jharrington22 @cblecker
diff --git a/pkg/frontend/openshiftcluster_putorpatch.go b/pkg/frontend/openshiftcluster_putorpatch.go
index b9d0f9848..bc0291e90 100644
--- a/pkg/frontend/openshiftcluster_putorpatch.go
+++ b/pkg/frontend/openshiftcluster_putorpatch.go
@@ -138,6 +138,7 @@ func (f *frontend) _putOrPatchOpenShiftCluster(ctx context.Context, log *logrus.
 					ClientSecret: doc.OpenShiftCluster.Properties.ServicePrincipalProfile.ClientSecret,
 				},
 			},
+			SystemData: doc.OpenShiftCluster.SystemData,
 		})
 
 		// In case of PATCH we take current cluster document, which is enriched
diff --git a/pkg/util/uuid/fake/fake_uuid.go b/pkg/util/uuid/fake/fake_uuid.go
new file mode 100644
index 000000000..7a581a624
--- /dev/null
+++ b/pkg/util/uuid/fake/fake_uuid.go
@@ -0,0 +1,31 @@
+package fake
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"github.com/Azure/ARO-RP/pkg/util/uuid"
+)
+
+type fakeGenerator struct {
+	words      []string
+	currentPos int
+}
+
+func NewGenerator(predefinedWords []string) uuid.Generator {
+	return &fakeGenerator{
+		words: predefinedWords,
+	}
+}
+
+func (f *fakeGenerator) movePos() {
+	f.currentPos++
+}
+
+func (f fakeGenerator) Generate() string {
+	defer f.movePos()
+	if len(f.words) < f.currentPos {
+		return ""
+	}
+	return f.words[f.currentPos]
+}


Test patch:
diff --git a/.pipelines/e2e.yml b/.pipelines/e2e.yml
index ca4d1797f..0a6cb5963 100644
--- a/.pipelines/e2e.yml
+++ b/.pipelines/e2e.yml
@@ -43,7 +43,9 @@ jobs:
 
       set -x
       . ./hack/e2e/run-rp-and-e2e.sh
-      trap 'set +e; kill_rp; clean_e2e_db' EXIT
+      trap 'set +e; kill_rp; clean_e2e_db; kill_vpn' EXIT
+
+      run_vpn
 
       deploy_e2e_db
 
diff --git a/hack/e2e/run-rp-and-e2e.sh b/hack/e2e/run-rp-and-e2e.sh
index 44d6c8de6..e6e3cdfe5 100755
--- a/hack/e2e/run-rp-and-e2e.sh
+++ b/hack/e2e/run-rp-and-e2e.sh
@@ -37,6 +37,15 @@ kill_rp(){
     wait $rppid
 }
 
+run_vpn() {
+    sudo openvpn --config secrets/$VPN --daemon --writepid vpnpid
+    sleep 10
+}
+
+kill_vpn() {
+    while read pid; do sudo kill $pid; done < vpnpid
+}
+
 deploy_e2e_db() {
     echo "########## 📦 Creating new DB $DATABASE_NAME in $DATABASE_ACCOUNT_NAME ##########"
 
diff --git a/pkg/frontend/openshiftcluster_putorpatch_test.go b/pkg/frontend/openshiftcluster_putorpatch_test.go
index 2f7069b1f..1cccd40e7 100644
--- a/pkg/frontend/openshiftcluster_putorpatch_test.go
+++ b/pkg/frontend/openshiftcluster_putorpatch_test.go
@@ -5,6 +5,7 @@ package frontend
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 	"net/http"
 	"reflect"
@@ -17,6 +18,7 @@ import (
 	"github.com/Azure/ARO-RP/pkg/api"
 	"github.com/Azure/ARO-RP/pkg/api/admin"
 	v20200430 "github.com/Azure/ARO-RP/pkg/api/v20200430"
+	v20220401 "github.com/Azure/ARO-RP/pkg/api/v20220401"
 	"github.com/Azure/ARO-RP/pkg/metrics"
 	"github.com/Azure/ARO-RP/pkg/metrics/noop"
 	"github.com/Azure/ARO-RP/pkg/proxy"
@@ -1463,6 +1465,318 @@ func TestPutOrPatchOpenShiftCluster(t *testing.T) {
 	}
 }
 
+func TestPutOrPatchOpenShiftClusterValidated(t *testing.T) {
+	ctx := context.Background()
+
+	mockSubID := "00000000-0000-0000-0000-000000000000"
+	mockCurrentTime := time.Date(1970, 1, 1, 0, 0, 0, 0, time.UTC)
+
+	createTime := time.Unix(199, 0)
+	lastModifyTime := time.Unix(299, 0)
+	newLastModifyTime := time.Unix(3000, 0)
+
+	type test struct {
+		name                   string
+		request                func() *v20220401.OpenShiftCluster
+		systemData             *api.SystemData
+		isPatch                bool
+		fixture                func(*testdatabase.Fixture)
+		wantEnriched           []string
+		wantSystemDataEnriched bool
+		wantDocuments          func(*testdatabase.Checker)
+		wantStatusCode         int
+		wantResponse           *v20220401.OpenShiftCluster
+		wantAsync              bool
+		wantError              string
+	}
+
+	for _, tt := range []*test{
+		{
+			name: "PUT a cluster from succeeded does not change SystemData",
+			request: func() *v20220401.OpenShiftCluster {
+				return &v20220401.OpenShiftCluster{
+					ID:       testdatabase.GetResourcePath(mockSubID, "resourceName"),
+					Name:     "resourceName",
+					Type:     "Microsoft.RedHatOpenShift/openShiftClusters",
+					Tags:     map[string]string{"tag": "tag"},
+					Location: "eastus",
+					Properties: v20220401.OpenShiftClusterProperties{
+						ClusterProfile: v20220401.ClusterProfile{
+							Domain:               "example.aroapp.io",
+							ResourceGroupID:      fmt.Sprintf("/subscriptions/%s/resourcegroups/clusterResourceGroup", mockSubID),
+							FipsValidatedModules: v20220401.FipsValidatedModulesDisabled,
+						},
+						MasterProfile: v20220401.MasterProfile{
+							EncryptionAtHost: v20220401.EncryptionAtHostDisabled,
+							VMSize:           v20220401.VMSize("Standard_D32s_v3"),
+							SubnetID:         fmt.Sprintf("/subscriptions/%s/resourcegroups/network/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/master", mockSubID),
+						},
+						ServicePrincipalProfile: v20220401.ServicePrincipalProfile{
+							ClientID:     "00000000-0000-0000-1111-000000000000",
+							ClientSecret: "00000000-0000-0000-0000-000000000000",
+						},
+						NetworkProfile: v20220401.NetworkProfile{
+							PodCIDR:     "10.0.0.0/16",
+							ServiceCIDR: "10.1.0.0/16",
+						},
+						APIServerProfile: v20220401.APIServerProfile{
+							Visibility: v20220401.VisibilityPrivate,
+						},
+					},
+				}
+			},
+			systemData: &api.SystemData{
+				LastModifiedBy:     "OtherUser",
+				LastModifiedByType: api.CreatedByTypeApplication,
+				LastModifiedAt:     &newLastModifyTime,
+			},
+			isPatch: false,
+			fixture: func(f *testdatabase.Fixture) {
+				f.AddSubscriptionDocuments(&api.SubscriptionDocument{
+					ID: mockSubID,
+					Subscription: &api.Subscription{
+						State: api.SubscriptionStateRegistered,
+						Properties: &api.SubscriptionProperties{
+							TenantID: "11111111-1111-1111-1111-111111111111",
+						},
+					},
+				})
+				f.AddOpenShiftClusterDocuments(&api.OpenShiftClusterDocument{
+					Key: strings.ToLower(testdatabase.GetResourcePath(mockSubID, "resourceName")),
+					OpenShiftCluster: &api.OpenShiftCluster{
+						ID:       testdatabase.GetResourcePath(mockSubID, "resourceName"),
+						Name:     "resourceName",
+						Type:     "Microsoft.RedHatOpenShift/openShiftClusters",
+						Location: "eastus",
+						Tags:     map[string]string{"tag": "will-not-be-kept"},
+						Properties: api.OpenShiftClusterProperties{
+							ProvisioningState: api.ProvisioningStateSucceeded,
+							ClusterProfile: api.ClusterProfile{
+								Domain:               "example.aroapp.io",
+								ResourceGroupID:      fmt.Sprintf("/subscriptions/%s/resourcegroups/clusterResourceGroup", mockSubID),
+								FipsValidatedModules: api.FipsValidatedModulesDisabled,
+							},
+							MasterProfile: api.MasterProfile{
+								EncryptionAtHost: api.EncryptionAtHostDisabled,
+								VMSize:           api.VMSize("Standard_D32s_v3"),
+								SubnetID:         fmt.Sprintf("/subscriptions/%s/resourcegroups/network/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/master", mockSubID),
+							},
+							ServicePrincipalProfile: api.ServicePrincipalProfile{
+								ClientID:     "00000000-0000-0000-1111-000000000000",
+								ClientSecret: "00000000-0000-0000-0000-000000000000",
+							},
+							NetworkProfile: api.NetworkProfile{
+								PodCIDR:     "10.0.0.0/16",
+								ServiceCIDR: "10.1.0.0/16",
+							},
+							APIServerProfile: api.APIServerProfile{
+								Visibility: api.VisibilityPrivate,
+							},
+							OperatorFlags: api.OperatorFlags{},
+						},
+						SystemData: api.SystemData{
+							CreatedBy:          "ExampleUser",
+							CreatedByType:      api.CreatedByTypeApplication,
+							CreatedAt:          &createTime,
+							LastModifiedBy:     "ExampleUser",
+							LastModifiedByType: api.CreatedByTypeApplication,
+							LastModifiedAt:     &lastModifyTime,
+						},
+					},
+				})
+			},
+			wantSystemDataEnriched: true,
+			wantDocuments: func(c *testdatabase.Checker) {
+				c.AddAsyncOperationDocuments(&api.AsyncOperationDocument{
+					OpenShiftClusterKey: strings.ToLower(testdatabase.GetResourcePath(mockSubID, "resourceName")),
+					AsyncOperation: &api.AsyncOperation{
+						InitialProvisioningState: api.ProvisioningStateUpdating,
+						ProvisioningState:        api.ProvisioningStateUpdating,
+					},
+				})
+				c.AddOpenShiftClusterDocuments(&api.OpenShiftClusterDocument{
+					Key: strings.ToLower(testdatabase.GetResourcePath(mockSubID, "resourceName")),
+					OpenShiftCluster: &api.OpenShiftCluster{
+						ID:       testdatabase.GetResourcePath(mockSubID, "resourceName"),
+						Name:     "resourceName",
+						Type:     "Microsoft.RedHatOpenShift/openShiftClusters",
+						Location: "eastus",
+						Tags:     map[string]string{"tag": "tag"},
+						Properties: api.OpenShiftClusterProperties{
+							ProvisioningState:     api.ProvisioningStateUpdating,
+							LastProvisioningState: api.ProvisioningStateSucceeded,
+							ClusterProfile: api.ClusterProfile{
+								Domain:               "example.aroapp.io",
+								ResourceGroupID:      fmt.Sprintf("/subscriptions/%s/resourcegroups/clusterResourceGroup", mockSubID),
+								FipsValidatedModules: api.FipsValidatedModulesDisabled,
+							},
+							MasterProfile: api.MasterProfile{
+								EncryptionAtHost: api.EncryptionAtHostDisabled,
+								VMSize:           api.VMSize("Standard_D32s_v3"),
+								SubnetID:         fmt.Sprintf("/subscriptions/%s/resourcegroups/network/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/master", mockSubID),
+							},
+							ServicePrincipalProfile: api.ServicePrincipalProfile{
+								ClientID:     "00000000-0000-0000-1111-000000000000",
+								ClientSecret: "00000000-0000-0000-0000-000000000000",
+							},
+							NetworkProfile: api.NetworkProfile{
+								PodCIDR:                "10.0.0.0/16",
+								ServiceCIDR:            "10.1.0.0/16",
+								SoftwareDefinedNetwork: api.SoftwareDefinedNetworkOpenShiftSDN,
+							},
+							APIServerProfile: api.APIServerProfile{
+								Visibility: api.VisibilityPrivate,
+							},
+							OperatorFlags: api.OperatorFlags{},
+						},
+						SystemData: api.SystemData{
+							CreatedBy:          "ExampleUser",
+							CreatedByType:      api.CreatedByTypeApplication,
+							CreatedAt:          &createTime,
+							LastModifiedBy:     "OtherUser",
+							LastModifiedByType: api.CreatedByTypeApplication,
+							LastModifiedAt:     &newLastModifyTime,
+						},
+					},
+				})
+			},
+			wantEnriched:   []string{testdatabase.GetResourcePath(mockSubID, "resourceName")},
+			wantAsync:      true,
+			wantStatusCode: http.StatusOK,
+			wantResponse: &v20220401.OpenShiftCluster{
+				ID:       testdatabase.GetResourcePath(mockSubID, "resourceName"),
+				Name:     "resourceName",
+				Type:     "Microsoft.RedHatOpenShift/openShiftClusters",
+				Tags:     map[string]string{"tag": "tag"},
+				Location: "eastus",
+				Properties: v20220401.OpenShiftClusterProperties{
+					ProvisioningState: v20220401.ProvisioningStateUpdating,
+					ClusterProfile: v20220401.ClusterProfile{
+						Domain:               "example.aroapp.io",
+						ResourceGroupID:      fmt.Sprintf("/subscriptions/%s/resourcegroups/clusterResourceGroup", mockSubID),
+						FipsValidatedModules: v20220401.FipsValidatedModulesDisabled,
+					},
+					MasterProfile: v20220401.MasterProfile{
+						EncryptionAtHost: v20220401.EncryptionAtHostDisabled,
+						VMSize:           v20220401.VMSize("Standard_D32s_v3"),
+						SubnetID:         fmt.Sprintf("/subscriptions/%s/resourcegroups/network/providers/Microsoft.Network/virtualNetworks/test-vnet/subnets/master", mockSubID),
+					},
+					ServicePrincipalProfile: v20220401.ServicePrincipalProfile{
+						ClientID: "00000000-0000-0000-1111-000000000000",
+					},
+					NetworkProfile: v20220401.NetworkProfile{
+						PodCIDR:     "10.0.0.0/16",
+						ServiceCIDR: "10.1.0.0/16",
+					},
+					APIServerProfile: v20220401.APIServerProfile{
+						Visibility: v20220401.VisibilityPrivate,
+					},
+				},
+				SystemData: &v20220401.SystemData{
+					CreatedBy:          "ExampleUser",
+					CreatedByType:      v20220401.CreatedByTypeApplication,
+					CreatedAt:          &createTime,
+					LastModifiedBy:     "OtherUser",
+					LastModifiedByType: v20220401.CreatedByTypeApplication,
+					LastModifiedAt:     &newLastModifyTime,
+				},
+			},
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			ti := newTestInfra(t).
+				WithOpenShiftClusters().
+				WithSubscriptions().
+				WithAsyncOperations()
+			defer ti.done()
+
+			err := ti.buildFixtures(tt.fixture)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			f, err := NewFrontend(ctx, ti.audit, ti.log, ti.env, ti.asyncOperationsDatabase, ti.openShiftClustersDatabase, ti.subscriptionsDatabase, api.APIs, &noop.Noop{}, nil, nil, nil, func(log *logrus.Entry, dialer proxy.Dialer, m metrics.Emitter) clusterdata.OpenShiftClusterEnricher {
+				return ti.enricher
+			})
+			if err != nil {
+				t.Fatal(err)
+			}
+			f.(*frontend).bucketAllocator = bucket.Fixed(1)
+			f.(*frontend).now = func() time.Time { return mockCurrentTime }
+
+			var systemDataEnricherCalled bool
+			f.(*frontend).systemDataEnricher = func(doc *api.OpenShiftClusterDocument, systemData *api.SystemData) {
+				enrichSystemData(doc, systemData)
+				systemDataEnricherCalled = true
+			}
+
+			go f.Run(ctx, nil, nil)
+
+			oc := tt.request()
+
+			method := http.MethodPut
+			if tt.isPatch {
+				method = http.MethodPatch
+			}
+
+			headers := http.Header{
+				"Content-Type": []string{"application/json"},
+			}
+
+			if tt.systemData != nil {
+				systemData, err := json.Marshal(tt.systemData)
+				if err != nil {
+					t.Fatal(err)
+				}
+				headers["X-Ms-Arm-Resource-System-Data"] = []string{string(systemData)}
+			} else {
+				headers["X-Ms-Arm-Resource-System-Data"] = []string{"{}"}
+			}
+
+			resp, b, err := ti.request(method,
+				"https://server"+testdatabase.GetResourcePath(mockSubID, "resourceName")+"?api-version=2022-04-01",
+				headers, oc)
+			if err != nil {
+				t.Error(err)
+			}
+
+			azureAsyncOperation := resp.Header.Get("Azure-AsyncOperation")
+			if tt.wantAsync {
+				if !strings.HasPrefix(azureAsyncOperation, fmt.Sprintf("https://localhost:8443/subscriptions/%s/providers/microsoft.redhatopenshift/locations/%s/operationsstatus/", mockSubID, ti.env.Location())) {
+					t.Error(azureAsyncOperation)
+				}
+			} else {
+				if azureAsyncOperation != "" {
+					t.Error(azureAsyncOperation)
+				}
+			}
+
+			err = validateResponse(resp, b, tt.wantStatusCode, tt.wantError, tt.wantResponse)
+			if err != nil {
+				t.Error(err)
+			}
+
+			errs := ti.enricher.Check(tt.wantEnriched)
+			for _, err := range errs {
+				t.Error(err)
+			}
+
+			if tt.wantDocuments != nil {
+				tt.wantDocuments(ti.checker)
+				errs := ti.checker.CheckOpenShiftClusters(ti.openShiftClustersClient)
+				errs = append(errs, ti.checker.CheckAsyncOperations(ti.asyncOperationsClient)...)
+				for _, err := range errs {
+					t.Error(err)
+				}
+			}
+
+			if tt.wantSystemDataEnriched != systemDataEnricherCalled {
+				t.Error(systemDataEnricherCalled)
+			}
+		})
+	}
+}
+
 func TestEnrichSystemData(t *testing.T) {
 	accountID1 := "00000000-0000-0000-0000-000000000001"
 	accountID2 := "00000000-0000-0000-0000-000000000002"
diff --git a/pkg/frontend/shared_test.go b/pkg/frontend/shared_test.go
index b1ea058ae..f699b3b4a 100644
--- a/pkg/frontend/shared_test.go
+++ b/pkg/frontend/shared_test.go
@@ -96,7 +96,7 @@ func newTestInfra(t *testing.T) *testInfra {
 	_env.EXPECT().ServiceKeyvault().AnyTimes().Return(keyvault)
 	_env.EXPECT().ArmClientAuthorizer().AnyTimes().Return(clientauthorizer.NewOne(clientcerts[0].Raw))
 	_env.EXPECT().AdminClientAuthorizer().AnyTimes().Return(clientauthorizer.NewOne(clientcerts[0].Raw))
-	_env.EXPECT().Domain().AnyTimes().Return("")
+	_env.EXPECT().Domain().AnyTimes().Return("aro.example")
 	_env.EXPECT().Listen().AnyTimes().Return(l, nil)
 	_env.EXPECT().FeatureIsSet(env.FeatureRequireD2sV3Workers).AnyTimes().Return(false)
 	_env.EXPECT().FeatureIsSet(env.FeatureDisableReadinessDelay).AnyTimes().Return(false)
diff --git a/pkg/hive/manager_test.go b/pkg/hive/manager_test.go
index 6a93d8c9d..d144c1fcc 100644
--- a/pkg/hive/manager_test.go
+++ b/pkg/hive/manager_test.go
@@ -13,8 +13,11 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	kruntime "k8s.io/apimachinery/pkg/runtime"
+	kubernetesfake "k8s.io/client-go/kubernetes/fake"
 
 	"github.com/Azure/ARO-RP/pkg/util/cmp"
+	"github.com/Azure/ARO-RP/pkg/util/uuid"
+	"github.com/Azure/ARO-RP/pkg/util/uuid/fake"
 )
 
 func TestIsClusterDeploymentReady(t *testing.T) {
@@ -158,3 +161,63 @@ func TestResetCorrelationData(t *testing.T) {
 		})
 	}
 }
+
+func TestCreateNamespace(t *testing.T) {
+	for _, tc := range []struct {
+		name             string
+		nsNames          []string
+		useFakeGenerator bool
+		shouldFail       bool
+	}{
+		{
+			name:             "not conflict names and real generator",
+			nsNames:          []string{"namespace1", "namespace2"},
+			useFakeGenerator: false,
+			shouldFail:       false,
+		},
+		{
+			name:             "conflict names and real generator",
+			nsNames:          []string{"namespace", "namespace"},
+			useFakeGenerator: false,
+			shouldFail:       false,
+		},
+		{
+			name:             "not conflict names and fake generator",
+			nsNames:          []string{"namespace1", "namespace2"},
+			useFakeGenerator: true,
+			shouldFail:       false,
+		},
+		{
+			name:             "conflict names and fake generator",
+			nsNames:          []string{"namespace", "namespace"},
+			useFakeGenerator: true,
+			shouldFail:       true,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			fakeClientset := kubernetesfake.NewSimpleClientset()
+			c := clusterManager{
+				kubernetescli: fakeClientset,
+			}
+
+			if tc.useFakeGenerator {
+				uuid.DefaultGenerator = fake.NewGenerator(tc.nsNames)
+			}
+
+			ns, err := c.CreateNamespace(context.Background())
+			if err != nil && !tc.shouldFail {
+				t.Error(err)
+			}
+
+			if err == nil {
+				res, err := fakeClientset.CoreV1().Namespaces().Get(context.Background(), ns.Name, metav1.GetOptions{})
+				if err != nil {
+					t.Error(err)
+				}
+				if !reflect.DeepEqual(ns, res) {
+					t.Errorf("results are not equal: \n wanted: %+v \n got %+v", ns, res)
+				}
+			}
+		})
+	}
+}