[ARO-13400] Old E2E pipeline uses MiWi clusters #4056

mrWinston · 2025-01-22T14:12:49Z

Which issue this PR addresses:

Fixes ARO-6153
Fixes ARO-13400
Fixes ARO-6196

What this PR does / why we need it:

This PR is the amalgamation of most miwi e2e efforts from @bitoku and Me. Roughly speaking, it implements the following:

Support miwi cluster creation in hack/cluster/cluster.go
Modify old e2e pipeline to create and test a miwi cluster
Fix E2E test failures that came up after running miwi e2e tests the first time

After this PR is merged, it is possible to run e2e tests against a miwi cluster on a PR by triggering the old pipeline with this comment:

/azp run e2e

.pipelines/e2e.yml

hlipsig

I noticed several new libraries being imported some of which are not in fully supported states. Do we already use these libraries elsewhere? Do we know the history behind these selections?

hlipsig · 2025-01-22T21:30:31Z

go.sum

@@ -601,6 +605,8 @@ github.com/rogpeppe/go-internal v1.12.0/go.mod h1:E+RYuTGaKKdloAfM02xzb0FW3Paa99
 github.com/russross/blackfriday v1.6.0 h1:KqfZb0pUVN2lYqZUYRddxF4OR8ZMURnJIG5Y3VRLtww=
 github.com/russross/blackfriday v1.6.0/go.mod h1:ti0ldHuxg49ri4ksnFxlkCfN+hvslNlmVHqNRXXJNAY=
 github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
+github.com/sagikazarmark/locafero v0.6.0 h1:ON7AQg37yzcRPU69mt7gwhFEBwxI6P9T4Qu3N51bwOk=


Is this a library we're already using?

I added viper as a dependency to help managing the different envioronment variables as a config struct ( see ClusterConfig definition and viper usage in NewClusterConfigFromEnv)

All the addiitonal dependencies are pulled in as dependencies of viper itself.

cadenmarchese

I left some comments on the pipeline / bash components here. Thanks for your efforts!

.pipelines/e2e.yml

hack/devtools/local_dev_env.sh

rajdeepc2792 · 2025-01-23T19:20:04Z

Just to confirm, this PR merge won't be impacting the Prod release pipeline, or in other words will the MIWI e2e cluster creation will run as part of Prod release pipeline? We don't want it to be running in the Prod until the preview API endpoints are available there.

github-actions · 2025-01-28T17:31:30Z

Please rebase pull request.

mrWinston · 2025-01-29T08:55:32Z

@rajdeepc2792

Just to confirm, this PR merge won't be impacting the Prod release pipeline, or in other words will the MIWI e2e cluster creation will run as part of Prod release pipeline?

This PR shouldn't have an impact on the prod pipeline. Using a miwi cluster for the E2E tests needs to be explicitly enabled in the pipeline.

Co-authored-by: Kipp Morris <[email protected]>

github-actions · 2025-02-07T03:47:47Z

Please rebase pull request.

Co-authored-by: Kipp Morris <[email protected]>

mrWinston · 2025-02-10T14:03:26Z

/azp run e2e

azure-pipelines · 2025-02-10T14:03:36Z

Azure Pipelines successfully started running 1 pipeline(s).

github-actions · 2025-02-10T20:04:31Z

Please rebase pull request.

mrWinston · 2025-02-11T14:48:31Z

I noticed several new libraries being imported some of which are not in fully supported states. Do we already use these libraries elsewhere? Do we know the history behind these selections?

@hlipsig I'm importing a pre-release version of viper to simplify the handling of env vars in the hack script. This version supports directly parsing the env vars into a config struct (like in

ARO-RP/pkg/util/cluster/cluster.go

Line 123 in 84f0863

func NewClusterConfigFromEnv() (*ClusterConfig, error) {

) and have the mapping from env vars to struct fields defined via struct fields, just like when unmarshalling json into a struct. See the ClusterConfig-struct for example.

Do you think it's an issue to have a pre-release dependency? If so, i could also rewrite that part.

cadenmarchese

This LGTM, I posed a few questions, but am otherwise good to approve this on next rebase.

cadenmarchese · 2025-02-12T19:57:22Z

