Enable Cert Manager #8509
Merged
Enable Cert Manager #8509
Conversation
This file contains 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
knative-prow
bot
added
the
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: matzew The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
bot
added
approved
knative-prow
bot
added
the
area/test-and-release
matzew
commented
Feb 26, 2025
| @@ -78,3 +76,7 @@ func MakeCertificate(obj kmeta.OwnerRefableAccessor, name string) *cmv1.Certific | |||
| }, | |||
| } | |||
| } | |||
|
|
|||
| func deploymentName(sinkName string) string { | |||
| return kmeta.ChildName(sinkName, "-deployment") | |||
There was a problem hiding this comment.
I think on the "generic" certificate factory we could just pass in the name of the deployment to the MakeCert function
matzew
commented
Feb 26, 2025
matzew
commented
Feb 26, 2025
| eventPolicyLister: eventPolicyInformer.Lister(), | ||
| //cmCertificateLister: cmCertificateInformer.Lister(), | ||
| //certManagerClient: cmclient.Get(ctx), | ||
| secretLister: secretInformer.Lister(), |
|
This needs the knob for turning on/off the "cert manager" hooks. As discussed in #8385 (review) |
5 tasks
pierDipi
reviewed
Mar 4, 2025
pierDipi
reviewed
Mar 4, 2025
pierDipi
reviewed
Mar 4, 2025
pierDipi
reviewed
Mar 4, 2025
|
With the manual created lister, I seem to get error And yes, it gets once created, but I end up in the |
|
@matzew you need to label the certificate with that label |
matzew
commented
Mar 5, 2025
| logging.FromContext(ctx).Errorw("Error reconciling Certificate", zap.Error(err)) | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
TODO: delete certificate once the feature is turned.
matzew
commented
Mar 5, 2025
| eventPolicyInformer.Informer().AddEventHandler(auth.EventPolicyEventHandler( | ||
| integrationSinkInformer.Informer().GetIndexer(), | ||
| integrationSinkGK, | ||
| impl.EnqueueKey, | ||
| )) | ||
|
|
||
| cmCertificateInformer.Informer().AddEventHandler(controller.HandleAll(impl.Enqueue)) |
knative-prow
bot
removed
the
do-not-merge/work-in-progress
|
TODO: delete certificate once the feature is turned off More unit tests for reconciler will come too |
Signed-off-by: Matthias Wessendorf <[email protected]>
Signed-off-by: Matthias Wessendorf <[email protected]>
Signed-off-by: Matthias Wessendorf <[email protected]>
Signed-off-by: Matthias Wessendorf <[email protected]>
Signed-off-by: Matthias Wessendorf <[email protected]>
Signed-off-by: Matthias Wessendorf <[email protected]>
Signed-off-by: Matthias Wessendorf <[email protected]>
Signed-off-by: Matthias Wessendorf <[email protected]>
pierDipi
reviewed
Mar 6, 2025
| }() | ||
|
|
||
| } else { | ||
| cancelFunc, cancelExists := ctx.Value("cancelFunc").(context.CancelFunc) |
pierDipi
reviewed
Mar 6, 2025
Comment on lines
+90
to
+94
| factory.Start(ctx.Done()) | ||
|
|
||
| if !cache.WaitForCacheSync(ctx.Done(), cmCertificateInformer.Informer().HasSynced) { | ||
| logging.FromContext(ctx).Error("Failed to sync Cert Manager Certificate informer") | ||
| } |
There was a problem hiding this comment.
this ctx will only complete when the pod exits, so this might leak informers watchers/connections, I think we need a "sub-context" for only this factory start/stop
There was a problem hiding this comment.
diff --git a/pkg/certificates/certificate.go b/pkg/certificates/certificate.go
index 67f7aa308..cb97e58be 100644
--- a/pkg/certificates/certificate.go
+++ b/pkg/certificates/certificate.go
@@ -17,16 +17,96 @@ limitations under the License.
package certificates
import (
+ "context"
"fmt"
+ "sync"
+ "sync/atomic"
"time"
cmv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
cmmeta "github.com/cert-manager/cert-manager/pkg/apis/meta/v1"
- "knative.dev/pkg/kmeta"
-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ "k8s.io/client-go/tools/cache"
+ "knative.dev/eventing/pkg/apis/feature"
+ cmclient "knative.dev/eventing/pkg/client/certmanager/clientset/versioned"
+ cminformers "knative.dev/eventing/pkg/client/certmanager/informers/externalversions"
+ cmlistersv1 "knative.dev/eventing/pkg/client/certmanager/listers/certmanager/v1"
+ "knative.dev/pkg/controller"
+ "knative.dev/pkg/injection"
+ "knative.dev/pkg/kmeta"
+ "knative.dev/pkg/logging"
)
+type DynamicCertificatesInformer struct {
+ cancel atomic.Pointer[context.CancelFunc]
+ lister atomic.Pointer[cmlistersv1.CertificateLister]
+ mu sync.Mutex
+}
+
+func NewDynamicCertificatesInformer() *DynamicCertificatesInformer {
+ return &DynamicCertificatesInformer{
+ cancel: atomic.Pointer[context.CancelFunc]{},
+ lister: atomic.Pointer[cmlistersv1.CertificateLister]{},
+ mu: sync.Mutex{},
+ }
+}
+
+func (df *DynamicCertificatesInformer) Reconcile(ctx context.Context, features feature.Flags, handler cache.ResourceEventHandler) error {
+ df.mu.Lock()
+ defer df.mu.Unlock()
+ if !features.IsPermissiveTransportEncryption() && !features.IsStrictTransportEncryption() {
+ return df.stop(ctx)
+ }
+ if df.cancel.Load() != nil {
+ return nil
+ }
+ ctx, cancel := context.WithCancel(ctx)
+
+ client := cmclient.NewForConfigOrDie(injection.GetConfig(ctx))
+
+ factory := cminformers.NewSharedInformerFactoryWithOptions(
+ client,
+ controller.DefaultResyncPeriod,
+ cminformers.WithTweakListOptions(func(options *metav1.ListOptions) {
+ options.LabelSelector = "app.kubernetes.io/component=knative-eventing"
+ }),
+ )
+ informer := factory.Certmanager().V1().Certificates()
+ informer.Informer().AddEventHandler(handler)
+
+ factory.Start(ctx.Done())
+ if !cache.WaitForCacheSync(ctx.Done(), informer.Informer().HasSynced) {
+ defer cancel()
+ return fmt.Errorf("failed to sync cert-manager Certificate informer")
+ }
+
+ lister := informer.Lister()
+ df.lister.Store(&lister)
+ df.cancel.Store(&cancel) // Cancel is always set as last field since it's used as a "guard".
+
+ return nil
+}
+
+func (df *DynamicCertificatesInformer) stop(ctx context.Context) error {
+ cancel := df.cancel.Load()
+ if cancel == nil {
+ logging.FromContext(ctx).Debugw("Certificate informer has not been started, nothing to stop")
+ return nil
+ }
+
+ (*cancel)()
+ df.lister.Store(nil)
+ df.cancel.Store(nil) // Cancel is always set as last field since it's used as a "guard".
+ return nil
+}
+
+func (df *DynamicCertificatesInformer) Lister() *atomic.Pointer[cmlistersv1.CertificateLister] {
+ df.mu.Lock()
+ defer df.mu.Unlock()
+
+ return &df.lister
+}
+
func CertificateName(objName string) string {
return kmeta.ChildName(objName, "-server-tls")
}
diff --git a/pkg/reconciler/integration/sink/controller.go b/pkg/reconciler/integration/sink/controller.go
index dcc181adc..3cdbab854 100644
--- a/pkg/reconciler/integration/sink/controller.go
+++ b/pkg/reconciler/integration/sink/controller.go
@@ -19,19 +19,18 @@ package sink
import (
"context"
- v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ "go.uber.org/zap"
"k8s.io/client-go/tools/cache"
- certmanagerclient "knative.dev/eventing/pkg/client/certmanager/clientset/versioned"
- certmanagerinformers "knative.dev/eventing/pkg/client/certmanager/informers/externalversions"
- "knative.dev/pkg/injection"
-
"knative.dev/eventing/pkg/apis/feature"
- v1alpha1 "knative.dev/eventing/pkg/apis/sinks/v1alpha1"
+ "knative.dev/eventing/pkg/apis/sinks/v1alpha1"
"knative.dev/eventing/pkg/auth"
+ "knative.dev/eventing/pkg/certificates"
+ cmclient "knative.dev/eventing/pkg/client/certmanager/clientset/versioned"
"knative.dev/eventing/pkg/client/injection/informers/eventing/v1alpha1/eventpolicy"
"knative.dev/eventing/pkg/client/injection/informers/sinks/v1alpha1/integrationsink"
deploymentinformer "knative.dev/pkg/client/injection/kube/informers/apps/v1/deployment"
"knative.dev/pkg/client/injection/kube/informers/core/v1/service"
+ "knative.dev/pkg/injection"
pkgreconciler "knative.dev/pkg/reconciler"
integrationsinkreconciler "knative.dev/eventing/pkg/client/injection/reconciler/sinks/v1alpha1/integrationsink"
@@ -52,12 +51,15 @@ func NewController(
deploymentInformer := deploymentinformer.Get(ctx)
serviceInformer := service.Get(ctx)
+ dynamicCertificateInformer := certificates.NewDynamicCertificatesInformer()
r := &Reconciler{
- kubeClientSet: kubeclient.Get(ctx),
- deploymentLister: deploymentInformer.Lister(),
- serviceLister: serviceInformer.Lister(),
- secretLister: secretInformer.Lister(),
- eventPolicyLister: eventPolicyInformer.Lister(),
+ secretLister: secretInformer.Lister(),
+ eventPolicyLister: eventPolicyInformer.Lister(),
+ kubeClientSet: kubeclient.Get(ctx),
+ deploymentLister: deploymentInformer.Lister(),
+ serviceLister: serviceInformer.Lister(),
+ cmCertificateLister: *dynamicCertificateInformer.Lister(),
+ certManagerClient: cmclient.NewForConfigOrDie(injection.GetConfig(ctx)),
}
logging.FromContext(ctx).Info("Creating IntegrationSink controller")
@@ -66,40 +68,15 @@ func NewController(
featureStore := feature.NewStore(logging.FromContext(ctx).Named("feature-config-store"), func(name string, value interface{}) {
if globalResync != nil {
- globalResync(nil)
- }
- if features, ok := value.(feature.Flags); ok {
- // we assume that Cert-Manager is installed in the cluster if the feature flag is enabled
- if features.IsPermissiveTransportEncryption() || features.IsStrictTransportEncryption() {
-
- go func() { // Start and register informer
- certManagerClient := certmanagerclient.NewForConfigOrDie(injection.GetConfig(ctx))
- factory := certmanagerinformers.NewSharedInformerFactoryWithOptions(
- certManagerClient,
- controller.DefaultResyncPeriod,
- certmanagerinformers.WithTweakListOptions(func(options *v1.ListOptions) {
- options.LabelSelector = "app.kubernetes.io/component=knative-eventing"
- }),
- )
- cmCertificateInformer := factory.Certmanager().V1().Certificates()
- r.cmCertificateLister = cmCertificateInformer.Lister()
- r.certManagerClient = certManagerClient
-
- cmCertificateInformer.Informer().AddEventHandler(controller.HandleAll(globalResync))
- factory.Start(ctx.Done())
-
- if !cache.WaitForCacheSync(ctx.Done(), cmCertificateInformer.Informer().HasSynced) {
- logging.FromContext(ctx).Error("Failed to sync Cert Manager Certificate informer")
- }
- }()
-
- } else {
- cancelFunc, cancelExists := ctx.Value("cancelFunc").(context.CancelFunc)
- if cancelExists {
- go cancelFunc()
+ if features, ok := value.(feature.Flags); ok {
+ // we assume that Cert-Manager is installed in the cluster if the feature flag is enabled
+ if err := dynamicCertificateInformer.Reconcile(ctx, features, controller.HandleAll(globalResync)); err != nil {
+ logging.FromContext(ctx).Errorw("Failed to start certificates dynamic factory", zap.Error(err))
}
}
+
+ globalResync(nil)
}
})
featureStore.WatchConfigs(cmw)
diff --git a/pkg/reconciler/integration/sink/integrationsink.go b/pkg/reconciler/integration/sink/integrationsink.go
index 0f5a338bc..8e7f77b44 100644
--- a/pkg/reconciler/integration/sink/integrationsink.go
+++ b/pkg/reconciler/integration/sink/integrationsink.go
@@ -19,8 +19,10 @@ package sink
import (
"context"
"fmt"
+ "sync/atomic"
"knative.dev/eventing/pkg/certificates"
+ certmanagerlisters "knative.dev/eventing/pkg/client/certmanager/listers/certmanager/v1"
cmv1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
@@ -47,8 +49,6 @@ import (
certmanagerclientset "knative.dev/eventing/pkg/client/certmanager/clientset/versioned"
- certmanagerlisters "knative.dev/eventing/pkg/client/certmanager/listers/certmanager/v1"
-
"knative.dev/eventing/pkg/eventingtls"
duckv1 "knative.dev/pkg/apis/duck/v1"
"knative.dev/pkg/controller"
@@ -74,7 +74,7 @@ type Reconciler struct {
deploymentLister appsv1listers.DeploymentLister
serviceLister corev1listers.ServiceLister
- cmCertificateLister certmanagerlisters.CertificateLister
+ cmCertificateLister atomic.Pointer[certmanagerlisters.CertificateLister]
certManagerClient certmanagerclientset.Interface
}
@@ -174,7 +174,12 @@ func (r *Reconciler) reconcileCMCertificate(ctx context.Context, sink *sinks.Int
expected := certificates.MakeCertificate(sink, sink.Name)
- cert, err := r.cmCertificateLister.Certificates(sink.Namespace).Get(expected.Name)
+ lister := r.cmCertificateLister.Load()
+ if lister == nil {
+ return nil, fmt.Errorf("no cer-manager certificate lister created yet, this should never happen")
+ }
+
+ cert, err := (*lister).Certificates(sink.Namespace).Get(expected.Name)
if apierrors.IsNotFound(err) {
cert, err := r.certManagerClient.CertmanagerV1().Certificates(sink.Namespace).Create(ctx, expected, metav1.CreateOptions{})
if err != nil {Something like this I guess
There was a problem hiding this comment.
hitting this:
logging.FromContext(ctx).Debugw("Certificate informer has not been started, nothing to stop")
than later it is stuck here:
return nil, fmt.Errorf("no cer-manager certificate lister created yet, this should never happen")
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8509 +/- ##
==========================================
- Coverage 64.26% 64.19% -0.08%
==========================================
Files 397 397
Lines 24337 24354 +17
==========================================
- Hits 15641 15634 -7
- Misses 7858 7880 +22
- Partials 838 840 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
We could merge my current and work on proper fix in separate PR... as an option. |
|
/lgtm |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #
Proposed Changes
Pre-review Checklist
Release Note
Docs