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

ARO-9382 prevent updating existing platform identities #3786

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

yithian
Copy link
Collaborator

@yithian yithian commented Aug 21, 2024

Which issue this PR addresses:

Fixes ARO-9382

What this PR does / why we need it:

This adds a check to v20240812preview static validation that raises an error if either the name or resource ID of an existing platform identity

Test plan for issue:

Unit tests

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

This is part of the general managed identity / workload identity feature development and this change should be reflected in the final documentation

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

Unit tests for now

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.

LGTM, just one suggestion to support updates that change the order of platform workload identities in the array but otherwise preserve the same name -> resource ID mappings.

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.

I recommended a change to the validation to make sure we account for all possible scenarios.

This adds a check to v20240812preview static validation that raises an
error if either the name or resource ID of an existing platform identity
This allows changing the order of platform identities while still
preventing the resource ID and operator name from being changed
This prevents removal of a platform identity or changing the identity's
OperatorName and ResourceID at the same time
@yithian yithian requested a review from bitoku as a code owner September 3, 2024 20:25
Copy link
Collaborator

@rajdeepc2792 rajdeepc2792 left a comment

Choose a reason for hiding this comment

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

LGTM from functional and unit tests pov, added a comment to use a different data structure for duplication check.

@cadenmarchese cadenmarchese added the chainsaw Pull requests or issues owned by Team Chainsaw label Sep 17, 2024
@SudoBrendan
Copy link
Collaborator

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@rajdeepc2792
Copy link
Collaborator

/azp run ci,e2e

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Collaborator

@rajdeepc2792 rajdeepc2792 left a comment

Choose a reason for hiding this comment

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

LGTM!!

@cadenmarchese
Copy link
Collaborator

/azp run e2e

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Collaborator

@slawande2 slawande2 left a comment

Choose a reason for hiding this comment

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

Just seeking clarification for my understanding- why is UsesWorkloadIdentity moved from pkg/api/openshiftcluster.go to pkg/api/v20240812preview/openshiftcluster.go ?

@tsatam
Copy link
Collaborator

tsatam commented Sep 17, 2024

Just seeking clarification for my understanding- why is UsesWorkloadIdentity moved from pkg/api/openshiftcluster.go to pkg/api/v20240812preview/openshiftcluster.go ?

It was copied, not moved. We maintain separate OpenShiftCluster struct definitions for each individual APIversion we support, so we'd need to maintain duplicate functions for each struct as well.

@cadenmarchese cadenmarchese merged commit 1a2096d into master Sep 18, 2024
24 checks passed
@yithian yithian deleted the yithian/ARO-9382 branch September 19, 2024 14:20
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants