Skip to content

Commit

Permalink
NETOBSERV-963, revert most of cert watching (#315)
Browse files Browse the repository at this point in the history
Reverting most of certificate watching (NETOBSERV-684) as it generates a
lot of pods restart.

We did not necessarily have to do this certificate watching as
CM/secrets are updated within volumes.

We might however monitor carefully if new (or old) problems arise,
potentially due to the kubelet sync delay for updating volumes

And also make sure certificates aren't cached in our different workloads
  • Loading branch information
jotak authored Apr 3, 2023
1 parent ac32da4 commit bce2e23
Show file tree
Hide file tree
Showing 16 changed files with 83 additions and 150 deletions.
9 changes: 3 additions & 6 deletions controllers/consoleplugin/consoleplugin_objects.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import (
flowslatest "github.com/netobserv/network-observability-operator/api/v1beta1"
"github.com/netobserv/network-observability-operator/controllers/constants"
"github.com/netobserv/network-observability-operator/pkg/helper"
"github.com/netobserv/network-observability-operator/pkg/watchers"
)

const secretName = "console-serving-cert"
Expand All @@ -39,10 +38,9 @@ type builder struct {
selector map[string]string
desired *flowslatest.FlowCollectorSpec
imageName string
cWatcher *watchers.CertificatesWatcher
}

func newBuilder(ns, imageName string, desired *flowslatest.FlowCollectorSpec, cWatcher *watchers.CertificatesWatcher) builder {
func newBuilder(ns, imageName string, desired *flowslatest.FlowCollectorSpec) builder {
version := helper.ExtractVersion(imageName)
return builder{
namespace: ns,
Expand All @@ -55,7 +53,6 @@ func newBuilder(ns, imageName string, desired *flowslatest.FlowCollectorSpec, cW
},
desired: desired,
imageName: imageName,
cWatcher: cWatcher,
}
}

Expand Down Expand Up @@ -236,12 +233,12 @@ func (b *builder) podTemplate(cmDigest string) *corev1.PodTemplateSpec {

args := buildArgs(b.desired)
if b.desired != nil && b.desired.Loki.TLS.Enable && !b.desired.Loki.TLS.InsecureSkipVerify {
volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &b.desired.Loki.TLS, lokiCerts, b.cWatcher)
volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &b.desired.Loki.TLS, lokiCerts)
}

statusTLS := helper.GetLokiStatusTLS(&b.desired.Loki)
if b.desired != nil && statusTLS.Enable && !statusTLS.InsecureSkipVerify {
volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &statusTLS, lokiStatusCerts, b.cWatcher)
volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &statusTLS, lokiStatusCerts)
}

