AGENT-1493: Promote NoRegistryClusterInstall Feature to Default#2859
AGENT-1493: Promote NoRegistryClusterInstall Feature to Default#2859sadasu wants to merge 4 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sadasu: This pull request references AGENT-1394 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target only the "5.0.0" version, but multiple target versions were set. 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR enables Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @sadasu! Some important instructions when contributing to openshift/api: |
openshift-ci
Bot
added
the
size/XXL
|
[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 |
sadasu
changed the title
|
@sadasu: This pull request references AGENT-1493 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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. |
|
/jira refresh |
|
@sadasu: This pull request references AGENT-1493 which is a valid 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. |
| api-approved.openshift.io: https://github.com/openshift/api/pull/2510 | ||
| api.openshift.io/merged-by-featuregates: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| release.openshift.io/feature-set: CustomNoUpgrade,DevPreviewNoUpgrade,TechPreviewNoUpgrade |
There was a problem hiding this comment.
Looks like the IRI object itself is still v1alpha1 only: https://github.com/openshift/api/blob/master/machineconfiguration/v1alpha1/types_internalreleaseimage.go ?
We don't allow GA'ing with a alpha API. We would need to graduate the API to v1 first, before we graduate the dependent feature.
There was a problem hiding this comment.
Do we require a separate PR for that?
There was a problem hiding this comment.
@andfasano Yes, lets create a separate PR for that.
|
Requires #2863 |
openshift-ci
Bot
added
the
needs-rebase
e5c1c9a to
230642e
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
Do we have any specific periodic tests that demonstrate this feature behaving as described? |
Yes, we added four different TP periodic jobs to cover all the supported platform type/network/topology combinations, here: https://sippy.dptools.openshift.org/sippy-ng/jobs/5.0?filters=%257B%2522items%2522%253A%255B%257B%2522columnField%2522%253A%2522variants%2522%252C%2522operatorValue%2522%253A%2522has%2520entry%2522%252C%2522value%2522%253A%2522never-stable%2522%252C%2522not%2522%253Atrue%257D%252C%257B%2522id%2522%253A99%252C%2522columnField%2522%253A%2522name%2522%252C%2522operatorValue%2522%253A%2522contains%2522%252C%2522value%2522%253A%2522iso-no-registry%2522%257D%255D%257D&sort=asc&sortField=net_improvement. Please note that the latest PR for bumping the IRI api to v1 successfully landed on Friday, but we didn't yet get a freshly updated green 5.0 nightly payload, thus the current stripe of failures. |
|
Could we please add a mapping that maps the |
Recently we did some work with TRT to add the mapping in Sippy CR openshift-eng/ci-test-mapping#767. We've been able also to start gathering some data from the aforementioned jobs, this view seems to work right now |
|
Functionally speaking the code is fine. I think this is ready for merge once feature-promotion is green |
|
/test verify-feature-promotion
|
|
/test verify-feature-promotion |
|
It looks like at least now the tests are fetched |
|
The fix for the single failing test openshift/origin#31394 landed and it was included in https://amd64.ocp.releases.ci.openshift.org/releasestream/5.0.0-0.nightly/release/5.0.0-0.nightly-2026-07-17-034502, so the trend should improve from the next green nightly (they are still red). The latest two runs per job installed and verified the cluster correctly, they just failed as per #31394 |
|
/test verify-feature-promotion |
Feature NoRegistryClusterInstall is ready to be supported during Agent installs by default.
230642e to
2db4418
Compare
|
/test verify-feature-promotion |
2db4418 to
888d0e1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tools/codegen/pkg/sippy/json_types.go (1)
182-278: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
QueriesForWithCapabilityis a near-verbatim copy ofQueriesFor.The only functional difference from
QueriesFor(Lines 90-180) is the addedCapability:<capability>base item; the network-stack/OS handling, jobTiers parse/dedupe/default logic, and per-tier query generation are duplicated. Consider building the base items and delegating, e.g. haveQueriesForWithCapabilityappend the capability item and share the tier-expansion tail (or add an optional-capability parameter to a single internal builder), so the two don't diverge on future changes.🤖 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 `@tools/codegen/pkg/sippy/json_types.go` around lines 182 - 278, Refactor QueriesForWithCapability and QueriesFor to share one internal query-building path, such as a builder accepting an optional capability filter. Preserve the existing networkStack/OS handling, jobTiers parsing, deduplication, defaults, and per-tier query generation, while adding the Capability variant only for QueriesForWithCapability and removing the duplicated implementation.tools/codegen/cmd/featuregate-test-analyzer.go (1)
1055-1145: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
getInstallTestLevelDatalargely duplicateslistTestResultForVariant.The transport/client construction,
getRelease()call, per-query URL building, request/response handling, and current-vs-previous accumulation are copied almost verbatim fromlistTestResultForVariant(Lines 1147-1266). Consider extracting a shared helper (e.g. one that takes the builtqueriesand returns aggregatedTestResults) to avoid the two paths drifting apart.🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 1055 - 1145, Refactor getInstallTestLevelData and listTestResultForVariant to share a helper that accepts the built queries and performs release lookup, Sippy client setup, request/response processing, and current-versus-previous run aggregation into TestResults. Remove the duplicated logic from both callers while preserving their existing query construction and returned results.
🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 1060-1069: Remove InsecureSkipVerify from the tls.Config used by
defaultTransport so Sippy requests use certificate verification. Rely on the
system trust store by default, or explicitly load the required private CA into
RootCAs if Sippy depends on one.
- Around line 529-560: Update checkIfTestingIsSufficient so Install feature
gates use the dedicated installTestLevelData results when locating “install
should succeed: overall”, rather than testedVariant.TestResults. Thread the
install results through the relevant callers and preserve the existing
run-count, pass-rate, and warning validation behavior.
- Around line 38-44: Set requiredPassRateForInstallTest to an explicit 1.0
instead of reusing requiredPassRateOfTestsPerVariant, keeping the
install-success threshold independent so only a 100% pass rate satisfies the
check and reporting.
---
Nitpick comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 1055-1145: Refactor getInstallTestLevelData and
listTestResultForVariant to share a helper that accepts the built queries and
performs release lookup, Sippy client setup, request/response processing, and
current-versus-previous run aggregation into TestResults. Remove the duplicated
logic from both callers while preserving their existing query construction and
returned results.
In `@tools/codegen/pkg/sippy/json_types.go`:
- Around line 182-278: Refactor QueriesForWithCapability and QueriesFor to share
one internal query-building path, such as a builder accepting an optional
capability filter. Preserve the existing networkStack/OS handling, jobTiers
parsing, deduplication, defaults, and per-tier query generation, while adding
the Capability variant only for QueriesForWithCapability and removing the
duplicated implementation.
🪄 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: 4cbe27d3-b167-420d-a2f5-1725ef100c38
📒 Files selected for processing (3)
tools/codegen/cmd/featuregate-test-analyzer.gotools/codegen/cmd/featuregate-test-analyzer_test.gotools/codegen/pkg/sippy/json_types.go
| defaultTransport := &http.Transport{ | ||
| Proxy: http.ProxyFromEnvironment, | ||
| ForceAttemptHTTP2: true, | ||
| MaxIdleConns: 100, | ||
| IdleConnTimeout: 90 * time.Second, | ||
| TLSHandshakeTimeout: 10 * time.Second, | ||
| ExpectContinueTimeout: 1 * time.Second, | ||
| TLSClientConfig: &tls.Config{ | ||
| InsecureSkipVerify: true, | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file and inspect the relevant area.
ast-grep outline tools/codegen/cmd/featuregate-test-analyzer.go --view expanded || true
echo '---'
sed -n '1020,1105p' tools/codegen/cmd/featuregate-test-analyzer.go
echo '--- SEARCH ---'
rg -n "InsecureSkipVerify|ProxyFromEnvironment|Sippy|sippy|RootCAs|MinVersion|tls.VersionTLS12|http.Transport" tools/codegen/cmd/featuregate-test-analyzer.go tools/codegen -SRepository: openshift/api
Length of output: 25449
Avoid disabling TLS verification here. InsecureSkipVerify: true leaves the Sippy requests open to MITM; use the system trust store or a dedicated RootCAs bundle instead. If Sippy needs a private CA, load it explicitly.
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 1059-1069: This http.Transport is configured with a tls.Config that sets InsecureSkipVerify: true, which disables TLS certificate verification for every request made through the resulting http.Client. The server's certificate chain and host name are not validated, exposing the connection to man-in-the-middle attacks. Remove InsecureSkipVerify (or set it to false) and supply a proper RootCAs pool if you need to trust custom certificates.
Context: http.Transport{
Proxy: http.ProxyFromEnvironment,
ForceAttemptHTTP2: true,
MaxIdleConns: 100,
IdleConnTimeout: 90 * time.Second,
TLSHandshakeTimeout: 10 * time.Second,
ExpectContinueTimeout: 1 * time.Second,
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true,
},
}
Note: [CWE-295] Improper Certificate Validation.
(http-transport-tls-skip-verify-go)
[warning] 1066-1068: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: true,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(missing-ssl-minversion-go)
🪛 OpenGrep (1.25.0)
[ERROR] 1067-1069: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
[ERROR] 1067-1069: TLS certificate verification is disabled via InsecureSkipVerify. This allows man-in-the-middle attacks. Remove InsecureSkipVerify or set it to false.
(coderabbit.tls.go-insecure-skip-verify)
🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 1060 - 1069,
Remove InsecureSkipVerify from the tls.Config used by defaultTransport so Sippy
requests use certificate verification. Rely on the system trust store by
default, or explicitly load the required private CA into RootCAs if Sippy
depends on one.
Source: Linters/SAST tools
|
/test verify-feature-promotion |
|
PR-Agent: could not find a component named |
Augment `verify-feature-promotion` output to indicate pass percentage for `install should succeed` tests for featuregates that include "Install" in their name. This update gives a better indication of whether Install features are failing at installation or later during execution of e2e conformance tests. This update does not change the criteria for reporting success but adds more information in the output for easier analysis of feature state.
888d0e1 to
a59e7e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tools/codegen/cmd/featuregate-test-analyzer.go (1)
996-1006: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
InsecureSkipVerify: truecopy-pasted into the newgetInstallTestLevelDatatransport.This repeats the same TLS-verification-disabled pattern already flagged elsewhere in this file for
listTestResultForVariant/verifyJobBasedFeatureGatePromotion. Every Sippy request from this client is exposed to MITM. Also missingMinVersion.🔒 Suggested fix
TLSClientConfig: &tls.Config{ - InsecureSkipVerify: true, + MinVersion: tls.VersionTLS12, },🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 996 - 1006, Update the transport used by getInstallTestLevelData to remove TLS verification bypass by eliminating InsecureSkipVerify, and configure TLSClientConfig with the required minimum TLS version. Preserve the existing HTTP transport settings and ensure all Sippy requests use certificate and hostname verification.
🧹 Nitpick comments (2)
tools/codegen/cmd/featuregate-test-analyzer.go (2)
991-1081: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated Sippy HTTP client construction.
The
defaultTransport/sippyClientblock here duplicates the identical boilerplate inlistTestResultForVariant(Lines 1098-1113) andverifyJobBasedFeatureGatePromotion(Lines 1219-1234). Adding a fourth copy each time a new Sippy caller is introduced compounds the TLS-config issue above and the general maintenance burden.🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 991 - 1081, Extract the shared defaultTransport and sippyClient construction from getInstallTestLevelData, listTestResultForVariant, and verifyJobBasedFeatureGatePromotion into a reusable helper. Update all three callers to use that helper, preserving the existing timeout, transport, proxy, HTTP/2, connection, and TLS settings.
868-874: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid appending duplicate metal
single/compactvariants.filterVariants(...)can already return theseJobVariants, so this unconditional loop can repeat the same Sippy calls for metal-specific feature gates.🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go` around lines 868 - 874, Update the loop over optionalSelfManagedPlatformVariants in the feature-gate variant selection flow to append metal single and compact variants only when they are not already present in jobVariantsToCheck. Reuse the existing JobVariant identity or comparison logic, preserving inclusion of missing metal variants without duplicating entries returned by filterVariants.
🤖 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 `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Line 187: Thread installTestLevelData from listTestResultFor through
checkIfTestingIsSufficient and use it for the “install should succeed: overall”
validation instead of testedVariant.TestResults. Preserve existing validation
for other feature-gate types, and ensure missing install data remains blocking
according to the intended 100% install requirement.
---
Duplicate comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 996-1006: Update the transport used by getInstallTestLevelData to
remove TLS verification bypass by eliminating InsecureSkipVerify, and configure
TLSClientConfig with the required minimum TLS version. Preserve the existing
HTTP transport settings and ensure all Sippy requests use certificate and
hostname verification.
---
Nitpick comments:
In `@tools/codegen/cmd/featuregate-test-analyzer.go`:
- Around line 991-1081: Extract the shared defaultTransport and sippyClient
construction from getInstallTestLevelData, listTestResultForVariant, and
verifyJobBasedFeatureGatePromotion into a reusable helper. Update all three
callers to use that helper, preserving the existing timeout, transport, proxy,
HTTP/2, connection, and TLS settings.
- Around line 868-874: Update the loop over optionalSelfManagedPlatformVariants
in the feature-gate variant selection flow to append metal single and compact
variants only when they are not already present in jobVariantsToCheck. Reuse the
existing JobVariant identity or comparison logic, preserving inclusion of
missing metal variants without duplicating entries returned by filterVariants.
🪄 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: e481f901-cac2-4c1e-87c1-b1aec2ff3eba
📒 Files selected for processing (3)
tools/codegen/cmd/featuregate-test-analyzer.gotools/codegen/cmd/featuregate-test-analyzer_test.gotools/codegen/pkg/sippy/json_types.go
🚧 Files skipped from review as they are similar to previous changes (2)
- tools/codegen/pkg/sippy/json_types.go
- tools/codegen/cmd/featuregate-test-analyzer_test.go
|
/test verify-feature-promotion |
|
PR-Agent: could not find a component named |
Ignore runs with internal and external infrastrcuture failures while calculating pass percentages.
|
@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. |
6 participants
Feature NoRegistryClusterInstall is ready to be supported during Agent installs by default.