-
Notifications
You must be signed in to change notification settings - Fork 600
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Integration of certmanager for IntegrationSink #8385
base: main
Are you sure you want to change the base?
Conversation
[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 |
a1209c6
to
3dd433f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8385 +/- ##
==========================================
- Coverage 64.22% 64.03% -0.19%
==========================================
Files 388 389 +1
Lines 23324 23413 +89
==========================================
+ Hits 14979 14993 +14
- Misses 7550 7624 +74
- Partials 795 796 +1 ☔ View full report in Codecov by Sentry. |
bcda118
to
80cb7ab
Compare
Basics for Cert-manager Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
…f TLS Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
… env vars Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
80cb7ab
to
d3306df
Compare
Signed-off-by: Matthias Wessendorf <mwessend@redhat.com>
featureStore = feature.NewStore(logging.FromContext(ctx).Named("feature-config-store"), func(name string, value interface{}) { | ||
featureFlags := value.(feature.Flags) | ||
if !featureFlags.IsDisabledTransportEncryption() && featureStore != nil { | ||
for _, inf := range []injection.InformerInjector{challenge.WithInformer, v1certificate.WithInformer, certificaterequest.WithInformer, clusterissuer.WithInformer, issuer.WithInformer} { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this does not work, when changing the feature
cfg afterwards. In the controller.go
of the integrationsink I am getting a nil
pointer... when accessing the cmCertificateLister
there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it doesn't, we could start / stop the informer manually without injection based on the value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could start / stop the informer manually without injection based on the value
you mean the "cert manager" informers, right? start / stop based on cfg change of the config-feature
CM?
Any pointer to similar code for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we start a "custom" configmap informer using the factory
eventing/pkg/adapter/v2/main.go
Lines 227 to 250 in b00da59
// Manually create a ConfigMap informer for the env.GetNamespace() namespace to have it | |
// optionally created when needed. | |
infFactory := informers.NewSharedInformerFactoryWithOptions( | |
kubeclient.Get(ctx), | |
controller.GetResyncPeriod(ctx), | |
informers.WithNamespace(env.GetNamespace()), | |
informers.WithTweakListOptions(func(options *metav1.ListOptions) { | |
options.LabelSelector = eventingtls.TrustBundleLabelSelector | |
}), | |
) | |
go func() { | |
<-ctx.Done() | |
infFactory.Shutdown() | |
}() | |
inf := infFactory.Core().V1().ConfigMaps() | |
_ = inf.Informer() // Actually create informer | |
trustBundleConfigMapLister = inf.Lister().ConfigMaps(env.GetNamespace()) | |
infFactory.Start(ctx.Done()) | |
_ = infFactory.WaitForCacheSync(ctx.Done()) |
the "stop" is based on the passed context and there we can use the usual "context / cancel" pair
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit of background info. We discussed a more dynamic approach in the past here. Serving requires for the controller pod to be restarted. In general making it dynamic here makes more sense as it reacts on some cm change and you can freely define your main stuff as in the adapter's main. For Serving once you disable encryption there will be downtime afaik and we wanted minimal changes back then to move with the internal encryption feature. cc @ReToCode if he has anything to add/correct me.
@matzew: The following tests failed, say
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. |
NOTE: this is not yet ready, and adds a raw support for cert-manager / TLS
Fixes #
Proposed Changes
Pre-review Checklist
Release Note
Docs