if helper.LokiUseHostToken(&b.desired.Loki) {
Expand Down
6 changes: 1 addition & 5 deletions controllers/consoleplugin/consoleplugin_reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ func (r *CPReconciler) Reconcile(ctx context.Context, desired *flowslatest.FlowC
}

// Create object builder
builder := newBuilder(ns, r.image, &desired.Spec, r.CertWatcher)
builder := newBuilder(ns, r.image, &desired.Spec)

if err := r.reconcilePermissions(ctx, &builder); err != nil {
return err
Expand Down Expand Up @@ -196,10 +196,6 @@ func (r *CPReconciler) reconcileDeployment(ctx context.Context, builder builder,
defer report.LogIfNeeded(ctx)

newDepl := builder.deployment(cmDigest)
// Annotate pod with certificate reference so that it is reloaded if modified
if err := r.CertWatcher.AnnotatePod(ctx, r.Client, &newDepl.Spec.Template, lokiCerts, lokiStatusCerts); err != nil {
return err
}
if !r.nobjMngr.Exists(r.owned.deployment) {
if err := r.CreateOwned(ctx, newDepl); err != nil {
return err
Expand Down
20 changes: 9 additions & 11 deletions controllers/consoleplugin/consoleplugin_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
flowslatest "github.com/netobserv/network-observability-operator/api/v1beta1"
"github.com/netobserv/network-observability-operator/controllers/constants"
"github.com/netobserv/network-observability-operator/pkg/helper"
"github.com/netobserv/network-observability-operator/pkg/watchers"

promConfig "github.com/prometheus/common/config"
)
Expand All @@ -28,7 +27,6 @@ var testResources = corev1.ResourceRequirements{
corev1.ResourceMemory: resource.MustParse("512Mi"),
},
}
var certWatcher = watchers.NewCertificatesWatcher()

func getPluginConfig() flowslatest.FlowCollectorConsolePlugin {
return flowslatest.FlowCollectorConsolePlugin{
Expand Down Expand Up @@ -111,7 +109,7 @@ func TestContainerUpdateCheck(t *testing.T) {
plugin := getPluginConfig()
loki := flowslatest.FlowCollectorLoki{URL: "http://loki:3100/", TenantID: "netobserv"}
spec := flowslatest.FlowCollectorSpec{ConsolePlugin: plugin, Loki: loki}
builder := newBuilder(testNamespace, testImage, &spec, &certWatcher)
builder := newBuilder(testNamespace, testImage, &spec)
old := builder.deployment("digest")
new := builder.deployment("digest")
report := helper.NewChangeReport("")
Expand Down Expand Up @@ -163,7 +161,7 @@ func TestContainerUpdateCheck(t *testing.T) {
},
}}
spec = flowslatest.FlowCollectorSpec{ConsolePlugin: plugin, Loki: loki}
builder = newBuilder(testNamespace, testImage, &spec, &certWatcher)
builder = newBuilder(testNamespace, testImage, &spec)
new = builder.deployment("digest")
report = helper.NewChangeReport("")
assert.True(helper.PodChanged(&old.Spec.Template, &new.Spec.Template, constants.PluginName, &report))
Expand All @@ -173,7 +171,7 @@ func TestContainerUpdateCheck(t *testing.T) {
//new loki cert name
loki.TLS.CACert.Name = "cm-name-2"
spec = flowslatest.FlowCollectorSpec{ConsolePlugin: plugin, Loki: loki}
builder = newBuilder(testNamespace, testImage, &spec, &certWatcher)
builder = newBuilder(testNamespace, testImage, &spec)
new = builder.deployment("digest")
report = helper.NewChangeReport("")
assert.True(helper.PodChanged(&old.Spec.Template, &new.Spec.Template, constants.PluginName, &report))
Expand All @@ -183,7 +181,7 @@ func TestContainerUpdateCheck(t *testing.T) {
//test again no change
loki.TLS.CACert.Name = "cm-name-2"
spec = flowslatest.FlowCollectorSpec{ConsolePlugin: plugin, Loki: loki}
builder = newBuilder(testNamespace, testImage, &spec, &certWatcher)
builder = newBuilder(testNamespace, testImage, &spec)
new = builder.deployment("digest")
report = helper.NewChangeReport("")
assert.False(helper.PodChanged(&old.Spec.Template, &new.Spec.Template, constants.PluginName, &report))
Expand All @@ -195,7 +193,7 @@ func TestContainerUpdateCheck(t *testing.T) {
loki.StatusTLS.Enable = true

spec = flowslatest.FlowCollectorSpec{ConsolePlugin: plugin, Loki: loki}
builder = newBuilder(testNamespace, testImage, &spec, &certWatcher)
builder = newBuilder(testNamespace, testImage, &spec)
new = builder.deployment("digest")
report = helper.NewChangeReport("")
assert.True(helper.PodChanged(&old.Spec.Template, &new.Spec.Template, constants.PluginName, &report))
Expand All @@ -210,7 +208,7 @@ func TestContainerUpdateCheck(t *testing.T) {
}

spec = flowslatest.FlowCollectorSpec{ConsolePlugin: plugin, Loki: loki}
builder = newBuilder(testNamespace, testImage, &spec, &certWatcher)
builder = newBuilder(testNamespace, testImage, &spec)
new = builder.deployment("digest")
report = helper.NewChangeReport("")
assert.True(helper.PodChanged(&old.Spec.Template, &new.Spec.Template, constants.PluginName, &report))
Expand All @@ -226,7 +224,7 @@ func TestContainerUpdateCheck(t *testing.T) {
}

spec = flowslatest.FlowCollectorSpec{ConsolePlugin: plugin, Loki: loki}
builder = newBuilder(testNamespace, testImage, &spec, &certWatcher)
builder = newBuilder(testNamespace, testImage, &spec)
new = builder.deployment("digest")
report = helper.NewChangeReport("")
assert.True(helper.PodChanged(&old.Spec.Template, &new.Spec.Template, constants.PluginName, &report))
Expand Down Expand Up @@ -264,7 +262,7 @@ func TestBuiltService(t *testing.T) {
plugin := getPluginConfig()
loki := flowslatest.FlowCollectorLoki{URL: "http://foo:1234"}
spec := flowslatest.FlowCollectorSpec{ConsolePlugin: plugin, Loki: loki}
builder := newBuilder(testNamespace, testImage, &spec, &certWatcher)
builder := newBuilder(testNamespace, testImage, &spec)
newService := builder.service(nil)
report := helper.NewChangeReport("")
assert.Equal(serviceNeedsUpdate(newService, &plugin, &report), false)
Expand All @@ -277,7 +275,7 @@ func TestLabels(t *testing.T) {
plugin := getPluginConfig()
loki := flowslatest.FlowCollectorLoki{URL: "http://foo:1234"}
spec := flowslatest.FlowCollectorSpec{ConsolePlugin: plugin, Loki: loki}
builder := newBuilder(testNamespace, testImage, &spec, &certWatcher)
builder := newBuilder(testNamespace, testImage, &spec)

// Deployment
depl := builder.deployment("digest")
Expand Down
7 changes: 1 addition & 6 deletions controllers/ebpf/agent_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,11 +131,6 @@ func (c *AgentController) Reconcile(
}
desired := c.desired(target)

// Annotate pod with certificate reference so that it is reloaded if modified
if err := c.client.CertWatcher.AnnotatePod(ctx, c.client, &desired.Spec.Template, kafkaCerts); err != nil {
return err
}

switch c.requiredAction(current, desired) {
case actionCreate:
rlog.Info("action: create agent")
Expand Down Expand Up @@ -175,7 +170,7 @@ func (c *AgentController) desired(coll *flowslatest.FlowCollector) *v1.DaemonSet
if helper.UseKafka(&coll.Spec) && coll.Spec.Kafka.TLS.Enable {
// NOTE: secrets need to be copied from the base netobserv namespace to the privileged one.
// This operation must currently be performed manually (run "make fix-ebpf-kafka-tls"). It could be automated here.
volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &coll.Spec.Kafka.TLS, kafkaCerts, c.client.CertWatcher)
volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &coll.Spec.Kafka.TLS, kafkaCerts)
}

return &v1.DaemonSet{
Expand Down
3 changes: 1 addition & 2 deletions controllers/flowcollector_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -368,8 +368,7 @@ func (r *FlowCollectorReconciler) finalize(ctx context.Context, desired *flowsla

func (r *FlowCollectorReconciler) newClientHelper(desired *flowslatest.FlowCollector) reconcilers.ClientHelper {
return reconcilers.ClientHelper{
CertWatcher: r.certWatcher,
Client: r.Client,
Client: r.Client,
SetControllerReference: func(obj client.Object) error {
return ctrl.SetControllerReference(desired, obj, r.Scheme)
},
Expand Down
37 changes: 5 additions & 32 deletions controllers/flowcollector_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -593,9 +593,8 @@ func flowCollectorControllerSpecs() {
})
})

Context("Using and watching certificates", func() {
Context("Using certificates", func() {
flpDS := appsv1.DaemonSet{}
var certStamp1, certStamp2 string
It("Should update Loki to use TLS", func() {
// Create CM certificate
Expect(k8sClient.Create(ctx, &v1.ConfigMap{
Expand All @@ -622,33 +621,8 @@ func flowCollectorControllerSpecs() {
if err := k8sClient.Get(ctx, flpKey1, &flpDS); err != nil {
return err
}
certStamp1 = flpDS.Spec.Template.Annotations["flows.netobserv.io/cert-loki-certs-ca"]
return certStamp1
}, timeout, interval).Should(Not(BeEmpty()))
Expect(flpDS.Spec.Template.Spec.Volumes).To(HaveLen(2))
Expect(flpDS.Spec.Template.Spec.Volumes[0].Name).To(Equal("config-volume"))
Expect(flpDS.Spec.Template.Spec.Volumes[1].Name).To(Equal("loki-certs-ca"))
})

It("Should watch certificate update", func() {
By("Updating certificate")
Expect(k8sClient.Update(ctx, &v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "loki-ca",
Namespace: operatorNamespace,
},
Data: map[string]string{"test": "test"},
})).Should(Succeed())

Eventually(func() interface{} {
if err := k8sClient.Get(ctx, flpKey1, &flpDS); err != nil {
return err
}
certStamp2 = flpDS.Spec.Template.Annotations["flows.netobserv.io/cert-loki-certs-ca"]
return certStamp2
}, timeout, interval).Should(Not(Equal(certStamp1)))
Expect(certStamp2).To(Not(BeEmpty()))
Expect(flpDS.Spec.Template.Spec.Volumes).To(HaveLen(2))
return flpDS.Spec.Template.Spec.Volumes
}, timeout, interval).Should(HaveLen(2))
Expect(flpDS.Spec.Template.Spec.Volumes[0].Name).To(Equal("config-volume"))
Expect(flpDS.Spec.Template.Spec.Volumes[1].Name).To(Equal("loki-certs-ca"))
})
Expand All @@ -663,9 +637,8 @@ func flowCollectorControllerSpecs() {
if err := k8sClient.Get(ctx, flpKey1, &flpDS); err != nil {
return err
}
return flpDS.Spec.Template.Annotations
}, timeout, interval).Should(Not(HaveKey("flows.netobserv.io/cert-loki-certs-ca")))
Expect(flpDS.Spec.Template.Spec.Volumes).To(HaveLen(1))
return flpDS.Spec.Template.Spec.Volumes
}, timeout, interval).Should(HaveLen(1))
Expect(flpDS.Spec.Template.Spec.Volumes[0].Name).To(Equal("config-volume"))
})
})
Expand Down
11 changes: 4 additions & 7 deletions controllers/flowlogspipeline/flp_common_objects.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
"github.com/netobserv/network-observability-operator/controllers/constants"
"github.com/netobserv/network-observability-operator/pkg/filters"
"github.com/netobserv/network-observability-operator/pkg/helper"
"github.com/netobserv/network-observability-operator/pkg/watchers"
)

const (
Expand Down Expand Up @@ -80,10 +79,9 @@ type builder struct {
confKind ConfKind
useOpenShiftSCC bool
image string
cWatcher *watchers.CertificatesWatcher
}

func newBuilder(ns, image string, desired *flowslatest.FlowCollectorSpec, ck ConfKind, useOpenShiftSCC bool, cWatcher *watchers.CertificatesWatcher) builder {
func newBuilder(ns, image string, desired *flowslatest.FlowCollectorSpec, ck ConfKind, useOpenShiftSCC bool) builder {
version := helper.ExtractVersion(image)
name := name(ck)
var promTLS flowslatest.CertificateReference
Expand Down Expand Up @@ -112,7 +110,6 @@ func newBuilder(ns, image string, desired *flowslatest.FlowCollectorSpec, ck Con
useOpenShiftSCC: useOpenShiftSCC,
promTLS: &promTLS,
image: image,
cWatcher: cWatcher,
}
}

Expand Down Expand Up @@ -185,20 +182,20 @@ func (b *builder) podTemplate(hasHostPort, hasLokiInterface, hostNetwork bool, c
}}

if helper.UseKafka(b.desired) && b.desired.Kafka.TLS.Enable {
volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &b.desired.Kafka.TLS, kafkaCerts, b.cWatcher)
volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &b.desired.Kafka.TLS, kafkaCerts)
}

if hasLokiInterface {
if b.desired.Loki.TLS.Enable && !b.desired.Loki.TLS.InsecureSkipVerify {
volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &b.desired.Loki.TLS, lokiCerts, b.cWatcher)
volumes, volumeMounts = helper.AppendCertVolumes(volumes, volumeMounts, &b.desired.Loki.TLS, lokiCerts)
}
if helper.LokiUseHostToken(&b.desired.Loki) || helper.LokiForwardUserToken(&b.desired.Loki) {
volumes, volumeMounts = helper.AppendTokenVolume(volumes, volumeMounts, lokiToken, constants.FLPName)
}
}

if b.desired.Processor.Metrics.Server.TLS.Type != flowslatest.ServerTLSDisabled {
volumes, volumeMounts = helper.AppendSingleCertVolumes(volumes, volumeMounts, b.promTLS, promCerts, b.cWatcher)
volumes, volumeMounts = helper.AppendSingleCertVolumes(volumes, volumeMounts, b.promTLS, promCerts)
}

var envs []corev1.EnvVar
Expand Down
5 changes: 2 additions & 3 deletions controllers/flowlogspipeline/flp_ingest_objects.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,14 @@ import (
"github.com/netobserv/flowlogs-pipeline/pkg/config"
flowslatest "github.com/netobserv/network-observability-operator/api/v1beta1"
"github.com/netobserv/network-observability-operator/pkg/helper"
"github.com/netobserv/network-observability-operator/pkg/watchers"
)

type ingestBuilder struct {
generic builder
}

func newIngestBuilder(ns, image string, desired *flowslatest.FlowCollectorSpec, useOpenShiftSCC bool, cWatcher *watchers.CertificatesWatcher) ingestBuilder {
gen := newBuilder(ns, image, desired, ConfKafkaIngester, useOpenShiftSCC, cWatcher)
func newIngestBuilder(ns, image string, desired *flowslatest.FlowCollectorSpec, useOpenShiftSCC bool) ingestBuilder {
gen := newBuilder(ns, image, desired, ConfKafkaIngester, useOpenShiftSCC)
return ingestBuilder{
generic: gen,
}
Expand Down
6 changes: 1 addition & 5 deletions controllers/flowlogspipeline/flp_ingest_reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func (r *flpIngesterReconciler) reconcile(ctx context.Context, desired *flowslat
return nil
}

builder := newIngestBuilder(r.nobjMngr.Namespace, r.image, &desired.Spec, r.useOpenShiftSCC, r.CertWatcher)
builder := newIngestBuilder(r.nobjMngr.Namespace, r.image, &desired.Spec, r.useOpenShiftSCC)
newCM, configDigest, err := builder.configMap()
if err != nil {
return err
Expand Down Expand Up @@ -147,10 +147,6 @@ func (r *flpIngesterReconciler) reconcileDaemonSet(ctx context.Context, desiredD
report := helper.NewChangeReport("FLP DaemonSet")
defer report.LogIfNeeded(ctx)

// Annotate pod with certificate reference so that it is reloaded if modified
if err := r.CertWatcher.AnnotatePod(ctx, r.Client, &desiredDS.Spec.Template, lokiCerts, kafkaCerts); err != nil {
return err
}
if !r.nobjMngr.Exists(r.owned.daemonSet) {
return r.CreateOwned(ctx, desiredDS)
} else if helper.PodChanged(&r.owned.daemonSet.Spec.Template, &desiredDS.Spec.Template, constants.FLPName, &report) {
Expand Down
5 changes: 2 additions & 3 deletions controllers/flowlogspipeline/flp_monolith_objects.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,14 @@ import (
"github.com/netobserv/flowlogs-pipeline/pkg/config"
flowslatest "github.com/netobserv/network-observability-operator/api/v1beta1"
"github.com/netobserv/network-observability-operator/pkg/helper"
"github.com/netobserv/network-observability-operator/pkg/watchers"
)

type monolithBuilder struct {
generic builder
}

func newMonolithBuilder(ns, image string, desired *flowslatest.FlowCollectorSpec, useOpenShiftSCC bool, cWatcher *watchers.CertificatesWatcher) monolithBuilder {
gen := newBuilder(ns, image, desired, ConfMonolith, useOpenShiftSCC, cWatcher)
func newMonolithBuilder(ns, image string, desired *flowslatest.FlowCollectorSpec, useOpenShiftSCC bool) monolithBuilder {
gen := newBuilder(ns, image, desired, ConfMonolith, useOpenShiftSCC)
return monolithBuilder{
generic: gen,
}
Expand Down
6 changes: 1 addition & 5 deletions controllers/flowlogspipeline/flp_monolith_reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ func (r *flpMonolithReconciler) reconcile(ctx context.Context, desired *flowslat
return nil
}

builder := newMonolithBuilder(r.nobjMngr.Namespace, r.image, &desired.Spec, r.useOpenShiftSCC, r.CertWatcher)
builder := newMonolithBuilder(r.nobjMngr.Namespace, r.image, &desired.Spec, r.useOpenShiftSCC)
newCM, configDigest, dbConfigMap, err := builder.configMap()
if err != nil {
return err
Expand Down Expand Up @@ -155,10 +155,6 @@ func (r *flpMonolithReconciler) reconcileDaemonSet(ctx context.Context, desiredD
report := helper.NewChangeReport("FLP DaemonSet")
defer report.LogIfNeeded(ctx)

// Annotate pod with certificate reference so that it is reloaded if modified
if err := r.CertWatcher.AnnotatePod(ctx, r.Client, &desiredDS.Spec.Template, lokiCerts, kafkaCerts); err != nil {
return err
}
if !r.nobjMngr.Exists(r.owned.daemonSet) {
return r.CreateOwned(ctx, desiredDS)
} else if helper.PodChanged(&r.owned.daemonSet.Spec.Template, &desiredDS.Spec.Template, constants.FLPName, &report) {
Expand Down
Loading

0 comments on commit bce2e23

Please sign in to comment.