CM-716: Add HTTP01 Challenge Proxy binary and CI Dockerfile#458
CM-716: Add HTTP01 Challenge Proxy binary and CI Dockerfile#458sebrandon1 wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sebrandon1: This pull request references CM-716 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 task 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. |
|
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:
WalkthroughAdds an OpenShift HTTP-01 reverse proxy that discovers cluster VIPs, applies nftables routing, forwards ACME challenge requests, and packages the binary in a UBI-based container image. ChangesHTTP-01 proxy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HTTP01Proxy
participant KubernetesAPI
participant DNS
participant AppsVIP
HTTP01Proxy->>KubernetesAPI: Read OpenShift configuration
HTTP01Proxy->>DNS: Resolve API and Apps VIP addresses
HTTP01Proxy->>KubernetesAPI: Apply nftables MachineConfig resources
HTTP01Proxy->>AppsVIP: Forward ACME challenge requests
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sebrandon1 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: 5
🧹 Nitpick comments (2)
images/ci/http01proxy.Dockerfile (1)
1-12: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPin the base images to approved immutable digests.
The builder tag and
ubi9-minimal:latestare mutable references, so identical builds can produce different binaries or runtime contents. This weakens reproducibility and FIPS/security attestation; Red Hat describes UBI minimal as regularly updated. (catalog.redhat.com)🤖 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 `@images/ci/http01proxy.Dockerfile` around lines 1 - 12, Update both FROM instructions in the Dockerfile to use approved immutable image digests instead of the mutable builder tag and ubi9-minimal:latest reference, preserving the existing builder and runtime images and stage structure.cmd/http01-proxy/pkg/utils.go (1)
47-69: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
req.Header.Set("Host", req.Host)has no effect on the outgoing request.For client requests, Go's
net/httpwrites the wire "Host:" line fromreq.Host(orreq.URL.Hostif empty) — theHeadermap's "Host" entry is ignored byRequest.Write/the transport. This line is dead code. It happens to be harmless today only because the customDirectoralso never setsreq.Host = target.Host, so the original client Host header (needed for the router at the Apps VIP to route the ACME challenge to the right Route) is preserved by default — not because of this line. Recommend removing the no-op call (or, if the intent actually was to override the Host header, usereq.Host = target.Host— but note that would break Host-based routing to the challenge Route at the Apps VIP).♻️ Proposed cleanup
proxy.Director = func(req *http.Request) { req.URL.Scheme = target.Scheme req.URL.Host = target.Host - req.Header.Set("Host", req.Host) req.Header.Set("X-Proxy-Server", "cert-manager-http01-proxy") }🤖 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 `@cmd/http01-proxy/pkg/utils.go` around lines 47 - 69, Remove the no-op req.Header.Set("Host", req.Host) call from the NewReverseProxy Director. Preserve req.Host unchanged so the original client Host remains available for Host-based routing, while retaining the existing target URL scheme and host assignments and other proxy headers.
🤖 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 `@cmd/http01-proxy/main.go`:
- Around line 20-21: Reduce or gate the informational logging in the startup
flow around the OCP API log and the similar log near line 61 so internal values
such as env.APIHostname, env.APIVIP, env.AppsVIP, and backendServer are not
emitted in normal or off-node logs. Preserve only non-sensitive context at the
default verbosity, and expose detailed endpoint values only when an explicit
debug setting is enabled.
- Around line 57-60: Update the http.Server configuration in the main server
setup to set explicit ReadTimeout, WriteTimeout, and IdleTimeout values
alongside ReadHeaderTimeout, using finite durations appropriate for the
unauthenticated ACME HTTP-01 endpoint so slow connections cannot remain open
indefinitely.
- Around line 32-35: Validate the port selected in the startup configuration
around PROXY_PORT before passing it to CreateNFTablesRuleMachineConfig or
constructing http.Server.Addr: parse it as a numeric TCP port and require a
value from 1 through 65535, falling back to defaultPort or returning a clear
configuration error for invalid input. Preserve the existing defaultPort
behavior when PROXY_PORT is unset.
In `@cmd/http01-proxy/pkg/utils.go`:
- Around line 91-235: Replace context.TODO() in the external-call paths with
caller-provided context.Context values, threading context through
GetOCPEnvDetails, CreateNFTablesRuleMachineConfig, applyManifest, and
resolveDNSRecord. Ensure every Kubernetes Get/Patch and DNS lookup uses that
context so callers can impose cancellation and deadlines, while preserving the
existing error handling and behavior.
In `@cmd/http01-proxy/templates/templates.go`:
- Around line 9-18: Remove the standalone `table inet crtmgr_proxy_table` line
from the `NFTRuleTemplate` string, leaving the existing delete command and
single braced table definition intact so the rendered template loads through
`nft -f`.
---
Nitpick comments:
In `@cmd/http01-proxy/pkg/utils.go`:
- Around line 47-69: Remove the no-op req.Header.Set("Host", req.Host) call from
the NewReverseProxy Director. Preserve req.Host unchanged so the original client
Host remains available for Host-based routing, while retaining the existing
target URL scheme and host assignments and other proxy headers.
In `@images/ci/http01proxy.Dockerfile`:
- Around line 1-12: Update both FROM instructions in the Dockerfile to use
approved immutable image digests instead of the mutable builder tag and
ubi9-minimal:latest reference, preserving the existing builder and runtime
images and stage structure.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 943033bd-c480-4844-8671-68d35cfd3ac7
📒 Files selected for processing (6)
.gitignoreMakefilecmd/http01-proxy/main.gocmd/http01-proxy/pkg/utils.gocmd/http01-proxy/templates/templates.goimages/ci/http01proxy.Dockerfile
| log.Printf("OCP API: %s, API VIP: %s, APPS VIP: %s, Platform: %s, Version: %s", | ||
| env.APIHostname, env.APIVIP, env.AppsVIP, env.PlatformType, env.ClusterVersion) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Logs internal cluster hostnames/VIPs.
This log.Printf (and similarly line 61) logs env.APIHostname, env.APIVIP, env.AppsVIP, and backendServer — internal cluster hostnames/IP addresses. As per coding guidelines, logging that may expose internal hostnames should be flagged. Consider lowering verbosity/gating this behind a debug flag if these logs are shipped off-node.
🤖 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 `@cmd/http01-proxy/main.go` around lines 20 - 21, Reduce or gate the
informational logging in the startup flow around the OCP API log and the similar
log near line 61 so internal values such as env.APIHostname, env.APIVIP,
env.AppsVIP, and backendServer are not emitted in normal or off-node logs.
Preserve only non-sensitive context at the default verbosity, and expose
detailed endpoint values only when an explicit debug setting is enabled.
Source: Coding guidelines
There was a problem hiding this comment.
These are startup-only log lines in a DaemonSet running on control-plane nodes. The information (API hostname, VIPs, platform type) is essential for debugging proxy issues. The logs stay on-node in the pod's stdout/stderr. This is the same logging pattern used by the upstream proxy. Leaving as-is.
There was a problem hiding this comment.
@sebrandon1, that context makes sense — startup-only diagnostic logging confined to on-node pod stdout/stderr for a DaemonSet, consistent with the upstream proxy's pattern, is a reasonable tradeoff given how essential this info is for debugging VIP/platform detection issues. I'll leave this as-is.
e699136 to
ed26462
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
cmd/http01-proxy/pkg/utils.go (2)
94-115: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
unstructured.NestedString's "found" flag is discarded at all three call sites.
platformType,ingressDomain, andclusterVersionare all extracted with_for the "found" return value. If any of these fields is absent from the resource (schema drift, unexpected platform, etc.),NestedStringreturns("", false, nil)— no error — and the code silently proceeds with an empty string instead of failing fast with a clear message.♻️ Proposed fix (repeat for all three)
- ingressDomain, _, err := unstructured.NestedString(ingressData.Object, "spec", "domain") - if err != nil { - return nil, fmt.Errorf("failed to read ingress domain: %w", err) - } + ingressDomain, found, err := unstructured.NestedString(ingressData.Object, "spec", "domain") + if err != nil { + return nil, fmt.Errorf("failed to read ingress domain: %w", err) + } + if !found || ingressDomain == "" { + return nil, fmt.Errorf("ingress domain not found in ingresses/cluster") + }🤖 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 `@cmd/http01-proxy/pkg/utils.go` around lines 94 - 115, Update the three unstructured.NestedString calls in the surrounding resource-loading flow to retain and validate the found flag for platformType, ingressDomain, and clusterVersion. Return a descriptive error when any required field is absent, while preserving the existing error handling for extraction failures and successful values.
117-117: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
apiHostnamederived by string-replacing "apps"→"api" in the ingress domain is a brittle heuristic.OpenShift's
Infrastructurestatus already exposes a canonicalapiServerURLfield, populated for this purpose. Deriving the API hostname viastrings.Replace(ingressDomain, "apps", "api", 1)assumes the literal substring "apps" appears exactly once and only in the expected position; any base domain that happens to contain "apps" elsewhere, or a non-standard ingress domain naming scheme, would silently produce a wrong hostname (which then feeds an incorrect DNS lookup at line 124) rather than a decode error.Consider reading
status.apiServerURLoffinfrastructureData(already fetched at line 90) and parsing the host out of it instead of pattern-matching the ingress domain string.
apiServerURL is a valid URI with scheme 'https', address and optionally a port (defaulting to 443), intended so components can find the Kubernetes API.🤖 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 `@cmd/http01-proxy/pkg/utils.go` at line 117, The apiHostname derivation in the surrounding infrastructure lookup uses a brittle ingress-domain string replacement. Read the canonical apiServerURL from infrastructureData.status, parse its HTTPS URI, and derive the API host (preserving its optional port, defaulting to 443) before the DNS lookup; remove the strings.Replace-based heuristic.
🤖 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 `@cmd/http01-proxy/pkg/utils.go`:
- Around line 119-136: The DNS lookups in the environment construction flow must
return IPv4 addresses because AppsVIP and APIVIP are consumed by IPv4-specific
paths. Update the resolveDNSRecord calls used for appsIPs and apiIPs to request
IPv4 results, or explicitly select an IPv4 entry before assigning AppsVIP and
APIVIP.
- Around line 63-65: Add bounded timeouts to the http.Transport assigned to
proxy.Transport, configuring DialContext and ResponseHeaderTimeout (or an
equivalent client timeout) so stalled backend connections and response headers
cannot block indefinitely. Preserve the existing ProxyFromEnvironment behavior.
In `@images/ci/http01proxy.Dockerfile`:
- Around line 4-9: Update the Dockerfile build configuration around
GO_BUILD_TAGS so callers cannot override or remove the mandatory
strictfipsruntime and openssl tags. Hardcode both required tags in the go build
invocation, or validate any supplied value and fail unless both tags are
present; preserve the existing FIPS-related GOEXPERIMENT and CGO_ENABLED
settings.
- Around line 9-10: Create the /app directory before the go build command in the
Dockerfile, using RUN mkdir -p /app or setting WORKDIR /app, so the output path
for cert-manager-http01-proxy is valid.
- Around line 12-15: Update the final runtime stage based on the UBI9-minimal
image to install the OpenSSL/FIPS runtime dependencies required by the
strictfipsruntime,openssl build, specifically openssl-libs and
openssl-fips-provider or an equivalent image-provided set, before copying and
launching cert-manager-http01-proxy.
---
Nitpick comments:
In `@cmd/http01-proxy/pkg/utils.go`:
- Around line 94-115: Update the three unstructured.NestedString calls in the
surrounding resource-loading flow to retain and validate the found flag for
platformType, ingressDomain, and clusterVersion. Return a descriptive error when
any required field is absent, while preserving the existing error handling for
extraction failures and successful values.
- Line 117: The apiHostname derivation in the surrounding infrastructure lookup
uses a brittle ingress-domain string replacement. Read the canonical
apiServerURL from infrastructureData.status, parse its HTTPS URI, and derive the
API host (preserving its optional port, defaulting to 443) before the DNS
lookup; remove the strings.Replace-based heuristic.
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8f79c938-8ea6-4198-a1e5-92b4d68afbba
📒 Files selected for processing (6)
.gitignoreMakefilecmd/http01-proxy/main.gocmd/http01-proxy/pkg/utils.gocmd/http01-proxy/templates/templates.goimages/ci/http01proxy.Dockerfile
🚧 Files skipped from review as they are similar to previous changes (4)
- .gitignore
- cmd/http01-proxy/main.go
- cmd/http01-proxy/templates/templates.go
- Makefile
ed26462 to
1d1a239
Compare
Adds the HTTP01 Challenge Proxy binary source code and CI build infrastructure. The proxy runs as a DaemonSet on baremetal control-plane nodes, redirecting HTTP-01 ACME challenge traffic from the API VIP to the ingress router via NFTables rules and a Go reverse proxy. This is split out from the full HTTP01Proxy controller PR to allow the image build pipeline to start early while the controller is reviewed. New files: - cmd/http01-proxy/ - proxy binary source (~200 LOC) - images/ci/http01proxy.Dockerfile - FIPS-compliant CI image build - Makefile targets: build-http01-proxy, image-build-http01-proxy
1d1a239 to
9bfd18f
Compare
Adds the cert-manager-http01-proxy image build to the ci-operator config for cert-manager-operator. The proxy binary source and CI Dockerfile were added in openshift/cert-manager-operator#458. This enables CI to build the FIPS-compliant proxy image alongside the existing operator and cert-manager operand images, and substitutes it into the OLM bundle for e2e testing.
Adds the cert-manager-http01-proxy image build to the ci-operator config for cert-manager-operator. The proxy binary source and CI Dockerfile were added in openshift/cert-manager-operator#458. This enables CI to build the FIPS-compliant proxy image alongside the existing operator and cert-manager operand images, and substitutes it into the OLM bundle for e2e testing.
|
@sebrandon1: all tests passed! 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. |
|
Closing — the DNAT+SNAT experiment (branch The new approach was validated on baremetal cluster cnfdt16 (3-node, OCP 5.0) — all tests pass. See the new PR for details. PR #398 remains open for reference/comparison of the two approaches. |
2 participants
Summary
cmd/http01-proxy/images/ci/http01proxy.Dockerfilebuild-http01-proxy,image-build-http01-proxyThe proxy runs as a DaemonSet on baremetal control-plane nodes, redirecting HTTP-01 ACME challenge traffic from the API VIP to the ingress router via NFTables rules and a Go reverse proxy.
This is split out from the full HTTP01Proxy controller PR (#398) to allow the image build pipeline to start early while the controller integration is reviewed. PR #398 will rebase on top of this once merged.
Related PRs
What the proxy does (~200 LOC)
<API_VIP>:80→ proxy port 8888/.well-known/acme-challenge/*requests to the Apps VIPTest plan
make build-http01-proxycompiles successfullymake lintpasses (no issues incmd/http01-proxy/)go vet ./cmd/http01-proxy/...passes