Add support for Azure RedhatOpenshift Clusters #4152

super-harsh · 2024-07-08T06:35:16Z

What this PR does / why we need it:

This PR adds support for 2023-11-22 Azure RedhatOpenshift Clusters.

Special notes for your reviewer:

Have excluded samples testing and controller tests from running in live mode since we require Service Principal credentials to run them.

If applicable:

this PR contains documentation
this PR contains tests
this PR contains YAML Samples

v2/internal/testcommon/vcr/redact.go

…rom configmap

matthchr · 2024-07-11T20:48:53Z

Looks like an APIManagement test flake possibly? May need to re-run test-generator.

matthchr · 2024-07-11T20:49:10Z

docs/hugo/content/reference/_index.md

+
+| Resource         | ARM Version | CRD Version   | Supported From | Sample                                                                                                                                 |
+|------------------|-------------|---------------|----------------|----------------------------------------------------------------------------------------------------------------------------------------|
+| OpenShiftCluster | 2023-11-22  | v1api20231122 | v2.8.0         | [View](https://github.com/Azure/azure-service-operator/tree/main/v2/samples/redhatopenshift/v1api/v1api20231122_openshiftcluster.yaml) |


Should be from 2.9.0 now, right?

matthchr · 2024-07-11T20:54:01Z

v2/azure-arm.yaml

+    2023-11-22:
+      OpenShiftCluster:
+        $export: true
+        $supportedFrom: v2.8.0


True, I had this from pre 2.8.0. Thanks for pointing out, will update.

matthchr · 2024-07-11T20:55:00Z

v2/internal/controllers/crd_redhatopenshift_openshiftcluster_20230401_test.go

+	// TODO: Replace the principalID, clientSecret and clientId vars below with principalId, clientSecret and clientId of your SP
+	principalId := "5c6be76c-5fc4-4817-992d-22027b44c402"
+	clientId := "5b7a18b9-8aec-4456-a4c3-0865cbfa1512"
+	clientSecret := "your-client-secret-here"


Probably better to have the code read this from an env variable that's documented at the top of the test?

Putting credentials in source is a checkin-risk

and if we're gonna move these to env variables we probably ought to just move clientId/principalId too.

Nice idea for the clientSecret, although the ClientId and PrincipalId don't get redacted in the tests hence I've left them here. Maybe we need another matcher to match guids and replace them?

matthchr · 2024-07-11T20:55:40Z

v2/internal/controllers/crd_redhatopenshift_openshiftcluster_20230401_test.go

+	"github.com/Azure/azure-service-operator/v2/internal/util/to"
+)
+
+// TODO: TO re-record this test, create a new service principal and follow the todos below in the test code.


minor: Clarify that it's ARO that requires an SP, so there is no workaround.

Maybe by saying something like:

Suggested change

// TODO: TO re-record this test, create a new service principal and follow the todos below in the test code.

// TODO: To re-record this test, create a new service principal and save its details into the

// TODO: ARO_CLIENT_ID, ARO_CLIENT_SECRET, and ARO_PRINCIPAL_ID variables.

// TODO: This is required because the ARO resource requires a service principal for input currently. Hopefully

// TODO: we can revisit this in the future when they support other options.

v2/internal/controllers/crd_redhatopenshift_openshiftcluster_20230401_test.go

v2/samples/redhatopenshift/v1api/refs/v1api20220401_roleassignment_from_sp_to_vnet.yaml

matthchr · 2024-07-11T21:03:12Z

v2/internal/testcommon/vcr/redact.go

@@ -131,6 +132,18 @@ func hidePasswords(s string) string {
 	return passwordMatcher.ReplaceAllLiteralString(s, "\"{PASSWORD}\"")
 }