hack/devtools/platform_workload_identity_role_sets.json

+        ]
+    },
+    {
+        "openShiftVersion": "4.14",


Note for a future PR - the ARO team should find a way to consolidate PWIRS between OCP versions between 4.14 and 4.18 so we don't duplicate this JSON for each version.

Since these are JSON manifests representing static data, I think duplicating the JSON for each version is probably the correct approach here. As a developer trying to read and understand the role sets for a given version, I wouldn't want to have to, for example, read the role set for 4.14 and then additionally parse through the incremental changes added in each version between 4.14 and the version I care about.

cadenmarchese · 2025-02-12T19:58:22Z

pkg/cluster/deploybaseresources.go

@@ -209,6 +209,12 @@ func (m *manager) deployBaseResourceTemplate(ctx context.Context) error {
 		}

 		resources = append(resources, r...)
+
+		storageBlobContributorRBAC, err := m.fpspStorageBlobContributorRBAC(clusterStorageAccountName, m.fpServicePrincipalID)


This extra role assignment is needed because of a missing permission in our built-in role, correct?

cadenmarchese · 2025-02-12T19:59:42Z

pkg/cluster/deploybaseresources.go

@@ -231,24 +237,16 @@ func (m *manager) deployBaseResourceTemplate(ctx context.Context) error {
 		)
 	}

+	if !m.env.FeatureIsSet(env.FeatureDisableDenyAssignments) {


Is there a reason we're moving the deny assignment append from its original location?

cadenmarchese · 2025-02-12T20:05:03Z

.pipelines/e2e.yml

@@ -58,6 +58,20 @@ jobs:
        displayName: Enable regression tests in CI
        condition: in(variables['Build.Reason'], 'IndividualCI', 'BatchedCI')

+      - script: |


Is it easy for us to rename this pipeline/azp run target to something like /azp run e2e-wimi or something similar rather than /azp run e2e? We can handle it in a future PR as well, if that's easier.

This was raised as something that could cause confusion for contributors who want to re-run CI e2e as normal but end up kicking off a MIWI e2e instead.

i'm looking into that. currently i'm unsure how to rename a pipeline.

kimorris27

LGTM pending a rebase and responses to Caden's feedback.

kimorris27 · 2025-02-12T20:25:14Z

hack/devtools/platform_workload_identity_role_sets.json

+        ]
+    },
+    {
+        "openShiftVersion": "4.14",


Since these are JSON manifests representing static data, I think duplicating the JSON for each version is probably the correct approach here. As a developer trying to read and understand the role sets for a given version, I wouldn't want to have to, for example, read the role set for 4.14 and then additionally parse through the incremental changes added in each version between 4.14 and the version I care about.

mrWinston · 2025-02-14T14:17:04Z

/azp list

azure-pipelines · 2025-02-14T14:17:13Z

CI/CD Pipelines for this repository: CI Purge Red Hat ARO HCP Gratis Sub e2e

github-actions · 2025-02-14T21:30:06Z

Please rebase pull request.

mrWinston requested review from jewzaam, bennerv, hawkowl, rogbas, petrkotas, jharrington22, cblecker, cadenmarchese, UlrichSchlueter, SudoBrendan, yjst2012, jaitaiwan, anshulvermapatel, hlipsig, mociarain, kimorris27, tsatam, bitoku, fahlmant and sankur-codes as code owners January 22, 2025 14:12

rajdeepc2792 reviewed Jan 22, 2025

View reviewed changes

.pipelines/e2e.yml Show resolved Hide resolved

hlipsig requested changes Jan 22, 2025

View reviewed changes

cadenmarchese reviewed Jan 23, 2025

View reviewed changes

.pipelines/e2e.yml Show resolved Hide resolved

hack/devtools/local_dev_env.sh Outdated Show resolved Hide resolved

hack/devtools/local_dev_env.sh Outdated Show resolved Hide resolved

hack/devtools/local_dev_env.sh Show resolved Hide resolved

rajdeepc2792 mentioned this pull request Jan 27, 2025

Fix bugs for MIWI e2e tests #4049

Closed

github-actions bot added the needs-rebase branch needs a rebase label Jan 28, 2025

mrWinston force-pushed the poc/hack-miwi branch from db9f224 to 62e78f3 Compare January 30, 2025 13:11

github-actions bot removed the needs-rebase branch needs a rebase label Jan 30, 2025

mrWinston force-pushed the poc/hack-miwi branch from e8180be to bbe0738 Compare February 6, 2025 10:09

mrWinston requested a review from tiguelu as a code owner February 6, 2025 10:09

github-actions bot removed the needs-rebase branch needs a rebase label Feb 6, 2025

mrWinston and others added 2 commits February 6, 2025 14:40

Update pkg/util/cluster/cluster.go

ba659a8

Co-authored-by: Kipp Morris <[email protected]>

remove unneccessary comments

06d87b7

github-actions bot added the needs-rebase branch needs a rebase label Feb 7, 2025

mrWinston and others added 3 commits February 10, 2025 13:43

Update pkg/util/cluster/cluster.go

5074790

Co-authored-by: Kipp Morris <[email protected]>

rename

9ebe2fc

Merge branch 'master' into poc/hack-miwi

17e8e46

github-actions bot removed the needs-rebase branch needs a rebase label Feb 10, 2025

mrWinston added 2 commits February 10, 2025 14:44

don't create an app when miwi is enabled

c08b52f

remove early exit from checkAccessv2

e7acaca

github-actions bot added the needs-rebase branch needs a rebase label Feb 10, 2025

mrWinston added 3 commits February 11, 2025 13:33

comment fixing

452fba5

make sure create_miwi_env_file behaviour stays the same in dev env

d918149

uppercase env vars in cluster_config

84f0863

cadenmarchese reviewed Feb 12, 2025

View reviewed changes

kimorris27 reviewed Feb 12, 2025

View reviewed changes

Merge branch 'master' into poc/hack-miwi

8d683fb

github-actions bot removed the needs-rebase branch needs a rebase label Feb 14, 2025

fix formatting, see what renaming does to a pipe

9b84b07

github-actions bot added the needs-rebase branch needs a rebase label Feb 14, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ARO-13400] Old E2E pipeline uses MiWi clusters #4056

