From 582351a8afe08d762efc1b1a0ffbcc27b70df990 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Fri, 12 Jul 2024 12:27:28 +0200 Subject: [PATCH 01/17] Update the docs --- docs/deploy-development-rp.md | 3 --- 1 file changed, 3 deletions(-) diff --git a/docs/deploy-development-rp.md b/docs/deploy-development-rp.md index e8be02f66e3..87b4cf97684 100644 --- a/docs/deploy-development-rp.md +++ b/docs/deploy-development-rp.md @@ -119,9 +119,6 @@ 1. use the create utility: ```bash - # Create the application to run the cluster as and load it - CLUSTER= go run ./hack/cluster createapp - source clusterapp.env # Create the cluster CLUSTER= go run ./hack/cluster create ``` From 1f857e1ed1c315b8be4ee83ba1530a9ce9249126 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Fri, 12 Jul 2024 16:36:24 +0200 Subject: [PATCH 02/17] Revert createApp --- .gitignore | 1 - hack/cluster/cluster.go | 4 +--- pkg/util/cluster/cluster.go | 39 +++++++++++++++++++------------------ 3 files changed, 21 insertions(+), 23 deletions(-) diff --git a/.gitignore b/.gitignore index d51d176e912..0ff89866fd2 100644 --- a/.gitignore +++ b/.gitignore @@ -42,4 +42,3 @@ megalinter-reports/ /jq /portalauth .kiota.log -/clusterapp.env diff --git a/hack/cluster/cluster.go b/hack/cluster/cluster.go index fd822d32dd2..1b2fa59aad7 100644 --- a/hack/cluster/cluster.go +++ b/hack/cluster/cluster.go @@ -25,7 +25,7 @@ const ( func run(ctx context.Context, log *logrus.Entry) error { if len(os.Args) != 2 { - return fmt.Errorf("usage: CLUSTER=x %s {create,createApp,deleteApp,delete}", os.Args[0]) + return fmt.Errorf("usage: CLUSTER=x %s {create,deleteApp,delete}", os.Args[0]) } if err := env.ValidateVars(Cluster); err != nil { @@ -59,8 +59,6 @@ func run(ctx context.Context, log *logrus.Entry) error { switch strings.ToLower(os.Args[1]) { case "create": return c.Create(ctx, vnetResourceGroup, clusterName, osClusterVersion) - case "createapp": - return c.CreateApp(ctx, clusterName) case "deleteapp": return c.DeleteApp(ctx) case "delete": diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index 293cfa12298..8c347113c80 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -133,20 +133,26 @@ func New(log *logrus.Entry, environment env.Core, ci bool) (*Cluster, error) { return c, nil } -func (c *Cluster) CreateApp(ctx context.Context, clusterName string) error { +type AppDetails struct { + applicationId string + applicationSecret string + spId string +} + +func (c *Cluster) createApp(ctx context.Context, clusterName string) (applicationDetails AppDetails, err error) { c.log.Infof("creating AAD application") appID, appSecret, err := c.createApplication(ctx, "aro-"+clusterName) if err != nil { - return err + return AppDetails{}, err } c.log.Infof("creating service principal") spID, err := c.createServicePrincipal(ctx, appID) if err != nil { - return err + return AppDetails{}, err } - return os.WriteFile("clusterapp.env", []byte(fmt.Sprintf("export AZURE_CLUSTER_SERVICE_PRINCIPAL_ID=%s\nexport AZURE_CLUSTER_APP_ID=%s\nexport AZURE_CLUSTER_APP_SECRET=%s", spID, appID, appSecret)), 0o600) + return AppDetails{appID, appSecret, spID}, nil } func (c *Cluster) DeleteApp(ctx context.Context) error { @@ -170,21 +176,16 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str return nil } - err = env.ValidateVars( - "AZURE_FP_SERVICE_PRINCIPAL_ID", - "AZURE_CLUSTER_SERVICE_PRINCIPAL_ID", - "AZURE_CLUSTER_APP_ID", - "AZURE_CLUSTER_APP_SECRET", - ) + fpSPId := os.Getenv("AZURE_FP_SERVICE_PRINCIPAL_ID") + if fpSPId == "" { + return fmt.Errorf("fp service principal id is not found") + } + + appDetails, err := c.createApp(ctx, clusterName) if err != nil { return err } - fpSPID := os.Getenv("AZURE_FP_SERVICE_PRINCIPAL_ID") - spID := os.Getenv("AZURE_CLUSTER_SERVICE_PRINCIPAL_ID") - appID := os.Getenv("AZURE_CLUSTER_APP_ID") - appSecret := os.Getenv("AZURE_CLUSTER_APP_SECRET") - visibility := api.VisibilityPublic if os.Getenv("PRIVATE_CLUSTER") != "" || os.Getenv("NO_INTERNET") != "" { @@ -240,8 +241,8 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str parameters := map[string]*arm.ParametersParameter{ "clusterName": {Value: clusterName}, "ci": {Value: c.ci}, - "clusterServicePrincipalId": {Value: spID}, - "fpServicePrincipalId": {Value: fpSPID}, + "clusterServicePrincipalId": {Value: appDetails.spId}, + "fpServicePrincipalId": {Value: fpSPId}, "vnetAddressPrefix": {Value: addressPrefix}, "masterAddressPrefix": {Value: masterSubnet}, "workerAddressPrefix": {Value: workerSubnet}, @@ -292,7 +293,7 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str {"/subscriptions/" + c.env.SubscriptionID() + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/routeTables/" + clusterName + "-rt", rbac.RoleNetworkContributor}, {diskEncryptionSetID, rbac.RoleReader}, } { - for _, principalID := range []string{spID, fpSPID} { + for _, principalID := range []string{appDetails.spId, fpSPId} { for i := 0; i < 5; i++ { _, err = c.roleassignments.Create( ctx, @@ -331,7 +332,7 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str } c.log.Info("creating cluster") - err = c.createCluster(ctx, vnetResourceGroup, clusterName, appID, appSecret, diskEncryptionSetID, visibility, osClusterVersion) + err = c.createCluster(ctx, vnetResourceGroup, clusterName, appDetails.applicationId, appDetails.applicationSecret, diskEncryptionSetID, visibility, osClusterVersion) if err != nil { return err From 6431df80190ca21de8589f578a428ec22767cdbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Fri, 12 Jul 2024 16:44:06 +0200 Subject: [PATCH 03/17] Revert DeleteApp --- docs/deploy-development-rp.md | 1 - hack/cluster/cluster.go | 4 +--- hack/e2e/run-rp-and-e2e.sh | 1 - pkg/util/cluster/cluster.go | 5 ++++- 4 files changed, 5 insertions(+), 6 deletions(-) diff --git a/docs/deploy-development-rp.md b/docs/deploy-development-rp.md index 87b4cf97684..edad0dcddc2 100644 --- a/docs/deploy-development-rp.md +++ b/docs/deploy-development-rp.md @@ -127,7 +127,6 @@ ```bash CLUSTER= go run ./hack/cluster delete - CLUSTER= go run ./hack/cluster deleteapp ``` By default, a public cluster will be created. In order to create a private cluster, set the `PRIVATE_CLUSTER` environment variable to `true` prior to creation. Internet access from the cluster can also be restricted by setting the `NO_INTERNET` environment variable to `true`. diff --git a/hack/cluster/cluster.go b/hack/cluster/cluster.go index 1b2fa59aad7..8fab0aa9a06 100644 --- a/hack/cluster/cluster.go +++ b/hack/cluster/cluster.go @@ -25,7 +25,7 @@ const ( func run(ctx context.Context, log *logrus.Entry) error { if len(os.Args) != 2 { - return fmt.Errorf("usage: CLUSTER=x %s {create,deleteApp,delete}", os.Args[0]) + return fmt.Errorf("usage: CLUSTER=x %s {create,delete}", os.Args[0]) } if err := env.ValidateVars(Cluster); err != nil { @@ -59,8 +59,6 @@ func run(ctx context.Context, log *logrus.Entry) error { switch strings.ToLower(os.Args[1]) { case "create": return c.Create(ctx, vnetResourceGroup, clusterName, osClusterVersion) - case "deleteapp": - return c.DeleteApp(ctx) case "delete": return c.Delete(ctx, vnetResourceGroup, clusterName) default: diff --git a/hack/e2e/run-rp-and-e2e.sh b/hack/e2e/run-rp-and-e2e.sh index aee67f90b59..bb03f2a1ab4 100755 --- a/hack/e2e/run-rp-and-e2e.sh +++ b/hack/e2e/run-rp-and-e2e.sh @@ -211,7 +211,6 @@ delete_e2e_cluster() { ./cluster delete else go run ./hack/cluster delete - go run ./hack/cluster deleteApp fi } diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index 8c347113c80..78ccc2e94ba 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -387,7 +387,10 @@ func (c *Cluster) Delete(ctx context.Context, vnetResourceGroup, clusterName str c.deleteVnetPeerings(ctx, vnetResourceGroup), ) case c.ci: // Prod E2E - errs = append(errs, c.deleteClusterResourceGroup(ctx, vnetResourceGroup)) + errs = append(errs, + c.deleteClusterResourceGroup(ctx, vnetResourceGroup), + c.DeleteApp(ctx), + ) default: errs = append(errs, c.deleteRoleAssignments(ctx, vnetResourceGroup, clusterName), From 92cfabebd09730b5405f4e352bc7dcd8229fc295 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Fri, 12 Jul 2024 16:44:30 +0200 Subject: [PATCH 04/17] s/DeleteApp/deleteApp --- pkg/util/cluster/cluster.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index 78ccc2e94ba..58b775db3f7 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -155,7 +155,7 @@ func (c *Cluster) createApp(ctx context.Context, clusterName string) (applicatio return AppDetails{appID, appSecret, spID}, nil } -func (c *Cluster) DeleteApp(ctx context.Context) error { +func (c *Cluster) deleteApp(ctx context.Context) error { err := env.ValidateVars( "AZURE_CLUSTER_APP_ID", ) @@ -389,7 +389,7 @@ func (c *Cluster) Delete(ctx context.Context, vnetResourceGroup, clusterName str case c.ci: // Prod E2E errs = append(errs, c.deleteClusterResourceGroup(ctx, vnetResourceGroup), - c.DeleteApp(ctx), + c.deleteApp(ctx), ) default: errs = append(errs, From 830068abe670b1c05bd2462d1e2cae9d2d2c0776 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Fri, 12 Jul 2024 16:58:05 +0200 Subject: [PATCH 05/17] Tidy up --- hack/cluster/cluster.go | 3 ++- pkg/util/cluster/cluster.go | 34 +++++++++++++++------------------- test/e2e/setup.go | 2 +- 3 files changed, 18 insertions(+), 21 deletions(-) diff --git a/hack/cluster/cluster.go b/hack/cluster/cluster.go index 8fab0aa9a06..adbe5690b63 100644 --- a/hack/cluster/cluster.go +++ b/hack/cluster/cluster.go @@ -54,13 +54,14 @@ func run(ctx context.Context, log *logrus.Entry) error { c, err := cluster.New(log, env, os.Getenv("CI") != "") if err != nil { return err + } switch strings.ToLower(os.Args[1]) { case "create": return c.Create(ctx, vnetResourceGroup, clusterName, osClusterVersion) case "delete": - return c.Delete(ctx, vnetResourceGroup, clusterName) + return c.Delete(ctx, log, vnetResourceGroup, clusterName) default: return fmt.Errorf("invalid command %s", os.Args[1]) } diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index 58b775db3f7..d0e545d8683 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -136,7 +136,7 @@ func New(log *logrus.Entry, environment env.Core, ci bool) (*Cluster, error) { type AppDetails struct { applicationId string applicationSecret string - spId string + SPId string } func (c *Cluster) createApp(ctx context.Context, clusterName string) (applicationDetails AppDetails, err error) { @@ -155,17 +155,6 @@ func (c *Cluster) createApp(ctx context.Context, clusterName string) (applicatio return AppDetails{appID, appSecret, spID}, nil } -func (c *Cluster) deleteApp(ctx context.Context) error { - err := env.ValidateVars( - "AZURE_CLUSTER_APP_ID", - ) - if err != nil { - return err - } - - return c.deleteApplication(ctx, os.Getenv("AZURE_CLUSTER_APP_ID")) -} - func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName string, osClusterVersion string) error { clusterGet, err := c.openshiftclusters.Get(ctx, vnetResourceGroup, clusterName) if err == nil { @@ -241,7 +230,7 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str parameters := map[string]*arm.ParametersParameter{ "clusterName": {Value: clusterName}, "ci": {Value: c.ci}, - "clusterServicePrincipalId": {Value: appDetails.spId}, + "clusterServicePrincipalId": {Value: appDetails.SPId}, "fpServicePrincipalId": {Value: fpSPId}, "vnetAddressPrefix": {Value: addressPrefix}, "masterAddressPrefix": {Value: masterSubnet}, @@ -293,7 +282,7 @@ func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName str {"/subscriptions/" + c.env.SubscriptionID() + "/resourceGroups/" + vnetResourceGroup + "/providers/Microsoft.Network/routeTables/" + clusterName + "-rt", rbac.RoleNetworkContributor}, {diskEncryptionSetID, rbac.RoleReader}, } { - for _, principalID := range []string{appDetails.spId, fpSPId} { + for _, principalID := range []string{appDetails.SPId, fpSPId} { for i := 0; i < 5; i++ { _, err = c.roleassignments.Create( ctx, @@ -376,7 +365,7 @@ func (c *Cluster) generateSubnets() (vnetPrefix string, masterSubnet string, wor return } -func (c *Cluster) Delete(ctx context.Context, vnetResourceGroup, clusterName string) error { +func (c *Cluster) Delete(ctx context.Context, log *logrus.Entry, vnetResourceGroup, clusterName string) error { var errs []error switch { @@ -387,10 +376,17 @@ func (c *Cluster) Delete(ctx context.Context, vnetResourceGroup, clusterName str c.deleteVnetPeerings(ctx, vnetResourceGroup), ) case c.ci: // Prod E2E - errs = append(errs, - c.deleteClusterResourceGroup(ctx, vnetResourceGroup), - c.deleteApp(ctx), - ) + oc, err := c.openshiftclusters.Get(ctx, vnetResourceGroup, clusterName) + if err != nil { + log.Errorf("Prod E2E cluster %s not found in RG %s", clusterName, vnetResourceGroup) + errs = append(errs, err) + } else { + errs = append(errs, + c.deleteRoleAssignments(ctx, vnetResourceGroup, clusterName), + c.deleteClusterResourceGroup(ctx, vnetResourceGroup), + c.deleteApplication(ctx, *oc.OpenShiftClusterProperties.ServicePrincipalProfile.ClientID), + ) + } default: errs = append(errs, c.deleteRoleAssignments(ctx, vnetResourceGroup, clusterName), diff --git a/test/e2e/setup.go b/test/e2e/setup.go index 2d7995d28c7..1bd0f3ead31 100644 --- a/test/e2e/setup.go +++ b/test/e2e/setup.go @@ -445,7 +445,7 @@ func done(ctx context.Context) error { return err } - err = cluster.Delete(ctx, vnetResourceGroup, clusterName) + err = cluster.Delete(ctx, log, vnetResourceGroup, clusterName) if err != nil { return err } From 2aca66076ca37bf2695640b8eba40fcc15018de5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Fri, 12 Jul 2024 16:59:21 +0200 Subject: [PATCH 06/17] Drop get_sp function --- .pipelines/e2e.yml | 1 - hack/e2e/run-rp-and-e2e.sh | 20 -------------------- 2 files changed, 21 deletions(-) diff --git a/.pipelines/e2e.yml b/.pipelines/e2e.yml index 6a21ab71fb5..94328e1dd51 100644 --- a/.pipelines/e2e.yml +++ b/.pipelines/e2e.yml @@ -52,7 +52,6 @@ jobs: export CI=true . ./hack/e2e/run-rp-and-e2e.sh - get_cluster_sp deploy_e2e_db displayName: Setup (Azure) diff --git a/hack/e2e/run-rp-and-e2e.sh b/hack/e2e/run-rp-and-e2e.sh index bb03f2a1ab4..5dc1878642f 100755 --- a/hack/e2e/run-rp-and-e2e.sh +++ b/hack/e2e/run-rp-and-e2e.sh @@ -214,26 +214,6 @@ delete_e2e_cluster() { fi } -get_cluster_sp() { - echo "########## Downloading SP secrets ##########" - az keyvault secret download --vault-name=$CSP_VAULT_NAME \ - --name=aro-v4-e2e-devops-spn-1-app-id \ - --file=secrets/app-id - az keyvault secret download --vault-name=$CSP_VAULT_NAME \ - --name=aro-v4-e2e-devops-spn-1-sp-id \ - --file=secrets/sp-id - az keyvault secret download --vault-name=$CSP_VAULT_NAME \ - --name=aro-v4-e2e-devops-spn-1-secret-value \ - --file=secrets/secret-value - - echo -e -n "\nexport AZURE_CLUSTER_SERVICE_PRINCIPAL_ID=" >>secrets/env - cat secrets/sp-id >>secrets/env - echo -e -n "\nexport AZURE_CLUSTER_APP_ID=" >>secrets/env - cat secrets/app-id >>secrets/env - echo -e -n "\nexport AZURE_CLUSTER_APP_SECRET=" >>secrets/env - cat secrets/secret-value >>secrets/env -} - # TODO: CLUSTER and is also recalculated in multiple places # in the billing pipelines :-( From 1c9f3f66f77bf4df267113263392efd5196c2805 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Fri, 12 Jul 2024 17:10:58 +0200 Subject: [PATCH 07/17] Update logging --- hack/cluster/cluster.go | 3 +-- pkg/util/cluster/cluster.go | 10 +++++----- test/e2e/setup.go | 2 +- 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/hack/cluster/cluster.go b/hack/cluster/cluster.go index adbe5690b63..8fab0aa9a06 100644 --- a/hack/cluster/cluster.go +++ b/hack/cluster/cluster.go @@ -54,14 +54,13 @@ func run(ctx context.Context, log *logrus.Entry) error { c, err := cluster.New(log, env, os.Getenv("CI") != "") if err != nil { return err - } switch strings.ToLower(os.Args[1]) { case "create": return c.Create(ctx, vnetResourceGroup, clusterName, osClusterVersion) case "delete": - return c.Delete(ctx, log, vnetResourceGroup, clusterName) + return c.Delete(ctx, vnetResourceGroup, clusterName) default: return fmt.Errorf("invalid command %s", os.Args[1]) } diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index d0e545d8683..e5f8a87c00d 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -140,13 +140,13 @@ type AppDetails struct { } func (c *Cluster) createApp(ctx context.Context, clusterName string) (applicationDetails AppDetails, err error) { - c.log.Infof("creating AAD application") + c.log.Infof("Creating AAD application") appID, appSecret, err := c.createApplication(ctx, "aro-"+clusterName) if err != nil { return AppDetails{}, err } - c.log.Infof("creating service principal") + c.log.Infof("Creating service principal") spID, err := c.createServicePrincipal(ctx, appID) if err != nil { return AppDetails{}, err @@ -365,7 +365,8 @@ func (c *Cluster) generateSubnets() (vnetPrefix string, masterSubnet string, wor return } -func (c *Cluster) Delete(ctx context.Context, log *logrus.Entry, vnetResourceGroup, clusterName string) error { +func (c *Cluster) Delete(ctx context.Context, vnetResourceGroup, clusterName string) error { + c.log.Infof("Deleting cluster %s in RG %s", clusterName, vnetResourceGroup) var errs []error switch { @@ -378,11 +379,10 @@ func (c *Cluster) Delete(ctx context.Context, log *logrus.Entry, vnetResourceGro case c.ci: // Prod E2E oc, err := c.openshiftclusters.Get(ctx, vnetResourceGroup, clusterName) if err != nil { - log.Errorf("Prod E2E cluster %s not found in RG %s", clusterName, vnetResourceGroup) + c.log.Errorf("Prod E2E cluster %s not found in RG %s", clusterName, vnetResourceGroup) errs = append(errs, err) } else { errs = append(errs, - c.deleteRoleAssignments(ctx, vnetResourceGroup, clusterName), c.deleteClusterResourceGroup(ctx, vnetResourceGroup), c.deleteApplication(ctx, *oc.OpenShiftClusterProperties.ServicePrincipalProfile.ClientID), ) diff --git a/test/e2e/setup.go b/test/e2e/setup.go index 1bd0f3ead31..2d7995d28c7 100644 --- a/test/e2e/setup.go +++ b/test/e2e/setup.go @@ -445,7 +445,7 @@ func done(ctx context.Context) error { return err } - err = cluster.Delete(ctx, log, vnetResourceGroup, clusterName) + err = cluster.Delete(ctx, vnetResourceGroup, clusterName) if err != nil { return err } From 996f5bd659a04441568375d6d3eb001f1a76ed8b Mon Sep 17 00:00:00 2001 From: Roland Kunkel Date: Tue, 16 Jul 2024 16:03:51 +0200 Subject: [PATCH 08/17] add more logging --- pkg/util/cluster/cluster.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index e5f8a87c00d..c3f8949cca2 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -82,6 +82,18 @@ func New(log *logrus.Entry, environment env.Core, ci bool) (*Cluster, error) { return nil, err } + clientId, _ := os.LookupEnv("AZURE_CLIENT_ID") + log.Infof("AZURE_CLIENT_ID: %s", clientId) + + fpClientId, _ := os.LookupEnv("AZURE_FP_CLIENT_ID") + log.Infof("AZURE_FP_CLIENT_ID: %s", fpClientId) + + tenantId, _ := os.LookupEnv("AZURE_TENANT_ID") + log.Infof("AZURE_TENANT_ID: %s", tenantId) + + subscriptionId, _ := os.LookupEnv("AZURE_SUBSCRIPTION_ID") + log.Infof("AZURE_SUBSCRIPTION_ID: %s", subscriptionId) + spGraphClient, err := environment.Environment().NewGraphServiceClient(spTokenCredential) if err != nil { return nil, err From 3275219918958f494c737d3dcc5d397661f464ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Wed, 17 Jul 2024 10:56:11 +0200 Subject: [PATCH 09/17] Use better deleting logic --- pkg/util/cluster/cluster.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index c3f8949cca2..d6cbe68c95b 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -383,20 +383,26 @@ func (c *Cluster) Delete(ctx context.Context, vnetResourceGroup, clusterName str switch { case c.ci && env.IsLocalDevelopmentMode(): // PR E2E - errs = append(errs, - c.deleteCluster(ctx, vnetResourceGroup, clusterName), - c.deleteClusterResourceGroup(ctx, vnetResourceGroup), - c.deleteVnetPeerings(ctx, vnetResourceGroup), - ) - case c.ci: // Prod E2E oc, err := c.openshiftclusters.Get(ctx, vnetResourceGroup, clusterName) if err != nil { c.log.Errorf("Prod E2E cluster %s not found in RG %s", clusterName, vnetResourceGroup) errs = append(errs, err) } else { errs = append(errs, + c.deleteApplication(ctx, *oc.OpenShiftClusterProperties.ServicePrincipalProfile.ClientID), c.deleteClusterResourceGroup(ctx, vnetResourceGroup), + c.deleteVnetPeerings(ctx, vnetResourceGroup), + ) + } + case c.ci: // Prod E2E + oc, err := c.openshiftclusters.Get(ctx, vnetResourceGroup, clusterName) + if err != nil { + c.log.Errorf("Prod E2E cluster %s not found in RG %s", clusterName, vnetResourceGroup) + errs = append(errs, err) + } else { + errs = append(errs, c.deleteApplication(ctx, *oc.OpenShiftClusterProperties.ServicePrincipalProfile.ClientID), + c.deleteClusterResourceGroup(ctx, vnetResourceGroup), ) } default: From 5909beaacfbfe21e983f0b5aae195b523a120aa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Wed, 17 Jul 2024 12:17:28 +0200 Subject: [PATCH 10/17] Skip tests for testing purposes --- test/e2e/setup.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/e2e/setup.go b/test/e2e/setup.go index 2d7995d28c7..ae2db3fea64 100644 --- a/test/e2e/setup.go +++ b/test/e2e/setup.go @@ -99,19 +99,19 @@ var ( ) func skipIfNotInDevelopmentEnv() { - if !_env.IsLocalDevelopmentMode() { + if _env.IsLocalDevelopmentMode() { Skip("skipping tests in non-development environment") } } func skipIfSeleniumNotEnabled() { - if os.Getenv("ARO_SELENIUM_HOSTNAME") == "" { + if os.Getenv("ARO_SELENIUM_HOSTNAME") != "" { Skip("ARO_SELENIUM_HOSTNAME not set, skipping portal e2e") } } func skipIfNotHiveManagedCluster(adminAPICluster *admin.OpenShiftCluster) { - if adminAPICluster.Properties.HiveProfile == (admin.HiveProfile{}) { + if adminAPICluster.Properties.HiveProfile != (admin.HiveProfile{}) { Skip("skipping tests because this ARO cluster has not been created/adopted by Hive") } } From 56ff3f45abb4326ec63c9f5abd4eb5b33f26bc1b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Wed, 17 Jul 2024 17:33:32 +0200 Subject: [PATCH 11/17] Test: Just deleteCluster --- pkg/util/cluster/cluster.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index d6cbe68c95b..093d6931275 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -390,7 +390,8 @@ func (c *Cluster) Delete(ctx context.Context, vnetResourceGroup, clusterName str } else { errs = append(errs, c.deleteApplication(ctx, *oc.OpenShiftClusterProperties.ServicePrincipalProfile.ClientID), - c.deleteClusterResourceGroup(ctx, vnetResourceGroup), + c.deleteCluster(ctx, vnetResourceGroup, clusterName), + // c.deleteClusterResourceGroup(ctx, vnetResourceGroup), c.deleteVnetPeerings(ctx, vnetResourceGroup), ) } From 5996096d4e5336b559a9259747ba5b4f6803ef3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Wed, 17 Jul 2024 22:44:29 +0200 Subject: [PATCH 12/17] Add explict logic to ensure the cluster has been deleted --- pkg/util/cluster/cluster.go | 54 ++++++++++++++++++++++--------------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index 093d6931275..6b9a22f6305 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -42,6 +42,7 @@ import ( "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/features" "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/network" redhatopenshift20231122 "github.com/Azure/ARO-RP/pkg/util/azureclient/mgmt/redhatopenshift/2023-11-22/redhatopenshift" + "github.com/Azure/ARO-RP/pkg/util/azureerrors" utilgraph "github.com/Azure/ARO-RP/pkg/util/graph" "github.com/Azure/ARO-RP/pkg/util/rbac" "github.com/Azure/ARO-RP/pkg/util/rolesets" @@ -378,35 +379,29 @@ func (c *Cluster) generateSubnets() (vnetPrefix string, masterSubnet string, wor } func (c *Cluster) Delete(ctx context.Context, vnetResourceGroup, clusterName string) error { - c.log.Infof("Deleting cluster %s in RG %s", clusterName, vnetResourceGroup) + c.log.Infof("Deleting cluster %s in resource group %s", clusterName, vnetResourceGroup) var errs []error - switch { - case c.ci && env.IsLocalDevelopmentMode(): // PR E2E + if c.ci { oc, err := c.openshiftclusters.Get(ctx, vnetResourceGroup, clusterName) + clusterResourceGroup := fmt.Sprintf("aro-%s", clusterName) if err != nil { - c.log.Errorf("Prod E2E cluster %s not found in RG %s", clusterName, vnetResourceGroup) + c.log.Errorf("CI E2E cluster %s not found in resource group %s", clusterName, vnetResourceGroup) errs = append(errs, err) - } else { - errs = append(errs, - c.deleteApplication(ctx, *oc.OpenShiftClusterProperties.ServicePrincipalProfile.ClientID), - c.deleteCluster(ctx, vnetResourceGroup, clusterName), - // c.deleteClusterResourceGroup(ctx, vnetResourceGroup), - c.deleteVnetPeerings(ctx, vnetResourceGroup), - ) } - case c.ci: // Prod E2E - oc, err := c.openshiftclusters.Get(ctx, vnetResourceGroup, clusterName) - if err != nil { - c.log.Errorf("Prod E2E cluster %s not found in RG %s", clusterName, vnetResourceGroup) - errs = append(errs, err) - } else { + errs = append(errs, + c.deleteApplication(ctx, *oc.OpenShiftClusterProperties.ServicePrincipalProfile.ClientID), + c.deleteCluster(ctx, vnetResourceGroup, clusterName), + c.ensureResourceGroupDeleted(ctx, clusterResourceGroup), + c.deleteResourceGroup(ctx, vnetResourceGroup), + ) + + if env.IsLocalDevelopmentMode() { //PR E2E errs = append(errs, - c.deleteApplication(ctx, *oc.OpenShiftClusterProperties.ServicePrincipalProfile.ClientID), - c.deleteClusterResourceGroup(ctx, vnetResourceGroup), + c.deleteVnetPeerings(ctx, vnetResourceGroup), ) } - default: + } else { errs = append(errs, c.deleteRoleAssignments(ctx, vnetResourceGroup, clusterName), c.deleteCluster(ctx, vnetResourceGroup, clusterName), @@ -733,13 +728,28 @@ func (c *Cluster) deleteCluster(ctx context.Context, resourceGroup, clusterName return nil } -func (c *Cluster) deleteClusterResourceGroup(ctx context.Context, resourceGroup string) error { +func (c *Cluster) ensureResourceGroupDeleted(ctx context.Context, resourceGroupName string) error { + c.log.Printf("Deleting resource group %s", resourceGroupName) + timeoutCtx, cancel := context.WithTimeout(ctx, 10*time.Minute) + defer cancel() + + return wait.PollImmediateUntil(5*time.Second, func() (bool, error) { + _, err := c.groups.Get(ctx, resourceGroupName) + if azureerrors.ResourceGroupNotFound(err) { + c.log.Infof("Finished deleting resource group %s", resourceGroupName) + return true, nil + } + return false, fmt.Errorf("Failed to delete resource group %s", resourceGroupName) + }, timeoutCtx.Done()) +} + +func (c *Cluster) deleteResourceGroup(ctx context.Context, resourceGroup string) error { + c.log.Printf("deleting resource group %s", resourceGroup) if _, err := c.groups.Get(ctx, resourceGroup); err != nil { c.log.Printf("error getting resource group %s, skipping deletion: %v", resourceGroup, err) return nil } - c.log.Printf("deleting resource group %s", resourceGroup) if err := c.groups.DeleteAndWait(ctx, resourceGroup); err != nil { return fmt.Errorf("error deleting resource group: %w", err) } From 8914f1ad4fe396feea7cce5fdbca4a29998afb08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Thu, 18 Jul 2024 09:58:21 +0200 Subject: [PATCH 13/17] Fix log lint warnings --- pkg/util/cluster/cluster.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index 6b9a22f6305..78bc13842b5 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -729,17 +729,17 @@ func (c *Cluster) deleteCluster(ctx context.Context, resourceGroup, clusterName } func (c *Cluster) ensureResourceGroupDeleted(ctx context.Context, resourceGroupName string) error { - c.log.Printf("Deleting resource group %s", resourceGroupName) + c.log.Printf("deleting resource group %s", resourceGroupName) timeoutCtx, cancel := context.WithTimeout(ctx, 10*time.Minute) defer cancel() return wait.PollImmediateUntil(5*time.Second, func() (bool, error) { _, err := c.groups.Get(ctx, resourceGroupName) if azureerrors.ResourceGroupNotFound(err) { - c.log.Infof("Finished deleting resource group %s", resourceGroupName) + c.log.Infof("finished deleting resource group %s", resourceGroupName) return true, nil } - return false, fmt.Errorf("Failed to delete resource group %s", resourceGroupName) + return false, fmt.Errorf("failed to delete resource group %s", resourceGroupName) }, timeoutCtx.Done()) } From e8b11df1ba0d165b723c6d96a2cafa2c5a3074f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Thu, 18 Jul 2024 10:39:21 +0200 Subject: [PATCH 14/17] Revert skipping some tests --- test/e2e/setup.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/e2e/setup.go b/test/e2e/setup.go index ae2db3fea64..2d7995d28c7 100644 --- a/test/e2e/setup.go +++ b/test/e2e/setup.go @@ -99,19 +99,19 @@ var ( ) func skipIfNotInDevelopmentEnv() { - if _env.IsLocalDevelopmentMode() { + if !_env.IsLocalDevelopmentMode() { Skip("skipping tests in non-development environment") } } func skipIfSeleniumNotEnabled() { - if os.Getenv("ARO_SELENIUM_HOSTNAME") != "" { + if os.Getenv("ARO_SELENIUM_HOSTNAME") == "" { Skip("ARO_SELENIUM_HOSTNAME not set, skipping portal e2e") } } func skipIfNotHiveManagedCluster(adminAPICluster *admin.OpenShiftCluster) { - if adminAPICluster.Properties.HiveProfile != (admin.HiveProfile{}) { + if adminAPICluster.Properties.HiveProfile == (admin.HiveProfile{}) { Skip("skipping tests because this ARO cluster has not been created/adopted by Hive") } } From a941ecdef9a0c216864dd7756f654babfed3f8ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Thu, 18 Jul 2024 15:44:31 +0200 Subject: [PATCH 15/17] Remove debug lines --- pkg/util/cluster/cluster.go | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index 78bc13842b5..816667a8f4c 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -83,18 +83,6 @@ func New(log *logrus.Entry, environment env.Core, ci bool) (*Cluster, error) { return nil, err } - clientId, _ := os.LookupEnv("AZURE_CLIENT_ID") - log.Infof("AZURE_CLIENT_ID: %s", clientId) - - fpClientId, _ := os.LookupEnv("AZURE_FP_CLIENT_ID") - log.Infof("AZURE_FP_CLIENT_ID: %s", fpClientId) - - tenantId, _ := os.LookupEnv("AZURE_TENANT_ID") - log.Infof("AZURE_TENANT_ID: %s", tenantId) - - subscriptionId, _ := os.LookupEnv("AZURE_SUBSCRIPTION_ID") - log.Infof("AZURE_SUBSCRIPTION_ID: %s", subscriptionId) - spGraphClient, err := environment.Environment().NewGraphServiceClient(spTokenCredential) if err != nil { return nil, err From 256446f05cf678ec933bc81d25aa473054f7a0fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Fri, 19 Jul 2024 16:35:29 +0200 Subject: [PATCH 16/17] Add error to log message --- pkg/util/cluster/cluster.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index 816667a8f4c..48d0e379089 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -727,7 +727,7 @@ func (c *Cluster) ensureResourceGroupDeleted(ctx context.Context, resourceGroupN c.log.Infof("finished deleting resource group %s", resourceGroupName) return true, nil } - return false, fmt.Errorf("failed to delete resource group %s", resourceGroupName) + return false, fmt.Errorf("failed to delete resource group %s with %s", resourceGroupName, err) }, timeoutCtx.Done()) } From 245d6f4f91f7563adea44bd6924a273386156ff4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maiti=C3=BA=20=C3=93=20Ciar=C3=A1in?= Date: Fri, 19 Jul 2024 16:35:51 +0200 Subject: [PATCH 17/17] Make appDetails private --- pkg/util/cluster/cluster.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/util/cluster/cluster.go b/pkg/util/cluster/cluster.go index 48d0e379089..9860c9d5706 100644 --- a/pkg/util/cluster/cluster.go +++ b/pkg/util/cluster/cluster.go @@ -134,26 +134,26 @@ func New(log *logrus.Entry, environment env.Core, ci bool) (*Cluster, error) { return c, nil } -type AppDetails struct { +type appDetails struct { applicationId string applicationSecret string SPId string } -func (c *Cluster) createApp(ctx context.Context, clusterName string) (applicationDetails AppDetails, err error) { +func (c *Cluster) createApp(ctx context.Context, clusterName string) (applicationDetails appDetails, err error) { c.log.Infof("Creating AAD application") appID, appSecret, err := c.createApplication(ctx, "aro-"+clusterName) if err != nil { - return AppDetails{}, err + return appDetails{}, err } c.log.Infof("Creating service principal") spID, err := c.createServicePrincipal(ctx, appID) if err != nil { - return AppDetails{}, err + return appDetails{}, err } - return AppDetails{appID, appSecret, spID}, nil + return appDetails{appID, appSecret, spID}, nil } func (c *Cluster) Create(ctx context.Context, vnetResourceGroup, clusterName string, osClusterVersion string) error {