Skip to content

certrotation: restructure controller to own plumbing and cert creation#2151

Closed
sanchezl wants to merge 2 commits intoopenshift:masterfrom
sanchezl:certrotation-refactor
Closed

certrotation: restructure controller to own plumbing and cert creation#2151
sanchezl wants to merge 2 commits intoopenshift:masterfrom
sanchezl:certrotation-refactor

Conversation

@sanchezl
Copy link
Copy Markdown
Contributor

@sanchezl sanchezl commented Apr 2, 2026

Context

This addresses structural concerns raised during review of PR #2145. The certrotation package had hybrid data+behavior structs (RotatedSigningCASecret, CABundleConfigMap, RotatedSelfSignedCertKeySecret) that carried informers, listers, clients, event recorders, AND CRUD methods. This PR restructures the package so the controller owns all plumbing and cert creation logic, and the caller passes pure config structs.

PKI support is added separately in PR #2152.

Proof PR: openshift/cluster-kube-apiserver-operator#2089

What changed

New config types replace old hybrid structs

Old (data + behavior) New (pure config)
RotatedSigningCASecret SigningCAConfig
CABundleConfigMap CABundleConfig
RotatedSelfSignedCertKeySecret TargetCertKeyPairConfig

The new types carry only configuration (Namespace, Name, Validity, Refresh, AdditionalAnnotations, etc.) — no Informer, Lister, Client, or EventRecorder fields.

Sealed interface replaces TargetCertCreator

The TargetCertCreator interface and its implementations (ClientRotation, ServingRotation, SignerRotation) are removed. In their place:

type TargetCertConfig interface {
    targetCertConfig() // unexported marker, seals the interface
}

type ClientCertConfig struct { UserInfo user.Info }
type ServingCertConfig struct { Hostnames func() []string; ... }
type SignerCertConfig struct { SignerName string }

The controller type-switches on the variant to create certs. No more KeyPairGeneratorConfigurable interface or runtime mutation.

Controller owns all plumbing

NewCertRotationController now takes kubernetes.Interface and KubeInformersForNamespaces and derives informers/listers from the config struct Namespace+Name fields:

func NewCertRotationController(
    name string,
    signingCA SigningCAConfig,
    caBundle CABundleConfig,
    targetCert TargetCertKeyPairConfig,
    kubeClient kubernetes.Interface,
    kubeInformers v1helpers.KubeInformersForNamespaces,
    recorder events.Recorder,
    reporter StatusReporter,
) factory.Controller

All Ensure*, set*, and needNew* methods moved from the data structs to the controller.

Removed types and interfaces

  • TargetCertCreator interface
  • TargetCertRechecker interface
  • KeyPairGeneratorConfigurable interface
  • ClientRotation, ServingRotation, SignerRotation structs
  • ServingHostnameFunc type
  • configurablePKIEnabled bool fields

Files guide

File What to look at
signer.go SigningCAConfig struct (pure data), helper functions stay as package-level
cabundle.go CABundleConfig struct (pure data), manageCABundleConfigMap stays as package-level function
target.go TargetCertKeyPairConfig, sealed TargetCertConfig interface, ClientCertConfig/ServingCertConfig/SignerCertConfig variants, needNewTargetCertKeyPair with integrated hostname checking
client_cert_rotation_controller.go CertRotationController with all moved methods, ensureSigningCertKeyPair/ensureConfigMapCABundle/ensureTargetCertKeyPair
*_test.go Incremental migration — tests construct controller directly (same-package access to unexported fields)

Test plan

@openshift-ci openshift-ci Bot requested review from deads2k and p0lyn0mial April 2, 2026 19:41
sanchezl added a commit to sanchezl/cluster-kube-apiserver-operator that referenced this pull request Apr 2, 2026
Update certrotation controller to use the new library-go certrotation
API from openshift/library-go#2151:
- SigningCAConfig, CABundleConfig, TargetCertKeyPairConfig replace old types
- ClientCertConfig/ServingCertConfig replace ClientRotation/ServingRotation
- Controller takes kubeClient and kubeInformersForNamespaces
- PKI provider created by caller, gated on ConfigurablePKI feature gate
- PKI informer cache sync registered with cachesToSync

Does not compile until library-go is vendored with the new types.
sanchezl added a commit to sanchezl/cluster-kube-apiserver-operator that referenced this pull request Apr 2, 2026
Update certrotation controller to use the new library-go certrotation
API from openshift/library-go#2151:
- SigningCAConfig, CABundleConfig, TargetCertKeyPairConfig replace old types
- ClientCertConfig/ServingCertConfig replace ClientRotation/ServingRotation
- Controller takes kubeClient and kubeInformersForNamespaces
- PKI provider created by caller, gated on ConfigurablePKI feature gate
- PKI informer cache sync registered with cachesToSync