[ARO-13400] Old E2E pipeline uses MiWi clusters #4056

mrWinston commented Jan 22, 2025

hlipsig left a comment

hlipsig Jan 22, 2025

mrWinston Jan 29, 2025

cadenmarchese left a comment

rajdeepc2792 commented Jan 23, 2025

github-actions bot commented Jan 28, 2025

mrWinston commented Jan 29, 2025 •

edited

Loading

github-actions bot commented Feb 7, 2025

mrWinston commented Feb 10, 2025

azure-pipelines bot commented Feb 10, 2025

github-actions bot commented Feb 10, 2025

mrWinston commented Feb 11, 2025

cadenmarchese left a comment

cadenmarchese Feb 12, 2025

kimorris27 Feb 12, 2025

cadenmarchese Feb 12, 2025

cadenmarchese Feb 12, 2025

cadenmarchese Feb 12, 2025

mrWinston Feb 14, 2025

kimorris27 left a comment

kimorris27 Feb 12, 2025

mrWinston commented Feb 14, 2025

azure-pipelines bot commented Feb 14, 2025

github-actions bot commented Feb 14, 2025

[ARO-13400] Old E2E pipeline uses MiWi clusters #4056

Are you sure you want to change the base?

[ARO-13400] Old E2E pipeline uses MiWi clusters #4056

Conversation

mrWinston commented Jan 22, 2025

Which issue this PR addresses:

What this PR does / why we need it:

hlipsig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cadenmarchese left a comment

Choose a reason for hiding this comment

rajdeepc2792 commented Jan 23, 2025

github-actions bot commented Jan 28, 2025

mrWinston commented Jan 29, 2025 • edited Loading

github-actions bot commented Feb 7, 2025

mrWinston commented Feb 10, 2025

azure-pipelines bot commented Feb 10, 2025

github-actions bot commented Feb 10, 2025

mrWinston commented Feb 11, 2025

cadenmarchese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kimorris27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrWinston commented Feb 14, 2025

azure-pipelines bot commented Feb 14, 2025

github-actions bot commented Feb 14, 2025

mrWinston commented Jan 29, 2025 •

edited

Loading