OCPBUGS-88511: Pass live ingress domain into import-cluster-config - #10719
OCPBUGS-88511: Pass live ingress domain into import-cluster-config#10719PratikUplenchwar wants to merge 1 commit into
Conversation
Read ingress.config.openshift.io/cluster during add-nodes inspection and include the domain in import-cluster-config.json so assisted-service can validate apps DNS against the live ingress domain instead of the install-time apps.<name>.<baseDomain> default. Assisted-by: Claude Code <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-moderate
|
@PratikUplenchwar: This pull request references Jira Issue OCPBUGS-88511, 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. |
📝 WalkthroughWalkthroughChangesIngress domain propagation
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
![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 (1)
pkg/asset/agent/joiner/clusterinfo_test.go (1)
480-486: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
appsDomainprecedence, not only fallback.The fixture currently sets only
Spec.Domain, so the newSpec.AppsDomainbranch is untested. Set both fields to different values and assert thatIngressDomainequalsAppsDomain.As per path instructions, test files should cover edge cases, especially validation and defaulting logic.
Also applies to: 550-551
🤖 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/asset/agent/joiner/clusterinfo_test.go` around lines 480 - 486, The cluster info fixture in the relevant test cases currently exercises only Spec.Domain fallback. Set both Spec.Domain and Spec.AppsDomain to distinct values, then update the IngressDomain assertions to verify AppsDomain takes precedence; apply the same coverage to the tests around the additional referenced lines.Source: Path instructions
🤖 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 `@pkg/asset/agent/joiner/clusterinfo.go`:
- Around line 220-223: Update ClusterInfo.retrieveIngressDomain to accept the
caller’s context, thread that context through its callback invocation, and pass
it to Ingresses().Get instead of context.Background(). Preserve the existing
ingress lookup and error propagation behavior.
---
Nitpick comments:
In `@pkg/asset/agent/joiner/clusterinfo_test.go`:
- Around line 480-486: The cluster info fixture in the relevant test cases
currently exercises only Spec.Domain fallback. Set both Spec.Domain and
Spec.AppsDomain to distinct values, then update the IngressDomain assertions to
verify AppsDomain takes precedence; apply the same coverage to the tests around
the additional referenced lines.
🪄 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: cb625c3b-c283-4d09-97c0-18ab0555e461
📒 Files selected for processing (8)
cmd/node-joiner/nodejoiner_integration_test.gocmd/node-joiner/testdata/add-nodes.txtcmd/node-joiner/testdata/setup/crds/0000_ingresses_crd.yamlcmd/node-joiner/testdata/setup/default/0010_ingress.yamlpkg/asset/agent/joiner/clusterinfo.gopkg/asset/agent/joiner/clusterinfo_test.gopkg/asset/agent/joiner/importclusterconfig.gopkg/asset/agent/joiner/importclusterconfig_test.go
| func (ci *ClusterInfo) retrieveIngressDomain() error { | ||
| ingress, err := ci.OpenshiftClient.ConfigV1().Ingresses().Get(context.Background(), "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the relevant file and inspect the surrounding methods.
grep -n "retrieveIngressDomain\|Generate(\|ctx" -n pkg/asset/agent/joiner/clusterinfo.go || true
nl -ba pkg/asset/agent/joiner/clusterinfo.go | sed -n '100,260p'Repository: openshift/installer
Length of output: 766
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant section with line numbers using tools available in the sandbox.
sed -n '90,155p' pkg/asset/agent/joiner/clusterinfo.go | cat -n
printf '\n---\n'
sed -n '210,235p' pkg/asset/agent/joiner/clusterinfo.go | cat -n
printf '\n---\n'
# Show the callback type used by Generate if it comes from a helper in the same package.
grep -Rni "func (.*retrieveIngressDomain" pkg/asset/agent/joiner || true
grep -Rni "retrieveIngressDomain," pkg/asset/agent/joiner || trueRepository: openshift/installer
Length of output: 3483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any helper that accepts the functions passed from Generate.
grep -Rni "func .*inspect\|func .*run\|func .*generate" pkg/asset/agent/joiner | head -n 40Repository: openshift/installer
Length of output: 908
Pass the caller’s context into the ingress lookup. retrieveIngressDomain uses context.Background(), so add-nodes cancellation and deadlines won’t stop a hung API request. Thread ctx through the callback and use it in Ingresses().Get(...).
🤖 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/asset/agent/joiner/clusterinfo.go` around lines 220 - 223, Update
ClusterInfo.retrieveIngressDomain to accept the caller’s context, thread that
context through its callback invocation, and pass it to Ingresses().Get instead
of context.Background(). Preserve the existing ingress lookup and error
propagation behavior.
Source: Path instructions
|
@PratikUplenchwar: 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. |
2 participants
Summary
node-joineradd-nodes cluster inspection, readingress.config.openshift.io/clusterand capturespec.appsDomain(if set) orspec.domain.ingressDomaininimport-cluster-config.jsonembedded in the node ISO.ingress_domainso Day-2 apps DNS validation uses the live ingress domain instead of always derivingapps.<name>.<baseDomain>.This is the installer/node-joiner companion for OCPBUGS-88511.
Motivation
oc adm node-image createfailsapps-domain-name-resolved-correctlywhen the cluster ingress domain was changed post-install, because Day-2 validation only had install-time cluster name + base domain.Test plan
go test ./pkg/asset/agent/joiner/IngressDomainimport-cluster-config.jsoncontains it and apps validation uses the custom domainMade with Cursor
Summary by CodeRabbit
New Features
Bug Fixes