PoC/WIP: Add cert-manager-proxy scaffolding #313
Conversation
|
Warning Rate limit exceeded@sebrandon1 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (22)
WalkthroughAdds a new namespaced v1alpha1 HTTP01Proxy CRD, its API types and deepcopy helpers, RBAC entries, sample manifest, controller scaffold, clientset (typed and fake), informers, listers, apply-configuration support, and integrates the controller into the operator manager; includes scheme registration, status subresource, and singleton name validation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (11)
config/rbac/role.yaml (2)
215-223: Scope verbs for http01proxies to least privilege (read‑only for the main resource).For the PoC scaffold, consider removing write verbs for the primary resource and keeping writes only on status/finalizers. This avoids unnecessary privileges until reconcile logic needs them.
Apply within this rule:
- - http01proxies + # http01proxies moved to a dedicated read-only rule belowAdd a dedicated rule (outside this block):
- apiGroups: - operator.openshift.io resources: - http01proxies verbs: ["get", "list", "watch"]
228-231: Finalizers permission: keep only if you’ll actually set finalizers.If the controller doesn’t add finalizers yet, you can temporarily drop this to reduce RBAC surface.
Would you confirm whether the HTTP01Proxy controller currently adds a finalizer?
config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml (1)
1-11: Add explicit namespace in the sample for clarity.Resource is namespaced; making it explicit reduces surprises during quick tests.
metadata: name: default + # namespace: default # or your target namespacepkg/operator/setup_manager.go (1)
75-82: Add a startup log for the new controller (and optionally gate behind a flag).A brief log improves observability during bring‑up. Optional: gate wiring behind an env/flag while the feature is PoC.
Apply this diff:
// http01proxy controller + setupLog.Info("wiring controller", "controller", http01proxy.ControllerName) rh, err := http01proxy.New(mgr)pkg/controller/http01proxy/controller.go (2)
19-22: Wire an EventRecorder and make recordEventf actually emit events.Preps future reconcile logic and avoids dead code. Also prefer client.Object in signatures.
import ( "context" - "fmt" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" ) type Reconciler struct { client.Client Scheme *runtime.Scheme + Recorder record.EventRecorder } func New(mgr ctrl.Manager) (*Reconciler, error) { - return &Reconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme()}, nil + return &Reconciler{ + Client: mgr.GetClient(), + Scheme: mgr.GetScheme(), + Recorder: mgr.GetEventRecorderFor(ControllerName), + }, nil } // helper to record events (future use) -func (r *Reconciler) recordEventf(obj runtime.Object, eventtype, reason, messageFmt string, args ...interface{}) { - _ = fmt.Sprintf(messageFmt, args...) - // intentionally no-op for now; will wire an EventRecorder when we add logic - _ = corev1.EventTypeNormal +func (r *Reconciler) recordEventf(obj client.Object, eventtype, reason, messageFmt string, args ...interface{}) { + if r.Recorder == nil || obj == nil { + return + } + // eventtype should be corev1.EventTypeNormal or corev1.EventTypeWarning + r.Recorder.Eventf(obj, eventtype, reason, messageFmt, args...) }Also applies to: 24-26, 48-53, 3-13
42-46: Filter to the singleton name to reduce reconcile noise.The CR is singleton (name must be "default"). Filter events accordingly.
func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error { - return ctrl.NewControllerManagedBy(mgr). - For(&operatorv1alpha1.HTTP01Proxy{}). - Complete(r) + return ctrl.NewControllerManagedBy(mgr). + For(&operatorv1alpha1.HTTP01Proxy{}). + WithEventFilter(ctrl.PredicateFuncs{ + CreateFunc: func(e ctrl.Event) bool { o, ok := e.Object.(client.Object); return ok && o.GetName() == "default" }, + UpdateFunc: func(e ctrl.UpdateEvent) bool { return e.ObjectNew.GetName() == "default" }, + DeleteFunc: func(e ctrl.DeleteEvent) bool { return e.Object.GetName() == "default" }, + GenericFunc: func(e ctrl.GenericEvent) bool { return e.Object.GetName() == "default" }, + }). + Complete(r) }Alternatively, use predicate.NewPredicateFuncs with a small helper.
api/operator/v1alpha1/http01proxy_types.go (2)
47-54: Spec is marked Required but tagged omitempty; drop omitempty for clarity.Keeps API consistent with k8s conventions for required, non-pointer Spec fields.
- Spec HTTP01ProxySpec `json:"spec,omitempty"` + Spec HTTP01ProxySpec `json:"spec"`
19-23: ListMeta usually uses ,omitempty; minor consistency tweak.- metav1.ListMeta `json:"metadata"` + metav1.ListMeta `json:"metadata,omitempty"`config/crd/bases/operator.openshift.io_http01proxies.yaml (3)
117-119: Fix copy/paste: “istio-csr” references in descriptions.These descriptions should reference HTTP01Proxy, not istio-csr.
Apply this diff:
- description: labels to apply to all resources created for istio-csr - agent deployment. + description: labels to apply to all resources created by the HTTP01Proxy + controller. @@ - description: conditions holds information of the current state of - the istio-csr agent deployment. + description: conditions holds information about the current state of + HTTP01Proxy reconciliation.Also applies to: 136-138
17-21: Add a printer column for .spec.enabled.Helps quickly see whether the proxy is on for a namespace.
- additionalPrinterColumns: + - jsonPath: .spec.enabled + name: Enabled + type: boolean - jsonPath: .metadata.creationTimestamp name: Age type: date
24-31: Tighten wording on singleton + scope (optional).Minor clarity: emphasize singleton-per-namespace behavior in the description.
- HTTP01Proxy describes configuration for a cluster-managed HTTP-01 challenge proxy. - The name must be `default` to make it a singleton per namespace. + HTTP01Proxy configures a namespaced, operator-managed HTTP-01 challenge proxy. + Only one instance is allowed per namespace; the resource name must be `default`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (22)
api/operator/v1alpha1/http01proxy_types.go(1 hunks)api/operator/v1alpha1/zz_generated.deepcopy.go(2 hunks)config/crd/bases/operator.openshift.io_http01proxies.yaml(1 hunks)config/rbac/role.yaml(2 hunks)config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml(1 hunks)pkg/controller/http01proxy/controller.go(1 hunks)pkg/operator/applyconfigurations/internal/internal.go(1 hunks)pkg/operator/applyconfigurations/operator/v1alpha1/http01proxy.go(1 hunks)pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go(1 hunks)pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go(1 hunks)pkg/operator/applyconfigurations/utils.go(1 hunks)pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go(1 hunks)pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go(1 hunks)pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go(1 hunks)pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go(1 hunks)pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go(2 hunks)pkg/operator/informers/externalversions/generic.go(1 hunks)pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go(1 hunks)pkg/operator/informers/externalversions/operator/v1alpha1/interface.go(2 hunks)pkg/operator/listers/operator/v1alpha1/expansion_generated.go(1 hunks)pkg/operator/listers/operator/v1alpha1/http01proxy.go(1 hunks)pkg/operator/setup_manager.go(2 hunks)
🔇 Additional comments (22)
pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go (1)
7-8: Add HTTP01Proxy expansion point — LGTMThe empty expansion interface is consistent with client-gen patterns and enables future custom methods.
pkg/operator/informers/externalversions/generic.go (1)
42-43: Informer wiring for http01proxies — LGTMCorrect GVR and informer hookup via HTTP01Proxies().Informer().
pkg/operator/applyconfigurations/internal/internal.go (1)
36-45: Applyconfig generation for HTTP01Proxy verified — LGTMHTTP01Proxy applyconfig types present (pkg/operator/applyconfigurations/operator/v1alpha1/http01proxy*.go), ForKind wiring in pkg/operator/applyconfigurations/utils.go, and parser entry in pkg/operator/applyconfigurations/internal/internal.go exist.
config/rbac/role.yaml (1)
236-241: Status RBAC OK — controller doesn't appear to update HTTP01Proxy status; either update status via client.Status().Update/Patch or remove the status verbs.No client.Status().Update or .Status().Patch found for HTTP01Proxy (pkg/controller/http01proxy/controller.go); IstioCSR uses Status().Update (pkg/controller/istiocsr/client.go); generated clientset exposes UpdateStatus/ApplyStatus (pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go).
pkg/operator/setup_manager.go (1)
24-24: Import looks correct; scheme already includes v1alpha1.pkg/operator/informers/externalversions/operator/v1alpha1/interface.go (2)
13-15: New informer accessor — OK.API surface consistent with other informers.
35-38: Backing informer type exists — no action required.
pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go declares HTTP01ProxyInformer (line 21) and hTTP01ProxyInformer (line 26), so the HTTP01Proxies() factory call is consistent.pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go (1)
1-37: Fake HTTP01Proxy client scaffolding — LGTM.Resource/kind, list/apply helpers, and pointer conversions look correct.
pkg/operator/listers/operator/v1alpha1/expansion_generated.go (1)
9-16: Lister expansion hooks added — OK.Standard empty interfaces for future extensions.
pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go (2)
16-16: Interface extended with HTTP01ProxiesGetter — LGTM.
29-31: HTTP01Proxies accessor added — LGTM.Namespace-scoped like IstioCSRs.
api/operator/v1alpha1/zz_generated.deepcopy.go (1)
26-26: Import style change is harmless.Aligned with other generated files.
pkg/operator/applyconfigurations/utils.go (1)
37-42: Approve — HTTP01Proxy ForKind mappings present (LGTM).
Verified ApplyConfiguration types exist for object, spec, and status in pkg/operator/applyconfigurations/operator/v1alpha1.pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go (1)
19-21: Approve — fake client parity for HTTP01Proxy confirmed. FakeOperatorV1alpha1.HTTP01Proxies(namespace string) is implemented in pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go and delegates to newFakeHTTP01Proxies; see pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go for the fake implementation — matches typed client shape and namespace scoping.pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go (1)
1-75: Generated informer looks consistent with clients/listers; LGTM.pkg/operator/listers/operator/v1alpha1/http01proxy.go (1)
1-55: Generated lister wiring is correct; LGTM.pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go (1)
12-16: Apply configuration matches Spec; LGTM.pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go (1)
26-34: Nil-guard panic in WithConditions is expected — no change required.
Matches upstream applyconfiguration-gen; ConditionalStatusApplyConfiguration confirmed at pkg/operator/applyconfigurations/operator/v1alpha1/conditionalstatus.go. Ensure callers never pass nil.pkg/controller/http01proxy/controller.go (1)
28-33: Narrow RBAC for controller-owned CR (least privilege).Controllers must not modify their own Spec — restrict http01proxies to get,list,watch; keep status/finalizers update; add events RBAC only if emitting events.
-//+kubebuilder:rbac:groups=operator.openshift.io,resources=http01proxies,verbs=get;list;watch;create;update;patch;delete +//+kubebuilder:rbac:groups=operator.openshift.io,resources=http01proxies,verbs=get;list;watch //+kubebuilder:rbac:groups=operator.openshift.io,resources=http01proxies/status,verbs=get;update;patch //+kubebuilder:rbac:groups=operator.openshift.io,resources=http01proxies/finalizers,verbs=update + +// If/when emitting Events: +//+kubebuilder:rbac:groups="",resources=events,verbs=create;patch +//+kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patchVerification: search of config/ returned no matches; unable to confirm manifests reflect this change.
api/operator/v1alpha1/http01proxy_types.go (1)
36-40: Add explicit kubebuilder resource scope: NamespacedMake the CRD scope explicit in generated manifests.
File: api/operator/v1alpha1/http01proxy_types.go (around lines 36–40)
// +kubebuilder:object:root=true // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp" // +kubebuilder:validation:XValidation:rule="self.metadata.name == 'default'",message="http01proxy is a singleton, .metadata.name must be 'default'" +// +kubebuilder:resource:scope=Namespaced // +operator-sdk:csv:customresourcedefinitions:displayName="HTTP01Proxy"Verify generated CRD scope with:
rg -nA2 'kind:\s*CustomResourceDefinition' config/crd/bases/*.yaml rg -n 'scope:\s*Namespaced' config/crd/bases/*.yaml || truepkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go (1)
17-21: Approve — HTTP01Proxies getter and scheme registration verifiedVerified: OperatorV1alpha1Client.HTTP01Proxies is implemented (pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go) and operatorv1alpha1.AddToScheme is registered (pkg/operator/clientset/versioned/scheme/register.go).
pkg/operator/applyconfigurations/operator/v1alpha1/http01proxy.go (1)
34-68: Apply extractors: path mapping and ForKind coverage verified. ManagedFields extraction targets the internal HTTP01Proxy type; WithKind/WithAPIVersion are set. utils.ForKind includes a case returning HTTP01ProxyApplyConfiguration.
sebrandon1
force-pushed
the
poc_cert_manager_proxy
branch
from
969c3e3 to
30353a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
api/operator/v1alpha1/http01proxy_types.go (2)
90-99: Bound status valuesConsider enforcing non-negative for ActiveChallenges to prevent accidental underflow on status updates.
Apply this diff:
- // +kubebuilder:validation:Optional + // +kubebuilder:validation:Optional + // +kubebuilder:validation:Minimum:=0 // +optional ActiveChallenges int32 `json:"activeChallenges,omitempty"`Optional: add a max length to LastError (e.g., 1024) to keep status size bounded.
129-146: Use generated DeepCopy for ControllerConfigControllerConfig already has generated DeepCopyInto/DeepCopy (api/operator/v1alpha1/zz_generated.deepcopy.go:232–249); replace the manual shallow copy in HTTP01ProxySpec.DeepCopyInto (api/operator/v1alpha1/http01proxy_types.go:129–146) with either
out.ControllerConfig = in.ControllerConfig.DeepCopy()or allocateout.ControllerConfigand callin.ControllerConfig.DeepCopyInto(out.ControllerConfig).pkg/operator/applyconfigurations/operator/v1alpha1/http01proxy.go (1)
95-102: Avoid WithGenerateName for this singleton CRGenerateName won’t satisfy the XValidation requiring name "default". Recommend not using WithGenerateName for HTTP01Proxy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (22)
api/operator/v1alpha1/http01proxy_types.go(1 hunks)api/operator/v1alpha1/zz_generated.deepcopy.go(2 hunks)config/crd/bases/operator.openshift.io_http01proxies.yaml(1 hunks)config/rbac/role.yaml(2 hunks)config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml(1 hunks)pkg/controller/http01proxy/controller.go(1 hunks)pkg/operator/applyconfigurations/internal/internal.go(1 hunks)pkg/operator/applyconfigurations/operator/v1alpha1/http01proxy.go(1 hunks)pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go(1 hunks)pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go(1 hunks)pkg/operator/applyconfigurations/utils.go(1 hunks)pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go(1 hunks)pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go(1 hunks)pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go(1 hunks)pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go(1 hunks)pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go(2 hunks)pkg/operator/informers/externalversions/generic.go(1 hunks)pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go(1 hunks)pkg/operator/informers/externalversions/operator/v1alpha1/interface.go(2 hunks)pkg/operator/listers/operator/v1alpha1/expansion_generated.go(1 hunks)pkg/operator/listers/operator/v1alpha1/http01proxy.go(1 hunks)pkg/operator/setup_manager.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (17)
- pkg/operator/applyconfigurations/utils.go
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxystatus.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_operator_client.go
- config/rbac/role.yaml
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/operator_client.go
- pkg/operator/listers/operator/v1alpha1/expansion_generated.go
- config/crd/bases/operator.openshift.io_http01proxies.yaml
- api/operator/v1alpha1/zz_generated.deepcopy.go
- pkg/controller/http01proxy/controller.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go
- pkg/operator/informers/externalversions/generic.go
- pkg/operator/setup_manager.go
- config/samples/operator.openshift.io_v1alpha1_http01proxy.yaml
- pkg/operator/applyconfigurations/operator/v1alpha1/http01proxyspec.go
- pkg/operator/clientset/versioned/typed/operator/v1alpha1/fake/fake_http01proxy.go
- pkg/operator/informers/externalversions/operator/v1alpha1/interface.go
- pkg/operator/listers/operator/v1alpha1/http01proxy.go
🔇 Additional comments (4)
pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go (1)
7-8: LGTM — HTTP01ProxyExpansion is present and embeddedHTTP01ProxyInterface (pkg/operator/clientset/versioned/typed/operator/v1alpha1/http01proxy.go) includes HTTP01ProxyExpansion; pkg/operator/clientset/versioned/typed/operator/v1alpha1/generated_expansion.go defines
type HTTP01ProxyExpansion interface{}.pkg/operator/applyconfigurations/internal/internal.go (1)
36-45: HTTP01Proxy added to applyconfig schema — LGTMType path matches the extractor in applyconfigurations and sentinel types are present. No action.
pkg/operator/informers/externalversions/operator/v1alpha1/http01proxy.go (1)
39-62: Informer wiring looks correctList/Watch use the typed client, default indexer is Namespace, and tweakListOptions is honored. Good to go.
pkg/operator/applyconfigurations/operator/v1alpha1/http01proxy.go (1)
25-32: Builder defaults are correct; ensure name='default'Kind/APIVersion are set properly. Since the CR enforces name == "default", callers should pass name "default" here.
| // spec is the specification of the desired behavior of the HTTP01Proxy. | ||
| // +kubebuilder:validation:Required | ||
| // +required | ||
| Spec HTTP01ProxySpec `json:"spec,omitempty"` | ||
|
|
There was a problem hiding this comment.
Make spec truly required (drop omitempty)
You’ve marked Spec as required, but json:",omitempty" can cause clients to omit it when zero-valued, leading to CRD validation failures. Remove omitempty.
Apply this diff:
- // +kubebuilder:validation:Required
- // +required
- Spec HTTP01ProxySpec `json:"spec,omitempty"`
+ // +kubebuilder:validation:Required
+ // +required
+ Spec HTTP01ProxySpec `json:"spec"`🤖 Prompt for AI Agents
In api/operator/v1alpha1/http01proxy_types.go around lines 47 to 51, the Spec
field is marked required but the struct tag uses `json:"spec,omitempty"`, which
allows omission; remove `omitempty` so the JSON tag is `json:"spec"` to ensure
the field is always serialized and CRD validation works as intended. Update the
struct tag accordingly and run `make generate`/rebuild CRDs if your project uses
codegen to propagate the change.
sebrandon1
force-pushed
the
poc_cert_manager_proxy
branch
from
30353a5 to
941be60
Compare
|
[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 |
|
@sebrandon1: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
1 participant
I'm trying to build a proof-of-concept for the HTTP01Proxy as partially described in:
Changes:
Usage:
kubectl apply -f config/crd/bases/operator.openshift.io_http01proxies.yamlThen install the operator with
make local-run.kubectl apply -f config/samples/operator.openshift.io_v1alpha1_http01proxy.yamlNote: This is a no-op scaffold. It adds the HTTP01Proxy CRD, a sample CR, and wires a minimal controller into the operator’s controller-runtime manager. It does not deploy any new operand/image, does not change routing, and has no functional proxy logic yet. Potential follow-up commits will add RBAC and reconcile behavior to watch Challenges and manage Routes.