NO-JIRA: fix(awseps): cache local zone ID in non-shared VPC case #7409
Conversation
This PR caches the local zone ID when no using a shared VPC to avoid a AWS Route53 ListHostedZones call on every reconcile.
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
WalkthroughThis pull request refactors the AWS client builder in the awsprivatelink controller to replace direct method access to local hosted zone ID with a getter/setter API pattern. The previous Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
openshift-ci
bot
added
do-not-merge/needs-area
area/control-plane-operator
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sjenning 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
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
sjenning
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@sjenning: This pull request explicitly references no jira issue. 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go (1)
701-709: Consider clearing cached zone ID when transitioning from shared to non-shared VPC.The caching logic works correctly for the primary use case (non-shared VPC). However, when transitioning from shared VPC to non-shared VPC, the
localZoneIDfield retains its value from the shared VPC configuration (set on line 305) rather than being cleared and re-looked up.In
setFromHCP(lines 306-309), consider explicitly clearingb.localZoneID = ""in the else branch to ensure a fresh lookup occurs when switching to non-shared VPC mode.Apply this diff to ensure zone ID is re-looked up when transitioning to non-shared VPC:
} else { b.assumeSharedVPCEndpointRoleARN = "" b.assumeSharedVPCRoute53RoleARN = "" + b.localZoneID = "" }Note: While this scenario (shared→non-shared transition) may be rare in practice, clearing the cached value ensures correctness if such transitions occur.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Red Hat Konflux / control-plane-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-operator-main-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-release-mce-211-on-pull-request
- GitHub Check: Red Hat Konflux / hypershift-cli-mce-211-on-pull-request
🔇 Additional comments (2)
control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go (2)
254-259: LGTM: Thread-safe getter implementation.The method correctly uses mutex protection to safely read the cached zone ID.
261-266: LGTM: Thread-safe setter implementation.The method correctly uses mutex protection to safely update the cached zone ID.
|
/lgtm |
|
/test e2e-aws-minimal |
|
/verified by e2e artifacts |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
verified
|
@sjenning: This PR has been marked as verified by 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. |
|
@sjenning: This pull request explicitly references no jira issue. 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. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| b.assumeSharedVPCEndpointRoleARN = "" | ||
| b.assumeSharedVPCRoute53RoleARN = "" | ||
| b.localZoneID = "" | ||
| } |
There was a problem hiding this comment.
Bug: Stale zone ID used after SharedVPC removal
When transitioning from SharedVPC to non-SharedVPC, the cached localZoneID from the SharedVPC spec is not cleared because the b.localZoneID = "" line was removed from the else branch in setFromHCP. This means that after such a transition, getLocalHostedZoneID() returns the old SharedVPC zone ID instead of empty string, causing the lookupZoneID call to be skipped. DNS records would then be created in the wrong hosted zone. The warnOnDifferentValues function will log a warning but the stale value is still used.
Additional Locations (1)
There was a problem hiding this comment.
we do not support transitioning to/from shared VPC
|
/retest-required |
|
/hold Revision 6237410 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest-required |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/hold Revision 6237410 was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
/retest |
|
/test e2e-aks |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/test e2e-aks-4-21 |
|
/test e2e-aks-4-21 |
|
@sjenning: all tests passed! 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. |
4 participants
This PR caches the local zone ID when not using a shared VPC to avoid a AWS Route53
ListHostedZonescall on every reconcile.Note
Caches and reuses the Route53 local hosted zone ID via clientBuilder to avoid repeated lookups, adding getter/setter and updating reconciliation logic.
localZoneIDinclientBuilderand avoid resetting it when not using Shared VPC.localHostedZoneID()togetLocalHostedZoneID(); addsetLocalHostedZoneID(string).lookupZoneIDonce and store viasetLocalHostedZoneID; subsequent reconciles usegetLocalHostedZoneID()instead of calling Route53 each time.Written by Cursor Bugbot for commit 6237410. This will update automatically on new commits. Configure here.