ROSAENG-6793 | feat: read noproxy default domains from ocm api#1174
Merged
openshift-merge-bot[bot] merged 1 commit intoJun 10, 2026
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 49 minutes and 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
WalkthroughRefactors proxy no_proxy filtering to accept caller-supplied default domains and adds a cluster helper (reviseZeroEgressNoProxy) that fetches API zero-egress defaults and removes them from state.Proxy.NoProxy during Create, Read, and Update. Tests and PATCH expectations updated accordingly. ChangesZero-egress proxy domain filtering
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
provider/proxy/utils.go (1)
9-10: ⚡ Quick winRemove unused regex pattern variables.
These regex patterns are no longer used after the refactor from regex-based to exact-match filtering.
♻️ Proposed fix
-var awsRegionRegexFmt = "(?:af|ap|ca|eu|me|sa|us)(?:-gov)?-(?:central|north|(?:north(?:east|west))|south|south(?:east|west)|east|west)-\\d+(?:[a-z]{1})?" -var awsAccountIdRegexFmt = "\\d{12}" - func RemoveNoProxyZeroEgressDefaultDomains(input string, separator string, defaultDomains []string) string {🤖 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 `@provider/proxy/utils.go` around lines 9 - 10, Remove the now-unused regex pattern variables awsRegionRegexFmt and awsAccountIdRegexFmt from provider/proxy/utils.go; locate and delete the declarations of awsRegionRegexFmt and awsAccountIdRegexFmt so the file no longer contains dead variables after the refactor to exact-match filtering.provider/clusterrosa/hcp/resource.go (2)
2251-2254: ⚡ Quick winWrap error with actionable context.
The error should include context identifying the operation that failed.
♻️ Proposed fix
get, err := r.ClusterCollection.Cluster(object.ID()).Get().Parameter("fetch_service_inquiries", true).SendContext(ctx) if err != nil { - return err + return fmt.Errorf("failed to fetch cluster with service inquiries for zero-egress no_proxy filtering: %w", err) }As per coding guidelines: "In helper functions, wrap errors with
fmt.Errorf("...: %w", err)."🤖 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 `@provider/clusterrosa/hcp/resource.go` around lines 2251 - 2254, The call to r.ClusterCollection.Cluster(object.ID()).Get().Parameter("fetch_service_inquiries", true).SendContext(ctx) returns err but is returned as-is; wrap this error with actionable context using fmt.Errorf, e.g. return fmt.Errorf("failed to fetch cluster via ClusterCollection.Cluster(%s).Get().SendContext: %w", object.ID(), err) so callers know which operation failed; update the error return where get, err are assigned in the helper function.
2234-2259: ⚡ Quick winAdd defensive nil check before accessing
state.Proxy.NoProxy.While the call sites ensure
populateRosaHcpClusterStateruns first (which initializesstate.Proxywhen the API object has a proxy), a defensive nil check would make this function safer and more self-contained.🛡️ Proposed fix
defaultDomains := get.Body().AWS().ZeroEgress().NoProxyDefaultDomains() deDuplicatedNoProxy := proxy.RemoveNoProxyZeroEgressDefaultDomains(noProxy, ",", defaultDomains) + if state.Proxy == nil { + state.Proxy = &proxy.Proxy{} + } state.Proxy.NoProxy = types.StringValue(deDuplicatedNoProxy) return nil }🤖 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 `@provider/clusterrosa/hcp/resource.go` around lines 2234 - 2259, In reviseZeroEgressNoProxy, guard against state.Proxy being nil before assigning state.Proxy.NoProxy: check if state.Proxy == nil and if so initialize it (e.g., state.Proxy = &<proxy-struct-type>{}) then set state.Proxy.NoProxy = types.StringValue(deDuplicatedNoProxy); this makes reviseZeroEgressNoProxy self-contained and prevents nil-pointer panics when writing to state.Proxy.NoProxy.subsystem/hcp/cluster_resource_test.go (1)
4538-4670: ⚡ Quick winConsider adding an Update scenario test.
The test correctly verifies zero-egress default domain filtering during Create and Read operations. However, per the PR description, the filtering should also apply during Update. Consider adding a test case that:
- Creates a cluster with zero_egress and custom no_proxy
- Updates the no_proxy value
- Verifies the updated state correctly filters defaults from the new value
This would ensure the filtering logic works correctly across all CRUD operations mentioned in the PR objectives.
As per coding guidelines, behavior-changing changes should add or update subsystem tests that cover the changed behavior. The Update path is part of the behavior change.
🤖 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 `@subsystem/hcp/cluster_resource_test.go` around lines 4538 - 4670, Add an Update-path test mirroring the existing "Filters zero-egress default domains from no_proxy during read" It block: create the cluster as currently done, then simulate an Update by appending TestServer handlers for the update request (use VerifyRequest for the cluster update endpoint—same route used for Read, e.g., cluster123Route—and RespondWithPatchedJSON to return a payload where the proxy.no_proxy contains the custom value plus appended zero_egress default domains), run a Terraform update/apply with a changed proxy.no_proxy value, and then assert (using Terraform.Resource("rhcs_cluster_rosa_hcp", "my_cluster") and MatchJQ on .attributes.proxy.no_proxy) that the state filters out the default domains; reference the existing test helpers VerifyRequest, RespondWithPatchedJSON, Terraform.Apply(), and Terraform.Resource() to locate where to add the new handlers and assertions.
🤖 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.
Inline comments:
In `@provider/proxy/utils.go`:
- Around line 12-23: The function RemoveNoProxyZeroEgressDefaultDomains splits
by the provided separator but mistakenly rejoins using a hardcoded ","; change
the final join to use the same separator parameter (replace
strings.Join(domains, ",") with strings.Join(domains, separator)) so the output
matches the input delimiter; locate the code in the
RemoveNoProxyZeroEgressDefaultDomains function and update that call.
---
Nitpick comments:
In `@provider/clusterrosa/hcp/resource.go`:
- Around line 2251-2254: The call to
r.ClusterCollection.Cluster(object.ID()).Get().Parameter("fetch_service_inquiries",
true).SendContext(ctx) returns err but is returned as-is; wrap this error with
actionable context using fmt.Errorf, e.g. return fmt.Errorf("failed to fetch
cluster via ClusterCollection.Cluster(%s).Get().SendContext: %w", object.ID(),
err) so callers know which operation failed; update the error return where get,
err are assigned in the helper function.
- Around line 2234-2259: In reviseZeroEgressNoProxy, guard against state.Proxy
being nil before assigning state.Proxy.NoProxy: check if state.Proxy == nil and
if so initialize it (e.g., state.Proxy = &<proxy-struct-type>{}) then set
state.Proxy.NoProxy = types.StringValue(deDuplicatedNoProxy); this makes
reviseZeroEgressNoProxy self-contained and prevents nil-pointer panics when
writing to state.Proxy.NoProxy.
In `@provider/proxy/utils.go`:
- Around line 9-10: Remove the now-unused regex pattern variables
awsRegionRegexFmt and awsAccountIdRegexFmt from provider/proxy/utils.go; locate
and delete the declarations of awsRegionRegexFmt and awsAccountIdRegexFmt so the
file no longer contains dead variables after the refactor to exact-match
filtering.
In `@subsystem/hcp/cluster_resource_test.go`:
- Around line 4538-4670: Add an Update-path test mirroring the existing "Filters
zero-egress default domains from no_proxy during read" It block: create the
cluster as currently done, then simulate an Update by appending TestServer
handlers for the update request (use VerifyRequest for the cluster update
endpoint—same route used for Read, e.g., cluster123Route—and
RespondWithPatchedJSON to return a payload where the proxy.no_proxy contains the
custom value plus appended zero_egress default domains), run a Terraform
update/apply with a changed proxy.no_proxy value, and then assert (using
Terraform.Resource("rhcs_cluster_rosa_hcp", "my_cluster") and MatchJQ on
.attributes.proxy.no_proxy) that the state filters out the default domains;
reference the existing test helpers VerifyRequest, RespondWithPatchedJSON,
Terraform.Apply(), and Terraform.Resource() to locate where to add the new
handlers and assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0203422d-61e2-4db8-a4f5-59979cb758e8
📒 Files selected for processing (4)
provider/clusterrosa/hcp/resource.goprovider/proxy/utils.goprovider/proxy/utils_test.gosubsystem/hcp/cluster_resource_test.go
marcolan018
force-pushed
the
ROSAENG-6793
branch
2 times, most recently
from
9be46d8 to
6b5ab26
Compare
Contributor
Author
|
/retest |
1 similar comment
Contributor
Author
|
/retest |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
provider/proxy/utils.go (1)
23-26: 💤 Low valueConsider using
struct{}for map values when using map as a set.Using
map[string]struct{}{}instead ofmap[string]stringis more idiomatic in Go when the map is used purely for membership checks, as it makes the intent clearer and saves a few bytes per entry.♻️ Optional refactor
- defaultDomainsMap := make(map[string]string) + defaultDomainsMap := make(map[string]struct{}) for _, item := range defaultDomains { - defaultDomainsMap[item] = "" + defaultDomainsMap[item] = struct{}{} }🤖 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 `@provider/proxy/utils.go` around lines 23 - 26, The map defaultDomainsMap is created as map[string]string but used as a set; change its type to map[string]struct{} and populate it with defaultDomainsMap[item] = struct{}{} in the loop that iterates over defaultDomains; also update any membership checks (places using defaultDomainsMap[item] or lookups) to use the idiomatic _, ok := defaultDomainsMap[key] pattern so the code compiles and reflects the set semantics.provider/clusterrosa/hcp/resource.go (3)
2254-2255: ⚡ Quick winAdd defensive nil check for
state.Proxybefore dereferencing.Although the current calling sequence ensures
state.Proxyis non-nil when this line executes (becausepopulateRosaHcpClusterStateinitializes it when the object has proxy), a defensive check would make the code more robust and self-documenting.🛡️ Proposed defensive check
defaultDomains := zeroEgress.NoProxyDefaultDomains() deDuplicatedNoProxy := proxy.RemoveNoProxyZeroEgressDefaultDomains(noProxy, ",", defaultDomains) + if state.Proxy == nil { + return fmt.Errorf("internal error: state.Proxy is nil but object has proxy configured") + } state.Proxy.NoProxy = types.StringValue(deDuplicatedNoProxy) return nilAs per coding guidelines: "Prefer explicit over implicit — especially in exported APIs" and defensive coding against possible future refactoring.
🤖 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 `@provider/clusterrosa/hcp/resource.go` around lines 2254 - 2255, Add a defensive nil check for state.Proxy before assigning state.Proxy.NoProxy: verify that state.Proxy != nil and if nil, initialize it (e.g., allocate a Proxy struct) or skip the assignment so you don't dereference a nil pointer; update the block around deDuplicatedNoProxy := proxy.RemoveNoProxyZeroEgressDefaultDomains(noProxy, ",", defaultDomains) and the subsequent state.Proxy.NoProxy = types.StringValue(deDuplicatedNoProxy) to first ensure state.Proxy is non-nil (or create it) before setting NoProxy.
2249-2251: ⚡ Quick winWrap error with context to identify the operation.
The function returns the raw error from
SendContextwithout adding context about which operation failed. This makes debugging harder for users.📝 Proposed fix
get, err := r.ClusterCollection.Cluster(object.ID()).Get().Parameter("fetch_service_inquiries", true).SendContext(ctx) if err != nil { - return err + return fmt.Errorf("failed to fetch cluster with service inquiries for zero-egress no_proxy filtering: %w", err) }As per coding guidelines: "In helper functions, wrap errors with
fmt.Errorf(\"...: %w\", err). Error messages must identify their origin (e.g.,\"package: message\")."🤖 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 `@provider/clusterrosa/hcp/resource.go` around lines 2249 - 2251, The call to r.ClusterCollection.Cluster(object.ID()).Get().Parameter("fetch_service_inquiries", true).SendContext(ctx) returns a raw error; wrap that error with context per guidelines so callers can identify the failing operation. Replace the direct return of err in the surrounding helper (the function that invokes Cluster(...).Get().SendContext) with a wrapped error using fmt.Errorf including the package/origin and a short description like "provider/clusterrosa: failed to get cluster (Cluster.Get().SendContext) for id %s: %w", referencing object.ID() and the original err; ensure you import fmt if not already imported.
2231-2232: 💤 Low valueAdd a doc comment explaining the function's purpose.
While this is an unexported function, a brief comment would help maintainers understand its purpose and when it should be called, especially given the subtle contract with
populateRosaHcpClusterState.📝 Suggested comment
+// reviseZeroEgressNoProxy filters zero-egress default no_proxy domains from state.Proxy.NoProxy +// when the cluster has zero_egress enabled. Must be called after populateRosaHcpClusterState. func (r *ClusterRosaHcpResource) reviseZeroEgressNoProxy(ctx context.Context, object *cmv1.Cluster, state *ClusterRosaHcpState) error {🤖 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 `@provider/clusterrosa/hcp/resource.go` around lines 2231 - 2232, Add a short doc comment immediately above the unexported function reviseZeroEgressNoProxy describing its purpose, when it should be called, and its contract with populateRosaHcpClusterState; explicitly state preconditions (what fields in ClusterRosaHcpState or *cmv1.Cluster are expected), side-effects (what the function mutates or returns), and why it only runs for the zero-egress/no-proxy case so future maintainers understand its intent and interaction with populateRosaHcpClusterState.
🤖 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.
Inline comments:
In `@provider/clusterrosa/hcp/resource.go`:
- Around line 2249-2253: The call that assigns defaultDomains chains through
get.Body().AWS().ZeroEgress().NoProxyDefaultDomains() without nil checks and can
panic; modify the code around get, err :=
r.ClusterCollection.Cluster(object.ID()).Get().Parameter("fetch_service_inquiries",
true).SendContext(ctx) to validate get and its nested responses before calling
NoProxyDefaultDomains(): verify get != nil, then body := get.Body(); check body
!= nil, body.AWS() != nil, body.AWS().ZeroEgress() != nil and only then call
NoProxyDefaultDomains(); if any intermediate is nil, return an informative error
(or use a safe default) rather than dereferencing nil. Ensure the checks
reference get, get.Body(), AWS(), ZeroEgress(), and NoProxyDefaultDomains() so
reviewers can find and update the exact call site.
In `@subsystem/hcp/cluster_resource_test.go`:
- Around line 4539-4884: Add a new test case that asserts the zero-egress
default-domain filtering runs on update: implement an It block (e.g., "Filters
zero-egress default domains from no_proxy during update (patch)") that first
creates the resource (reuse the POST/GET handlers similar to the Create test),
then simulates an update by changing proxy.no_proxy in Terraform.Source and
triggering Terraform.Apply to perform a PATCH (verify a VerifyRequest with
http.MethodPatch to cluster123Route and respond with a patched JSON body that
includes aws.zero_egress.no_proxy_default_domains and the API-appended domains
in proxy.no_proxy), and finally assert
Terraform.Resource("rhcs_cluster_rosa_hcp","my_cluster").attributes.proxy.no_proxy
equals only the user-specified domains (e.g., "new.custom.domain"); follow the
same handler patterns used in the Create/Read tests so the update path exercises
the same filtering helper.
---
Nitpick comments:
In `@provider/clusterrosa/hcp/resource.go`:
- Around line 2254-2255: Add a defensive nil check for state.Proxy before
assigning state.Proxy.NoProxy: verify that state.Proxy != nil and if nil,
initialize it (e.g., allocate a Proxy struct) or skip the assignment so you
don't dereference a nil pointer; update the block around deDuplicatedNoProxy :=
proxy.RemoveNoProxyZeroEgressDefaultDomains(noProxy, ",", defaultDomains) and
the subsequent state.Proxy.NoProxy = types.StringValue(deDuplicatedNoProxy) to
first ensure state.Proxy is non-nil (or create it) before setting NoProxy.
- Around line 2249-2251: The call to
r.ClusterCollection.Cluster(object.ID()).Get().Parameter("fetch_service_inquiries",
true).SendContext(ctx) returns a raw error; wrap that error with context per
guidelines so callers can identify the failing operation. Replace the direct
return of err in the surrounding helper (the function that invokes
Cluster(...).Get().SendContext) with a wrapped error using fmt.Errorf including
the package/origin and a short description like "provider/clusterrosa: failed to
get cluster (Cluster.Get().SendContext) for id %s: %w", referencing object.ID()
and the original err; ensure you import fmt if not already imported.
- Around line 2231-2232: Add a short doc comment immediately above the
unexported function reviseZeroEgressNoProxy describing its purpose, when it
should be called, and its contract with populateRosaHcpClusterState; explicitly
state preconditions (what fields in ClusterRosaHcpState or *cmv1.Cluster are
expected), side-effects (what the function mutates or returns), and why it only
runs for the zero-egress/no-proxy case so future maintainers understand its
intent and interaction with populateRosaHcpClusterState.
In `@provider/proxy/utils.go`:
- Around line 23-26: The map defaultDomainsMap is created as map[string]string
but used as a set; change its type to map[string]struct{} and populate it with
defaultDomainsMap[item] = struct{}{} in the loop that iterates over
defaultDomains; also update any membership checks (places using
defaultDomainsMap[item] or lookups) to use the idiomatic _, ok :=
defaultDomainsMap[key] pattern so the code compiles and reflects the set
semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6a0fd42c-e127-4291-954a-b85a6f9e73a2
📒 Files selected for processing (4)
provider/clusterrosa/hcp/resource.goprovider/proxy/utils.goprovider/proxy/utils_test.gosubsystem/hcp/cluster_resource_test.go
Contributor
Author
|
/retest |
marcolan018
force-pushed
the
ROSAENG-6793
branch
from
a0e0123 to
bb1cc8a
Compare
Signed-off-by: marcolan018 <llan@redhat.com>
marcolan018
force-pushed
the
ROSAENG-6793
branch
from
6769f83 to
72e76e4
Compare
Contributor
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: olucasfreitas 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-merge-bot
Bot
merged commit 2c2bf1e
into
terraform-redhat:main
15 checks passed
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
PR Summary
Detailed Description of the Issue
Current in production env, for zero egress enabled clusters, client can query the default domains in noproxy thru OCM API. This PR removes the hardcoded domain templates in TF provider, changes to read the default domain list from OCM API.
Related Issues and PRs
#Type of Change
Previous Behavior
Behavior After This Change
How to Test (Step-by-Step)
Preconditions
Test Steps
terraform applyExpected Results
the
terraform applycommand should complete successfullyProof of the Fix
Breaking Changes
Breaking Change Details / Migration Plan
Developer Verification Checklist
[JIRA-TICKET] | [TYPE][(scope)][!]: <MESSAGE>.make install-hookshas been run in this clone.make pre-push-checkspasses.make fmt-checkpasses.make buildpasses.Summary by CodeRabbit