CM-716: HTTP01 Proxy with nftables MachineConfig#459
Open
sebrandon1 wants to merge 1 commit into
Open
Conversation
Contributor
|
[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 |
WalkthroughAdds an Alpha ChangesHTTP01Proxy feature
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FeatureGate
participant ControllerManager
participant HTTP01ProxyController
participant InfrastructureAPI
participant MachineConfigAPI
FeatureGate->>ControllerManager: Enable HTTP01Proxy
ControllerManager->>HTTP01ProxyController: Register controller and cache
HTTP01ProxyController->>InfrastructureAPI: Read infrastructure/cluster
InfrastructureAPI-->>HTTP01ProxyController: Return platform and VIPs
HTTP01ProxyController->>MachineConfigAPI: Create, update, or delete MachineConfig
Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
|
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. |
sebrandon1
force-pushed
the
experiment/dnat-snat-operator
branch
from
July 16, 2026 19:45
dda93d9 to
9ad0516
Compare
sebrandon1
changed the title
sebrandon1
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
config/rbac/role.yaml (1)
262-270: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftAdd MachineConfig RBAC to both generated and source roles.
The controller’s MachineConfig workflow needs permissions in
machineconfiguration.openshift.io; neither role grants access tomachineconfigs.
config/rbac/role.yaml#L262-L270: add the source rule with the verbs used by reconciliation.bundle/manifests/cert-manager-operator.clusterserviceversion.yaml#L672-L674: regenerate or update the CSV’s installation ClusterRole with the same permissions.🤖 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 `@config/rbac/role.yaml` around lines 262 - 270, Add a MachineConfig RBAC rule for resource machineconfigs in the machineconfiguration.openshift.io API group, granting the reconciliation verbs used by the controller, to config/rbac/role.yaml lines 262-270. Regenerate or apply the same permission rule to bundle/manifests/cert-manager-operator.clusterserviceversion.yaml lines 672-674 so both source and generated installation roles match.
🧹 Nitpick comments (1)
hack/test-dnat-snat-baremetal.sh (1)
15-15: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo cleanup-on-failure trap; a mid-run
die()leaves iptables rules and the test namespace behind.
cleanuponly runs at the start and at the normal end (Line 278). If the script exits early viadie()(e.g., TEST B or TEST C failure), the DNAT/MASQUERADE/FORWARD rules installed on masters and thednat-snat-testnamespace are left in place.♻️ Suggested fix: run cleanup on any exit unless KEEP=1
+trap '[[ "$KEEP" == "1" ]] || cleanup' EXIT + phase "0) Cleanup previous run" cleanup say " done"Also applies to: 71-87, 178-264
🤖 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 `@hack/test-dnat-snat-baremetal.sh` at line 15, Add an EXIT trap in the script’s main setup flow so cleanup runs for both successful and failure exits, while honoring KEEP=1 to preserve the current opt-out behavior. Reuse the existing cleanup function and ensure the trap is installed before rules or the dnat-snat-test namespace are created; avoid duplicate cleanup at normal completion.
🤖 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 `@api/operator/v1alpha1/http01proxy_types.go`:
- Around line 35-42: The legacy reverse-proxy configuration must be removed
across all listed sites: in api/operator/v1alpha1/http01proxy_types.go lines
35-42, rewrite the HTTP01Proxy documentation to describe MachineConfig
reconciliation; in lines 62-109, remove or deprecate deployment-specific fields
and status.proxyImage. In
bundle/manifests/cert-manager-operator.clusterserviceversion.yaml lines 501,
711-718, 822-831, and 948-949, remove DaemonSet permission, privileged SCC
access, proxy-image environment variables, and the obsolete related image
respectively. In config/manager/manager.yaml lines 89-98, remove obsolete
proxy-image environment variables. In config/rbac/role.yaml lines 89 and
299-306, remove DaemonSet permission and privileged SCC access, preserving
least-privilege restricted/custom-scoped SCC usage.
In `@bundle/manifests/operator.openshift.io_http01proxies.yaml`:
- Around line 40-46: Remove stale HTTP01 proxy DaemonSet/image-based deployment
wording and references. Update the descriptions in
bundle/manifests/operator.openshift.io_http01proxies.yaml lines 40-46 and
config/crd/bases/operator.openshift.io_http01proxies.yaml lines 40-46 to reflect
the MachineConfig-only reconciler, and remove obsolete http01-proxy artifact
entries from Makefile lines 63-377 and .gitignore line 15. Also clean
corresponding stale wiring in the HTTP01Proxy API types, manager manifest, and
operator CSV.
In `@hack/verify-http01proxy.sh`:
- Around line 4-14: Update verify-http01proxy.sh to remove checks for the
obsolete HTTP01 Proxy DaemonSet, proxyImage status, RBAC resources, and
NetworkPolicies. Replace those verification phases with a check that
MachineConfig 98-nftables-crtmgr-http01-dnat exists, and verify
crtmgr-http01-dnat.service is active on master nodes via oc debug where
supported.
In `@Makefile`:
- Line 63: Remove the obsolete HTTP01 proxy build plumbing, including the
HTTP01PROXY_VERSION definition and the build-http01-proxy and
image-build-http01-proxy targets referencing ./cmd/http01-proxy or
images/ci/http01proxy.Dockerfile. Also remove the associated old proxy path
configuration while leaving unrelated Makefile targets unchanged.
In `@pkg/controller/http01proxy/install_http01proxy.go`:
- Around line 17-23: Update the platform-validation failure branch in the
reconcile handler to propagate a failed r.cleanUp(ctx, proxy) result, or return
an error that requests retry, instead of always returning
common.NewIrrecoverableError. Preserve the irrecoverable platform-validation
response only when cleanup succeeds.
In `@pkg/controller/http01proxy/machineconfig.go`:
- Around line 17-29: Update nftRulesTemplate and renderMachineConfig so the
PREROUTING DNAT rule matches the API VIP destination as well as tcp dport 80,
rather than all inbound port-80 traffic. Add the API VIP as an input to
renderMachineConfig, pass it into the template data, and use it in the nftables
destination-address condition while preserving the existing ingress-VIP DNAT
target and postrouting rule.
- Around line 17-29: Make createOrApplyMachineConfig reject an IPv6-first
info.ingressVIPs[0] before rendering the IPv4-only MachineConfig, returning a
clear error instead of applying invalid nft rules. Validate the selected VIP
with the existing networking utilities and preserve the current IPv4 rendering
path.
- Line 1: Scope the HTTP-01 DNAT rule to the API VIP in both production and
manual validation. Update renderMachineConfig and createOrApplyMachineConfig to
pass the available info.apiVIPs value into the template, add the API destination
match to the prerouting DNAT rule in machineconfig.go, and add the equivalent
API_VIP destination restriction to the iptables rule in
hack/test-dnat-snat-baremetal.sh.
- Around line 49-68: Update the systemd unit’s ExecStop commands to also delete
the RELATED,ESTABLISHED FORWARD rule inserted by the second ExecStart command,
preserving the existing tolerant behavior when the rule is absent. Keep the
existing dport-80 cleanup unchanged.
In `@pkg/features/features_test.go`:
- Around line 106-117: Update the “all pre-GA features should be disabled by
default” test to assert !spec.Default only when isPreGA is true, while leaving
non-pre-GA features unrestricted. Preserve the existing AllBeta and AllAlpha
exclusions and pre-release classification.
In `@pkg/operator/applyconfigurations/operator/v1alpha1/http01proxy.go`:
- Around line 17-22: Update the HTTP01Proxy source type in http01proxy_types.go
to document the current MachineConfig-only reconciliation behavior, and remove
or deprecate the obsolete mode, customDeployment, and proxyImage fields.
Regenerate the apply-configuration clients so HTTP01Proxy and related generated
documentation no longer describe the retired DaemonSet design.
---
Outside diff comments:
In `@config/rbac/role.yaml`:
- Around line 262-270: Add a MachineConfig RBAC rule for resource machineconfigs
in the machineconfiguration.openshift.io API group, granting the reconciliation
verbs used by the controller, to config/rbac/role.yaml lines 262-270. Regenerate
or apply the same permission rule to
bundle/manifests/cert-manager-operator.clusterserviceversion.yaml lines 672-674
so both source and generated installation roles match.
---
Nitpick comments:
In `@hack/test-dnat-snat-baremetal.sh`:
- Line 15: Add an EXIT trap in the script’s main setup flow so cleanup runs for
both successful and failure exits, while honoring KEEP=1 to preserve the current
opt-out behavior. Reuse the existing cleanup function and ensure the trap is
installed before rules or the dnat-snat-test namespace are created; avoid
duplicate cleanup at normal completion.
🪄 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: 9a7de29c-1fe4-4fb7-8b84-f53fb27fc435
⛔ Files ignored due to path filters (1)
api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (42)
.gitignoreMakefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.gobundle/manifests/cert-manager-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_http01proxies.yamlconfig/crd/bases/operator.openshift.io_http01proxies.yamlconfig/crd/kustomization.yamlconfig/manager/manager.yamlconfig/rbac/role.yamlconfig/samples/operator.openshift.io_v1alpha1_http01proxy.yamlhack/test-dnat-snat-baremetal.shhack/verify-http01proxy.shpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/controller_test.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/infrastructure_test.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/machineconfig.gopkg/controller/http01proxy/machineconfig_test.gopkg/controller/http01proxy/utils.gopkg/features/features_test.gopkg/operator/applyconfigurations/internal/internal.gopkg/operator/applyconfigurations/operator/v1alpha1/http01proxy.gopkg/operator/applyconfigurations/operator/v1alpha1/http01proxycustomdeploymentspec.gopkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.gopkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.gopkg/operator/applyconfigurations/utils.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.gopkg/operator/informers/externalversions/generic.gopkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.gopkg/operator/informers/externalversions/operator/v1alpha1/interface.gopkg/operator/listers/operator/v1alpha1/expansion_generated.gopkg/operator/listers/operator/v1alpha1/http01proxy.gopkg/operator/setup_manager.gopkg/operator/starter.gotest/e2e/http01proxy_test.go
2 tasks
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@hack/test-dnat-snat-baremetal.sh`:
- Around line 276-288: Ensure cleanup runs on failure as well as successful
completion by registering an EXIT trap after the cleanup function is defined.
The trap should preserve the original exit status and invoke cleanup when KEEP
is not 1, while avoiding duplicate cleanup during the existing final cleanup
path.
- Around line 50-54: Stop exposing raw infrastructure identifiers in
hack/test-dnat-snat-baremetal.sh: redact or omit API_HOST, API_VIP, and
INGRESS_VIP in the initial say summary at lines 50-54; remove or redact repeated
endpoint values in the summary at lines 268-270; and change the copy-paste
commands at lines 283-284 so they do not emit internal hostnames or VIPs.
Preserve useful status information without printing the actual endpoint values.
- Around line 75-83: Create an owned iptables chain for the test rules in the
setup block around lines 210-217, route all DNAT, MASQUERADE, FORWARD, and
RELATED,ESTABLISHED rules through it, and add the required jump rules. Replace
the cleanup loop around lines 75-83 with removal of the test-chain jump and
deletion of the dedicated chain, ensuring only test-created rules are removed
and the chain is fully flushed and deleted.
- Around line 35-36: Update API_HOST derivation in
hack/test-dnat-snat-baremetal.sh to extract the real hostname from oc whoami
--show-server instead of constructing it from BASE. Update the curl requests to
bypass proxies and pin API_HOST to API_VIP using --noproxy '*' and --resolve
"${API_HOST}:80:${API_VIP}".
In `@pkg/controller/http01proxy/machineconfig_test.go`:
- Around line 324-355: Update createOrApplyMachineConfig to compare the existing
MachineConfig’s controller-owned labels, including
machineconfiguration.openshift.io/role=master, alongside the spec; trigger
Update when the role label is missing or incorrect. Adjust
TestCreateOrApplyMachineConfigNoOpWhenUnchanged to set the required label for
the no-op case, and add or update coverage so a matching spec without that label
expects an update.
- Around line 90-92: Update the test setup around unstructured helpers in the
affected test cases to capture and validate every returned found/error value
before indexing or continuing. Use t.Fatalf for unexpected errors or missing
fields, and check that SetNestedField succeeds; apply this consistently to the
paths near the units extraction and the other noted helper calls.
- Around line 100-113: Update the unit-generation logic and its test around the
machine-config service so iptables cleanup is symmetric and ownership-aware:
ensure the conntrack rule is removed on ExecStop, while preserving any VIP rule
that predated the service instead of unconditionally deleting it. Use a
dedicated chain/jump or equivalent state tracking, and extend the test
assertions to cover both pre-existing and service-added rules.
- Around line 80-82: The MachineConfig generation and its test only support IPv4
despite declaring an inet nftables table. Update the relevant HTTP01 proxy
rendering logic to either generate valid IPv6 rules and addressing when given an
IPv6 ingress VIP, or explicitly reject IPv6 input; extend the test around the
existing nftRules assertion to verify the chosen behavior.
🪄 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: c2380c12-e704-42e9-8ddf-ad340c918804
📒 Files selected for processing (9)
hack/test-dnat-snat-baremetal.shpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/controller_test.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/machineconfig.gopkg/controller/http01proxy/machineconfig_test.gopkg/controller/http01proxy/utils.gopkg/operator/setup_manager.go
💤 Files with no reviewable changes (2)
- pkg/controller/http01proxy/utils.go
- pkg/operator/setup_manager.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/controller/http01proxy/constants.go
- pkg/controller/http01proxy/controller_test.go
- pkg/controller/http01proxy/machineconfig.go
- pkg/controller/http01proxy/install_http01proxy.go
- pkg/controller/http01proxy/controller.go
sebrandon1
force-pushed
the
experiment/dnat-snat-operator
branch
from
July 16, 2026 20:12
9ad0516 to
2706832
Compare
Member
Author
|
Addressed CodeRabbit pre-merge check findings in the latest force-push: No-Sensitive-Data-In-Logs (was ❌ Error)
Docstring Coverage (was
Microshift Test Compatibility (was
Also fixed verify CI:
|
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@hack/test-dnat-snat-baremetal.sh`:
- Around line 203-217: Update the DNAT+SNAT setup in the master-node loop to
reconcile the production HTTP01Proxy/MachineConfig and wait for its rollout
before testing traffic. Validate the resulting nftables DNAT and MASQUERADE
rules and cleanup behavior, while retaining iptables only for the production
FORWARD accept rules.
- Around line 71-87: Update the master setup and cleanup flow around cleanup()
to capture each node’s original net.ipv4.ip_forward value before the line-209
modification, retain it per node, and restore it during cleanup when KEEP is not
1. Preserve the existing iptables and namespace cleanup behavior, and avoid
restoring the setting when KEEP equals 1.
In `@pkg/controller/http01proxy/machineconfig.go`:
- Around line 34-35: Update the MachineConfig role selector in the routing-rule
configuration so it does not target every control-plane node via the fixed
master role. Restrict application to eligible VIP-serving control-plane nodes,
or explicitly reject unsupported arbiter topologies, ensuring arbiter nodes
cannot receive the forwarding/NAT configuration.
- Around line 17-18: Update the nftRulesTemplate definition to replace the
leading table declaration and delete-table sequence with a destroy-table command
for crtmgr_http01_dnat, preserving the subsequent rules block so nft -f removes
the old table before loading the new configuration.
🪄 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: 343cbf76-9c06-446f-b3a8-7302db9b03b4
⛔ Files ignored due to path filters (1)
api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (42)
.gitignoreMakefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.gobundle/manifests/cert-manager-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_http01proxies.yamlconfig/crd/bases/operator.openshift.io_http01proxies.yamlconfig/crd/kustomization.yamlconfig/manager/manager.yamlconfig/rbac/role.yamlconfig/samples/operator.openshift.io_v1alpha1_http01proxy.yamlhack/test-dnat-snat-baremetal.shhack/verify-http01proxy.shpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/controller_test.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/infrastructure_test.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/machineconfig.gopkg/controller/http01proxy/machineconfig_test.gopkg/controller/http01proxy/utils.gopkg/features/features_test.gopkg/operator/applyconfigurations/internal/internal.gopkg/operator/applyconfigurations/operator/v1alpha1/http01proxy.gopkg/operator/applyconfigurations/operator/v1alpha1/http01proxycustomdeploymentspec.gopkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.gopkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.gopkg/operator/applyconfigurations/utils.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.gopkg/operator/informers/externalversions/generic.gopkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.gopkg/operator/informers/externalversions/operator/v1alpha1/interface.gopkg/operator/listers/operator/v1alpha1/expansion_generated.gopkg/operator/listers/operator/v1alpha1/http01proxy.gopkg/operator/setup_manager.gopkg/operator/starter.gotest/e2e/http01proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (31)
- config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml
- pkg/controller/http01proxy/constants.go
- config/crd/kustomization.yaml
- pkg/operator/applyconfigurations/utils.go
- pkg/features/features_test.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go
- pkg/operator/informers/externalversions/generic.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go
- .gitignore
- api/operator/v1alpha1/http01proxy_types.go
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- config/manager/manager.yaml
- test/e2e/http01proxy_test.go
- pkg/operator/applyconfigurations/internal/internal.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go
- pkg/controller/http01proxy/infrastructure.go
- pkg/operator/listers/operator/v1alpha1/http01proxy.go
- api/operator/v1alpha1/features.go
- Makefile
- config/crd/bases/operator.openshift.io_http01proxies.yaml
- pkg/operator/starter.go
- pkg/controller/http01proxy/utils.go
- bundle/manifests/operator.openshift.io_http01proxies.yaml
- pkg/controller/http01proxy/install_http01proxy.go
- pkg/controller/http01proxy/controller_test.go
- pkg/controller/http01proxy/controller.go
- hack/verify-http01proxy.sh
- pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go
Member
Author
CodeRabbit Review ResponseTriaged all 23 actionable comments across 3 reviews. Summary: 10 accepted, 10 deferred, 3 dismissed. Accepted (will fix in follow-up push)
Deferred (valid, out of scope for this PR)
Dismissed
|
Adds an HTTP01Proxy controller gated by the HTTP01Proxy feature flag. On baremetal clusters, the controller renders and applies a MachineConfig that delivers nftables DNAT rules (PREROUTING + POSTROUTING MASQUERADE) and iptables FORWARD rules to forward HTTP-01 ACME challenges from the API VIP to the ingress VIP. A systemd oneshot service enables ip_forward, loads the nftables config, and inserts FORWARD ACCEPT rules at boot. ExecStop cleans up all rules when the service is stopped. Includes the HTTP01Proxy CRD, generated clients/listers/informers, RBAC for MachineConfig management, unit tests (43 tests), and an e2e test that validates platform rejection on non-baremetal clusters.
sebrandon1
force-pushed
the
experiment/dnat-snat-operator
branch
from
July 16, 2026 20:52
2706832 to
c9bd99d
Compare
Member
Author
|
Pushed fixes for accepted CodeRabbit findings: DNAT scoped to API VIP — nftables prerouting rule now matches ExecStop conntrack cleanup — added Cleanup error propagation — Test updates — all |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/controller/http01proxy/infrastructure_test.go (1)
97-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the discovered ingress VIPs.
The BareMetal fixture supplies
ingressIPs, but the test only checks API VIPs. A regression dropping the DNAT target would pass unnoticed.Proposed test extension
wantPlatform string wantAPIVIPs int + wantIngressVIPs int @@ wantPlatform: "BareMetal", wantAPIVIPs: 1, + wantIngressVIPs: 1, @@ if len(info.apiVIPs) != tt.wantAPIVIPs { t.Errorf("len(apiVIPs) = %d, want %d", len(info.apiVIPs), tt.wantAPIVIPs) } + if len(info.ingressVIPs) != tt.wantIngressVIPs { + t.Errorf("len(ingressVIPs) = %d, want %d", len(info.ingressVIPs), tt.wantIngressVIPs) + }Also applies to: 142-160, 202-207
🤖 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/controller/http01proxy/infrastructure_test.go` around lines 97 - 104, Extend the test cases and assertions around the existing infrastructure test table to validate the discovered ingress VIPs from the BareMetal fixture’s ingressIPs, not just API VIPs. Add the expected ingress VIP field to the table, populate it for relevant cases, and assert the controller result matches it so a missing DNAT target fails the test.
🤖 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.
Nitpick comments:
In `@pkg/controller/http01proxy/infrastructure_test.go`:
- Around line 97-104: Extend the test cases and assertions around the existing
infrastructure test table to validate the discovered ingress VIPs from the
BareMetal fixture’s ingressIPs, not just API VIPs. Add the expected ingress VIP
field to the table, populate it for relevant cases, and assert the controller
result matches it so a missing DNAT target fails the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: de6e82e9-655d-4434-8d0b-2c706351b509
⛔ Files ignored due to path filters (1)
api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (42)
.gitignoreMakefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.gobundle/manifests/cert-manager-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_http01proxies.yamlconfig/crd/bases/operator.openshift.io_http01proxies.yamlconfig/crd/kustomization.yamlconfig/manager/manager.yamlconfig/rbac/role.yamlconfig/samples/operator.openshift.io_v1alpha1_http01proxy.yamlhack/test-dnat-snat-baremetal.shhack/verify-http01proxy.shpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/controller_test.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/infrastructure_test.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/machineconfig.gopkg/controller/http01proxy/machineconfig_test.gopkg/controller/http01proxy/utils.gopkg/features/features_test.gopkg/operator/applyconfigurations/internal/internal.gopkg/operator/applyconfigurations/operator/v1alpha1/http01proxy.gopkg/operator/applyconfigurations/operator/v1alpha1/http01proxycustomdeploymentspec.gopkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.gopkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.gopkg/operator/applyconfigurations/utils.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.gopkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.gopkg/operator/informers/externalversions/generic.gopkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.gopkg/operator/informers/externalversions/operator/v1alpha1/interface.gopkg/operator/listers/operator/v1alpha1/expansion_generated.gopkg/operator/listers/operator/v1alpha1/http01proxy.gopkg/operator/setup_manager.gopkg/operator/starter.gotest/e2e/http01proxy_test.go
🚧 Files skipped from review as they are similar to previous changes (37)
- config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go
- pkg/operator/applyconfigurations/utils.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go
- pkg/operator/listers/operator/v1alpha1/http01proxy.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxycustomdeploymentspec.go
- pkg/operator/informers/externalversions/operator/v1alpha1/interface.go
- pkg/controller/http01proxy/constants.go
- .gitignore
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- pkg/operator/informers/externalversions/generic.go
- pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go
- api/operator/v1alpha1/features.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- test/e2e/http01proxy_test.go
- pkg/controller/http01proxy/install_http01proxy.go
- pkg/features/features_test.go
- pkg/operator/applyconfigurations/internal/internal.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go
- config/crd/kustomization.yaml
- pkg/controller/http01proxy/infrastructure.go
- pkg/controller/http01proxy/utils.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go
- pkg/controller/http01proxy/machineconfig.go
- config/rbac/role.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go
- bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
- api/operator/v1alpha1/http01proxy_types.go
- config/manager/manager.yaml
- pkg/controller/http01proxy/controller_test.go
- bundle/manifests/operator.openshift.io_http01proxies.yaml
- pkg/operator/setup_manager.go
- config/crd/bases/operator.openshift.io_http01proxies.yaml
- pkg/controller/http01proxy/controller.go
- Makefile
- hack/verify-http01proxy.sh
Contributor
|
@sebrandon1: The following test 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. |
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.
Summary
Replaces the reverse proxy DaemonSet with nftables DNAT rules delivered via MachineConfig on baremetal master nodes.
ip_forward, loads nftables NAT rules (PREROUTING DNAT + POSTROUTING MASQUERADE), and adds iptables FORWARD rulesWhy iptables FORWARD alongside nftables NAT?
OpenShift's existing FORWARD chain is iptables-nft with
policy DROP. The kernel evaluates both iptables-nft and native nftables hooks — both must ACCEPT for packets to pass.Related
Test plan
go test ./pkg/controller/http01proxy/...— 43 tests passmake lintcleanSummary by CodeRabbit
New Features
HTTP01Proxycustom resource (feature-gated) to enable HTTP-01 challenge proxying on supported bare-metal environments.DefaultDeploymentandCustomDeploymentmodes, exposing status conditions and the selected proxy image.Bug Fixes
HTTP01Proxyis marked degraded and proxy components are not installed.Tests
Chores
HTTP01ProxyCRD.