OCPBUGS-33994: Don't unnecessarily re-reconcile when secrets are externally managed#1042
OCPBUGS-33994: Don't unnecessarily re-reconcile when secrets are externally managed#1042dlom wants to merge 1 commit into
Conversation
openshift-ci-robot
added
jira/severity-low
|
@dlom: This pull request references Jira Issue OCPBUGS-33994, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe PR fixes STS credential handling in two files: the AWS actuator's ChangesSTS Credential Handling Fixes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dlom The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@dlom: This pull request references Jira Issue OCPBUGS-33994, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: jianping-shu. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
@dlom: This pull request references Jira Issue OCPBUGS-33994, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: jianping-shu. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1042 +/- ##
==========================================
- Coverage 47.06% 47.01% -0.05%
==========================================
Files 97 97
Lines 12560 12578 +18
==========================================
+ Hits 5911 5914 +3
- Misses 5994 6008 +14
- Partials 655 656 +1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/aws/actuator/actuator.go (1)
361-363: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider reusing the ARN from the earlier decode instead of calling
awsSTSIAMRoleARNtwice.The function
awsSTSIAMRoleARNis called at line 336 and again at line 363 for the samecr. Since the first call already succeeded (otherwise we'd have returned), the second call is redundant. Consider hoisting the variable from the first call's scope so it can be reused here.♻️ Proposed refactor to avoid redundant decode
if stsDetected { - awsSTSIAMRoleARN, err := awsSTSIAMRoleARN(minterv1.Codec, cr) + roleARN, err := awsSTSIAMRoleARN(minterv1.Codec, cr) if err != nil { return err } - if awsSTSIAMRoleARN == "" { + if roleARN == "" { logger.Debug("CredentialsRequest has no awsSTSIAMRoleARN, no reason to sync") return nil } + // Store for later use in the STS branch + _ = roleARN // will be used below } // ... needsUpdate check ... if stsDetected { logger.Debug("actuator detected STS enabled cluster, enabling STS secret brokering for CredentialsRequests providing an IAM Role ARN") - awsSTSIAMRoleARN, _ := awsSTSIAMRoleARN(minterv1.Codec, cr) + // roleARN was already decoded aboveNote: This requires restructuring to keep
roleARNin scope across both blocks, which may involve declaring it before the firstif stsDetectedblock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/aws/actuator/actuator.go` around lines 361 - 363, The function awsSTSIAMRoleARN is being called redundantly on the same credential request object cr at two different locations. Refactor by declaring the awsSTSIAMRoleARN variable outside and before the first if stsDetected block where it is initially computed, then reuse this variable in the second if stsDetected block instead of calling awsSTSIAMRoleARN again. This eliminates the redundant function call while keeping the ARN value available for both code paths that need it.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/aws/actuator/actuator.go`:
- Around line 361-363: The function awsSTSIAMRoleARN is being called redundantly
on the same credential request object cr at two different locations. Refactor by
declaring the awsSTSIAMRoleARN variable outside and before the first if
stsDetected block where it is initially computed, then reuse this variable in
the second if stsDetected block instead of calling awsSTSIAMRoleARN again. This
eliminates the redundant function call while keeping the ARN value available for
both code paths that need it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c03ba1fd-9efd-4004-a1d5-30e86d86e43f
📒 Files selected for processing (2)
pkg/aws/actuator/actuator.gopkg/operator/credentialsrequest/credentialsrequest_controller.go
|
/assign @jstuever |
|
@dlom: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
xref: OCPBUGS-33994
Summary by CodeRabbit