-
Notifications
You must be signed in to change notification settings - Fork 265
OCPBUGS-66104: Update no_proxy list to include the AWS DNS resolver #2853
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
base: master
Are you sure you want to change the base?
Conversation
This well known IP is added as a second resolver when ClusterHostedDNS is enabled. Add it to the no_proxy list.
|
@sadasu: This pull request references Jira Issue OCPBUGS-66104, 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. |
WalkthroughAdds AWS Route 53 resolver IP (169.254.169.253) to the no-proxy configuration for AWS clusters with cluster-hosted DNS. The implementation file updates the no_proxy assembly logic, while the test file introduces a helper function and new test cases to validate the behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sadasu The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/util/proxyconfig/no_proxy.go (1)
120-123: LGTM! AWS Route 53 resolver IP correctly added to no_proxy list.The implementation correctly adds the AWS Route 53 resolver IP for all AWS installations, which aligns with the PR objective. The documentation link provides good context.
Optional: Consider refining the comment for clarity:
- // Add AWS Route 53 resolver IP needs to be added to the no_proxy list for ClusterHostedDNS. + // AWS Route 53 resolver IP should be added to the no_proxy list for ClusterHostedDNS.
📜 Review details
Configuration used: CodeRabbit 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 (2)
pkg/util/proxyconfig/no_proxy.go(1 hunks)pkg/util/proxyconfig/no_proxy_test.go(10 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:
pkg/util/proxyconfig/no_proxy.gopkg/util/proxyconfig/no_proxy_test.go
🔇 Additional comments (3)
pkg/util/proxyconfig/no_proxy_test.go (3)
83-108: LGTM! AWS-specific test helper appropriately structured.The helper function correctly constructs Infrastructure config with ClusterHostedDNSType for testing AWS scenarios. The AWS-only implementation is appropriate since the Route 53 resolver feature is AWS-specific.
160-160: LGTM! Test expectations consistently updated across all AWS scenarios.All AWS test cases now correctly expect both AWS metadata service IP (169.254.169.254) and Route 53 resolver IP (169.254.169.253) in the no_proxy list. The updates are consistent and comprehensive.
Also applies to: 182-182, 193-193, 204-204, 215-215, 226-226, 292-292, 303-303, 314-314
317-327: LGTM! New test case provides explicit ClusterHostedDNS coverage.The test correctly validates that the AWS Route 53 resolver IP is included in the no_proxy list for ClusterHostedDNS scenarios, directly addressing the PR objective.
|
@sadasu: 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. |
This well known IP is added as a second resolver when ClusterHostedDNS is enabled. Add it to the no_proxy list for all cases.