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

MIWI cluster Dynamic Validation update for strict 1:1 matching for provided Platform Workload Identity to expected OCP Operators #3966

Merged
merged 7 commits into from
Dec 16, 2024

Conversation

rajdeepc2792
Copy link
Collaborator

@rajdeepc2792 rajdeepc2792 commented Nov 20, 2024

Which issue this PR addresses:

Fixes ARO-10859

What this PR does / why we need it:

  • Static validation at RP/frontend when platformworkloadidentity profile is nil or map is empty.
  • Stop using platformWorkloadIdentityRolesByVersion for the delete flow so that dependency on the roleset for the OCP version can be removed.
  • Fails create/update flow whenever an unexpected Platform Workload Identity is found.
  • Update dynamic validation for strictly matching the Platform Workload Identity's operator names with expected OCP Operators.(Previously additional identities were allowed)

Test plan for issue:

[x] Unit tests were added/updated for the above implementation
[x] Create/Update MIWI cluster in local
[x] CI
[x] e2e

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

Not yet.

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

Feature is not in production yet.

@rajdeepc2792 rajdeepc2792 self-assigned this Nov 20, 2024
@rajdeepc2792 rajdeepc2792 added hold Hold chainsaw Pull requests or issues owned by Team Chainsaw labels Nov 20, 2024
@rajdeepc2792 rajdeepc2792 force-pushed the rajdeepc2792/ARO-10859 branch 3 times, most recently from 744b3ad to da1decc Compare November 22, 2024 13:39
@github-actions github-actions bot added the needs-rebase branch needs a rebase label Nov 22, 2024
Copy link

Please rebase pull request.

Copy link
Contributor

@kimorris27 kimorris27 left a comment

Choose a reason for hiding this comment

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

LGTM except for one tiny suggestion on a cx-facing error message.

@kimorris27
Copy link
Contributor

/azp run e2e

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kimorris27
Copy link
Contributor

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@kimorris27
Copy link
Contributor

E2E failed with a known flake that's already captured in our CI failure epic. I'll wait to run it again until Rajdeep responds to my most recent review.

@rajdeepc2792 rajdeepc2792 force-pushed the rajdeepc2792/ARO-10859 branch from ec9136e to 79f70e5 Compare December 4, 2024 14:39
@rajdeepc2792
Copy link
Collaborator Author

/azp run ci

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

cadenmarchese
cadenmarchese previously approved these changes Dec 5, 2024
Copy link
Collaborator

@cadenmarchese cadenmarchese left a comment

Choose a reason for hiding this comment

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

approving with some small comments. thank you!

Co-authored-by: Caden Marchese <[email protected]>
@cadenmarchese cadenmarchese dismissed kimorris27’s stale review December 16, 2024 15:37

cx-facing error message was changed according to feedback.

@cadenmarchese cadenmarchese merged commit 750f8c5 into master Dec 16, 2024
21 checks passed
slawande2 pushed a commit that referenced this pull request Dec 18, 2024
…ovided Platform Workload Identity to expected OCP Operators (#3966)

* ARO-10859 static validation to reject empty or nil PlatformWorkloadIdentityProfile

* ARO-10859 avoid using platformWorkloadIdentityRolesByVersion for the cluster deletion flow

* ARO-10859 throw an error for create/update flow whenever an unexpected platform identity is found

* ARO-10859 update dynamic validation to reject the create/update flow for unexpected platform workload identity

* ARO-10859 sort required identities for consistent error messaging

* ARO-10859 update empty pwi map validation error message

* ARO-10859 update test case name

Co-authored-by: Caden Marchese <[email protected]>

---------

Co-authored-by: Caden Marchese <[email protected]>
ehvs pushed a commit that referenced this pull request Jan 23, 2025
…ovided Platform Workload Identity to expected OCP Operators (#3966)

* ARO-10859 static validation to reject empty or nil PlatformWorkloadIdentityProfile

* ARO-10859 avoid using platformWorkloadIdentityRolesByVersion for the cluster deletion flow

* ARO-10859 throw an error for create/update flow whenever an unexpected platform identity is found

* ARO-10859 update dynamic validation to reject the create/update flow for unexpected platform workload identity

* ARO-10859 sort required identities for consistent error messaging

* ARO-10859 update empty pwi map validation error message

* ARO-10859 update test case name

Co-authored-by: Caden Marchese <[email protected]>

---------

Co-authored-by: Caden Marchese <[email protected]>
ArrisLee pushed a commit that referenced this pull request Feb 9, 2025
…ovided Platform Workload Identity to expected OCP Operators (#3966)

* ARO-10859 static validation to reject empty or nil PlatformWorkloadIdentityProfile

* ARO-10859 avoid using platformWorkloadIdentityRolesByVersion for the cluster deletion flow

* ARO-10859 throw an error for create/update flow whenever an unexpected platform identity is found

* ARO-10859 update dynamic validation to reject the create/update flow for unexpected platform workload identity

* ARO-10859 sort required identities for consistent error messaging

* ARO-10859 update empty pwi map validation error message

* ARO-10859 update test case name

Co-authored-by: Caden Marchese <[email protected]>

---------

Co-authored-by: Caden Marchese <[email protected]>
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 ready-for-review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants