MIWI API endpoints #3608

kimorris27 · 2024-05-31T15:43:54Z

Which issue this PR addresses:

https://issues.redhat.com/browse/ARO-6448

What this PR does / why we need it:

This PR introduces the API endpoints for the PlatformWorkloadIdentityRoleSet proxy resources as discussed in MIWI design documents. The only deviation from the design docs is that I did not implement an admin PATCH - it's extra work, and we are hardly if ever going to use it (refer to update_ocp_versions.go to see how we do OpenShiftVersions today and talk directly to Cosmos DB without even using the PUT API endpoint). But if the team reviews and thinks a PATCH is worth adding, we can add it.

The three new endpoints are:

External GET (list)
Admin GET (list)
Admin PUT

Test plan for issue:

Unit tests
Tested the endpoints with a full service dev RP
(Should I test in INT as well?)

Is there any documentation that needs to be updated for this PR?

No (I think?)

How do you know this will function as expected in production?

Tested the endpoints with a full service RP, reused existing battle-tested frontend code, and was thorough in writing unit test cases.

cadenmarchese

mostly lgtm! Just a few questions / comments. thanks!

pkg/api/admin/platformworkloadidentityroleset_validatestatic.go

pkg/api/v20240812preview/platformworkloadidentityroleset_convert.go

pkg/frontend/changefeed.go

cmd/aro/rp.go

niontive · 2024-06-03T15:09:35Z

pkg/api/admin/platformworkloadidentityroleset_convert.go