+var (
+	secretMatcher  = regexp.MustCompile(`"([A-Za-z]+)?[Ss]ecret":".*"`)


Are we sure this doesn't match other fields that aren't secrets and don't need to be redacted? Should we be looking specifically for clientSecret?

Also did the keyMatcher not work for this?

keyMatcher does not work for this. I've added a comment and and updated the regex to match ([A-Za-z]+)[Ss]ecret":".*. Let me know wdyt about that?

matthchr · 2024-07-11T21:05:53Z

v2/internal/testcommon/vcr/redact.go

@@ -131,6 +132,18 @@ func hidePasswords(s string) string {
 	return passwordMatcher.ReplaceAllLiteralString(s, "\"{PASSWORD}\"")
 }

+var (
+	secretMatcher  = regexp.MustCompile(`"([A-Za-z]+)?[Ss]ecret":".*"`)


If we do need this, consider leaving a comment describing what it does and why we need it in addition to the other matchers. See for example the matchers here: toolkit/testhttprecorder/recorder.go

In fact yours maybe should go there too?

matthchr · 2024-07-15T17:42:09Z

v2/internal/controllers/crd_redhatopenshift_openshiftcluster_20230401_test.go

+	secretName := "aro-secret"
+	secretKey := "client-secret"
+	// TODO: Replace the principalID, clientSecret and clientId vars below with principalId, clientSecret and clientId of your SP
+	principalId := "5c6be76c-5fc4-4817-992d-22027b44c402"


I think these should be env variables too, or hardcoded to 00000's GUIDs?

Is the issue with changing these that the test recordings don't pass then?

super-harsh added 4 commits July 3, 2024 17:53

Add support for ARO clusters

300bb38

Merge branch 'main' into feature/aro-openshift

644c4d5

Mark required properties as secretReference and resourceReference

20b143c

Add recordings and test; Add redact secret logic; update generated files

2650bda

super-harsh requested review from davefellows, theunrepentantgeek, matthchr and babbageclunk as code owners July 8, 2024 06:35

super-harsh added 2 commits July 8, 2024 18:35

Revert

99a809d

update comments

821505a

super-harsh commented Jul 8, 2024

View reviewed changes

v2/internal/testcommon/vcr/redact.go Outdated Show resolved Hide resolved

Update redact secret logic; Make clientID to be imported optionally f…

01664d2

…rom configmap

matthchr reviewed Jul 11, 2024

View reviewed changes

super-harsh added 4 commits July 15, 2024 10:13

Resolve comments

dc9714c

Add new line

ef2e90e

Add makeSecureStringOfLength method

2e77596

update error message

7b7738b

matthchr reviewed Jul 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Azure RedhatOpenshift Clusters #4152

Add support for Azure RedhatOpenshift Clusters #4152

super-harsh commented Jul 8, 2024 •

edited

Loading

matthchr commented Jul 11, 2024

matthchr Jul 11, 2024

matthchr Jul 11, 2024

super-harsh Jul 12, 2024

matthchr Jul 11, 2024

matthchr Jul 11, 2024

super-harsh Jul 12, 2024

matthchr Jul 11, 2024

matthchr Jul 11, 2024

matthchr Jul 11, 2024

super-harsh Jul 15, 2024

matthchr Jul 11, 2024

matthchr Jul 15, 2024

-// TODO: TO re-record this test, create a new service principal and follow the todos below in the test code.
+// TODO: To re-record this test, create a new service principal and save its details into the
+// TODO: ARO_CLIENT_ID, ARO_CLIENT_SECRET, and ARO_PRINCIPAL_ID variables.
+// TODO: This is required because the ARO resource requires a service principal for input currently. Hopefully
+// TODO: we can revisit this in the future when they support other options.

Add support for Azure RedhatOpenshift Clusters #4152

Are you sure you want to change the base?

Add support for Azure RedhatOpenshift Clusters #4152

Conversation

super-harsh commented Jul 8, 2024 • edited Loading

matthchr commented Jul 11, 2024

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

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

super-harsh commented Jul 8, 2024 •

edited

Loading