Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix MIWI admin update #4046

Merged
merged 4 commits into from
Jan 23, 2025
Merged

Conversation

kimorris27
Copy link
Contributor

@kimorris27 kimorris27 commented Jan 13, 2025

Which issue this PR addresses:

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

What this PR does / why we need it:

This PR fixes several small issues with admin updates on MIWI clusters. Each one of my original three commits to this branch fixes one issue.

Test plan for issue:

  • Tested all four different types of admin updates against a dev cluster and made sure that the update succeeded and no fields in the cluster doc were clobbered/erased
  • After each of the admin updates runs, did an admin GET and verified that all MIWI-specific fields were present in cluster doc and available via the admin API (except for the identity URL and the bound service account signing key, which I don't think we need to be there)
  • Updated unit tests where applicable

Note that the Jira says we should add unit tests to ensure that none of the MIWI fields are clobbered/erased. It can't really be effectively unit tested. We could add it to MIWI E2E tests though - it would be very easy to check there.

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

N/A

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

Will be tested in canary before the MIWI feature goes live.

Copy link
Collaborator

@tsatam tsatam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change LGTM as-is, but some follow-up questions/actions:

  • After each of the admin updates runs, did an admin GET and verified that all MIWI-specific fields were present in cluster doc and available via the admin API (except for the identity URL and the bound service account signing key, which I don't think we need to be there)

Are the identity URL and boundSASigningKey still present on the clusterdoc in Cosmos and just not displayed on the admin API, or are they actually wiped from the database? Even if we don't technically need these values after a successful install, I think we should try to preserve them in the database in case there's something in the future that requires their use, but we absolutely should not be exposing them on the admin API.

Note that the Jira says we should add unit tests to ensure that none of the MIWI fields are clobbered/erased. It can't really be effectively unit tested. We could add it to MIWI E2E tests though - it would be very easy to check there.

They're a little cumbersome to write (apologies), but the frontend's putorpatch test would be the place to add unit tests: https://github.com/Azure/ARO-RP/blob/master/pkg/frontend/openshiftcluster_putorpatch_test.go. These are our "these put/patch payloads result in this database result and responses" unit tests as far as I'm aware. I'd be fine with these tests added in a follow-up PR though.

@kimorris27
Copy link
Contributor Author

Are the identity URL and boundSASigningKey still present on the clusterdoc in Cosmos and just not displayed on the admin API, or are they actually wiped from the database?

They're preserved in CosmosDB but not included in the admin API response.

the frontend's putorpatch test would be the place to add unit tests

Oh, right. I can fit those unit tests into this PR!

@cadenmarchese cadenmarchese added the next-release To be included in the next RP release rollout label Jan 21, 2025
@cadenmarchese
Copy link
Collaborator

They're a little cumbersome to write (apologies), but the frontend's putorpatch test would be the place to add unit tests: https://github.com/Azure/ARO-RP/blob/master/pkg/frontend/openshiftcluster_putorpatch_test.go. These are our "these put/patch payloads result in this database result and responses" unit tests as far as I'm aware. I'd be fine with these tests added in a follow-up PR though.

I'm going to opt to merge this now so it makes it in our release - we can handle this in a followup PR.

@cadenmarchese cadenmarchese merged commit 354cc86 into master Jan 23, 2025
22 checks passed
LiniSusan pushed a commit that referenced this pull request Jan 30, 2025
* Fix admin API - add readOnly tag to two fields that need it

* Fix admin update - if cluster is MIWI, skip step specific to service
principal clusters

* Fix admin update - ensure that MIWI cluster doc fields are not overwritten

* Remove extra whitespace to appease linter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chainsaw Pull requests or issues owned by Team Chainsaw next-release To be included in the next RP release rollout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants