fix: handle transient errors and IsServerTimeout in TLS profile resolution - #160
fix: handle transient errors and IsServerTimeout in TLS profile resolution#160ugiordan wants to merge 1 commit into
Conversation
Sorry, something went wrong.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ugiordan The full list of commands accepted by this bot can be found here. Details Needs approval from an approver in each of these files:Approvers can indicate their approval by writing /approve in a comment |
Sorry, something went wrong.
📝 Walkthrough
WalkthroughThe operator now bounds APIServer TLS profile and adherence requests with 10-second timeout contexts. Transient TLS profile errors use Intermediate defaults and enable watcher-based reloads. TLS adherence errors no longer prevent startup; they are logged for watcher retries. TLS option storage is pre-sized, error comparisons use the standard errors package, and registry permissions requests advertise HTTP/2 and HTTP/1.1. Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (2 warnings)
Comment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify 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 `@infra/feast-operator/cmd/main.go`: - Around line 142-144: Update the bootstrap API call flow around bootstrapCtx so FetchAPIServerTLSProfile and FetchAPIServerTLSAdherencePolicy each receive their own independent 10-second context, rather than sharing a single timeout. Create and cancel each context per call while preserving the existing timeout and cancellation behavior. - Around line 174-178: Update the TLS adherence fetch flow around FetchAPIServerTLSAdherencePolicy so tlsAdherenceFetched is true only when the fetch succeeds; leave it false on error, matching the existing tlsProfileFetched behavior. Ensure watcher.InitialTLSAdherencePolicy is not wired from the zero-value tlsAdherence when the policy fetch failed.
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 6319f14e-a132-4db8-8c99-857b968b24de
📥 CommitsReviewing files that changed from the base of the PR and between c1e64e2 and da15060.
📒 Files selected for processing (1)- infra/feast-operator/cmd/main.go
Sorry, something went wrong.
|
/retest |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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 `@infra/feast-operator/internal/controller/registry/client.go`: - Around line 82-85: Remove InsecureSkipVerify from the TLSClientConfig used by the registry client, and configure RootCAs with the trusted registry CA so certificate and hostname validation remain enabled for all HTTPS requests. Preserve the existing NextProtos settings.
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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c52be753-115b-4b3b-bf3d-34b4406c36b6
📥 CommitsReviewing files that changed from the base of the PR and between c7b9f67 and 1a7c340.
📒 Files selected for processing (1)- infra/feast-operator/internal/controller/registry/client.go
Sorry, something went wrong.
| TLSClientConfig: &tls.Config{ | ||
| InsecureSkipVerify: true, | ||
| NextProtos: []string{"h2", "http/1.1"}, | ||
| }, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Remove InsecureSkipVerify (CWE-295).
Every registry HTTPS request bypasses certificate and hostname validation, allowing a man-in-the-middle attacker to intercept the bearer token and permission responses. Configure the registry’s trusted CA through RootCAs instead.
Proposed fix TLSClientConfig: &tls.Config{
- InsecureSkipVerify: true,
- NextProtos: []string{"h2", "http/1.1"},
+ RootCAs: trustedRegistryCAs,
+ NextProtos: []string{"h2", "http/1.1"},
},[warning] 80-85: 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{
TLSClientConfig: &tls.Config{
InsecureSkipVerify: true,
NextProtos: []string{"h2", "http/1.1"},
},
}
Note: [CWE-295] Improper Certificate Validation.
(http-transport-tls-skip-verify-go)
🪛 OpenGrep (1.25.0)[ERROR] 82-85: 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] 82-85: 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 AgentsVerify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@infra/feast-operator/internal/controller/registry/client.go` around lines 82 - 85, Remove InsecureSkipVerify from the TLSClientConfig used by the registry client, and configure RootCAs with the trusted registry CA so certificate and hostname validation remain enabled for all HTTPS requests. Preserve the existing NextProtos settings.
Sources: Path instructions, Linters/SAST tools
Sorry, something went wrong.
Follow-up to feast-dev#6567. Fixes two issues in the TLS profile integration: 1. Else-only pattern: NewTLSConfigFromProfile was only called in the success branch of the TLS profile fetch. On any error path (non-OpenShift cluster, resource not found), no TLS config was applied, leaving the operator running with Go's bare defaults (no MinVersion, no cipher restrictions). This PR moves NewTLSConfigFromProfile outside the if/else and explicitly falls back to Intermediate on all error paths. 2. No transient error handling: The error switch only handled IsNotFound and IsNoMatchError. Transient API errors (ServiceUnavailable, Timeout, ServerTimeout, TooManyRequests, context.DeadlineExceeded) hit the default case and crashed the operator with os.Exit(1). This PR adds a dedicated case for these errors with a graceful Intermediate fallback. tlsProfileFetched is set to true so the SecurityProfileWatcher self-heals when the API recovers. Signed-off-by: Ugo Giordano <ugiordan@redhat.com>
|
Closing in favor of upstream feast-dev#6587 which contains all the same fixes. Will sync to midstream after upstream merges. |
Sorry, something went wrong.
|
@ugiordan: The following test has Failed: OCI Artifact Browser URLInspecting Test Artifacts ManuallyTo inspect your test artifacts manually, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:feast-group-test-gz6kt |
Sorry, something went wrong.
|
@ugiordan: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
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. |
Sorry, something went wrong.
Description
Cherry-pick from upstream feast-dev/feast#6567 plus IsServerTimeout fix.
The current TLS integration calls os.Exit(1) on all API errors except IsNoMatchError and IsNotFound. This means transient errors (API server unavailable, timeout, throttling) during startup prevent the operator from starting instead of falling back to Intermediate TLS defaults.
Changes
Summary by CodeRabbit