-		out.Properties.PlatformWorkloadIdentityRoles[i].ServiceAccounts = make([]string, 0, len(r.ServiceAccounts))
-		out.Properties.PlatformWorkloadIdentityRoles[i].ServiceAccounts = append(out.Properties.PlatformWorkloadIdentityRoles[i].ServiceAccounts, r.ServiceAccounts...)
+	for _, r := range s.Properties.PlatformWorkloadIdentityRoles {
+		role := PlatformWorkloadIdentityRole{


Nit: space between PlatformWorkloadIdentityRole and {

I'd put a space before the curly brace if for example this were line 23 and I had left out the space after the range expression, but I think it's normal Go style to avoid leaving blank space in that spot in a struct literal, and that's what's done throughout the codebase.

niontive · 2024-06-03T15:20:31Z

pkg/api/admin/platformworkloadidentityroleset_validatestatic.go

 	for i, r := range new.Properties.PlatformWorkloadIdentityRoles {
 		if r.OperatorName == "" {
-			errs = append(errs, api.NewCloudError(http.StatusBadRequest, api.CloudErrorCodeInvalidParameter, fmt.Sprintf("properties.platformWorkloadIdentityRoles[%d].operatorName", i), "Must be provided"))


Instead of comparing struct fields individually, consider using this: https://github.com/go-playground/validator

I played around with it, but I don't think it's good for our static validators simply because it doesn't give the user friendly error messages that we want going to those making API requests. For example:

Key: 'PlatformWorkloadIdentityRole.OperatorName' Error:Field validation for 'OperatorName' failed on the 'required' tag

This error message would be perfectly fine for me as a regular developer of the RP, but it doesn't even include the fully qualified path of the API field presenting the issue, which won't be great for external users. (Well, I say "external users", but this is the admin API... 🙂)

I think adding it in just for this PR would create an inconsistency with respect to our error messaging and that with that in mind it would be best considered as a future refactor that we do across all of the static validators. Your thoughts?

hlipsig · 2024-06-03T17:51:03Z

I agree that patch seems unnecessary. This PR is already large, if we decide we need patch after a while then let's do it in its own PR. Still reviewing code may have more comments later.

hlipsig

half done review for discussion points.

pkg/api/admin/platformworkloadidentityroleset_convert.go

pkg/frontend/admin_platformworkloadidentityroleset_list_test.go

hlipsig · 2024-06-03T23:50:51Z

/azp run e2e

azure-pipelines · 2024-06-03T23:51:01Z

Azure Pipelines successfully started running 1 pipeline(s).

hlipsig · 2024-06-04T19:19:27Z

/azp run ci

I originally wrote the code the way I did so that we could aggregate errors so that we could provide a better UX in cases where there are multiple similar errors in the request content. I found while writing unit tests that aggregating the errors in this way and not wrapping them in a CloudError causes the RP to return an internal server error instead of a 400 bad request. Is there a way we can aggregate the errors and still wrap them in a CloudError? I'm not sure of the formatting requirements for the text of CloudErrors.

…ame time where applicable for better UX

… minor version more readable

kimorris27 · 2024-06-05T21:13:12Z

/azp run ci, e2e

azure-pipelines · 2024-06-05T21:13:26Z

Azure Pipelines successfully started running 2 pipeline(s).

SudoBrendan

LGTM. I left one final comment I honestly don't know the answer to - just want to get a thought out there. Great work!

SudoBrendan · 2024-06-05T22:08:44Z

pkg/frontend/openshiftcluster_preflightvalidation_test.go

 			if err != nil {
 				t.Fatal(err)
 			}
 			oc := tt.preflightRequest()

 			go f.Run(ctx, nil, nil)
-			f.mu.Lock()
+			f.ocpVersionsMu.Lock()


correct - I'm just trying to wrap my head around concurrent writes to different tables. I understand that from a Cosmos perspective, we should be able to write to both containers at the same time, but my question is, can the RP handle that? Would it be safer code-wise to keep all locks as the same thing, even if it is inefficient, so we don't risk a future change mistakingly using the wrong lock, because the RP doesn't write enough to Cosmos for us to care? ... Maybe I'm over-thinking this. 😅

cadenmarchese

lgtm!

kimorris27 added ready-for-review chainsaw Pull requests or issues owned by Team Chainsaw labels May 31, 2024

kimorris27 self-assigned this May 31, 2024

kimorris27 requested review from jewzaam, bennerv, hawkowl, rogbas, petrkotas, jharrington22, cblecker, cadenmarchese, UlrichSchlueter, SudoBrendan, Shivkumar13, yjst2012, jaitaiwan, anshulvermapatel, hlipsig and tiguelu as code owners May 31, 2024 15:43

cadenmarchese reviewed Jun 3, 2024

View reviewed changes

niontive reviewed Jun 3, 2024

View reviewed changes

cmd/aro/rp.go Show resolved Hide resolved

niontive reviewed Jun 3, 2024

View reviewed changes

hlipsig reviewed Jun 3, 2024

View reviewed changes

pkg/api/admin/platformworkloadidentityroleset_convert.go Show resolved Hide resolved

pkg/api/admin/platformworkloadidentityroleset_convert.go Show resolved Hide resolved

kimorris27 force-pushed the kimorris27/ARO-6448-api-endpoints branch from a6c513a to 9607267 Compare June 3, 2024 19:52

cadenmarchese reviewed Jun 3, 2024

View reviewed changes

pkg/frontend/admin_platformworkloadidentityroleset_list_test.go Outdated Show resolved Hide resolved

kimorris27 added 21 commits June 5, 2024 16:12

Implement the external API endpoint for listing role sets

3d74980

Fix a small oversight from earlier on

4e214cf

Add unit tests for the list endpoint

cb91f12

Add unit tests for changefeed changes

3348774

Uncomment the static validator

61d643c

Fix more slice out of bounds bugs in the converters...

3cb6965

Add converter and static validator to the admin API register

b3cccb1

Add list and put endpoints

dd592e1

Fix name of function to match convention

8c76e96

Add unit tests for new API endpoints

3bf8c1f

Fix typo

071294b

Appease the linter

93f970f

Appease the linter

15a1fde

Add TODO comment re: the number of parameters

8476463

Update static validator to return multiple validation issues at the s…

dbf6867

…ame time where applicable for better UX

Add a simple utility function to make semver comparisons of OpenShift…

7f67b55

… minor version more readable

Log error before returning 500 to user

90d2819

Log errors before returning 500 to user

36e847c

Improve naming of unit test cases

35274fb

Add additional unit test cases

d7e58e2

kimorris27 force-pushed the kimorris27/ARO-6448-api-endpoints branch from bae3131 to d7e58e2 Compare June 5, 2024 21:12

SudoBrendan self-requested a review June 5, 2024 21:58

SudoBrendan approved these changes Jun 5, 2024

View reviewed changes

cadenmarchese approved these changes Jun 6, 2024

View reviewed changes

rajdeepc2792 approved these changes Jun 6, 2024

View reviewed changes

cadenmarchese merged commit 31c7252 into master Jun 6, 2024
20 checks passed

SudoBrendan deleted the kimorris27/ARO-6448-api-endpoints branch July 24, 2024 15:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MIWI API endpoints #3608

MIWI API endpoints #3608

kimorris27 commented May 31, 2024

cadenmarchese left a comment

niontive Jun 3, 2024

kimorris27 Jun 3, 2024

niontive Jun 3, 2024

kimorris27 Jun 3, 2024

hlipsig commented Jun 3, 2024

hlipsig left a comment

hlipsig commented Jun 3, 2024

azure-pipelines bot commented Jun 3, 2024

hlipsig commented Jun 4, 2024

kimorris27 commented Jun 5, 2024

azure-pipelines bot commented Jun 5, 2024

SudoBrendan left a comment

SudoBrendan Jun 5, 2024

cadenmarchese left a comment

MIWI API endpoints #3608

MIWI API endpoints #3608

Conversation

kimorris27 commented May 31, 2024

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

How do you know this will function as expected in production?

cadenmarchese left a comment

Choose a reason for hiding this comment

niontive Jun 3, 2024

Choose a reason for hiding this comment

kimorris27 Jun 3, 2024

Choose a reason for hiding this comment

niontive Jun 3, 2024

Choose a reason for hiding this comment

kimorris27 Jun 3, 2024

Choose a reason for hiding this comment

hlipsig commented Jun 3, 2024

hlipsig left a comment

Choose a reason for hiding this comment

hlipsig commented Jun 3, 2024

azure-pipelines bot commented Jun 3, 2024

hlipsig commented Jun 4, 2024

kimorris27 commented Jun 5, 2024

azure-pipelines bot commented Jun 5, 2024

SudoBrendan left a comment

Choose a reason for hiding this comment

SudoBrendan Jun 5, 2024

Choose a reason for hiding this comment

cadenmarchese left a comment

Choose a reason for hiding this comment