From dd3812c02710f71a9f5670616a952979b8ddf472 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Radek=20Ma=C5=88=C3=A1k?= Date: Tue, 21 Jul 2026 13:22:28 +0200 Subject: [PATCH] Deny superseded duplicate node CSRs and remove approve-stop Exclude Denied/Failed from the pending filter and count, prune older pending CSRs per (system:node CN, signer), keep approving when pending exceeds max, and alert after 15m of sustained superseded denials. --- docs/dev/metrics.md | 21 +- go.mod | 2 +- ...luster-machine-approver_04_alertrules.yaml | 18 + pkg/controller/controller.go | 188 ++++++---- pkg/controller/csr_check.go | 110 +++++- pkg/controller/csr_check_test.go | 337 ++++++++++++++++++ pkg/metrics/metrics.go | 5 +- 7 files changed, 592 insertions(+), 89 deletions(-) diff --git a/docs/dev/metrics.md b/docs/dev/metrics.md index eb0b36a47..9ddfa3d3d 100644 --- a/docs/dev/metrics.md +++ b/docs/dev/metrics.md @@ -4,19 +4,28 @@ The Cluster Machine Approver reports the following metrics: ## Metrics about pending certificate signing requests (CSRs) -These metrics show how many CSRs are currently pending as well as the -maximum number allowed to be pending. These can be useful to help diagnose -the flow of new Nodes being added to the cluster. +These metrics show how many recently pending node CSRs are currently counted +as well as the threshold used by MachineApproverMaxPendingCSRsReached. +They help diagnose Node bootstrap and CSR approval behavior during scale-up. -``` -# HELP mapi_current_pending_csr Count of pending CSRs at the cluster level +```text +# HELP mapi_current_pending_csr Count of recently pending node CSRs at the cluster level # TYPE mapi_current_pending_csr gauge mapi_current_pending_csr 0 -# HELP mapi_max_pending_csr Threshold value of the pending CSRs beyond which any new CSR requests will be ignored +# HELP mapi_max_pending_csr Recently pending node CSR count threshold used by MachineApproverMaxPendingCSRsReached # TYPE mapi_max_pending_csr gauge mapi_max_pending_csr 108 +# HELP mapi_duplicate_csr_denied_total Count of pending node CSRs denied because a newer CSR exists for the same node and signer +# TYPE mapi_duplicate_csr_denied_total counter +mapi_duplicate_csr_denied_total 0 ``` +`MachineApproverMaxPendingCSRsReached` fires when +`mapi_current_pending_csr > mapi_max_pending_csr` for 5m. +`MachineApproverDuplicateCSRDenied` fires when +`increase(mapi_duplicate_csr_denied_total[5m]) > 0` for 15m +(sustained prune pressure; brief one-off denials do not alert). + ## Metrics about the Prometheus collectors Prometheus provides some default metrics about the internal state diff --git a/go.mod b/go.mod index 214a0b87d..9673d8cc2 100644 --- a/go.mod +++ b/go.mod @@ -91,7 +91,7 @@ require ( github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect github.com/modern-go/reflect2 v1.0.3-0.20250322232337-35a7c28c31ee // indirect github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect - github.com/prometheus/client_model v0.6.2 // indirect + github.com/prometheus/client_model v0.6.2 github.com/prometheus/common v0.66.1 // indirect github.com/prometheus/procfs v0.16.1 // indirect github.com/robfig/cron v1.2.0 // indirect diff --git a/manifests/0000_90_cluster-machine-approver_04_alertrules.yaml b/manifests/0000_90_cluster-machine-approver_04_alertrules.yaml index f9829d479..1ae05d254 100644 --- a/manifests/0000_90_cluster-machine-approver_04_alertrules.yaml +++ b/manifests/0000_90_cluster-machine-approver_04_alertrules.yaml @@ -28,3 +28,21 @@ spec: pending CSRs to determine which machines need approval, also check that the nodelink controller is running in the openshift-machine-api namespace. + - alert: MachineApproverDuplicateCSRDenied + expr: | + increase(mapi_duplicate_csr_denied_total[5m]) > 0 + for: 15m + labels: + severity: warning + annotations: + summary: "duplicate node CSRs were denied." + description: | + Cluster Machine Approver has been denying superseded pending node + CertificateSigningRequests for at least 15 minutes (a newer CSR + exists for the same node and signer). Short one-off denials during + address population lag are tolerated and do not fire this alert. + Sustained denials usually mean kubelets keep reissuing while the + newest CSR stays unauthorizable (for example Machine addresses + missing), approval is deferred, or CMA is not approving in time. + Inspect pending/denied CSRs, Machine status.addresses, and + machine-approver logs. diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 72fbe4549..e1a5fba00 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -14,6 +14,7 @@ import ( "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" certificatesv1client "k8s.io/client-go/kubernetes/typed/certificates/v1" "k8s.io/client-go/rest" @@ -32,6 +33,8 @@ const ( configNamespace = "openshift-config-managed" kubeletCAConfigMap = "csr-controller-ca" csrConditionApproveMessage = "This CSR was approved by the Node CSR Approver (cluster-machine-approver)" + csrConditionDenyReason = "NodeCSRSuperseded" + csrConditionDenyMessage = "This CSR was denied by the Node CSR Approver (cluster-machine-approver) because it was superseded by a newer CSR for the same node and signer" ) // MachineApproverReconciler reconciles a machine-approver object @@ -71,39 +74,57 @@ func (m *CertificateApprover) buildWithManager(mgr ctrl.Manager, options control })).Complete(c) } -// pendingNodeCertFilter filters CSRs that need to be reconciled -func pendingNodeCertFilter(obj runtime.Object) bool { - cert, ok := obj.(*certificatesv1.CertificateSigningRequest) - // Reconcile unapproved or approved by another controller to update our metrics - reconcileRequired := ok && (!isApproved(*cert) || (isRecentlyApproved(*cert) && !isApprovedByCMA(*cert))) - - if !reconcileRequired { - return false - } - +// isNodeCSRKind reports whether csr is a node client or serving CSR we care about +// (signer/group/username), ignoring approval status. +func isNodeCSRKind(cert *certificatesv1.CertificateSigningRequest) bool { switch cert.Spec.SignerName { case certificatesv1.KubeletServingSignerName: groupSet := sets.NewString(cert.Spec.Groups...) - // Reconcile kubernetes.io/kubelet-serving when it has the system:nodes group if !groupSet.Has(nodeGroup) { klog.V(3).Infof("%s: Ignoring csr because it does not have the system:nodes group", cert.Name) return false } case certificatesv1.KubeAPIServerClientKubeletSignerName: - // Reconcile kubernetes.io/kube-apiserver-client-kubelet when it is created by the node bootstrapper if cert.Spec.Username != nodeBootstrapperUsername { klog.V(3).Infof("%s: Ignoring csr because it is not from the node bootstrapper", cert.Name) return false } default: - // Ignore all other CSRs klog.V(3).Infof("%s: Ignoring csr because of unsupported signerName: %s", cert.Name, cert.Spec.SignerName) return false } - return true } +// isUnsignedPendingNodeCSR is true for node CSRs that are not Approved, Denied, or Failed. +// Used for supersede selection and pending metrics (not for reconcile wake). +func isUnsignedPendingNodeCSR(cert *certificatesv1.CertificateSigningRequest) bool { + if isApproved(*cert) || isDenied(*cert) || isFailed(*cert) { + return false + } + return isNodeCSRKind(cert) +} + +// pendingNodeCertFilter wakes reconcile for unsigned node CSRs, or recently approved +// ones not approved by CMA (metrics / catch-up). Not used for supersede candidates. +func pendingNodeCertFilter(obj runtime.Object) bool { + cert, ok := obj.(*certificatesv1.CertificateSigningRequest) + if !ok { + return false + } + + if isDenied(*cert) || isFailed(*cert) { + return false + } + + reconcileRequired := !isApproved(*cert) || (isRecentlyApproved(*cert) && !isApprovedByCMA(*cert)) + if !reconcileRequired { + return false + } + + return isNodeCSRKind(cert) +} + func (m *CertificateApprover) toCSRs(ctx context.Context, obj client.Object) []reconcile.Request { requests := []reconcile.Request{} csrs, err := listNodeCSRs(ctx, m.WorkloadClient) @@ -194,100 +215,103 @@ func (m *CertificateApprover) Reconcile(ctx context.Context, req ctrl.Request) ( return reconcile.Result{}, fmt.Errorf("Failed to get Nodes: %w", err) } - if offLimits := reconcileLimits(req.Name, machines, nodes, csrs); offLimits { - // Stop all reconciliation - return reconcile.Result{}, nil + denyErr := m.denySupersededDuplicates(ctx, csrs) + if denyErr != nil { + klog.Errorf("%v: failed to deny some superseded CSRs: %v", req.Name, denyErr) } - for _, csr := range csrs { - if csr.Name == req.Name { - if err := m.reconcileCSR(csr, machines); err != nil { - return reconcile.Result{}, fmt.Errorf("could not reconcile CSR: %v", err) - } - - // Reconcile the limits at the end of a reconcile so that the currently - // pending CSRs metric has an up to date value if we approved a CSR. - // When an error occurs, we requeue and so update the limits on the - // next reconcile. - // Don't use a cached client here else we may not have up to date CSRs. - return reconcile.Result{}, reconcileLimitsUncached(m.NodeRestCfg, csr.Name, machines, nodes) + var reconcileErr error + found := false + for i := range csrs { + if csrs[i].Name != req.Name { + continue } + found = true + if err := m.reconcileCSR(ctx, &csrs[i], machines); err != nil { + reconcileErr = fmt.Errorf("could not reconcile CSR: %v", err) + } + break } - - klog.Errorf("Failed to find CSR: %v", req) - - return reconcile.Result{}, nil -} - -// reconcileLimits will short circut logic if number of pending CSRs is exceeding limit -func reconcileLimits(csrName string, machines []machinehandlerpkg.Machine, nodes *corev1.NodeList, csrs []certificatesv1.CertificateSigningRequest) bool { - maxPending := getMaxPending(machines, nodes) - atomic.StoreUint32(&MaxPendingCSRs, uint32(maxPending)) - pending := recentlyPendingNodeCSRs(csrs) - atomic.StoreUint32(&PendingCSRs, uint32(pending)) - if pending > maxPending { - klog.Errorf("%v: Pending CSRs: %d; Max pending allowed: %d. Difference between pending CSRs and machines > %v. Ignoring all CSRs as too many recent pending CSRs seen", csrName, pending, maxPending, maxDiffBetweenPendingCSRsAndMachinesCount) - return true + if !found { + klog.Errorf("Failed to find CSR: %v", req.Name) } - return false + // csrs reflects approve/deny from this pass, so gauges can use this list. + refreshPendingCSRMetrics(req.Name, machines, nodes, csrs) + return reconcile.Result{}, errors.NewAggregate([]error{denyErr, reconcileErr}) } -// reconcileLimitsUncached is used to update the limits using an uncached certificates list. -// This is used at the end of the approval process to ensure that the limits (and therefore) -// the metrics are always up to date. -func reconcileLimitsUncached(cfg *rest.Config, csrName string, machines []machinehandlerpkg.Machine, nodes *corev1.NodeList) error { - certClient, err := certificatesv1client.NewForConfig(cfg) - if err != nil { - return fmt.Errorf("could not initialise certificates client: %v", err) +// denySupersededDuplicates denies older unsigned duplicates in place on csrs. +// Individual deny failures are aggregated; remaining candidates are still attempted. +func (m *CertificateApprover) denySupersededDuplicates(ctx context.Context, csrs []certificatesv1.CertificateSigningRequest) error { + superseded := supersededPendingNodeCSRs(csrs) + if len(superseded) == 0 { + return nil } - clientCertificates, err := certClient.CertificateSigningRequests().List(context.Background(), metav1.ListOptions{FieldSelector: clientKubeletFieldSelector}) + certClient, err := certificatesv1client.NewForConfig(m.NodeRestCfg) if err != nil { - return fmt.Errorf("could not list CSRs: %v", err) + return err } + csrClient := certClient.CertificateSigningRequests() - servingCertificates, err := certClient.CertificateSigningRequests().List(context.Background(), metav1.ListOptions{FieldSelector: kubeletServingFieldSelector}) - if err != nil { - return fmt.Errorf("could not list CSRs: %v", err) + var denyErrs []error + for _, idx := range superseded { + csr := &csrs[idx] + if err := deny(ctx, csrClient, csr); err != nil { + denyErrs = append(denyErrs, fmt.Errorf("unable to deny superseded CSR %s: %w", csr.Name, err)) + continue + } + klog.Infof("CSR %s denied as superseded by a newer CSR for the same node and signer", csr.Name) } + return errors.NewAggregate(denyErrs) +} - csrs := clientCertificates.Items - csrs = append(csrs, servingCertificates.Items...) - reconcileLimits(csrName, machines, nodes, csrs) - return nil +// refreshPendingCSRMetrics updates PendingCSRs/MaxPendingCSRs gauges. It never blocks approval. +func refreshPendingCSRMetrics(csrName string, machines []machinehandlerpkg.Machine, nodes *corev1.NodeList, csrs []certificatesv1.CertificateSigningRequest) { + maxPending := getMaxPending(machines, nodes) + atomic.StoreUint32(&MaxPendingCSRs, uint32(maxPending)) + pending := recentlyPendingNodeCSRs(csrs) + atomic.StoreUint32(&PendingCSRs, uint32(pending)) + if pending > maxPending { + klog.Warningf("%v: pending node CSRs %d exceed alert threshold %d (machines/nodes + %v)", csrName, pending, maxPending, maxDiffBetweenPendingCSRsAndMachinesCount) + } } -func (m *CertificateApprover) reconcileCSR(csr certificatesv1.CertificateSigningRequest, machines []machinehandlerpkg.Machine) error { +func (m *CertificateApprover) reconcileCSR(ctx context.Context, csr *certificatesv1.CertificateSigningRequest, machines []machinehandlerpkg.Machine) error { // If a CSR is approved after being added to the queue, but before we reconcile it, // it may have already been approved. If it has already been approved, trying to // approve it again will result in an error and cause a loop. // Return early if the CSR has been approved externally. - if isApproved(csr) { + if isApproved(*csr) { klog.Infof("%v: CSR is already approved", csr.Name) return nil } + if isDenied(*csr) || isFailed(*csr) { + klog.Infof("%v: CSR is already denied or failed", csr.Name) + return nil + } - parsedCSR, err := parseCSR(&csr) + parsedCSR, err := parseCSR(csr) if err != nil { klog.Errorf("%v: Failed to parse csr: %v", csr.Name, err) return fmt.Errorf("error parsing request CSR: %v", err) } - kubeletCA := m.getKubeletCA() + kubeletCA := m.getKubeletCA(ctx) if kubeletCA == nil { // This is not a fatal error. The renewal authorization flow // depending on the existing serving cert will be skipped. klog.Errorf("failed to get kubelet CA") } - if authorize, err := authorizeCSR(m.WorkloadClient, m.Config, machines, &csr, parsedCSR, kubeletCA); !authorize { + if authorize, err := authorizeCSR(m.WorkloadClient, m.Config, machines, csr, parsedCSR, kubeletCA); !authorize { // Don't deny since it might be someone else's CSR klog.Infof("%s: CSR not authorized", csr.Name) return err } - if err := approve(m.NodeRestCfg, &csr); err != nil { + if err := approve(ctx, m.NodeRestCfg, csr); err != nil { return fmt.Errorf("Unable to approve CSR %s: %w", csr.Name, err) } klog.Infof("CSR %s approved", csr.Name) @@ -297,13 +321,13 @@ func (m *CertificateApprover) reconcileCSR(csr certificatesv1.CertificateSigning // getKubeletCA fetches the kubelet CA from the ConfigMap in the // openshift-config-managed namespace. -func (m *CertificateApprover) getKubeletCA() *x509.CertPool { +func (m *CertificateApprover) getKubeletCA(ctx context.Context) *x509.CertPool { configMap := &corev1.ConfigMap{} key := client.ObjectKey{ Namespace: configNamespace, Name: kubeletCAConfigMap, } - if err := m.WorkloadClient.Get(context.Background(), key, configMap); err != nil { + if err := m.WorkloadClient.Get(ctx, key, configMap); err != nil { klog.Errorf("failed to get kubelet CA: %v", err) return nil } @@ -324,7 +348,7 @@ func (m *CertificateApprover) getKubeletCA() *x509.CertPool { return certPool } -func approve(rest *rest.Config, csr *certificatesv1.CertificateSigningRequest) error { +func approve(ctx context.Context, rest *rest.Config, csr *certificatesv1.CertificateSigningRequest) error { needsupdate := false now := metav1.Now() condition := certificatesv1.CertificateSigningRequestCondition{ @@ -333,7 +357,7 @@ func approve(rest *rest.Config, csr *certificatesv1.CertificateSigningRequest) e Message: csrConditionApproveMessage, LastUpdateTime: now, LastTransitionTime: now, - Status: "True", + Status: corev1.ConditionTrue, } // Check if the new condition already exists, and change it only if there is a status @@ -363,7 +387,7 @@ func approve(rest *rest.Config, csr *certificatesv1.CertificateSigningRequest) e return err } if _, err := certClient.CertificateSigningRequests(). - UpdateApproval(context.Background(), csr.Name, csr, metav1.UpdateOptions{}); err != nil { + UpdateApproval(ctx, csr.Name, csr, metav1.UpdateOptions{}); err != nil { return err } } @@ -371,6 +395,26 @@ func approve(rest *rest.Config, csr *certificatesv1.CertificateSigningRequest) e return nil } +func deny(ctx context.Context, certClient certificatesv1client.CertificateSigningRequestInterface, csr *certificatesv1.CertificateSigningRequest) error { + now := metav1.Now() + updated := csr.DeepCopy() + updated.Status.Conditions = append(updated.Status.Conditions, certificatesv1.CertificateSigningRequestCondition{ + Type: certificatesv1.CertificateDenied, + Reason: csrConditionDenyReason, + Message: csrConditionDenyMessage, + LastUpdateTime: now, + LastTransitionTime: now, + Status: corev1.ConditionTrue, + }) + + if _, err := certClient.UpdateApproval(ctx, updated.Name, updated, metav1.UpdateOptions{}); err != nil { + return err + } + csr.Status = updated.Status + DuplicateCSRDeniedTotal.Inc() + return nil +} + // parseCSR extracts the CSR from the API object and decodes it. func parseCSR(obj *certificatesv1.CertificateSigningRequest) (*x509.CertificateRequest, error) { // extract PEM from request object diff --git a/pkg/controller/csr_check.go b/pkg/controller/csr_check.go index b3f88e24e..300b32938 100644 --- a/pkg/controller/csr_check.go +++ b/pkg/controller/csr_check.go @@ -16,6 +16,7 @@ import ( configv1 "github.com/openshift/api/config/v1" networkv1 "github.com/openshift/api/network/v1" machinehandlerpkg "github.com/openshift/cluster-machine-approver/pkg/machinehandler" + "github.com/prometheus/client_golang/prometheus" certificatesv1 "k8s.io/api/certificates/v1" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" @@ -45,9 +46,6 @@ const ( networkClusterName = "cluster" ) -var clientKubeletFieldSelector = fmt.Sprintf("%s=%s", signerNameField, certificatesv1.KubeAPIServerClientKubeletSignerName) -var kubeletServingFieldSelector = fmt.Sprintf("%s=%s", signerNameField, certificatesv1.KubeletServingSignerName) - var nodeBootstrapperGroups = sets.NewString( "system:serviceaccounts:openshift-machine-config-operator", "system:serviceaccounts", @@ -61,8 +59,92 @@ var nodeServingGroups = sets.NewString( var now = time.Now -var MaxPendingCSRs uint32 -var PendingCSRs uint32 +// Metrics updated by the controller and scraped via pkg/metrics. +var ( + MaxPendingCSRs uint32 + PendingCSRs uint32 + + DuplicateCSRDeniedTotal = prometheus.NewCounter(prometheus.CounterOpts{ + Name: "mapi_duplicate_csr_denied_total", + Help: "Total number of pending node CSRs denied because they were superseded by a newer CSR for the same node and signer", + }) +) + +func nodeCSRCommonName(parsed *x509.CertificateRequest) (string, bool) { + if parsed == nil { + return "", false + } + cn := parsed.Subject.CommonName + if !strings.HasPrefix(cn, nodeUserPrefix) { + return "", false + } + if len(strings.TrimPrefix(cn, nodeUserPrefix)) == 0 { + return "", false + } + return cn, true +} + +// nodeCSRSupersedeCN returns the system:node CN used for supersede grouping. +// Serving CSRs must also have Spec.Username equal to the parsed CN (same bar as approval). +func nodeCSRSupersedeCN(csr *certificatesv1.CertificateSigningRequest, parsed *x509.CertificateRequest) (string, bool) { + cn, ok := nodeCSRCommonName(parsed) + if !ok { + return "", false + } + if csr.Spec.SignerName == certificatesv1.KubeletServingSignerName && parsed.Subject.CommonName != csr.Spec.Username { + return "", false + } + return cn, true +} + +type csrSeenKey struct { + cn string + signer string +} + +// supersededPendingNodeCSRs returns indices into csrs of older unsigned node CSRs +// superseded by a newer unsigned CSR for the same (parsed system:node CN, signerName). +// Candidates use isUnsignedPendingNodeCSR and are not limited to the 1h metrics window. +func supersededPendingNodeCSRs(csrs []certificatesv1.CertificateSigningRequest) []int { + pendingIdx := make([]int, 0, len(csrs)) + for i := range csrs { + if isUnsignedPendingNodeCSR(&csrs[i]) { + pendingIdx = append(pendingIdx, i) + } + } + + // Newest first so the first (CN, signer) we see is kept; later ones are superseded. + sort.SliceStable(pendingIdx, func(i, j int) bool { + a, b := csrs[pendingIdx[i]], csrs[pendingIdx[j]] + ti, tj := a.CreationTimestamp.Time, b.CreationTimestamp.Time + if !ti.Equal(tj) { + return ti.After(tj) + } + return a.Name > b.Name + }) + + seen := make(map[csrSeenKey]struct{}, len(pendingIdx)) + superseded := make([]int, 0) + // Walk newest→oldest: first valid CN+signer is kept; further matches are superseded. + for _, idx := range pendingIdx { + csr := &csrs[idx] + parsed, err := parseCSR(csr) + if err != nil { + continue + } + cn, ok := nodeCSRSupersedeCN(csr, parsed) + if !ok { + continue + } + key := csrSeenKey{cn: cn, signer: csr.Spec.SignerName} + if _, exists := seen[key]; !exists { + seen[key] = struct{}{} + continue + } + superseded = append(superseded, idx) + } + return superseded +} func validateCSRContents(req *certificatesv1.CertificateSigningRequest, csr *x509.CertificateRequest) (string, error) { if !strings.HasPrefix(req.Spec.Username, nodeUserPrefix) { @@ -476,15 +558,27 @@ func inTimeSpan(start, end, check time.Time) bool { return check.After(start) && check.Before(end) } -func isApproved(csr certificatesv1.CertificateSigningRequest) bool { +func hasCondition(csr certificatesv1.CertificateSigningRequest, condType certificatesv1.RequestConditionType) bool { for _, condition := range csr.Status.Conditions { - if condition.Type == certificatesv1.CertificateApproved { + if condition.Type == condType { return true } } return false } +func isApproved(csr certificatesv1.CertificateSigningRequest) bool { + return hasCondition(csr, certificatesv1.CertificateApproved) +} + +func isDenied(csr certificatesv1.CertificateSigningRequest) bool { + return hasCondition(csr, certificatesv1.CertificateDenied) +} + +func isFailed(csr certificatesv1.CertificateSigningRequest) bool { + return hasCondition(csr, certificatesv1.CertificateFailed) +} + func isRecentlyApproved(csr certificatesv1.CertificateSigningRequest) bool { // assumes we are scheduled on the master meaning our clock is the same currentTime := now() @@ -522,7 +616,7 @@ func recentlyPendingNodeCSRs(csrs []certificatesv1.CertificateSigningRequest) in continue } - if pendingNodeCertFilter(&csr) { + if isUnsignedPendingNodeCSR(&csr) { pending++ } } diff --git a/pkg/controller/csr_check_test.go b/pkg/controller/csr_check_test.go index dd41cc40e..2d1f31eee 100644 --- a/pkg/controller/csr_check_test.go +++ b/pkg/controller/csr_check_test.go @@ -2,6 +2,7 @@ package controller import ( "bytes" + "context" "crypto" "crypto/ecdsa" "crypto/elliptic" @@ -21,12 +22,14 @@ import ( configv1 "github.com/openshift/api/config/v1" networkv1 "github.com/openshift/api/network/v1" + dto "github.com/prometheus/client_model/go" certificatesv1 "k8s.io/api/certificates/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/kubernetes/scheme" + certificatesv1client "k8s.io/client-go/kubernetes/typed/certificates/v1" testingclock "k8s.io/utils/clock/testing" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -2056,6 +2059,54 @@ func TestRecentlyPendingNodeBootstrapperCSRs(t *testing.T) { Request: []byte(multusCSRPEM), }, } + deniedNodeBootstrapperCSR := certificatesv1.CertificateSigningRequest{ + Spec: certificatesv1.CertificateSigningRequestSpec{ + SignerName: certificatesv1.KubeAPIServerClientKubeletSignerName, + Username: nodeBootstrapperUsername, + Groups: nodeBootstrapperGroups.List(), + }, + Status: certificatesv1.CertificateSigningRequestStatus{ + Conditions: []certificatesv1.CertificateSigningRequestCondition{{ + Type: certificatesv1.CertificateDenied, + }}, + }, + } + failedNodeBootstrapperCSR := certificatesv1.CertificateSigningRequest{ + Spec: certificatesv1.CertificateSigningRequestSpec{ + SignerName: certificatesv1.KubeAPIServerClientKubeletSignerName, + Username: nodeBootstrapperUsername, + Groups: nodeBootstrapperGroups.List(), + }, + Status: certificatesv1.CertificateSigningRequestStatus{ + Conditions: []certificatesv1.CertificateSigningRequestCondition{{ + Type: certificatesv1.CertificateFailed, + }}, + }, + } + deniedNodeServerCSR := certificatesv1.CertificateSigningRequest{ + Spec: certificatesv1.CertificateSigningRequestSpec{ + Username: nodeUserPrefix + "clustername-abcde-master-us-west-1a-0", + SignerName: certificatesv1.KubeletServingSignerName, + Groups: nodeServingGroups.List(), + }, + Status: certificatesv1.CertificateSigningRequestStatus{ + Conditions: []certificatesv1.CertificateSigningRequestCondition{{ + Type: certificatesv1.CertificateDenied, + }}, + }, + } + failedNodeServerCSR := certificatesv1.CertificateSigningRequest{ + Spec: certificatesv1.CertificateSigningRequestSpec{ + Username: nodeUserPrefix + "clustername-abcde-master-us-west-1a-0", + SignerName: certificatesv1.KubeletServingSignerName, + Groups: nodeServingGroups.List(), + }, + Status: certificatesv1.CertificateSigningRequestStatus{ + Conditions: []certificatesv1.CertificateSigningRequestCondition{{ + Type: certificatesv1.CertificateFailed, + }}, + }, + } pendingTime := baseTime.Add(time.Second) pastApprovalTime := baseTime.Add(-maxPendingDelta) @@ -2091,6 +2142,16 @@ func TestRecentlyPendingNodeBootstrapperCSRs(t *testing.T) { csrs: []certificatesv1.CertificateSigningRequest{createdAt(pendingTime, approvedNodeBootstrapperCSR)}, expectPending: 0, }, + { + name: "recently denied node bootstrapper csr", + csrs: []certificatesv1.CertificateSigningRequest{createdAt(pendingTime, deniedNodeBootstrapperCSR)}, + expectPending: 0, + }, + { + name: "recently failed node bootstrapper csr", + csrs: []certificatesv1.CertificateSigningRequest{createdAt(pendingTime, failedNodeBootstrapperCSR)}, + expectPending: 0, + }, { name: "pending past approval time", csrs: []certificatesv1.CertificateSigningRequest{createdAt(pastApprovalTime, pendingNodeBootstrapperCSR)}, @@ -2130,6 +2191,18 @@ func TestRecentlyPendingNodeBootstrapperCSRs(t *testing.T) { }, expectPending: 3, }, + { + name: "pending mixed with denied and failed", + csrs: []certificatesv1.CertificateSigningRequest{ + createdAt(pendingTime, pendingNodeBootstrapperCSR), + createdAt(pendingTime, pendingNodeServerCSR), + createdAt(pendingTime, deniedNodeBootstrapperCSR), + createdAt(pendingTime, failedNodeBootstrapperCSR), + createdAt(pendingTime, deniedNodeServerCSR), + createdAt(pendingTime, failedNodeServerCSR), + }, + expectPending: 2, + }, } for _, tt := range tests { @@ -2604,3 +2677,267 @@ func respond(server net.Listener) { conn.Write([]byte(server.Addr().String())) } } + +func TestSupersededPendingNodeCSRs(t *testing.T) { + clientPEM := createCSR("system:node:worker-1", defaultOrgs, nil, nil) + clientPEMOther := createCSR("system:node:worker-2", defaultOrgs, nil, nil) + servingPEM := createCSR("system:node:worker-1", defaultOrgs, defaultIPs, defaultDNSNames) + badCNPEM := createCSR("not-a-node", defaultOrgs, nil, nil) + emptyNodePEM := createCSR("system:node:", defaultOrgs, nil, nil) + + pendingClient := func(name string, cnPEM string, created time.Time) certificatesv1.CertificateSigningRequest { + return certificatesv1.CertificateSigningRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + CreationTimestamp: metav1.NewTime(created), + }, + Spec: certificatesv1.CertificateSigningRequestSpec{ + Request: []byte(cnPEM), + SignerName: certificatesv1.KubeAPIServerClientKubeletSignerName, + Username: nodeBootstrapperUsername, + Groups: nodeBootstrapperGroups.List(), + }, + } + } + pendingServing := func(name string, created time.Time) certificatesv1.CertificateSigningRequest { + return certificatesv1.CertificateSigningRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + CreationTimestamp: metav1.NewTime(created), + }, + Spec: certificatesv1.CertificateSigningRequestSpec{ + Request: []byte(servingPEM), + SignerName: certificatesv1.KubeletServingSignerName, + Username: nodeUserPrefix + "worker-1", + Groups: nodeServingGroups.List(), + }, + } + } + pendingServingAs := func(name, username string, pem string, created time.Time) certificatesv1.CertificateSigningRequest { + return certificatesv1.CertificateSigningRequest{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + CreationTimestamp: metav1.NewTime(created), + }, + Spec: certificatesv1.CertificateSigningRequestSpec{ + Request: []byte(pem), + SignerName: certificatesv1.KubeletServingSignerName, + Username: username, + Groups: nodeServingGroups.List(), + }, + } + } + + old := baseTime.Add(-2 * time.Hour) + mid := baseTime.Add(-time.Hour) + newest := baseTime.Add(-time.Minute) + // Older than the 1h pending metric window — prune must still consider any age. + veryOld := baseTime.Add(-3 * time.Hour) + + tests := []struct { + name string + csrs []certificatesv1.CertificateSigningRequest + wantDenied []string + }{ + { + name: "client signer keeps newest denies older for same CN", + csrs: []certificatesv1.CertificateSigningRequest{ + pendingClient("client-old", clientPEM, old), + pendingClient("client-mid", clientPEM, mid), + pendingClient("client-new", clientPEM, newest), + }, + wantDenied: []string{"client-mid", "client-old"}, + }, + { + name: "serving signer keeps newest denies older for same CN", + csrs: []certificatesv1.CertificateSigningRequest{ + pendingServing("serving-old", old), + pendingServing("serving-new", newest), + }, + wantDenied: []string{"serving-old"}, + }, + { + name: "client and serving for same CN are independent groups", + csrs: []certificatesv1.CertificateSigningRequest{ + pendingClient("client-old", clientPEM, old), + pendingClient("client-new", clientPEM, newest), + pendingServing("serving-old", old), + pendingServing("serving-new", newest), + }, + wantDenied: []string{"client-old", "serving-old"}, + }, + { + name: "multi CSR per CN fixture across nodes", + csrs: []certificatesv1.CertificateSigningRequest{ + pendingClient("w1-a", clientPEM, old), + pendingClient("w1-b", clientPEM, mid), + pendingClient("w1-c", clientPEM, newest), + pendingClient("w2-a", clientPEMOther, old), + pendingClient("w2-b", clientPEMOther, newest), + }, + wantDenied: []string{"w1-a", "w1-b", "w2-a"}, + }, + { + name: "bootstrapper-issued client CSRs group by parsed CN not username", + csrs: []certificatesv1.CertificateSigningRequest{ + pendingClient("boot-old", clientPEM, old), + pendingClient("boot-new", clientPEM, newest), + }, + wantDenied: []string{"boot-old"}, + }, + { + name: "prunes CSRs older than pending metric window", + csrs: []certificatesv1.CertificateSigningRequest{ + pendingClient("ancient", clientPEM, veryOld), + pendingClient("fresh", clientPEM, newest), + }, + wantDenied: []string{"ancient"}, + }, + { + name: "skips deny when CN is not system:node", + csrs: []certificatesv1.CertificateSigningRequest{ + pendingClient("bad-cn-old", badCNPEM, old), + pendingClient("bad-cn-new", badCNPEM, newest), + }, + wantDenied: nil, + }, + { + name: "skips deny when CN is system:node: with empty node name", + csrs: []certificatesv1.CertificateSigningRequest{ + pendingClient("empty-old", emptyNodePEM, old), + pendingClient("empty-new", emptyNodePEM, newest), + }, + wantDenied: nil, + }, + { + name: "name descending tie-break when creationTimestamp equal", + csrs: []certificatesv1.CertificateSigningRequest{ + pendingClient("csr-aaa", clientPEM, newest), + pendingClient("csr-zzz", clientPEM, newest), + }, + wantDenied: []string{"csr-aaa"}, + }, + { + name: "already denied CSRs are not prune candidates", + csrs: []certificatesv1.CertificateSigningRequest{ + func() certificatesv1.CertificateSigningRequest { + csr := pendingClient("already-denied", clientPEM, old) + csr.Status.Conditions = []certificatesv1.CertificateSigningRequestCondition{{ + Type: certificatesv1.CertificateDenied, + }} + return csr + }(), + pendingClient("kept", clientPEM, newest), + }, + wantDenied: nil, + }, + { + name: "already approved CSRs are not prune candidates", + csrs: []certificatesv1.CertificateSigningRequest{ + func() certificatesv1.CertificateSigningRequest { + csr := pendingClient("already-approved", clientPEM, old) + csr.Status.Conditions = []certificatesv1.CertificateSigningRequestCondition{{ + Type: certificatesv1.CertificateApproved, + LastUpdateTime: metav1.NewTime(baseTime), + LastTransitionTime: metav1.NewTime(baseTime), + Message: "approved by someone else", + Status: corev1.ConditionTrue, + }} + return csr + }(), + pendingClient("kept", clientPEM, newest), + }, + wantDenied: nil, + }, + { + name: "serving CSR with CN != Username is ignored for supersede", + csrs: []certificatesv1.CertificateSigningRequest{ + pendingServing("serving-legit-old", old), + // Newest by timestamp, but PEM CN is worker-1 while Username is worker-2. + pendingServingAs("serving-spoof-new", nodeUserPrefix+"worker-2", servingPEM, newest), + }, + // Spoof must not keep the (worker-1, serving) group or cause deny of the legit CSR. + wantDenied: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gotIdx := supersededPendingNodeCSRs(tt.csrs) + gotNames := make([]string, 0, len(gotIdx)) + for _, idx := range gotIdx { + gotNames = append(gotNames, tt.csrs[idx].Name) + } + if !sets.NewString(gotNames...).Equal(sets.NewString(tt.wantDenied...)) { + t.Errorf("supersededPendingNodeCSRs() = %v, want %v", gotNames, tt.wantDenied) + } + }) + } +} + +func TestDenyIncrementsDuplicateCSRDeniedTotal(t *testing.T) { + csr := &certificatesv1.CertificateSigningRequest{ + ObjectMeta: metav1.ObjectMeta{Name: "csr-deny-metric"}, + } + + t.Run("increments after successful UpdateApproval", func(t *testing.T) { + before := duplicateCSRDeniedTotalValue(t) + client := &denyCSRClient{} + if err := deny(context.Background(), client, csr.DeepCopy()); err != nil { + t.Fatalf("deny() error = %v", err) + } + after := duplicateCSRDeniedTotalValue(t) + if after-before != 1 { + t.Fatalf("mapi_duplicate_csr_denied_total delta = %v, want 1", after-before) + } + if client.updated == nil { + t.Fatal("expected UpdateApproval to be called") + } + if !isDenied(*client.updated) { + t.Fatal("expected denied condition on updated CSR") + } + cond := client.updated.Status.Conditions[len(client.updated.Status.Conditions)-1] + if cond.Reason != csrConditionDenyReason { + t.Errorf("deny reason = %q, want %q", cond.Reason, csrConditionDenyReason) + } + if cond.Message != csrConditionDenyMessage { + t.Errorf("deny message = %q, want %q", cond.Message, csrConditionDenyMessage) + } + }) + + t.Run("does not increment when UpdateApproval fails", func(t *testing.T) { + before := duplicateCSRDeniedTotalValue(t) + client := &denyCSRClient{err: fmt.Errorf("update failed")} + if err := deny(context.Background(), client, csr.DeepCopy()); err == nil { + t.Fatal("deny() error = nil, want update failure") + } + after := duplicateCSRDeniedTotalValue(t) + if after != before { + t.Fatalf("mapi_duplicate_csr_denied_total delta = %v, want 0", after-before) + } + }) +} + +// denyCSRClient is a minimal CertificateSigningRequestInterface stub for deny(). +type denyCSRClient struct { + certificatesv1client.CertificateSigningRequestInterface + err error + updated *certificatesv1.CertificateSigningRequest +} + +func (c *denyCSRClient) UpdateApproval(_ context.Context, _ string, csr *certificatesv1.CertificateSigningRequest, _ metav1.UpdateOptions) (*certificatesv1.CertificateSigningRequest, error) { + if c.err != nil { + return nil, c.err + } + c.updated = csr.DeepCopy() + return csr, nil +} + +func duplicateCSRDeniedTotalValue(t *testing.T) float64 { + t.Helper() + var metric dto.Metric + if err := DuplicateCSRDeniedTotal.Write(&metric); err != nil { + t.Fatalf("Write metric: %v", err) + } + return metric.GetCounter().GetValue() +} diff --git a/pkg/metrics/metrics.go b/pkg/metrics/metrics.go index b547db16d..4922bedd8 100644 --- a/pkg/metrics/metrics.go +++ b/pkg/metrics/metrics.go @@ -15,12 +15,13 @@ const DefaultMetricsBindAddress = "0.0.0.0:9192" var ( // CurrentPendingCSRCountDesc is a metric to report count of pending node CSRs in the cluster CurrentPendingCSRCountDesc = prometheus.NewDesc("mapi_current_pending_csr", "Count of recently pending node CSRs at the cluster level", nil, nil) - // MaxPendingCSRDesc is a metric to report threshold value of the pending node CSRs beyond which all CSR will be ignored by machine approver - MaxPendingCSRDesc = prometheus.NewDesc("mapi_max_pending_csr", "Threshold value of the pending node CSRs beyond which all CSR will be ignored by machine approver", nil, nil) + // MaxPendingCSRDesc is a metric to report the pending node CSR threshold used by MachineApproverMaxPendingCSRsReached + MaxPendingCSRDesc = prometheus.NewDesc("mapi_max_pending_csr", "Recently pending node CSR count threshold used by MachineApproverMaxPendingCSRsReached", nil, nil) ) func init() { metrics.Registry.MustRegister(&MetricsCollector{}) + metrics.Registry.MustRegister(controller.DuplicateCSRDeniedTotal) } // MetricsCollector is implementing prometheus.Collector interface.