Vendors library-go from sanchezl/library-go certrotation-refactor branch.
@sanchezl sanchezl force-pushed the certrotation-refactor branch from 3a74583 to 1bc2ee6 Compare April 2, 2026 20:28
sanchezl added a commit to sanchezl/cluster-kube-apiserver-operator that referenced this pull request Apr 2, 2026
Update certrotation controller to use the new library-go certrotation
API from openshift/library-go#2151:
- SigningCAConfig, CABundleConfig, TargetCertKeyPairConfig replace old types
- ClientCertConfig/ServingCertConfig replace ClientRotation/ServingRotation
- Controller takes kubeClient and kubeInformersForNamespaces
- PKI provider created by caller, gated on ConfigurablePKI feature gate
- PKI informer cache sync registered with cachesToSync
- AdditionalAnnotations referenced from certrotation package (no tlsartifact)

Vendors library-go from sanchezl/library-go certrotation-refactor branch.
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 2, 2026

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sanchezl
Once this PR has been reviewed and has the lgtm label, please assign jsafrane for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@sanchezl sanchezl force-pushed the certrotation-refactor branch from 1bc2ee6 to 8dcae8d Compare April 3, 2026 02:05
sanchezl added a commit to sanchezl/cluster-kube-apiserver-operator that referenced this pull request Apr 3, 2026
Update certrotation controller to use the new library-go certrotation
API from openshift/library-go#2151:
- SigningCAConfig, CABundleConfig, TargetCertKeyPairConfig replace old types
- ClientCertConfig/ServingCertConfig replace ClientRotation/ServingRotation
- Controller takes kubeClient and kubeInformersForNamespaces
- PKI provider created by caller, gated on ConfigurablePKI feature gate
- PKI informer cache sync registered with cachesToSync
- AdditionalAnnotations referenced from certrotation package (no tlsartifact)

Vendors library-go from sanchezl/library-go certrotation-refactor branch.
sanchezl added a commit to sanchezl/cluster-kube-apiserver-operator that referenced this pull request Apr 3, 2026
Update certrotation controller to use the new library-go certrotation
API from openshift/library-go#2151 (refactor-only, no PKI support):
- SigningCAConfig, CABundleConfig, TargetCertKeyPairConfig replace old types
- ClientCertConfig/ServingCertConfig replace ClientRotation/ServingRotation
- Controller takes kubeClient and kubeInformersForNamespaces
- No PKI provider, no CertificateName fields

Vendors library-go from sanchezl/library-go certrotation-refactor branch.
@sanchezl sanchezl changed the title certrotation: restructure controller and add configurable PKI support certrotation: restructure controller to own plumbing and cert creation Apr 3, 2026
@sanchezl sanchezl force-pushed the certrotation-refactor branch from 8dcae8d to 062d3b2 Compare April 3, 2026 14:27
sanchezl added 2 commits April 3, 2026 16:26
Replace hybrid data+behavior structs with pure config types:
- RotatedSigningCASecret -> SigningCAConfig
- CABundleConfigMap -> CABundleConfig
- RotatedSelfSignedCertKeySecret -> TargetCertKeyPairConfig

Flatten TargetCertCreator interface into sealed TargetCertConfig with
typed variants (ClientCertConfig, ServingCertConfig, SignerCertConfig).
The controller type-switches on the variant to create certs.

Move all Ensure/set/needNew methods from data structs to
CertRotationController. The controller now takes kubernetes.Interface
and KubeInformersForNamespaces, deriving informers and listers from
the config struct Namespace+Name fields.

This eliminates the per-struct Informer/Lister/Client/EventRecorder
plumbing and removes the TargetCertCreator, TargetCertRechecker, and
KeyPairGeneratorConfigurable interfaces along with the ClientRotation,
ServingRotation, and SignerRotation types.
Move ensureSigningCertKeyPair, ensureConfigMapCABundle,
ensureTargetCertKeyPair, and setTargetCertKeyPairSecret from signer.go,
cabundle.go, and target.go into client_cert_rotation_controller.go.

This consolidates all controller sync logic in one file, leaving only
config types and pure helper functions in the per-artifact files.
@sanchezl sanchezl force-pushed the certrotation-refactor branch from 062d3b2 to a957edb Compare April 3, 2026 20:28
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Apr 3, 2026

@sanchezl: all tests passed!

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@p0lyn0mial
Copy link
Copy Markdown
Contributor

@sanchezl please ask people who work with you on the feature for review. thanks.

@sanchezl
Copy link
Copy Markdown
Contributor Author

sanchezl commented Apr 9, 2026

Closing in favor of library-go#2145. The structural refactor approach didn't work well with how cluster-etcd-operator uses the certrotation package — went with the original PR instead.

@sanchezl sanchezl closed this Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants