CM-716: Add HTTP01 Challenge Proxy controller#398
Open
sebrandon1 wants to merge 1 commit into
Open
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 "4.22.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 the HTTP01Proxy v1alpha1 API and CRD, a feature-gated controller that deploys and manages an HTTP-01 proxy on validated BareMetal platforms, required Kubernetes assets and RBAC, generated clients, manager wiring, and verification tests/scripts. ChangesHTTP01Proxy feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: ✨ Finishing Touches🧪 Generate unit tests (beta)
|
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (1)
pkg/controller/http01proxy/controller.go (1)
40-42: Context should be passed through, not stored in struct.The
ctxfield is initialized tocontext.Background()and stored in the Reconciler. Helper methods (e.g.,deleteDaemonSet,deleteServiceAccount) user.ctxinstead of the context passed toReconcile(). This means operations won't be cancelled if the reconcile request is cancelled, potentially causing reliability issues with long-running cleanup operations.♻️ Recommended approach
Pass the context from
Reconcile(ctx, req)down to helper methods rather than storing it:type Reconciler struct { common.CtrlClient - ctx context.Context eventRecorder record.EventRecorder log logr.Logger scheme *runtime.Scheme }Then update helper method signatures to accept context and pass it from Reconcile:
func (r *Reconciler) deleteDaemonSet(ctx context.Context, proxy *v1alpha1.HTTP01Proxy) error { return r.deleteIfExists(ctx, &appsv1.DaemonSet{}, ...) }Also applies to: 62-62
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/http01proxy/controller.go` around lines 40 - 42, The struct field ctx should not be stored or initialized to context.Background(); remove the ctx field from Reconciler and stop using r.ctx. Update Reconcile(ctx, req) to pass its ctx into all helper calls (e.g., deleteDaemonSet, deleteServiceAccount and any other helpers) and change those helper signatures to accept ctx context.Context as their first parameter (for example func (r *Reconciler) deleteDaemonSet(ctx context.Context, proxy *v1alpha1.HTTP01Proxy) error) and ensure internal calls like deleteIfExists use the passed ctx instead of r.ctx. Search for uses of r.ctx across the file (and functions like deleteIfExists) and replace them to use the ctx parameter propagated from Reconcile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yaml`:
- Around line 26-34: The ClusterRole block granting verbs ["create","update"] on
the cluster-scoped resource "machineconfigs" is too broad; remove the "create"
and "update" verbs from the cluster-wide ClusterRole in
cert-manager-http01-proxy-clusterrole.yaml and instead either (a) move necessary
write privileges into a namespaced Role bound to a dedicated controller
ServiceAccount, or (b) if cluster-scoped writes are unavoidable, replace the
ClusterRole entry with a more restrictive rule using "resourceNames" limited to
the specific deterministic MachineConfig names and bind it only to the dedicated
controller identity; update any RoleBinding/ClusterRoleBinding to target that
controller ServiceAccount accordingly.
In `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml`:
- Around line 45-52: The securityContext for the proxy container currently
grants NET_ADMIN and runs as root but does not prevent privilege escalation;
update the proxy container's securityContext (the block containing capabilities:
add: - NET_ADMIN / drop: - ALL and runAsNonRoot: false) to include
allowPrivilegeEscalation: false so escalation is explicitly blocked while
keeping the existing capabilities intact.
In `@config/manager/manager.yaml`:
- Around line 89-90: The RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY env var is using
an unpinned image tag ("quay.io/bapalm/cert-mgr-http01-proxy:latest"); change it
to a fixed version tag or digest (for example
"quay.io/bapalm/cert-mgr-http01-proxy:v0.1.0" or a sha256@digest) to match other
operand pins and ensure reproducibility and verifiability—update the value for
RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY accordingly and confirm the chosen
tag/digest matches the tested/released HTTP01Proxy version.
In `@Makefile`:
- Around line 325-330: The RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY value
currently uses the mutable tag ":latest" while HTTP01PROXY_OPERAND_IMAGE_VERSION
is set to "0.1.0"; change RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY to use a fixed
tag or digest that matches HTTP01PROXY_OPERAND_IMAGE_VERSION (or derive the tag
from HTTP01PROXY_OPERAND_IMAGE_VERSION) so both variables are aligned and the
image is pinned for reproducible, reviewable runs; update any
documentation/comments to reflect the pinning.
In `@pkg/controller/http01proxy/infrastructure.go`:
- Around line 67-68: The current check only compares info.apiVIPs[0] and
info.ingressVIPs[0]; update the validation to ensure no VIP appears in both
slices anywhere by checking the full lists (e.g., build a set from info.apiVIPs
and test each entry of info.ingressVIPs or vice versa) and if any overlap exists
return a clear error message referencing the duplicated VIP(s). Modify the
comparison around the block that currently uses info.apiVIPs[0] and
info.ingressVIPs[0] to perform this full-list intersection check and return the
same formatted message (including the conflicting VIP(s)) when overlaps are
found.
- Around line 33-44: The code is ignoring errors from unstructured.NestedString
and NestedStringSlice which can mask malformed Infrastructure status; update the
extraction around platformType (using unstructured.NestedString) and the VIP
slices (using unstructured.NestedStringSlice) to capture and check the returned
ok/err values, and handle failures explicitly (e.g., return an error from the
function or log and fail fast) instead of silently treating them as unsupported;
specifically modify the block that builds platformInfo (referencing variables
infra.Object, platformType, platformInfo, platformBareMetal, apiVIPs,
ingressVIPs) to validate the extraction results and propagate a clear error when
parsing fails.
In `@pkg/controller/http01proxy/utils.go`:
- Around line 115-137: The daemonSetSpecModified function currently only checks
a few fields and misses many important pod and controller settings; update it to
detect any meaningful drift by comparing the full managed DaemonSet spec (or at
minimum the full PodTemplateSpec and controller-level fields) rather than only
selector/template labels and container[0] subsets—specifically include
Template.Annotations, Template.Spec (hostNetwork, tolerations, volumes,
securityContext, affinity, dnsPolicy, imagePullSecrets, init containers and all
containers with their names/images/env/ports/resources), and top-level Spec
fields like Strategy; easiest fix: replace the ad-hoc checks in
daemonSetSpecModified with a deep equality comparison between desired.Spec and
fetched.Spec (or between desired.Spec.Template and fetched.Spec.Template) or
switch to server-side apply (SSA) for this resource so any difference in those
fields triggers an update.
- Around line 141-145: The status-update error handling currently discards the
original reconcile error (prependErr) by aggregating only err and errUpdate;
change the aggregation inside the if block that follows r.updateStatus(r.ctx,
proxy) to include prependErr as well (use utilerrors.NewAggregate with
prependErr, err, and errUpdate or otherwise append prependErr into the errors
slice) so the returned aggregate preserves the original reconcile error and the
update failure; modify the code paths around r.updateStatus, prependErr,
errUpdate, and utilerrors.NewAggregate accordingly.
---
Nitpick comments:
In `@pkg/controller/http01proxy/controller.go`:
- Around line 40-42: The struct field ctx should not be stored or initialized to
context.Background(); remove the ctx field from Reconciler and stop using r.ctx.
Update Reconcile(ctx, req) to pass its ctx into all helper calls (e.g.,
deleteDaemonSet, deleteServiceAccount and any other helpers) and change those
helper signatures to accept ctx context.Context as their first parameter (for
example func (r *Reconciler) deleteDaemonSet(ctx context.Context, proxy
*v1alpha1.HTTP01Proxy) error) and ensure internal calls like deleteIfExists use
the passed ctx instead of r.ctx. Search for uses of r.ctx across the file (and
functions like deleteIfExists) and replace them to use the ctx parameter
propagated from Reconcile.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9c15de22-9f29-499f-b07d-8f12c902ce3e
📒 Files selected for processing (44)
Makefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.goapi/operator/v1alpha1/zz_generated.deepcopy.gobindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yamlbindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-daemonset.yamlbindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yamlbindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yamlbindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yamlconfig/crd/bases/operator.openshift.io_http01proxies.yamlconfig/manager/manager.yamlconfig/rbac/role.yamlconfig/samples/operator.openshift.io_v1alpha1_http01proxy.yamlpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/daemonsets.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/networkpolicies.gopkg/controller/http01proxy/rbacs.gopkg/controller/http01proxy/serviceaccounts.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/assets/bindata.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.go
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
April 6, 2026 20:30
28e765b to
baa4972
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bundle/manifests/cert-manager-operator.clusterserviceversion.yaml (1)
347-399:⚠️ Potential issue | 🟠 MajorDeclare
HTTP01Proxyas an owned CRD in the CSV.This PR now exposes
http01proxiesin CSV RBAC, butspec.customresourcedefinitions.ownedstill doesn't listhttp01proxies.operator.openshift.io. OLM metadata for the new API stays incomplete until that entry is added.Suggested patch
- description: CertManager is the Schema for the certmanagers API displayName: CertManager kind: CertManager name: certmanagers.operator.openshift.io version: v1alpha1 + - description: HTTP01Proxy describes the configuration for the managed HTTP-01 challenge proxy. + displayName: HTTP01Proxy + kind: HTTP01Proxy + name: http01proxies.operator.openshift.io + version: v1alpha1 - description: Challenge is a type to represent a Challenge request with an ACME serverAs per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 657-658
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundle/manifests/cert-manager-operator.clusterserviceversion.yaml` around lines 347 - 399, The CSV is missing an owned CRD entry for the new HTTP01Proxy API: add an entry under spec.customresourcedefinitions.owned with displayName "HTTP01Proxy", kind "HTTP01Proxy", name "http01proxies.operator.openshift.io" and the appropriate version (e.g., v1alpha1) so OLM recognizes the CRD as owned; update the list near the other owned CRDs (similar to entries for IstioCSR/TrustManager) to match the RBAC exposure of http01proxies.
♻️ Duplicate comments (5)
bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml (1)
45-51:⚠️ Potential issue | 🟠 MajorSet
allowPrivilegeEscalation: falseon the proxy container.This pod already runs with
hostNetwork, root, andNET_ADMIN; leaving privilege escalation implicit keeps the attack surface wider than necessary for a very privileged workload.Suggested patch
securityContext: + allowPrivilegeEscalation: false capabilities: add: - NET_ADMIN drop: - ALL runAsNonRoot: falseAs per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml` around lines 45 - 51, The container securityContext is missing allowPrivilegeEscalation explicitly and currently runAsNonRoot is false while adding NET_ADMIN and hostNetwork increases risk; update the container's securityContext for the proxy (the block where "securityContext:", "capabilities: add: - NET_ADMIN", and "runAsNonRoot: false" are defined) to set allowPrivilegeEscalation: false so privilege escalation is explicitly denied for the privileged workload, keeping other capability and runAsNonRoot settings as intended.pkg/controller/http01proxy/infrastructure.go (2)
66-68:⚠️ Potential issue | 🟠 MajorCheck the full VIP sets for overlap, not just index 0.
This only compares the first API/ingress VIP. Multi-VIP baremetal clusters can still overlap on later entries, which would wrongly let the proxy deploy onto an unsupported topology.
Suggested patch
- // If API VIP == Ingress VIP, proxy is not needed - if info.apiVIPs[0] == info.ingressVIPs[0] { - return fmt.Sprintf("API VIP (%s) and ingress VIP (%s) are the same; HTTP01 proxy is not needed", info.apiVIPs[0], info.ingressVIPs[0]) - } + // If any API VIP overlaps any ingress VIP, the proxy is not needed. + apiVIPSet := make(map[string]struct{}, len(info.apiVIPs)) + for _, vip := range info.apiVIPs { + apiVIPSet[vip] = struct{}{} + } + for _, vip := range info.ingressVIPs { + if _, found := apiVIPSet[vip]; found { + return fmt.Sprintf("API VIP (%s) and ingress VIP (%s) are the same; HTTP01 proxy is not needed", vip, vip) + } + }As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/http01proxy/infrastructure.go` around lines 66 - 68, The current check only compares info.apiVIPs[0] and info.ingressVIPs[0]; change it to detect any overlap between the two slices instead. Replace the single-index comparison with a proper intersection test (e.g., build a set from info.apiVIPs and check each entry of info.ingressVIPs for membership, or vice versa) and return the same message when any VIP exists in both sets; update the conditional that uses info.apiVIPs and info.ingressVIPs accordingly so multi-VIP clusters with any shared VIP are handled correctly.
33-43:⚠️ Potential issue | 🟠 MajorPropagate Infrastructure parse failures instead of masking them.
If any of these
unstructured.NestedString*calls fail,validatePlatformfalls back to generic "unsupported" messages and hides the real problem. Please checkfound/errand return a parse error here.Suggested patch
- platformType, _, _ := unstructured.NestedString(infra.Object, "status", "platformStatus", "type") + platformType, found, err := unstructured.NestedString(infra.Object, "status", "platformStatus", "type") + if err != nil { + return nil, fmt.Errorf("failed to parse infrastructure status.platformStatus.type: %w", err) + } + if !found { + return nil, fmt.Errorf("infrastructure status.platformStatus.type not found") + } @@ switch platformType { case platformBareMetal: - apiVIPs, _, _ := unstructured.NestedStringSlice(infra.Object, "status", "platformStatus", "baremetal", "apiServerInternalIPs") - ingressVIPs, _, _ := unstructured.NestedStringSlice(infra.Object, "status", "platformStatus", "baremetal", "ingressIPs") + apiVIPs, _, err := unstructured.NestedStringSlice(infra.Object, "status", "platformStatus", "baremetal", "apiServerInternalIPs") + if err != nil { + return nil, fmt.Errorf("failed to parse baremetal.apiServerInternalIPs: %w", err) + } + ingressVIPs, _, err := unstructured.NestedStringSlice(infra.Object, "status", "platformStatus", "baremetal", "ingressIPs") + if err != nil { + return nil, fmt.Errorf("failed to parse baremetal.ingressIPs: %w", err) + } info.apiVIPs = apiVIPs info.ingressVIPs = ingressVIPs }As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/http01proxy/infrastructure.go` around lines 33 - 43, The current infrastructure parsing uses unstructured.NestedString and unstructured.NestedStringSlice but ignores the found and err values, which masks parse failures and causes validatePlatform to return generic "unsupported" messages; update the parsing in pkg/controller/http01proxy/infrastructure.go (around platformInfo construction and the switch handling platformBareMetal) to check the returned (found, err) for each unstructured.NestedString/NestedStringSlice call and, on error or missing expected fields, return a clear parse error (propagate instead of swallowing) so callers like validatePlatform receive and can report the real parsing failure; reference the platformInfo struct, platformBareMetal constant, and the unstructured.NestedString/NestedStringSlice calls when making the changes.pkg/controller/http01proxy/utils.go (2)
115-137:⚠️ Potential issue | 🟠 MajorDaemonSet drift detection is still too narrow.
This only checks a small subset of
DaemonSetSpec, so changes tohostNetwork, tolerations, volumes, security context, template annotations, strategy, or any non-zero-index container will not trigger an update. That can leave the live proxy on stale networking or privilege settings after an upgrade. Please compare the full managed spec/template here or move this resource to SSA.
141-145:⚠️ Potential issue | 🟠 MajorKeep the original reconcile error in the aggregate.
This branch still returns the status-update failure twice and drops
prependErr, which hides the root cause and can change requeue behavior.Suggested fix
errUpdate := fmt.Errorf("failed to update %s/%s status: %w", proxy.GetNamespace(), proxy.GetName(), err) if prependErr != nil { - return utilerrors.NewAggregate([]error{err, errUpdate}) + return utilerrors.NewAggregate([]error{prependErr, errUpdate}) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/http01proxy/utils.go` around lines 141 - 145, The status-update error branch currently aggregates the update error and the update-creation error (err and errUpdate) and drops the original reconcile error stored in prependErr; change the logic in the block that calls r.updateStatus so that when prependErr != nil you return utilerrors.NewAggregate([]error{prependErr, errUpdate}) (i.e., include the original reconcile error), and when prependErr == nil return errUpdate (or the single update error), ensuring you reference the existing symbols updateStatus, prependErr, errUpdate and utilerrors.NewAggregate and avoid duplicating the same error twice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bundle/manifests/cert-manager-operator.clusterserviceversion.yaml`:
- Around line 797-798: The RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY entry is using
an immutably-tagged image (quay.io/...:latest) which should be pinned; replace
the :latest tag with the immutable release reference used by the operator (use
the same value as HTTP01PROXY_OPERAND_IMAGE_VERSION or hardcode that version,
e.g. :0.1.0) so the bundle is reproducible, and apply the same change to the
other RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY occurrences noted in the file (the
other RELATED_IMAGE entries mentioned). Ensure the RELATED_IMAGE value exactly
matches the operand version the operator publishes
(HTTP01PROXY_OPERAND_IMAGE_VERSION).
In `@pkg/controller/http01proxy/utils.go`:
- Around line 140-141: The updateCondition function currently uses the stored
background context r.ctx which can ignore reconcile cancellation; change the
signature of updateCondition to accept a context parameter (ctx
context.Context), propagate that ctx into the call to r.updateStatus (replace
r.ctx with ctx), and update all callers (notably Reconciler.Reconcile) to pass
the incoming request context instead of relying on r.ctx; ensure any other
internal uses of updateCondition are updated similarly and remove reliance on
r.ctx for status updates so status updates respect deadlines/cancellations.
In `@pkg/features/features_test.go`:
- Around line 114-117: The current assertion compares isPreGA to !spec.Default
(using assert.Equal), which forces GA features to be default-enabled; change the
test to only assert that pre-GA features are disabled by making the check
conditional: compute isPreGA (as you already do using spec.PreRelease,
featuregate.Alpha and featuregate.Beta) and then if isPreGA assert that
spec.Default is false (e.g., assert.False on spec.Default with the same message
referencing feat and spec.PreRelease); do not assert anything for non-pre-GA
features so GA features can be intentionally default-off.
---
Outside diff comments:
In `@bundle/manifests/cert-manager-operator.clusterserviceversion.yaml`:
- Around line 347-399: The CSV is missing an owned CRD entry for the new
HTTP01Proxy API: add an entry under spec.customresourcedefinitions.owned with
displayName "HTTP01Proxy", kind "HTTP01Proxy", name
"http01proxies.operator.openshift.io" and the appropriate version (e.g.,
v1alpha1) so OLM recognizes the CRD as owned; update the list near the other
owned CRDs (similar to entries for IstioCSR/TrustManager) to match the RBAC
exposure of http01proxies.
---
Duplicate comments:
In `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml`:
- Around line 45-51: The container securityContext is missing
allowPrivilegeEscalation explicitly and currently runAsNonRoot is false while
adding NET_ADMIN and hostNetwork increases risk; update the container's
securityContext for the proxy (the block where "securityContext:",
"capabilities: add: - NET_ADMIN", and "runAsNonRoot: false" are defined) to set
allowPrivilegeEscalation: false so privilege escalation is explicitly denied for
the privileged workload, keeping other capability and runAsNonRoot settings as
intended.
In `@pkg/controller/http01proxy/infrastructure.go`:
- Around line 66-68: The current check only compares info.apiVIPs[0] and
info.ingressVIPs[0]; change it to detect any overlap between the two slices
instead. Replace the single-index comparison with a proper intersection test
(e.g., build a set from info.apiVIPs and check each entry of info.ingressVIPs
for membership, or vice versa) and return the same message when any VIP exists
in both sets; update the conditional that uses info.apiVIPs and info.ingressVIPs
accordingly so multi-VIP clusters with any shared VIP are handled correctly.
- Around line 33-43: The current infrastructure parsing uses
unstructured.NestedString and unstructured.NestedStringSlice but ignores the
found and err values, which masks parse failures and causes validatePlatform to
return generic "unsupported" messages; update the parsing in
pkg/controller/http01proxy/infrastructure.go (around platformInfo construction
and the switch handling platformBareMetal) to check the returned (found, err)
for each unstructured.NestedString/NestedStringSlice call and, on error or
missing expected fields, return a clear parse error (propagate instead of
swallowing) so callers like validatePlatform receive and can report the real
parsing failure; reference the platformInfo struct, platformBareMetal constant,
and the unstructured.NestedString/NestedStringSlice calls when making the
changes.
In `@pkg/controller/http01proxy/utils.go`:
- Around line 141-145: The status-update error branch currently aggregates the
update error and the update-creation error (err and errUpdate) and drops the
original reconcile error stored in prependErr; change the logic in the block
that calls r.updateStatus so that when prependErr != nil you return
utilerrors.NewAggregate([]error{prependErr, errUpdate}) (i.e., include the
original reconcile error), and when prependErr == nil return errUpdate (or the
single update error), ensuring you reference the existing symbols updateStatus,
prependErr, errUpdate and utilerrors.NewAggregate and avoid duplicating the same
error twice.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5f8af34a-6eb0-4edc-86ac-c64d16215328
📒 Files selected for processing (45)
Makefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.goapi/operator/v1alpha1/zz_generated.deepcopy.gobindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yamlbindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-daemonset.yamlbindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yamlbindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yamlbindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/crd/bases/operator.openshift.io_http01proxies.yamlconfig/manager/manager.yamlconfig/rbac/role.yamlconfig/samples/operator.openshift.io_v1alpha1_http01proxy.yamlpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/daemonsets.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/networkpolicies.gopkg/controller/http01proxy/rbacs.gopkg/controller/http01proxy/serviceaccounts.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/assets/bindata.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.go
✅ Files skipped from review due to trivial changes (18)
- pkg/operator/applyconfigurations/utils.go
- bindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go
- bindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yaml
- bindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yaml
- config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml
- bindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yaml
- pkg/operator/applyconfigurations/internal/internal.go
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- pkg/operator/informers/externalversions/generic.go
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yaml
- config/manager/manager.yaml
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go
- config/crd/bases/operator.openshift.io_http01proxies.yaml
- pkg/controller/http01proxy/networkpolicies.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go
🚧 Files skipped from review as they are similar to previous changes (16)
- api/operator/v1alpha1/features.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yaml
- pkg/operator/starter.go
- Makefile
- pkg/controller/http01proxy/serviceaccounts.go
- config/rbac/role.yaml
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxycustomdeploymentspec.go
- pkg/operator/informers/externalversions/operator/v1alpha1/interface.go
- pkg/controller/http01proxy/rbacs.go
- pkg/controller/http01proxy/install_http01proxy.go
- pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go
- api/operator/v1alpha1/http01proxy_types.go
- pkg/controller/http01proxy/daemonsets.go
- pkg/controller/http01proxy/controller.go
- pkg/operator/assets/bindata.go
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
April 7, 2026 15:42
baa4972 to
8fc4aa5
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
pkg/controller/http01proxy/utils.go (1)
115-137:⚠️ Potential issue | 🟠 MajorDaemonSet drift detection still misses networking and privilege changes.
This comparator only checks labels, first-container image/env/ports,
serviceAccountName, andnodeSelector. Hardening or rollout changes in the current manifest—hostNetwork, tolerations,allowPrivilegeEscalation, capabilities, resources,priorityClassName, and most of the pod template—won't update existing clusters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/http01proxy/utils.go` around lines 115 - 137, The daemonSetSpecModified comparator currently only checks a few fields and misses important pod/podTemplate changes (networking, security, resources, scheduling) — update daemonSetSpecModified to compare the full pod template spec (or explicitly include the missing fields) instead of only labels/first container basics: ensure you compare desired.Spec.Template.Spec vs fetched.Spec.Template.Spec (or at minimum include hostNetwork, tolerations, securityContext (containers[].securityContext and podSecurityContext), allowPrivilegeEscalation, capabilities, resources, volume/volumeMounts, affinity/nodeAffinity, tolerations, priorityClassName, dnsPolicy, restartPolicy, and any other container-level fields) so drift detection triggers updates when those values diverge.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controller/http01proxy/controller.go`:
- Around line 101-109: The controller currently never watches the cluster
Infrastructure resource, so platform changes discovered by discoverPlatform()
(in pkg/controller/http01proxy/infrastructure.go) won't trigger reconciles for
HTTP01Proxy; add a Watch for the cluster Infrastructure object
(configv1.Infrastructure) to the controller builder—e.g. add
Watches(&configv1.Infrastructure{}, handler.EnqueueRequestsFromMapFunc(mapFunc),
controllerManagedResourcePredicates) (and import configv1) so changes to the
Infrastructure/cluster resource enqueue the HTTP01Proxy reconcile via the
existing mapFunc and predicates.
In `@pkg/controller/http01proxy/install_http01proxy.go`:
- Around line 18-20: validatePlatform returning unsupported currently bails out
without cleaning up already-deployed proxy resources; change the branch so that
before returning the irrecoverable error you invoke the same teardown used in
the deletion path (e.g., call the deletion/cleanup routine used for HTTP01 proxy
such as r.deleteHTTP01ProxyResources or the function(s) that remove the
DaemonSet, ServiceAccount, RBAC, NetworkPolicies and any nftables redirection),
ensure the cleanup is idempotent and ignores NotFound errors, then log and
return common.NewIrrecoverableError(fmt.Errorf("platform validation failed"),
"%s", reason) as before; keep references to validatePlatform and the
NewIrrecoverableError return path.
In `@pkg/operator/assets/bindata.go`:
- Around line 2299-2325: The ClusterRole in
bindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yaml grants
unnecessary MCO write permissions: remove the entire rule block that targets
apiGroups: ["operator.openshift.io"] with resources: ["machineconfigurations"]
and verbs: ["update"] and the rule block that targets apiGroups:
["machineconfiguration.openshift.io"] with resources: ["machineconfigs"] and
verbs: ["get","list","create","update"]; then regenerate the bindata so
pkg/operator/assets/bindata.go is updated accordingly and verify http01-proxy
DaemonSet/service account no longer receives machineconfig write rights.
---
Duplicate comments:
In `@pkg/controller/http01proxy/utils.go`:
- Around line 115-137: The daemonSetSpecModified comparator currently only
checks a few fields and misses important pod/podTemplate changes (networking,
security, resources, scheduling) — update daemonSetSpecModified to compare the
full pod template spec (or explicitly include the missing fields) instead of
only labels/first container basics: ensure you compare
desired.Spec.Template.Spec vs fetched.Spec.Template.Spec (or at minimum include
hostNetwork, tolerations, securityContext (containers[].securityContext and
podSecurityContext), allowPrivilegeEscalation, capabilities, resources,
volume/volumeMounts, affinity/nodeAffinity, tolerations, priorityClassName,
dnsPolicy, restartPolicy, and any other container-level fields) so drift
detection triggers updates when those values diverge.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e8402571-bb33-4ef5-9165-93da089dc08e
📒 Files selected for processing (45)
Makefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.goapi/operator/v1alpha1/zz_generated.deepcopy.gobindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yamlbindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-daemonset.yamlbindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yamlbindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yamlbindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/crd/bases/operator.openshift.io_http01proxies.yamlconfig/manager/manager.yamlconfig/rbac/role.yamlconfig/samples/operator.openshift.io_v1alpha1_http01proxy.yamlpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/daemonsets.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/networkpolicies.gopkg/controller/http01proxy/rbacs.gopkg/controller/http01proxy/serviceaccounts.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/assets/bindata.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.go
✅ Files skipped from review due to trivial changes (21)
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go
- pkg/operator/applyconfigurations/internal/internal.go
- bindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yaml
- config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml
- bindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yaml
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yaml
- pkg/operator/informers/externalversions/generic.go
- Makefile
- bindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yaml
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxycustomdeploymentspec.go
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yaml
- bindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yaml
- pkg/operator/informers/externalversions/operator/v1alpha1/interface.go
- config/rbac/role.yaml
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go
- pkg/controller/http01proxy/networkpolicies.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go
- pkg/operator/listers/operator/v1alpha1/http01proxy.go
- pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go
- pkg/controller/http01proxy/daemonsets.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxy.go
🚧 Files skipped from review as they are similar to previous changes (14)
- pkg/operator/applyconfigurations/utils.go
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- pkg/operator/starter.go
- config/manager/manager.yaml
- api/operator/v1alpha1/features.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- pkg/features/features_test.go
- pkg/controller/http01proxy/serviceaccounts.go
- pkg/operator/setup_manager.go
- pkg/controller/http01proxy/constants.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go
- bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
- api/operator/v1alpha1/http01proxy_types.go
- config/crd/bases/operator.openshift.io_http01proxies.yaml
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
April 17, 2026 15:26
8fc4aa5 to
b1138fe
Compare
|
@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. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
bundle/manifests/cert-manager-operator.clusterserviceversion.yaml (1)
797-798:⚠️ Potential issue | 🟠 MajorPin the HTTP01 proxy image to the release tag.
RELATED_IMAGE_CERT_MANAGER_HTTP01PROXYandspec.relatedImagesstill point at:latest, whileHTTP01PROXY_OPERAND_IMAGE_VERSIONsays0.1.0. That makes the bundle non-reproducible and lets the reported operand version drift from what clusters actually deploy. Use the same immutable tag in all three places.Also applies to: 805-806, 923-924
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bundle/manifests/cert-manager-operator.clusterserviceversion.yaml` around lines 797 - 798, The RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY environment var and spec.relatedImages entries are using :latest while HTTP01PROXY_OPERAND_IMAGE_VERSION is 0.1.0; update RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY and the corresponding spec.relatedImages entries to use the immutable release tag (e.g., quay.io/bapalm/cert-mgr-http01-proxy:0.1.0) so all three places (RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY, spec.relatedImages, and HTTP01PROXY_OPERAND_IMAGE_VERSION) reference the same pinned tag and eliminate the :latest drift.pkg/controller/http01proxy/controller.go (1)
104-112:⚠️ Potential issue | 🟠 MajorWatch
Infrastructure/clusteras a reconciliation trigger.
discoverPlatform()depends onInfrastructure/cluster, but this controller never watches that object. Because unsupported platform failures are treated as irrecoverable, later VIP/platform fixes will not enqueue a new reconcile unless someone touches theHTTP01Proxymanually.#!/bin/bash sed -n '72,112p' pkg/controller/http01proxy/controller.go printf '\n---\n' sed -n '37,74p' pkg/controller/http01proxy/infrastructure.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/http01proxy/controller.go` around lines 104 - 112, The controller never watches the Infrastructure/cluster object that discoverPlatform() relies on, so platform/VIP fixes won't requeue reconciles; update the controller setup in NewControllerManagedBy(...) (the chain that For(&v1alpha1.HTTP01Proxy{}, ...) and subsequent Watches(...)) to add a watch for the Infrastructure resource (e.g. &configv1.Infrastructure{}) using the same mapFunc enqueuer (handler.EnqueueRequestsFromMapFunc(mapFunc)) and appropriate predicates (controllerManagedResourcePredicates or the existing withIgnoreStatusUpdatePredicates as needed) so changes to Infrastructure/cluster trigger reconciles that call discoverPlatform().pkg/controller/http01proxy/install_http01proxy.go (1)
18-20:⚠️ Potential issue | 🟠 MajorTear down existing resources before returning unsupported.
This branch only blocks future applies. If the proxy was already deployed, the DaemonSet, RBAC, ServiceAccount, and NetworkPolicies remain until the CR is deleted, which leaves the old datapath in place after the controller has declared the platform unsupported. Call the same cleanup path used in
cleanUp()before returning the irrecoverable error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/http01proxy/install_http01proxy.go` around lines 18 - 20, The platform-validation branch returns an irrecoverable error but doesn't remove existing resources; before returning from the validatePlatform check in install_http01proxy.go, call the same cleanup path used by cleanUp() to delete the DaemonSet, RBAC, ServiceAccount and NetworkPolicies (e.g., invoke r.cleanUp(...) or the existing cleanUp helper used elsewhere), handle/propagate any cleanup errors appropriately (log them with r.log and include them in the returned error if needed), then return common.NewIrrecoverableError(fmt.Errorf("platform validation failed"), "%s", reason) as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml`:
- Around line 23-24: The DaemonSet currently sets hostNetwork: true which
prevents Kubernetes NetworkPolicies from applying; edit the
cert-manager-http01-proxy DaemonSet spec to remove or set hostNetwork to false
(i.e., stop joining the node network namespace) and instead expose required
ports via container hostPort or a NodePort/HostPort + proper selector, or
enforce node-level firewall rules if node networking is required; update the pod
spec (where serviceAccountName: cert-manager-http01-proxy appears) to use
hostPort or a Service-backed approach and verify NetworkPolicy rules now target
the podSelector so least-privilege egress works as intended.
In `@pkg/controller/http01proxy/controller.go`:
- Around line 40-46: The reconciler currently holds a long-lived context (ctx)
on the struct; remove the ctx field (and any initialization using
context.Background()) from the controller struct and any place it is set, then
propagate the context parameter from Reconcile through all helper methods
(cleanUp, reconcileHTTP01ProxyDeployment and any nested calls) so every
downstream API call uses the Reconcile-scoped ctx for cancellation and
deadlines; update signatures to accept ctx where needed, replace r.ctx uses with
the passed ctx, and ensure no code uses a long-lived background context or a
reconciler-level ctx variable.
In `@pkg/controller/http01proxy/infrastructure.go`:
- Around line 25-34: The current getOrDiscoverPlatform caches platformInfo in
r.cachedPlatform forever; change this so the controller doesn't use stale
Infrastructure data by invalidating the cache before each reconcile or on
Infrastructure change. Concretely: either set r.cachedPlatform = nil at the
start of Reconciler.Reconcile (so getOrDiscoverPlatform always re-reads
per-reconcile), or implement an Infrastructure watch/event handler that clears
r.cachedPlatform when the Infrastructure/cluster resource changes; update
getOrDiscoverPlatform/discoverPlatform usage accordingly to avoid permanent
caching.
---
Duplicate comments:
In `@bundle/manifests/cert-manager-operator.clusterserviceversion.yaml`:
- Around line 797-798: The RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY environment
var and spec.relatedImages entries are using :latest while
HTTP01PROXY_OPERAND_IMAGE_VERSION is 0.1.0; update
RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY and the corresponding spec.relatedImages
entries to use the immutable release tag (e.g.,
quay.io/bapalm/cert-mgr-http01-proxy:0.1.0) so all three places
(RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY, spec.relatedImages, and
HTTP01PROXY_OPERAND_IMAGE_VERSION) reference the same pinned tag and eliminate
the :latest drift.
In `@pkg/controller/http01proxy/controller.go`:
- Around line 104-112: The controller never watches the Infrastructure/cluster
object that discoverPlatform() relies on, so platform/VIP fixes won't requeue
reconciles; update the controller setup in NewControllerManagedBy(...) (the
chain that For(&v1alpha1.HTTP01Proxy{}, ...) and subsequent Watches(...)) to add
a watch for the Infrastructure resource (e.g. &configv1.Infrastructure{}) using
the same mapFunc enqueuer (handler.EnqueueRequestsFromMapFunc(mapFunc)) and
appropriate predicates (controllerManagedResourcePredicates or the existing
withIgnoreStatusUpdatePredicates as needed) so changes to Infrastructure/cluster
trigger reconciles that call discoverPlatform().
In `@pkg/controller/http01proxy/install_http01proxy.go`:
- Around line 18-20: The platform-validation branch returns an irrecoverable
error but doesn't remove existing resources; before returning from the
validatePlatform check in install_http01proxy.go, call the same cleanup path
used by cleanUp() to delete the DaemonSet, RBAC, ServiceAccount and
NetworkPolicies (e.g., invoke r.cleanUp(...) or the existing cleanUp helper used
elsewhere), handle/propagate any cleanup errors appropriately (log them with
r.log and include them in the returned error if needed), then return
common.NewIrrecoverableError(fmt.Errorf("platform validation failed"), "%s",
reason) as before.
🪄 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: Pro Plus
Run ID: 2127a61f-e15f-4258-bef3-d76f89e91141
⛔ Files ignored due to path filters (1)
api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (44)
Makefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.gobindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yamlbindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-daemonset.yamlbindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yamlbindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yamlbindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/crd/bases/operator.openshift.io_http01proxies.yamlconfig/manager/manager.yamlconfig/rbac/role.yamlconfig/samples/operator.openshift.io_v1alpha1_http01proxy.yamlpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/daemonsets.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/networkpolicies.gopkg/controller/http01proxy/rbacs.gopkg/controller/http01proxy/serviceaccounts.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/assets/bindata.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.go
✅ Files skipped from review due to trivial changes (25)
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go
- pkg/operator/applyconfigurations/internal/internal.go
- bindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yaml
- bindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yaml
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- bindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yaml
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yaml
- pkg/controller/http01proxy/serviceaccounts.go
- pkg/operator/informers/externalversions/operator/v1alpha1/interface.go
- pkg/features/features_test.go
- bindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go
- pkg/operator/informers/externalversions/generic.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxycustomdeploymentspec.go
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yaml
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go
- api/operator/v1alpha1/http01proxy_types.go
- pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go
- pkg/operator/listers/operator/v1alpha1/http01proxy.go
- pkg/controller/http01proxy/utils.go
- pkg/operator/assets/bindata.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go
- config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxy.go
🚧 Files skipped from review as they are similar to previous changes (12)
- config/manager/manager.yaml
- api/operator/v1alpha1/features.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- pkg/operator/applyconfigurations/utils.go
- config/rbac/role.yaml
- Makefile
- pkg/controller/http01proxy/networkpolicies.go
- config/crd/bases/operator.openshift.io_http01proxies.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go
- pkg/controller/http01proxy/daemonsets.go
- pkg/operator/setup_manager.go
- pkg/operator/starter.go
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
May 5, 2026 22:37
b1138fe to
6bf13d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml (1)
23-24:⚠️ Potential issue | 🟠 Major
hostNetworkmakes the added NetworkPolicies effectively non-enforcing.This pod joins the node network namespace, so the HTTP01Proxy NetworkPolicies won't reliably constrain it. If host networking is required here, isolation needs to move to a host-scoped mechanism instead of Kubernetes NetworkPolicies.
Do Kubernetes NetworkPolicies apply to Pods with `hostNetwork: true`? Please verify using the official Kubernetes documentation, and include any caveats called out there.🤖 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 `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml` around lines 23 - 24, The manifest sets hostNetwork: true for the cert-manager-http01-proxy pod which makes Kubernetes NetworkPolicies ineffective for it; verify the official Kubernetes docs on NetworkPolicy (Pods using hostNetwork bypass namespace/pod isolation) and either remove hostNetwork if not required or move isolation to a host-scoped mechanism (e.g., host firewall, Node-level iptables/iptables-restore, or hostNetwork-aware CNI policy) and document the caveat next to serviceAccountName: cert-manager-http01-proxy so reviewers know why NetworkPolicies won’t apply.pkg/controller/http01proxy/infrastructure.go (1)
24-35:⚠️ Potential issue | 🟠 MajorDon't cache
Infrastructure/clusterforever.
cachedPlatformis written once and never invalidated, so later API/ingress VIP changes are ignored for the lifetime of the controller process. Re-discover on each reconcile, or clear the cache whenInfrastructure/clusterchanges.🤖 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.go` around lines 24 - 35, The getOrDiscoverPlatform method currently caches platform info in Reconciler.cachedPlatform and never invalidates it, causing VIP changes to be ignored; update this by either removing the long-lived cache so getOrDiscoverPlatform always calls discoverPlatform(ctx) on each reconcile, or add logic to invalidate/update cachedPlatform when the cluster Infrastructure resource changes (e.g., detect changes to the Infrastructure/cluster resource and clear cachedPlatform), modifying getOrDiscoverPlatform, Reconciler.cachedPlatform usage, and discoverPlatform accordingly so the controller re-discovers platform info when Infrastructure/cluster is updated.pkg/controller/http01proxy/install_http01proxy.go (1)
18-20:⚠️ Potential issue | 🟠 MajorClean up existing proxy resources before returning unsupported.
This branch only stops future applies. If the proxy was already deployed, the DaemonSet/RBAC/NetworkPolicies/ServiceAccount stay behind, so the redirect path can remain active after the controller reports Degraded. Run the same teardown used in the deletion path before returning the irrecoverable error.
🤖 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/install_http01proxy.go` around lines 18 - 20, When validatePlatform(info) returns a non-empty reason, run the same teardown used by the deletion path to remove any existing proxy resources (DaemonSet, RBAC, NetworkPolicies, ServiceAccount) before returning the irrecoverable error; locate the deletion/teardown helper used elsewhere in this package (the function that removes the proxy resources) and invoke it here, handle/log any teardown errors but proceed to return common.NewIrrecoverableError(fmt.Errorf("platform validation failed"), "%s", reason) after cleanup so no stale redirect resources remain; keep the existing log r.log.V(1).Info("platform not supported for HTTP01 proxy", ...) around the operation.pkg/operator/assets/bindata.go (1)
2310-2324:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove unnecessary MachineConfig write permissions from the HTTP01 proxy ClusterRole.
The embedded ClusterRole still grants
updateonoperator.openshift.io/machineconfigurationsandcreate/updateonmachineconfiguration.openshift.io/machineconfigs. This is excessive for the proxy workload and expands privilege scope unnecessarily.Suggested manifest change (source YAML, then regenerate bindata)
rules: - apiGroups: - config.openshift.io resources: - clusterversions - infrastructures - ingresses verbs: - get - list - watch - - apiGroups: - - operator.openshift.io - resources: - - machineconfigurations - verbs: - - update - - apiGroups: - - machineconfiguration.openshift.io - resources: - - machineconfigs - verbs: - - get - - list - - create - - update🤖 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/operator/assets/bindata.go` around lines 2310 - 2324, Remove the unnecessary MachineConfig write permissions from the embedded ClusterRole by locating the ClusterRole manifest in pkg/operator/assets/bindata.go that contains apiGroups operator.openshift.io with resources machineconfigurations and machineconfiguration.openshift.io with resources machineconfigs, and delete the rule blocks granting "update" and "create/update" respectively; after editing the source ClusterRole YAML (the manifest embedded into bindata via Generate/asset tooling) regenerate the bindata assets so the compiled bindata.go reflects the removed rules.
🤖 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/controller/http01proxy/controller.go`:
- Around line 104-105: Remove the GenerationChangedPredicate from the primary
watch so deletions (which only set metadata.deletionTimestamp) still trigger
reconciliation: in the controller setup where
ctrl.NewControllerManagedBy(mgr).For(&v1alpha1.HTTP01Proxy{},
builder.WithPredicates(predicate.GenerationChangedPredicate{})) is defined, drop
the builder.WithPredicates(predicate.GenerationChangedPredicate{}) argument and
call For(&v1alpha1.HTTP01Proxy{}) so the reconciler (and the HTTP01Proxy cleanup
path) runs on deletion events.
In `@pkg/controller/http01proxy/daemonsets.go`:
- Around line 35-37: The Pod template labels are being overwritten by
ds.Spec.Template.Labels = resourceLabels which can remove required selector
labels (e.g., app: cert-manager-http01-proxy); instead merge resourceLabels into
the existing ds.Spec.Template.Labels (preserving existing keys) after calling
common.UpdateResourceLabels(ds, resourceLabels). Locate the assignment to
ds.Spec.Template.Labels and replace it with logic that iterates/merges keys from
resourceLabels into ds.Spec.Template.Labels (initializing the map if nil) so
existing selector-required labels remain alongside the new ones.
---
Duplicate comments:
In `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml`:
- Around line 23-24: The manifest sets hostNetwork: true for the
cert-manager-http01-proxy pod which makes Kubernetes NetworkPolicies ineffective
for it; verify the official Kubernetes docs on NetworkPolicy (Pods using
hostNetwork bypass namespace/pod isolation) and either remove hostNetwork if not
required or move isolation to a host-scoped mechanism (e.g., host firewall,
Node-level iptables/iptables-restore, or hostNetwork-aware CNI policy) and
document the caveat next to serviceAccountName: cert-manager-http01-proxy so
reviewers know why NetworkPolicies won’t apply.
In `@pkg/controller/http01proxy/infrastructure.go`:
- Around line 24-35: The getOrDiscoverPlatform method currently caches platform
info in Reconciler.cachedPlatform and never invalidates it, causing VIP changes
to be ignored; update this by either removing the long-lived cache so
getOrDiscoverPlatform always calls discoverPlatform(ctx) on each reconcile, or
add logic to invalidate/update cachedPlatform when the cluster Infrastructure
resource changes (e.g., detect changes to the Infrastructure/cluster resource
and clear cachedPlatform), modifying getOrDiscoverPlatform,
Reconciler.cachedPlatform usage, and discoverPlatform accordingly so the
controller re-discovers platform info when Infrastructure/cluster is updated.
In `@pkg/controller/http01proxy/install_http01proxy.go`:
- Around line 18-20: When validatePlatform(info) returns a non-empty reason, run
the same teardown used by the deletion path to remove any existing proxy
resources (DaemonSet, RBAC, NetworkPolicies, ServiceAccount) before returning
the irrecoverable error; locate the deletion/teardown helper used elsewhere in
this package (the function that removes the proxy resources) and invoke it here,
handle/log any teardown errors but proceed to return
common.NewIrrecoverableError(fmt.Errorf("platform validation failed"), "%s",
reason) after cleanup so no stale redirect resources remain; keep the existing
log r.log.V(1).Info("platform not supported for HTTP01 proxy", ...) around the
operation.
In `@pkg/operator/assets/bindata.go`:
- Around line 2310-2324: Remove the unnecessary MachineConfig write permissions
from the embedded ClusterRole by locating the ClusterRole manifest in
pkg/operator/assets/bindata.go that contains apiGroups operator.openshift.io
with resources machineconfigurations and machineconfiguration.openshift.io with
resources machineconfigs, and delete the rule blocks granting "update" and
"create/update" respectively; after editing the source ClusterRole YAML (the
manifest embedded into bindata via Generate/asset tooling) regenerate the
bindata assets so the compiled bindata.go reflects the removed rules.
🪄 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: 91a83d88-ebc2-4246-a600-9d1356a77dfe
⛔ Files ignored due to path filters (1)
api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (44)
Makefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.gobindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yamlbindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-daemonset.yamlbindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yamlbindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yamlbindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlconfig/crd/bases/operator.openshift.io_http01proxies.yamlconfig/manager/manager.yamlconfig/rbac/role.yamlconfig/samples/operator.openshift.io_v1alpha1_http01proxy.yamlpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/daemonsets.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/networkpolicies.gopkg/controller/http01proxy/rbacs.gopkg/controller/http01proxy/serviceaccounts.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/assets/bindata.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.go
🚧 Files skipped from review as they are similar to previous changes (7)
- bindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yaml
- bindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yaml
- pkg/operator/listers/operator/v1alpha1/http01proxy.go
- config/crd/bases/operator.openshift.io_http01proxies.yaml
- api/operator/v1alpha1/features.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
May 6, 2026 15:24
3510c2b to
69a74fd
Compare
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
May 14, 2026 19:07
69a74fd to
77ed50a
Compare
Member
Author
|
/retest |
anandkuma77
reviewed
May 28, 2026
anandkuma77
reviewed
May 28, 2026
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
2 times, most recently
from
May 29, 2026 18:54
7ada5c9 to
716a848
Compare
anandkuma77
reviewed
Jun 3, 2026
anandkuma77
reviewed
Jun 3, 2026
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
2 times, most recently
from
June 8, 2026 18:35
c2125f1 to
34d01f7
Compare
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
June 15, 2026 16:25
34d01f7 to
d6922d2
Compare
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
June 24, 2026 19:10
d6922d2 to
e4f1d04
Compare
Member
Author
|
/retest ci/prow/e2e-operator |
Member
Author
|
/test e2e-operator |
anandkuma77
reviewed
Jul 1, 2026
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
July 6, 2026 16:17
2a8621d to
3140cbf
Compare
Member
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml (1)
34-60: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo liveness/readiness probes defined.
The proxy container has no health checks, so a hung or crashed nftables/proxy process won't be automatically detected or restarted by the kubelet.
As per path instructions, "Liveness + readiness probes defined" for Kubernetes/OpenShift manifests.
🤖 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 `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml` around lines 34 - 60, The http01-proxy container in cert-manager-http01-proxy-daemonset.yaml is missing health checks, so add both liveness and readiness probes to the http01-proxy container spec. Use the existing container settings like name http01-proxy and PROXY_PORT to wire the probes to the proxy process endpoint, and keep the probes aligned with Kubernetes/OpenShift manifest requirements so kubelet can detect hung or unhealthy proxy instances.Source: Path instructions
.golangci.bck.yaml (1)
1-71: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stray
.golangci.bck.yamlbackup.Makefilepoints golangci-lint at.golangci.yaml, so this duplicate config is dead weight and can confuse 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 @.golangci.bck.yaml around lines 1 - 71, The repository contains a stray backup golangci-lint config that duplicates the active settings and can cause confusion. Remove the .golangci.bck.yaml file entirely, and make sure any future lint configuration changes are applied only to the main .golangci.yaml referenced by Makefile, keeping this backup out of the tree.bundle/manifests/cert-manager-operator.clusterserviceversion.yaml (1)
368-370: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
description/displayNameand an alm-example forHTTP01Proxy.Every other owned CRD has
description/displayName; this new entry has neither, and there's no correspondingalm-examplessample CR (unlike IstioCSR/TrustManager, which this feature follows).♻️ Suggested fix
- - kind: HTTP01Proxy + - description: HTTP01Proxy is the Schema for configuring the HTTP-01 challenge + proxy used to complete ACME challenges for the API endpoint on platforms + without an exposed API VIP. + displayName: HTTP01 Challenge Proxy + kind: HTTP01Proxy name: http01proxies.operator.openshift.io version: v1alpha1🤖 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 `@bundle/manifests/cert-manager-operator.clusterserviceversion.yaml` around lines 368 - 370, The new HTTP01Proxy CR entry in the CSV is missing the same metadata used by the other owned CRDs. Update the HTTP01Proxy resource block to include both description and displayName, and add a matching sample to the alm-examples payload so it follows the existing IstioCSR/TrustManager pattern. Use the HTTP01Proxy section in cert-manager-operator.clusterserviceversion.yaml to keep the new CRD entry consistent with the rest of the manifest.
🤖 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 `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml`:
- Around line 45-52: The container securityContext in
cert-manager-http01-proxy-daemonset should be tightened to follow the manifest
policy: change the explicit root allowance and declare a minimal filesystem
posture. Update the securityContext block used by the daemonset container to set
runAsNonRoot to true where possible, add readOnlyRootFilesystem, and keep
allowPrivilegeEscalation disabled; if root is still required for
NET_ADMIN/nftables behavior, document that exception in a nearby comment and
keep the scope as narrow as possible.
In `@rebase_all.sh`:
- Around line 19-31: The branch iteration in rebase_all.sh is not fully guarded,
so a failed git checkout in the loop can still terminate the script under set
-e. Update the loop around the branch handling logic to treat git checkout
"$branch" like the existing git pull --rebase upstream "$MAIN_BRANCH" step:
detect checkout failures, log a warning for that branch, optionally clean up any
partial state, and continue to the next branch instead of exiting the whole
script.
---
Nitpick comments:
In @.golangci.bck.yaml:
- Around line 1-71: The repository contains a stray backup golangci-lint config
that duplicates the active settings and can cause confusion. Remove the
.golangci.bck.yaml file entirely, and make sure any future lint configuration
changes are applied only to the main .golangci.yaml referenced by Makefile,
keeping this backup out of the tree.
In `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml`:
- Around line 34-60: The http01-proxy container in
cert-manager-http01-proxy-daemonset.yaml is missing health checks, so add both
liveness and readiness probes to the http01-proxy container spec. Use the
existing container settings like name http01-proxy and PROXY_PORT to wire the
probes to the proxy process endpoint, and keep the probes aligned with
Kubernetes/OpenShift manifest requirements so kubelet can detect hung or
unhealthy proxy instances.
In `@bundle/manifests/cert-manager-operator.clusterserviceversion.yaml`:
- Around line 368-370: The new HTTP01Proxy CR entry in the CSV is missing the
same metadata used by the other owned CRDs. Update the HTTP01Proxy resource
block to include both description and displayName, and add a matching sample to
the alm-examples payload so it follows the existing IstioCSR/TrustManager
pattern. Use the HTTP01Proxy section in
cert-manager-operator.clusterserviceversion.yaml to keep the new CRD entry
consistent with the rest of the manifest.
🪄 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: 9d7e102b-df28-4a4f-936b-b9479ef464c4
⛔ Files ignored due to path filters (1)
api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (51)
.golangci.bck.yamlCLAUDE.mdMakefileTODO-cleanup.mdapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.gobindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yamlbindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-daemonset.yamlbindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yamlbindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yamlbindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yamlbundle/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/verify-http01proxy.shpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/daemonsets.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/networkpolicies.gopkg/controller/http01proxy/rbacs.gopkg/controller/http01proxy/serviceaccounts.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/assets/bindata.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.gorebase_all.sh
✅ Files skipped from review due to trivial changes (14)
- CLAUDE.md
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxycustomdeploymentspec.go
- pkg/operator/applyconfigurations/internal/internal.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- TODO-cleanup.md
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- bundle/manifests/operator.openshift.io_http01proxies.yaml
- pkg/operator/informers/externalversions/generic.go
- pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go
- config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml
🚧 Files skipped from review as they are similar to previous changes (22)
- bindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yaml
- bindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yaml
- bindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yaml
- bindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yaml
- pkg/operator/applyconfigurations/utils.go
- pkg/features/features_test.go
- pkg/controller/http01proxy/serviceaccounts.go
- api/operator/v1alpha1/features.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go
- config/manager/manager.yaml
- pkg/operator/informers/externalversions/operator/v1alpha1/interface.go
- pkg/operator/starter.go
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yaml
- pkg/controller/http01proxy/rbacs.go
- pkg/controller/http01proxy/constants.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go
- api/operator/v1alpha1/http01proxy_types.go
- pkg/controller/http01proxy/infrastructure.go
- Makefile
- pkg/operator/setup_manager.go
- pkg/operator/assets/bindata.go
- config/crd/bases/operator.openshift.io_http01proxies.yaml
Member
Author
CodeRabbit Feedback - Status UpdateThank you for the comprehensive review! I've addressed the actionable feedback: ✅ Already Fixed (in earlier commits)
✅ Just Fixed (commit 5323304)
📝 Deferred (acknowledged design decisions)
SummaryAll critical CodeRabbit feedback has been addressed. The controller now properly handles platform changes, has comprehensive drift detection, and follows all best practices for context threading, error handling, and least-privilege RBAC. Tests pass, linter clean. Ready for human review. |
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
July 7, 2026 17:17
5323304 to
e7b9069
Compare
Member
Author
|
Squashed commits into a single atomic commit (e7b9069) with comprehensive commit message that includes:
PR now has clean single-commit history ready for review. |
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
July 13, 2026 17:58
e7b9069 to
c5ed26d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml (1)
45-52: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
readOnlyRootFilesystemstill not set.Same gap flagged in a previous review — still absent here.
As per path instructions, "securityContext: runAsNonRoot, readOnlyRootFilesystem, allowPrivilegeEscalation: false."
🤖 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 `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml` around lines 45 - 52, Add readOnlyRootFilesystem: true to the container securityContext alongside allowPrivilegeEscalation and runAsNonRoot in the DaemonSet manifest, while preserving the existing capabilities settings.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 `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml`:
- Around line 34-59: Add livenessProbe and readinessProbe definitions to the
http01-proxy container in the DaemonSet, using a probe target that verifies the
proxy is accepting traffic on its configured port 8888. Configure liveness to
restart a hung or wedged proxy and readiness to exclude it from service until
healthy, while preserving the existing container configuration.
In `@TODO-cleanup.md`:
- Around line 36-39: Update the “Open PRs (in flight)” section in
TODO-cleanup.md to remove PR `#242`, since it is closed and should not appear
among active work. Move that item to an appropriate completed/closed section or
represent it with an explicit closed status, while keeping PR `#314` listed as in
flight.
---
Duplicate comments:
In `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml`:
- Around line 45-52: Add readOnlyRootFilesystem: true to the container
securityContext alongside allowPrivilegeEscalation and runAsNonRoot in the
DaemonSet manifest, while preserving the existing capabilities settings.
🪄 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: 88fd65c9-9c73-42ce-84d4-8b879eadba0b
⛔ Files ignored due to path filters (1)
api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (51)
.golangci.bck.yamlCLAUDE.mdMakefileTODO-cleanup.mdapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.gobindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yamlbindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-daemonset.yamlbindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yamlbindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yamlbindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yamlbundle/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/verify-http01proxy.shpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/daemonsets.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/networkpolicies.gopkg/controller/http01proxy/rbacs.gopkg/controller/http01proxy/serviceaccounts.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/assets/bindata.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.gorebase_all.sh
🚧 Files skipped from review as they are similar to previous changes (48)
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yaml
- CLAUDE.md
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxycustomdeploymentspec.go
- bindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yaml
- bindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yaml
- pkg/operator/applyconfigurations/internal/internal.go
- .golangci.bck.yaml
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yaml
- config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go
- bindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yaml
- rebase_all.sh
- pkg/controller/http01proxy/serviceaccounts.go
- config/crd/kustomization.yaml
- bindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- config/manager/manager.yaml
- pkg/controller/http01proxy/constants.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go
- pkg/controller/http01proxy/install_http01proxy.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go
- pkg/controller/http01proxy/rbacs.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go
- pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go
- config/crd/bases/operator.openshift.io_http01proxies.yaml
- api/operator/v1alpha1/features.go
- pkg/controller/http01proxy/utils.go
- pkg/features/features_test.go
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- pkg/controller/http01proxy/infrastructure.go
- Makefile
- pkg/operator/informers/externalversions/generic.go
- api/operator/v1alpha1/http01proxy_types.go
- pkg/operator/applyconfigurations/utils.go
- pkg/operator/listers/operator/v1alpha1/http01proxy.go
- config/rbac/role.yaml
- pkg/operator/starter.go
- pkg/operator/assets/bindata.go
- pkg/controller/http01proxy/networkpolicies.go
- pkg/controller/http01proxy/daemonsets.go
- pkg/controller/http01proxy/controller.go
- bundle/manifests/operator.openshift.io_http01proxies.yaml
- pkg/operator/setup_manager.go
- bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
- hack/verify-http01proxy.sh
- pkg/operator/informers/externalversions/operator/v1alpha1/interface.go
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
3 times, most recently
from
July 14, 2026 19:00
cee1bf8 to
2aaec53
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml (1)
22-23: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider disabling
automountServiceAccountTokenunless the container itself calls the K8s API.This is a
hostNetwork+NET_ADMINDaemonSet whose sole described job is redirecting/forwarding HTTP traffic; the SA token appears to exist for RBAC/SCC purposes (bound by cluster role/SCC role bindings) rather than for in-pod API calls. If the running process doesn't call the K8s API directly, settingautomountServiceAccountToken: falsereduces the blast radius if this privileged container is ever compromised.As per path instructions, "automountServiceAccountToken: false unless needed" for Kubernetes/OpenShift manifests.
🛡️ Proposed change (only if the container doesn't call the K8s API)
serviceAccountName: cert-manager-http01-proxy + automountServiceAccountToken: false hostNetwork: true🤖 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 `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml` around lines 22 - 23, Update the DaemonSet pod spec associated with serviceAccountName cert-manager-http01-proxy to set automountServiceAccountToken to false. Preserve the service account and its RBAC/SCC bindings, and only retain token mounting if the container explicitly requires Kubernetes API access.Source: Path instructions
pkg/controller/http01proxy/infrastructure_test.go (1)
3-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winImport grouping has an extra separation not matching the declared local-prefix convention.
github.com/go-logr/logris split into its own group between the k8s.io/sigs.k8s.io group and the localgithub.com/openshift/cert-manager-operatorgroup. The declared convention is stdlib / third-party / local (with thegithub.com/openshift/cert-manager-operatorprefix) —go-logrshould merge into the third-party group.As per coding guidelines, "Use standard Go import organization, separating local imports from third-party packages with the local prefix
github.com/openshift/cert-manager-operator, as configured in.golangci.yaml."♻️ Proposed import grouping
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "sigs.k8s.io/controller-runtime/pkg/client" - - "github.com/go-logr/logr" + "github.com/go-logr/logr" "github.com/openshift/cert-manager-operator/pkg/controller/common/fakes"🤖 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 3 - 15, Merge the github.com/go-logr/logr import into the existing third-party import group, keeping the standard-library imports separate and github.com/openshift/cert-manager-operator/pkg/controller/common/fakes as the local group.Source: Coding guidelines
🤖 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 `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml`:
- Around line 22-23: Update the DaemonSet pod spec associated with
serviceAccountName cert-manager-http01-proxy to set automountServiceAccountToken
to false. Preserve the service account and its RBAC/SCC bindings, and only
retain token mounting if the container explicitly requires Kubernetes API
access.
In `@pkg/controller/http01proxy/infrastructure_test.go`:
- Around line 3-15: Merge the github.com/go-logr/logr import into the existing
third-party import group, keeping the standard-library imports separate and
github.com/openshift/cert-manager-operator/pkg/controller/common/fakes as the
local group.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2fb3c66a-383f-4ef7-9c47-ba3643d945f5
⛔ Files ignored due to path filters (1)
api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (53)
.golangci.bck.yamlCLAUDE.mdMakefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.gobindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yamlbindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-daemonset.yamlbindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yamlbindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yamlbindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yamlbundle/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/verify-http01proxy.shpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/controller_test.gopkg/controller/http01proxy/daemonsets.gopkg/controller/http01proxy/daemonsets_test.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/infrastructure_test.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/networkpolicies.gopkg/controller/http01proxy/rbacs.gopkg/controller/http01proxy/serviceaccounts.gopkg/controller/http01proxy/utils.gopkg/controller/http01proxy/utils_test.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/assets/bindata.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.go
🚧 Files skipped from review as they are similar to previous changes (47)
- bindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yaml
- config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml
- pkg/operator/applyconfigurations/utils.go
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yaml
- bindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yaml
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxycustomdeploymentspec.go
- bindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yaml
- CLAUDE.md
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go
- bindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- pkg/operator/starter.go
- .golangci.bck.yaml
- pkg/controller/http01proxy/constants.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yaml
- api/operator/v1alpha1/features.go
- pkg/controller/http01proxy/rbacs.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go
- pkg/controller/http01proxy/infrastructure.go
- pkg/controller/http01proxy/install_http01proxy.go
- config/manager/manager.yaml
- pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go
- Makefile
- pkg/operator/informers/externalversions/operator/v1alpha1/interface.go
- config/crd/bases/operator.openshift.io_http01proxies.yaml
- pkg/controller/http01proxy/controller.go
- api/operator/v1alpha1/http01proxy_types.go
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go
- pkg/operator/informers/externalversions/generic.go
- pkg/operator/listers/operator/v1alpha1/http01proxy.go
- pkg/operator/applyconfigurations/internal/internal.go
- config/rbac/role.yaml
- pkg/controller/http01proxy/daemonsets_test.go
- hack/verify-http01proxy.sh
- pkg/operator/setup_manager.go
- pkg/controller/http01proxy/utils.go
- pkg/controller/http01proxy/controller_test.go
- pkg/controller/http01proxy/serviceaccounts.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go
- bundle/manifests/operator.openshift.io_http01proxies.yaml
- pkg/controller/http01proxy/networkpolicies.go
- bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
- pkg/controller/http01proxy/daemonsets.go
- pkg/operator/assets/bindata.go
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
2 times, most recently
from
July 14, 2026 19:18
0748335 to
f8c9fa2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/controller/http01proxy/infrastructure_test.go (1)
142-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winVerify both discovered VIP collections.
The BareMetal case supplies
ingressIPs, but the assertions only check the API VIP count. A regression that drops, swaps, or misreads ingress VIPs would pass. Assert the exact API and ingress VIP slices.Also applies to: 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 142 - 160, The BareMetal test case currently validates only the API VIP count; update its expectations to assert the exact API and ingress VIP slices discovered from platform status. Apply the same change to the additional BareMetal case around the corresponding assertions, preserving the existing platform expectation.pkg/controller/http01proxy/utils_test.go (1)
97-165: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover security-sensitive DaemonSet drift.
Add cases mutating
SecurityContext, probes, resources, node selection, and tolerations. Otherwise, regressions that stop restoring manifest hardening or scheduling configuration can pass while “full drift detection” appears covered.🤖 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/utils_test.go` around lines 97 - 165, Extend TestDaemonSetSpecModified with table cases that independently alter SecurityContext, liveness/readiness probes, resource requests or limits, node-selection configuration, and tolerations on the DaemonSet pod template, asserting each mutation returns want: true. Reuse baseDaemonSet and keep the existing test structure and unchanged-spec expectation intact.
🤖 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 `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml`:
- Around line 25-26: Replace the hard-coded nodeSelector in the cert-manager
HTTP-01 proxy DaemonSet with platform-aware affinity that supports current
control-plane labels and HyperShift, or validate and reject unsupported
topologies before creating the DaemonSet. Preserve control-plane scheduling
intent without assuming the legacy node-role.kubernetes.io/master label or
standard HA topology.
---
Nitpick comments:
In `@pkg/controller/http01proxy/infrastructure_test.go`:
- Around line 142-160: The BareMetal test case currently validates only the API
VIP count; update its expectations to assert the exact API and ingress VIP
slices discovered from platform status. Apply the same change to the additional
BareMetal case around the corresponding assertions, preserving the existing
platform expectation.
In `@pkg/controller/http01proxy/utils_test.go`:
- Around line 97-165: Extend TestDaemonSetSpecModified with table cases that
independently alter SecurityContext, liveness/readiness probes, resource
requests or limits, node-selection configuration, and tolerations on the
DaemonSet pod template, asserting each mutation returns want: true. Reuse
baseDaemonSet and keep the existing test structure and unchanged-spec
expectation intact.
🪄 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: c1a99702-a2a4-44ce-b4aa-8a30950540dd
⛔ Files ignored due to path filters (1)
api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (51)
Makefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.gobindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yamlbindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-daemonset.yamlbindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yamlbindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yamlbindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yamlbundle/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/verify-http01proxy.shpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/controller_test.gopkg/controller/http01proxy/daemonsets.gopkg/controller/http01proxy/daemonsets_test.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/infrastructure_test.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/networkpolicies.gopkg/controller/http01proxy/rbacs.gopkg/controller/http01proxy/serviceaccounts.gopkg/controller/http01proxy/utils.gopkg/controller/http01proxy/utils_test.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/assets/bindata.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.go
🚧 Files skipped from review as they are similar to previous changes (45)
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go
- bindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yaml
- config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml
- bindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yaml
- bindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yaml
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- config/crd/bases/operator.openshift.io_http01proxies.yaml
- pkg/controller/http01proxy/rbacs.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxycustomdeploymentspec.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- pkg/operator/starter.go
- pkg/controller/http01proxy/serviceaccounts.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go
- pkg/controller/http01proxy/networkpolicies.go
- api/operator/v1alpha1/http01proxy_types.go
- pkg/controller/http01proxy/install_http01proxy.go
- bindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yaml
- Makefile
- config/crd/kustomization.yaml
- pkg/controller/http01proxy/infrastructure.go
- pkg/operator/informers/externalversions/operator/v1alpha1/interface.go
- pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go
- pkg/controller/http01proxy/daemonsets_test.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go
- pkg/operator/informers/externalversions/generic.go
- pkg/controller/http01proxy/constants.go
- config/manager/manager.yaml
- pkg/operator/listers/operator/v1alpha1/http01proxy.go
- pkg/controller/http01proxy/controller_test.go
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yaml
- hack/verify-http01proxy.sh
- pkg/features/features_test.go
- api/operator/v1alpha1/features.go
- pkg/controller/http01proxy/utils.go
- config/rbac/role.yaml
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go
- bundle/manifests/operator.openshift.io_http01proxies.yaml
- pkg/operator/assets/bindata.go
- bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
- pkg/controller/http01proxy/controller.go
- pkg/controller/http01proxy/daemonsets.go
- pkg/operator/setup_manager.go
Comment on lines
+25
to
+26
| nodeSelector: | ||
| node-role.kubernetes.io/master: "" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Avoid hard-coding the legacy master topology.
This required selector prevents scheduling where control-plane nodes lack the master label and does not account for HyperShift. Use platform-aware affinity supporting valid control-plane labels, or explicitly reject unsupported topologies before creating the DaemonSet.
As per coding guidelines, flag nodeSelector targeting control-plane nodes (breaks on HyperShift) and scheduling assumptions based on standard HA topology.
🤖 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 `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml` around lines
25 - 26, Replace the hard-coded nodeSelector in the cert-manager HTTP-01 proxy
DaemonSet with platform-aware affinity that supports current control-plane
labels and HyperShift, or validate and reject unsupported topologies before
creating the DaemonSet. Preserve control-plane scheduling intent without
assuming the legacy node-role.kubernetes.io/master label or standard HA
topology.
Source: Coding guidelines
2 tasks
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
July 14, 2026 20:53
f8c9fa2 to
2b47a76
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml`:
- Around line 22-24: Update the pod spec containing serviceAccountName
cert-manager-http01-proxy and hostNetwork to explicitly set
automountServiceAccountToken to false, unless the proxy requires Kubernetes API
access at runtime; preserve the existing service account and networking
configuration.
In `@pkg/operator/assets/bindata.go`:
- Around line 2395-2440: Update the http01-proxy container securityContext
around runAsNonRoot to either set it to true if the image supports NET_ADMIN as
a non-root user, or retain false and add a brief manifest comment documenting
why root privileges are required.
- Around line 2466-2482: Update the HTTP01 proxy SCC manifests referenced by
_http01ProxyCertManagerHttp01ProxySccRolebindingYaml to bind the service account
to a dedicated custom SCC rather than system:openshift:scc:privileged. Define
that SCC with only the permissions required by the proxy DaemonSet, specifically
hostNetwork and NET_ADMIN, and update the role binding’s roleRef to the custom
SCC while preserving the existing service account and namespace.
🪄 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: 45fbf2a7-28b3-44ed-a79e-a4a2c5210c4a
⛔ Files ignored due to path filters (1)
api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (52)
Makefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.gobindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yamlbindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-daemonset.yamlbindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yamlbindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yamlbindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yamlbundle/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/verify-http01proxy.shpkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/controller_test.gopkg/controller/http01proxy/daemonsets.gopkg/controller/http01proxy/daemonsets_test.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/infrastructure_test.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/networkpolicies.gopkg/controller/http01proxy/rbacs.gopkg/controller/http01proxy/serviceaccounts.gopkg/controller/http01proxy/utils.gopkg/controller/http01proxy/utils_test.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/assets/bindata.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 (45)
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go
- config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml
- config/crd/kustomization.yaml
- pkg/operator/applyconfigurations/utils.go
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yaml
- bindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yaml
- pkg/operator/informers/externalversions/generic.go
- pkg/controller/http01proxy/serviceaccounts.go
- bindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yaml
- pkg/controller/http01proxy/rbacs.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go
- bindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yaml
- api/operator/v1alpha1/features.go
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxycustomdeploymentspec.go
- bindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yaml
- pkg/controller/http01proxy/networkpolicies.go
- config/crd/bases/operator.openshift.io_http01proxies.yaml
- pkg/controller/http01proxy/controller_test.go
- pkg/operator/applyconfigurations/internal/internal.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go
- pkg/operator/listers/operator/v1alpha1/http01proxy.go
- pkg/operator/informers/externalversions/operator/v1alpha1/interface.go
- pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go
- pkg/controller/http01proxy/constants.go
- api/operator/v1alpha1/http01proxy_types.go
- Makefile
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go
- pkg/controller/http01proxy/daemonsets.go
- config/manager/manager.yaml
- pkg/controller/http01proxy/infrastructure.go
- pkg/features/features_test.go
- bundle/manifests/cert-manager-operator.clusterserviceversion.yaml
- pkg/controller/http01proxy/utils.go
- pkg/controller/http01proxy/controller.go
- bundle/manifests/operator.openshift.io_http01proxies.yaml
- hack/verify-http01proxy.sh
- pkg/operator/setup_manager.go
- config/rbac/role.yaml
- pkg/controller/http01proxy/install_http01proxy.go
- pkg/controller/http01proxy/daemonsets_test.go
Comment on lines
+22
to
+24
| spec: | ||
| serviceAccountName: cert-manager-http01-proxy | ||
| hostNetwork: true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Missing automountServiceAccountToken: false on a privileged host-network pod.
The pod spec doesn't set automountServiceAccountToken, so it defaults to true and mounts the SA token into a hostNetwork: true + NET_ADMIN pod. Unless the proxy process itself calls the Kubernetes API at runtime (the RBAC/SCC bindings referenced elsewhere appear to be for admission, not runtime calls), this token should be disabled to reduce the blast radius if this privileged pod is compromised.
As per path instructions, "automountServiceAccountToken: false unless needed" for Kubernetes/OpenShift manifests.
🛡️ Suggested fix
spec:
serviceAccountName: cert-manager-http01-proxy
+ automountServiceAccountToken: false
hostNetwork: true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| spec: | |
| serviceAccountName: cert-manager-http01-proxy | |
| hostNetwork: true | |
| spec: | |
| serviceAccountName: cert-manager-http01-proxy | |
| automountServiceAccountToken: false | |
| hostNetwork: true |
🧰 Tools
🪛 Trivy (0.69.3)
[error] 22-75: Default security context configured
daemonset cert-manager-http01-proxy in cert-manager-operator namespace is using the default security context, which allows root privileges
Rule: KSV-0118
(IaC/Kubernetes)
🤖 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 `@bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml` around lines
22 - 24, Update the pod spec containing serviceAccountName
cert-manager-http01-proxy and hostNetwork to explicitly set
automountServiceAccountToken to false, unless the proxy requires Kubernetes API
access at runtime; preserve the existing service account and networking
configuration.
Comment on lines
+2395
to
+2440
| spec: | ||
| serviceAccountName: cert-manager-http01-proxy | ||
| hostNetwork: true | ||
| nodeSelector: | ||
| node-role.kubernetes.io/master: "" | ||
| tolerations: | ||
| - key: node-role.kubernetes.io/master | ||
| operator: Exists | ||
| effect: NoSchedule | ||
| - key: node-role.kubernetes.io/control-plane | ||
| operator: Exists | ||
| effect: NoSchedule | ||
| containers: | ||
| - name: http01-proxy | ||
| image: ${RELATED_IMAGE_CERT_MANAGER_HTTP01PROXY} | ||
| ports: | ||
| - name: proxy | ||
| containerPort: 8888 | ||
| hostPort: 8888 | ||
| protocol: TCP | ||
| env: | ||
| - name: PROXY_PORT | ||
| value: "8888" | ||
| livenessProbe: | ||
| tcpSocket: | ||
| port: proxy | ||
| initialDelaySeconds: 10 | ||
| periodSeconds: 30 | ||
| timeoutSeconds: 5 | ||
| failureThreshold: 3 | ||
| readinessProbe: | ||
| tcpSocket: | ||
| port: proxy | ||
| initialDelaySeconds: 5 | ||
| periodSeconds: 10 | ||
| timeoutSeconds: 5 | ||
| failureThreshold: 3 | ||
| securityContext: | ||
| allowPrivilegeEscalation: false | ||
| readOnlyRootFilesystem: true | ||
| capabilities: | ||
| add: | ||
| - NET_ADMIN | ||
| drop: | ||
| - ALL | ||
| runAsNonRoot: false |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== file size =='
wc -l pkg/operator/assets/bindata.go
echo '== surrounding lines =='
sed -n '2365,2465p' pkg/operator/assets/bindata.go
echo '== search related identifiers =='
rg -n --hidden --no-messages -S 'cert-manager-http01-proxy|runAsNonRoot: false|NET_ADMIN|hostNetwork: true|node-role.kubernetes.io/master' .Repository: openshift/cert-manager-operator
Length of output: 20931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '== exact matches for proxy asset =='
rg -n --hidden --no-messages -S 'cert-manager-http01-proxy' . || true
echo '== runAsNonRoot mentions with nearby context =='
rg -n --hidden --no-messages -S 'runAsNonRoot: false|runAsNonRoot: true|runAsNonRoot' . | head -n 200
echo '== bindata asset source hints =='
rg -n --hidden --no-messages -S 'bindata.go|cert-manager-http01-proxy|http01-proxy' pkg .github manifests config deploy || trueRepository: openshift/cert-manager-operator
Length of output: 26457
Document why this proxy must run as root. runAsNonRoot: false should be justified here; if the image works with NET_ADMIN as a non-root user, switch it to true, otherwise add a brief explanation in the manifest.
🤖 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/operator/assets/bindata.go` around lines 2395 - 2440, Update the
http01-proxy container securityContext around runAsNonRoot to either set it to
true if the image supports NET_ADMIN as a non-root user, or retain false and add
a brief manifest comment documenting why root privileges are required.
Source: Coding guidelines
Comment on lines
+2466
to
+2482
| var _http01ProxyCertManagerHttp01ProxySccRolebindingYaml = []byte(`apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRoleBinding | ||
| metadata: | ||
| name: cert-manager-http01-proxy-scc | ||
| labels: | ||
| app: cert-manager-http01-proxy | ||
| app.kubernetes.io/name: cert-manager-http01-proxy | ||
| app.kubernetes.io/part-of: cert-manager-operator | ||
| roleRef: | ||
| apiGroup: rbac.authorization.k8s.io | ||
| kind: ClusterRole | ||
| name: system:openshift:scc:privileged | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: cert-manager-http01-proxy | ||
| namespace: cert-manager-operator | ||
| `) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo root: '
pwd
echo '--- locate relevant files ---'
git ls-files | rg 'cert-manager-http01-proxy|scc|bindata\.go|daemonset|deployment|proxy' || true
echo '--- search for asset name and service account ---'
rg -n --hidden --glob '!**/vendor/**' 'cert-manager-http01-proxy|system:openshift:scc:privileged|hostNetwork|NET_ADMIN|hostnetwork-v2|hostnetwork' .
echo '--- inspect candidate source files if found ---'
for f in $(git ls-files | rg 'cert-manager-http01-proxy|scc|bindata\.go|daemonset|deployment'); do
echo "### $f"
wc -l "$f"
doneRepository: openshift/cert-manager-operator
Length of output: 50387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --glob '!**/vendor/**' 'cert-manager-http01-proxy|system:openshift:scc:privileged|hostNetwork|NET_ADMIN|hostnetwork-v2|hostnetwork' .Repository: openshift/cert-manager-operator
Length of output: 12117
🌐 Web query:
OpenShift hostnetwork-v2 SCC allowedCapabilities NET_ADMIN privileged SCC hostNetwork
💡 Result:
The hostnetwork-v2 Security Context Constraint (SCC) in OpenShift is designed to allow pods to use host networking while enforcing stricter security defaults than the standard hostnetwork SCC [1][2]. By default, the hostnetwork-v2 SCC has the following characteristics: - It drops ALL capabilities from containers [1][2]. - Its allowedCapabilities list is limited to ["NET_BIND_SERVICE"] [1][2]. - It requires allowPrivilegeEscalation to be unset or set to false [1][2]. - It uses runtime/default for its seccompProfile [1][2]. Because NET_ADMIN is not included in the default allowedCapabilities list for hostnetwork-v2, you cannot use it with a pod running under this SCC without modification [1][2]. To enable NET_ADMIN for a pod, you have two primary options: 1. Create or Modify a Custom SCC: You can create a copy of the hostnetwork-v2 SCC and add NET_ADMIN to the allowedCapabilities field, or modify an existing custom SCC that your service account uses [3][2]. Ensure that you grant the appropriate service account access to this custom SCC [4][5]. 2. Use the Privileged SCC: The privileged SCC is the most permissive SCC and allows access to all host features, including arbitrary capabilities such as NET_ADMIN, and permits setting privileged: true in the pod specification [4][6]. However, it is highly recommended to use a custom SCC with the minimal required privileges rather than granting full privileged access, especially if you only need specific capabilities [4]. Note that setting privileged: true in a pod specification does not automatically assign the privileged SCC; the system selects the SCC with the highest priority that the pod's service account is authorized to use [4][5]. If your workload requires host networking and specific capabilities, the most secure approach is to define a custom SCC that includes allowHostNetwork: true and adds only the necessary capabilities (e.g., NET_ADMIN) to its allowedCapabilities list [3][2][4].
Citations:
- 1: https://docs.redhat.com/en/documentation/openshift_container_platform/4.21/html/authentication_and_authorization/managing-pod-security-policies
- 2: https://docs.redhat.com/en/documentation/openshift_container_platform/4.18/html/authentication_and_authorization/managing-pod-security-policies
- 3: https://docs.redhat.com/en/documentation/openshift_container_platform/4.14/html/security_apis/securitycontextconstraints-security-openshift-io-v1
- 4: https://docs.okd.io/4.14/authentication/managing-security-context-constraints.html
- 5: https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html/authentication_and_authorization/managing-pod-security-policies
- 6: https://docs.openshift.com/container-platform/4.11/authentication/managing-security-context-constraints.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- daemonset source ---'
sed -n '1,140p' bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml
echo '--- http01proxy docs mention ---'
sed -n '1,220p' docs/proxy.md
echo '--- controller helper around http01proxy security context ---'
sed -n '100,170p' pkg/controller/http01proxy/utils.goRepository: openshift/cert-manager-operator
Length of output: 7520
Scope the HTTP01 proxy SCC binding
bindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yaml grants the proxy SA system:openshift:scc:privileged, but the DaemonSet only requests hostNetwork and NET_ADMIN. Replace this with a custom SCC scoped to those requirements instead of granting full privileged access.
🤖 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/operator/assets/bindata.go` around lines 2466 - 2482, Update the HTTP01
proxy SCC manifests referenced by
_http01ProxyCertManagerHttp01ProxySccRolebindingYaml to bind the service account
to a dedicated custom SCC rather than system:openshift:scc:privileged. Define
that SCC with only the permissions required by the proxy DaemonSet, specifically
hostNetwork and NET_ADMIN, and update the role binding’s roleRef to the custom
SCC while preserving the existing service account and namespace.
Source: Path instructions
4 tasks
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
July 15, 2026 18:18
2b47a76 to
c103a89
Compare
2 tasks
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
pkg/controller/http01proxy/infrastructure_test.go (1)
97-104: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert both discovered VIP lists and their values.
The successful BareMetal case passes even if
ingressIPsextraction is broken because onlyapiVIPslength is checked. Compare the complete expected API and ingress VIP slices.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, Update the infrastructure tests’ expected results and assertions for the successful BareMetal cases to compare complete API VIP and ingress VIP slices, including their values, rather than checking only apiVIPs length. Apply the same validation to the cases around the additional referenced test sections, preserving existing error-case assertions.
🤖 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 `@bindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yaml`:
- Around line 20-37: Remove create, update, and patch permissions for
machineconfigs and machineconfigurations from the proxy ClusterRole, retaining
only the discovery permissions required by the HTTP proxy. Move the write
authority to a separate controller identity and ensure the corresponding
ClusterRoleBinding does not attach it to the proxy DaemonSet service account.
In `@cmd/http01-proxy/main.go`:
- Line 57: Update the request logging near the HTTP proxy handler so it no
longer logs r.URL.Path, which may contain the ACME challenge token. Replace it
with a fixed forwarding message or redact the final path component while
retaining useful request context.
In `@cmd/http01-proxy/templates/templates.go`:
- Around line 9-12: Update the NFTRuleTemplate constant to remove the standalone
table inet crtmgr_proxy_table preamble, retaining only the valid table block and
its rules. Do not alter the redirect configuration; if table reset behavior is
required, implement it through a supported nft command rather than embedding the
invalid declaration.
In `@pkg/controller/http01proxy/utils_test.go`:
- Around line 137-145: Update the reconciliation paths covered by the “different
selector” case so DaemonSet selector drift is handled by deleting and recreating
the resource instead of calling Update. Apply the same delete/recreate behavior
to ClusterRoleBinding roleRef drift, while preserving normal Update handling for
mutable changes.
---
Nitpick comments:
In `@pkg/controller/http01proxy/infrastructure_test.go`:
- Around line 97-104: Update the infrastructure tests’ expected results and
assertions for the successful BareMetal cases to compare complete API VIP and
ingress VIP slices, including their values, rather than checking only apiVIPs
length. Apply the same validation to the cases around the additional referenced
test sections, preserving existing error-case assertions.
🪄 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: e3101ba0-18e3-4a93-9f0e-c1f26ab62dc5
⛔ Files ignored due to path filters (1)
api/operator/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (57)
.gitignoreMakefileapi/operator/v1alpha1/features.goapi/operator/v1alpha1/http01proxy_types.gobindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yamlbindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-daemonset.yamlbindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yamlbindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yamlbindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yamlbindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yamlbundle/manifests/cert-manager-operator.clusterserviceversion.yamlbundle/manifests/operator.openshift.io_http01proxies.yamlcmd/http01-proxy/main.gocmd/http01-proxy/pkg/utils.gocmd/http01-proxy/templates/templates.goconfig/crd/bases/operator.openshift.io_http01proxies.yamlconfig/crd/kustomization.yamlconfig/manager/manager.yamlconfig/rbac/role.yamlconfig/samples/operator.openshift.io_v1alpha1_http01proxy.yamlhack/verify-http01proxy.shimages/ci/http01proxy.Dockerfilepkg/controller/http01proxy/constants.gopkg/controller/http01proxy/controller.gopkg/controller/http01proxy/controller_test.gopkg/controller/http01proxy/daemonsets.gopkg/controller/http01proxy/daemonsets_test.gopkg/controller/http01proxy/infrastructure.gopkg/controller/http01proxy/infrastructure_test.gopkg/controller/http01proxy/install_http01proxy.gopkg/controller/http01proxy/networkpolicies.gopkg/controller/http01proxy/rbacs.gopkg/controller/http01proxy/serviceaccounts.gopkg/controller/http01proxy/utils.gopkg/controller/http01proxy/utils_test.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/assets/bindata.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 (43)
- bindata/networkpolicies/http01-proxy-deny-all-networkpolicy.yaml
- config/crd/kustomization.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go
- bindata/networkpolicies/http01-proxy-allow-egress-networkpolicy.yaml
- config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml
- bindata/http01-proxy/cert-manager-http01-proxy-scc-rolebinding.yaml
- pkg/operator/informers/externalversions/generic.go
- pkg/operator/applyconfigurations/utils.go
- pkg/controller/http01proxy/constants.go
- pkg/controller/http01proxy/rbacs.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go
- api/operator/v1alpha1/features.go
- pkg/operator/applyconfigurations/internal/internal.go
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- config/crd/bases/operator.openshift.io_http01proxies.yaml
- pkg/controller/http01proxy/serviceaccounts.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go
- bindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yaml
- pkg/controller/http01proxy/networkpolicies.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go
- pkg/operator/informers/externalversions/operator/v1alpha1/interface.go
- pkg/operator/starter.go
- pkg/operator/listers/operator/v1alpha1/http01proxy.go
- bindata/http01-proxy/cert-manager-http01-proxy-serviceaccount.yaml
- config/manager/manager.yaml
- pkg/controller/http01proxy/infrastructure.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- pkg/controller/http01proxy/utils.go
- bundle/manifests/operator.openshift.io_http01proxies.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go
- hack/verify-http01proxy.sh
- pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go
- config/rbac/role.yaml
- pkg/controller/http01proxy/install_http01proxy.go
- pkg/controller/http01proxy/daemonsets.go
- api/operator/v1alpha1/http01proxy_types.go
- test/e2e/http01proxy_test.go
- pkg/controller/http01proxy/controller_test.go
- pkg/controller/http01proxy/controller.go
- pkg/operator/setup_manager.go
- pkg/controller/http01proxy/daemonsets_test.go
- pkg/operator/assets/bindata.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go
Comment on lines
+20
to
+37
| - apiGroups: | ||
| - machineconfiguration.openshift.io | ||
| resources: | ||
| - machineconfigs | ||
| verbs: | ||
| - get | ||
| - create | ||
| - update | ||
| - patch | ||
| - apiGroups: | ||
| - operator.openshift.io | ||
| resources: | ||
| - machineconfigurations | ||
| verbs: | ||
| - get | ||
| - create | ||
| - update | ||
| - patch |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Keep MCO write authority out of the proxy DaemonSet identity.
If the paired binding attaches this role to every HTTP proxy pod, a compromise of the port-80 process can create or patch MachineConfigs that modify control-plane files and systemd units. Move these applies to a dedicated controller identity and leave the proxy with only discovery permissions. MachineConfig changes can update node OS configuration and trigger node reboots. (docs.redhat.com)
As per path instructions, Kubernetes RBAC must use least privilege.
#!/bin/bash
set -euo pipefail
sed -n '1,220p' bindata/http01-proxy/cert-manager-http01-proxy-clusterrolebinding.yaml
sed -n '1,260p' bindata/http01-proxy/cert-manager-http01-proxy-daemonset.yaml🤖 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 `@bindata/http01-proxy/cert-manager-http01-proxy-clusterrole.yaml` around lines
20 - 37, Remove create, update, and patch permissions for machineconfigs and
machineconfigurations from the proxy ClusterRole, retaining only the discovery
permissions required by the HTTP proxy. Move the write authority to a separate
controller identity and ensure the corresponding ClusterRoleBinding does not
attach it to the proxy DaemonSet service account.
Source: Path instructions
|
|
||
| http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { | ||
| if strings.HasPrefix(r.URL.Path, "/.well-known/acme-challenge/") { | ||
| log.Printf("Forwarding request to APPS VIP: %s", r.URL.Path) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Do not log the ACME challenge token.
r.URL.Path includes the token. Log a fixed message or redact the final path component.
Proposed fix
- log.Printf("Forwarding request to APPS VIP: %s", r.URL.Path)
+ log.Printf("Forwarding ACME HTTP-01 challenge request to APPS VIP")As per coding guidelines, flag logging that may expose tokens.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| log.Printf("Forwarding request to APPS VIP: %s", r.URL.Path) | |
| log.Printf("Forwarding ACME HTTP-01 challenge request to APPS VIP") |
🤖 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` at line 57, Update the request logging near the
HTTP proxy handler so it no longer logs r.URL.Path, which may contain the ACME
challenge token. Replace it with a fixed forwarding message or redact the final
path component while retaining useful request context.
Source: Coding guidelines
Comment on lines
+9
to
+12
| const NFTRuleTemplate = ` | ||
| table inet crtmgr_proxy_table | ||
| delete table inet crtmgr_proxy_table | ||
| table inet crtmgr_proxy_table { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
command -v nft >/dev/null
cat > /tmp/http01-proxy.nft <<'EOF'
destroy table inet crtmgr_proxy_table
table inet crtmgr_proxy_table {
chain crtmgr_proxy_PREROUTING {
type nat hook prerouting priority 0;
ip daddr 192.0.2.10 tcp dport 80 redirect to 8888
}
}
EOF
nft --check --file /tmp/http01-proxy.nftRepository: openshift/cert-manager-operator
Length of output: 169
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'cmd/http01-proxy/templates/templates.go' 'cmd/http01-proxy/**/*' | sed -n '1,120p'
echo '--- templates.go ---'
cat -n cmd/http01-proxy/templates/templates.go
echo '--- usages ---'
rg -n "NFTRuleTemplate|crtmgr_proxy_table|destroy table|delete table|table inet" cmd/http01-proxy -SRepository: openshift/cert-manager-operator
Length of output: 3440
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- cmd/http01-proxy/pkg/utils.go (relevant section) ---'
sed -n '120,190p' cmd/http01-proxy/pkg/utils.go
echo '--- nftables syntax docs references in repo ---'
rg -n "ExecStop=/sbin/nft|delete table inet|add table inet|nft -f|crtmgr_proxy_table" -S .Repository: openshift/cert-manager-operator
Length of output: 3272
🌐 Web query:
nftables manpage destroy table syntax delete table create table nft -f bare table declaration
💡 Result:
In nftables, tables are containers for chains, rules, and other objects. Their management involves specific commands for creation and removal. Table Creation To create a table, use the following syntax [1][2]: nft {add | create} table [family] table_name The family parameter is optional and defaults to ip if not specified; valid families include ip, ip6, inet, arp, bridge, and netdev [3]. The difference between add and create is that add will not return an error if the table already exists, whereas create will return an error if it does [1][2]. Table Removal There are two primary commands for removing a table: 1. delete table [family] table_name: Deletes the specified table [4][1]. If the table does not exist, this command will return an error. 2. destroy table [family] table_name: Deletes the specified table and all objects associated with it, such as chains, rules, sets, maps, and stateful objects [5]. Crucially, destroy table will succeed even if the table does not exist (it ignores ENOENT errors) [6][7][8]. When using these commands, the family must be specified if the table was created with one, otherwise the default family is assumed [3].
Citations:
- 1: https://manpages.debian.org/buster/nftables/nftables.8.en.html
- 2: https://www.netfilter.org/projects/nftables/manpage.html
- 3: https://wiki.nftables.org/wiki-nftables/index.php/Quick%5Freference-nftables%5Fin%5F10%5Fminutes
- 4: https://manpages.debian.org/testing/nftables/nft.8.en.html
- 5: https://documentation.ubuntu.com/security/security-features/network/firewall/nftables/
- 6: https://wiki.archlinux.org/title/Nftables
- 7: https://docs.kernel.org/netlink/specs/nftables.html
- 8: https://www.kernel.org/doc/html/v7.1-rc7/netlink/specs/nftables.html
🌐 Web query:
site:netfilter.org nft destroy table delete table absent manpage
💡 Result:
In the nftables command-line tool, the correct command to remove a table is "nft delete table [family] [name]" [1]. The term "destroy" is not a valid command keyword for tables in the nftables CLI [1][2]. While some other network utilities use "destroy" for deletion, nftables exclusively uses "delete" for removing tables, chains, and rules [1][3][4]. If you are looking to remove a table, the standard syntax is: nft delete table [family] [table_name] Important considerations: 1. Deletion requirements: A table must typically be empty (contain no chains or other objects) to be deleted using the standard "delete table" command. If a table contains objects, you may first need to "flush" the table to remove its contents, or delete the contained objects individually [3][5]. 2. "Flush" command: Using "nft flush table [family] [table_name]" will remove all rules, chains, and sets within the table, effectively emptying it without removing the table itself [3]. 3. Man page documentation: The official nftables manual explicitly lists "delete" as the command for removing tables [6][7]. There is no "destroy" command for tables in the nftables documentation or toolset [2].
Citations:
- 1: http://wiki.netfilter.org/wiki-nftables/index.php/Quick_reference-nftables_in_10_minutes
- 2: https://people.netfilter.org/pablo/netdev0.1/slides/nftables-netdev-2015.pdf
- 3: http://wiki.netfilter.org/wiki-nftables/index.php/Simple_rule_management
- 4: https://git.netfilter.org/nftables/commit?id=0eb9a25
- 5: http://wiki.netfilter.org/wiki-nftables/index.php/Atomic_rule_replacement
- 6: https://git.netfilter.org/nftables/tree/doc/nftables.xml?id=b65f854278c7412b9dd2f6b335ad1f7e32d83d34
- 7: https://www.netfilter.org/projects/nftables/manpage.html
Remove the invalid nft preamble
cmd/http01-proxy/templates/templates.go:9-12 starts the generated ruleset with a standalone table inet crtmgr_proxy_table, which isn’t valid nft syntax and can stop nft -f before the redirect is loaded. Keep only the table ... {} block, or move any reset logic to a supported nft command.
🤖 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/templates/templates.go` around lines 9 - 12, Update the
NFTRuleTemplate constant to remove the standalone table inet crtmgr_proxy_table
preamble, retaining only the valid table block and its rules. Do not alter the
redirect configuration; if table reset behavior is required, implement it
through a supported nft command rather than embedding the invalid declaration.
Comment on lines
+137
to
+145
| name: "different selector", | ||
| desired: baseDaemonSet("img:v1"), | ||
| fetched: func() *appsv1.DaemonSet { | ||
| ds := baseDaemonSet("img:v1") | ||
| ds.Spec.Selector = &metav1.LabelSelector{MatchLabels: map[string]string{"app": "other"}} | ||
| return ds | ||
| }(), | ||
| want: true, | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline pkg/controller/http01proxy/daemonsets.go --items all --view expanded
ast-grep outline pkg/controller/http01proxy/rbacs.go --items all --view expanded
rg -n -C4 \
'hasObjectChanged|daemonSetSpecModified|Update\s*\(|Delete\s*\(|Create\s*\(' \
pkg/controller/http01proxyRepository: openshift/cert-manager-operator
Length of output: 9241
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- daemonsets.go ---'
cat -n pkg/controller/http01proxy/daemonsets.go | sed -n '1,220p'
echo
echo '--- rbacs.go ---'
cat -n pkg/controller/http01proxy/rbacs.go | sed -n '1,220p'
echo
echo '--- utils_test.go (relevant ranges) ---'
cat -n pkg/controller/http01proxy/utils_test.go | sed -n '120,270p'Repository: openshift/cert-manager-operator
Length of output: 12369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path, ranges in [
("pkg/controller/http01proxy/daemonsets.go", [(1,220)]),
("pkg/controller/http01proxy/rbacs.go", [(1,220)]),
("pkg/controller/http01proxy/utils_test.go", [(120,270)]),
]:
print(f"\n=== {path} ===")
lines = Path(path).read_text().splitlines()
for start, end in ranges:
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: openshift/cert-manager-operator
Length of output: 12134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path, start, end in [
("pkg/controller/http01proxy/daemonsets.go", 1, 220),
("pkg/controller/http01proxy/rbacs.go", 1, 220),
("pkg/controller/http01proxy/utils_test.go", 120, 270),
]:
print(f"\n=== {path} ===")
lines = Path(path).read_text().splitlines()
for i in range(start-1, min(end, len(lines))):
print(f"{i+1:4d}: {lines[i]}")
PYRepository: openshift/cert-manager-operator
Length of output: 12134
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'createOrApplyDaemonSet|createOrApplyRBACResources|deleteDaemonSet|deleteRBACResources|createOrUpdateResource|daemonSetSpecModified|ClusterRoleBinding' pkg/controller/http01proxyRepository: openshift/cert-manager-operator
Length of output: 20144
Route immutable drift through delete/recreate
DaemonSet.spec.selector and ClusterRoleBinding.roleRef are immutable, but these changed=true paths still flow into Update. That will fail on existing resources and leave reconciliation stuck; handle them by recreating the object instead.
🤖 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/utils_test.go` around lines 137 - 145, Update the
reconciliation paths covered by the “different selector” case so DaemonSet
selector drift is handled by deleting and recreating the resource instead of
calling Update. Apply the same delete/recreate behavior to ClusterRoleBinding
roleRef drift, while preserving normal Update handling for mutable changes.
Adds the HTTP01 Challenge Proxy feature for baremetal clusters where the API VIP differs from the ingress VIP. The proxy runs as a DaemonSet on control-plane nodes and redirects HTTP-01 ACME challenge traffic from the API VIP to the ingress router. Components: - HTTP01Proxy CRD (operator.openshift.io/v1alpha1) - Operator controller that manages DaemonSet, RBAC, NetworkPolicies - Proxy binary (cmd/http01-proxy/) with CI Dockerfile - Platform validation (BareMetal-only with distinct VIPs) - NFTables MachineConfig for traffic redirection The feature is gated behind an Alpha feature gate (HTTP01Proxy=true) activated via UNSUPPORTED_ADDON_FEATURES env var.
sebrandon1
force-pushed
the
cm-716-http01-proxy
branch
from
July 15, 2026 19:32
c103a89 to
6946710
Compare
Contributor
|
@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. |
6 tasks
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
3 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
Adds the HTTP01 Challenge Proxy controller to cert-manager-operator, enabling cert-manager to complete HTTP01 ACME challenges for the API endpoint (
api.cluster.example.com) on baremetal platforms where the API VIP is not exposed via OpenShift Ingress.This follows the enhancement proposal in openshift/enhancements#1929 and the existing IstioCSR/TrustManager controller pattern.
Related PRs
How It Works
When a user creates an
HTTP01ProxyCR withmode: DefaultDeployment, the operator:API_VIP:80to a local proxy port (default 8888)/.well-known/acme-challenge/*requests are forwarded to the Ingress VIP — all other requests are rejectedOn unsupported platforms (non-BareMetal, missing VIPs, or equal API/Ingress VIPs), the controller sets
Degraded=Truewith a descriptive message and does not deploy any workloads.New CRD
Feature Gate
HTTP01Proxy— Alpha, disabled by default. Enable via--unsupported-addon-features=HTTP01Proxy=true.Deployed Resources
When the HTTP01Proxy CR is created, the controller deploys:
cert-manager-http01-proxy) on control plane nodes withhostNetwork: trueandNET_ADMINcapabilitycert-manager-http01-proxy)All resources are cleaned up via a finalizer when the CR is deleted.
Proxy Image
The proxy binary source code and CI Dockerfile live in this repo under
cmd/http01-proxy/andimages/ci/http01proxy.Dockerfile(added in #458). The image is built in OpenShift CI with FIPS-compliant build tags and promoted via the ci-operator pipeline.The proxy image references in
config/manager/manager.yamland the CSV usequay.io/openshift/cert-manager-http01-proxy:v0.1.0as the default, which is overridden by ci-operator substitutions in CI.Testing & Deployment Guide
A how-to guide with pre-built images and step-by-step deployment instructions is available for reviewers:
Deployment & Testing Guide
The guide covers:
hack/verify-http01proxy.sh)Pre-built Images
quay.io/bapalm/cert-manager-operatorhttp01proxyquay.io/bapalm/cert-mgr-http01-proxyv0.2.0/latestVerified On
Degradedwith "platform type None is not supported" and creates no workloads654 tests in 28 packages)make verifypassesTracking