-
Notifications
You must be signed in to change notification settings - Fork 138
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
Wif creation improvements, including logic to grant support access as part of wif creation. #666
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ckandag
reviewed
Sep 16, 2024
JakobGray
approved these changes
Sep 17, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. In the future I think we could improve the helper to only retry based on the type of GCP error we get
The prior check was lead to custom roles being updated during every wif creation call if the permission set provided was not in the exact order that is returned by GCP- emperically found to be alphabetical. With this change, this assumption is no longer necassary.
…figuration It was discovered through testing that service accounts created on GCP need a duration of time between creation and being referenced, otherwise a BadRequest error occurs. A delayed retry logic is introduced to ensure the service account is available before making additional configuration calls.
renan-campos
force-pushed
the
OCM-10387
branch
from
September 17, 2024 14:46
82efbbb
to
0961981
Compare
ckandag
approved these changes
Sep 17, 2024
ckandag
added a commit
that referenced
this pull request
Oct 15, 2024
-e034b6b Update Konflux references to 2418e94 -5066ea0 Filter wif configs in interactive mode (#660) -878f5e3 Initial refactor to prepare to move the connection builder and config packages to ocm-common -1ea2e05 lint -2c66dc0 removes redundant api url -65bf8cf Add role prefix flag on create wif-config (#662) -a39ce2e Grant access to support group during WifConfig creation (#663) -0275d67 Revert "Grant access to support group during WifConfig creation (#663)" (#664) -7cddc94 Wif creation improvements, including logic to grant support access as part of wif creation. (#666) -7f41626 Update Konflux references -b9a750c UpdatesToKonflux (#668) -e4aa770 OCM-10615 | Implement 'gcp wif-config update' command (#667) -cf6e500 Dry-run wif config delete before tearing down cloud resources (#670) -e18ea10 OCM-11842 | feat: Updates to support GCP-PSC clusters (#672) -893acd5 wif-enable gcp-inquiries (#673) -664b2c4 Replace wif dry-run flag with mode (#671) -df87894 Update Konflux references (#669)
Merged
renan-campos
pushed a commit
that referenced
this pull request
Oct 15, 2024
-e034b6b Update Konflux references to 2418e94 -5066ea0 Filter wif configs in interactive mode (#660) -878f5e3 Initial refactor to prepare to move the connection builder and config packages to ocm-common -1ea2e05 lint -2c66dc0 removes redundant api url -65bf8cf Add role prefix flag on create wif-config (#662) -a39ce2e Grant access to support group during WifConfig creation (#663) -0275d67 Revert "Grant access to support group during WifConfig creation (#663)" (#664) -7cddc94 Wif creation improvements, including logic to grant support access as part of wif creation. (#666) -7f41626 Update Konflux references -b9a750c UpdatesToKonflux (#668) -e4aa770 OCM-10615 | Implement 'gcp wif-config update' command (#667) -cf6e500 Dry-run wif config delete before tearing down cloud resources (#670) -e18ea10 OCM-11842 | feat: Updates to support GCP-PSC clusters (#672) -893acd5 wif-enable gcp-inquiries (#673) -664b2c4 Replace wif dry-run flag with mode (#671) -df87894 Update Konflux references (#669)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes were pushed last week as part of #663, but needed to be reverted due to a timing-related issue on the GCP side.
The issue was investigated and determined to be as follows. The service account is created with an iam api call, but the binding of roles to the service account is made with a cloudresourcemanager api call. It was found that there is a window of time in which the service account created is not yet visible to cloudresourcemanager, resulting in sporadic BadRequest errors. This PR reintroduces the functionality with an additional mechanism to make wif creation robust to these out-of-sync issues.
Additionally there is an improvement to the logic used to determine if a custom role should be updated. I was finding that the osd_deployer_v4.17 role was getting updated every time I called wif create. This was caused by the list of permissions not being in alphabetical order. The new logic will only update the role if the existing and proposed permissions do not have the same elements, regardless of order.
To test this, the following bash command was run. After the changes, the BadRequest errors previously received were not experienced.