Skip to content
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

Ignore events that are outside of namespaces that the controller manages #5058

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
4f530ce
ignore events that are outside of namespaces that the controller mana…
naemono Nov 18, 2021
36b7e42
Add a test for ignoring unmanaged namespaces
naemono Nov 19, 2021
7039574
Fix newline issues
naemono Nov 19, 2021
518bf9e
Add helper method to cleanup the code a bit.
naemono Nov 19, 2021
0e7f728
Better name function
naemono Nov 19, 2021
d74e34b
Add predicate to additional missing CRD types.
naemono Nov 19, 2021
fb8459d
Ignore other objects outside managed namespaces in beats controller.
naemono Nov 22, 2021
a23d82b
Move predicates to it's own package to avoid import cycles
naemono Nov 22, 2021
5c2ec07
Clean up the code a bit by providing the predicates as a variadic arg…
naemono Nov 22, 2021
c4ad5f9
Adjust how garbage collection of secrets is handled on startup, takin…
naemono Nov 22, 2021
ddbc820
Prevent empty namespace "Orphan secrets garbage collection failed" er…
naemono Nov 22, 2021
78d8419
Adding some comments around corev1.Namespaceall
naemono Nov 22, 2021
ce19a37
Making license controller consistent with other packages
naemono Nov 22, 2021
a78898c
Fix garbage collection test
naemono Nov 23, 2021
b41c1bb
Also ignore generic events in unmanaged namespaces
naemono Dec 1, 2021
672067b
Cleanup predicate calling code per @barkbay suggestion.
naemono Dec 1, 2021
e7c9b23
Use IsNamespaceManaged in suggestion
naemono Dec 1, 2021
c161c41
remove predicate on associationinfo struct
naemono Dec 1, 2021
79d6cc8
remove redundant comments
naemono Dec 1, 2021
0517714
simplify GarbageCollectAllSoftOwnedOrphanSecrets per @barkbay comments
naemono Dec 2, 2021
d0c44e7
revert invalid change to Makefile
naemono Dec 2, 2021
50034ca
ensure managed namespace predicate is initialized prior to license in…
naemono Dec 2, 2021
f674256
Per Suggestion, simplify how predicates get used in Watches.
naemono Dec 3, 2021
1ab437f
remove the exported predicates.ManagedNamespacePredicate as it's unus…
naemono Dec 3, 2021
a73a871
Add some comments for newNamespaceAwareWatchersController
naemono Dec 3, 2021
8fe547f
Introduce a new function NewControllerWithOptions that allows setting…
naemono Dec 7, 2021
9ddb3cf
Actually use watchedNamespaces, not managedNamespaces
naemono Dec 9, 2021
62804ac
Do not append 'namespaceAll' to managed namespaces on initialization.
naemono Dec 13, 2021
9f83fff
ensure we do not append anything to the managed namespaces when it's …
naemono Dec 13, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 13 additions & 16 deletions cmd/manager/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -466,6 +466,7 @@ func startOperator(ctx context.Context) error {

// configure the manager cache based on the number of managed namespaces
managedNamespaces := viper.GetStringSlice(operator.NamespacesFlag)

switch {
case len(managedNamespaces) == 0:
log.Info("Operator configured to manage all namespaces")
Expand All @@ -478,12 +479,12 @@ func startOperator(ctx context.Context) error {
// The managed cache should always include the operator namespace so that we can work with operator-internal resources.
managedNamespaces = append(managedNamespaces, operatorNamespace)

opts.NewCache = cache.MultiNamespacedCacheBuilder(managedNamespaces)

// Add the empty namespace to allow watching cluster-scoped resources if storage class validation is enabled.
if viper.GetBool(operator.ValidateStorageClassFlag) {
managedNamespaces = append(managedNamespaces, "")
opts.NewCache = cache.MultiNamespacedCacheBuilder(append(managedNamespaces, ""))
}

opts.NewCache = cache.MultiNamespacedCacheBuilder(managedNamespaces)
}

// only expose prometheus metrics if provided a non-zero port
Expand Down Expand Up @@ -553,6 +554,7 @@ func startOperator(ctx context.Context) error {
Dialer: dialer,
ExposedNodeLabels: exposedNodeLabels,
IPFamily: ipFamily,
ManagedNamespaces: managedNamespaces,
OperatorNamespace: operatorNamespace,
OperatorInfo: operatorInfo,
CACertRotation: certificates.RotationParams{
Expand All @@ -570,7 +572,7 @@ func startOperator(ctx context.Context) error {
}

if viper.GetBool(operator.EnableWebhookFlag) {
setupWebhook(mgr, params.CertRotation, params.ValidateStorageClass, clientset, exposedNodeLabels)
setupWebhook(mgr, params, clientset)
naemono marked this conversation as resolved.
Show resolved Hide resolved
}

enforceRbacOnRefs := viper.GetBool(operator.EnforceRBACOnRefsFlag)
Expand Down Expand Up @@ -665,7 +667,7 @@ func asyncTasks(
// - association user secrets
garbageCollectUsers(cfg, managedNamespaces)
// - soft-owned secrets
garbageCollectSoftOwnedSecrets(mgr.GetClient())
garbageCollectSoftOwnedSecrets(mgr.GetClient(), managedNamespaces)
}

func chooseAndValidateIPFamily(ipFamilyStr string, ipFamilyDefault corev1.IPFamily) (corev1.IPFamily, error) {
Expand Down Expand Up @@ -766,7 +768,7 @@ func garbageCollectUsers(cfg *rest.Config, managedNamespaces []string) {
}
}

func garbageCollectSoftOwnedSecrets(k8sClient k8s.Client) {
func garbageCollectSoftOwnedSecrets(k8sClient k8s.Client, managedNamespaces []string) {
if err := reconciler.GarbageCollectAllSoftOwnedOrphanSecrets(k8sClient, map[string]client.Object{
esv1.Kind: &esv1.Elasticsearch{},
apmv1.Kind: &apmv1.ApmServer{},
Expand All @@ -775,19 +777,14 @@ func garbageCollectSoftOwnedSecrets(k8sClient k8s.Client) {
beatv1beta1.Kind: &beatv1beta1.Beat{},
agentv1alpha1.Kind: &agentv1alpha1.Agent{},
emsv1alpha1.Kind: &emsv1alpha1.ElasticMapsServer{},
}); err != nil {
}, managedNamespaces); err != nil {
log.Error(err, "Orphan secrets garbage collection failed, will be attempted again at next operator restart.")
return
}
log.Info("Orphan secrets garbage collection complete")
}

func setupWebhook(
mgr manager.Manager,
certRotation certificates.RotationParams,
validateStorageClass bool,
clientset kubernetes.Interface,
exposedNodeLabels esvalidation.NodeLabels) {
func setupWebhook(mgr manager.Manager, params operator.Parameters, clientset kubernetes.Interface) {
manageWebhookCerts := viper.GetBool(operator.ManageWebhookCertsFlag)
if manageWebhookCerts {
log.Info("Automatic management of the webhook certificates enabled")
Expand All @@ -796,7 +793,7 @@ func setupWebhook(
Name: viper.GetString(operator.WebhookNameFlag),
Namespace: viper.GetString(operator.OperatorNamespaceFlag),
SecretName: viper.GetString(operator.WebhookSecretFlag),
Rotation: certRotation,
Rotation: params.CertRotation,
}

// retrieve the current webhook configuration interface
Expand All @@ -812,7 +809,7 @@ func setupWebhook(
os.Exit(1)
}

if err := webhook.Add(mgr, webhookParams, clientset, wh); err != nil {
if err := webhook.Add(mgr, webhookParams, clientset, wh, params); err != nil {
log.Error(err, "unable to create controller", "controller", webhook.ControllerName)
os.Exit(1)
}
Expand Down Expand Up @@ -842,7 +839,7 @@ func setupWebhook(
}

// esv1 validating webhook is wired up differently, in order to access the k8s client
esvalidation.RegisterWebhook(mgr, validateStorageClass, exposedNodeLabels)
esvalidation.RegisterWebhook(mgr, params.ValidateStorageClass, params.ExposedNodeLabels)

// wait for the secret to be populated in the local filesystem before returning
interval := time.Second * 1
Expand Down
2 changes: 1 addition & 1 deletion cmd/manager/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ func Test_garbageCollectSoftOwnedSecrets(t *testing.T) {
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
c := k8s.NewFakeClient(tt.runtimeObjs...)
garbageCollectSoftOwnedSecrets(c)
garbageCollectSoftOwnedSecrets(c, []string{})
tt.assert(c, t)
})
}
Expand Down
3 changes: 1 addition & 2 deletions pkg/controller/association/controller/agent_fleetserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,7 @@ func AddAgentFleetServer(mgr manager.Manager, accessReviewer rbac.AccessReviewer
AssociationConfAnnotationNameBase: commonv1.FleetServerConfigAnnotationNameBase,
AssociationResourceNameLabelName: agent.NameLabelName,
AssociationResourceNamespaceLabelName: agent.NamespaceLabelName,

ElasticsearchUserCreation: nil,
ElasticsearchUserCreation: nil,
})
}

Expand Down
1 change: 0 additions & 1 deletion pkg/controller/association/controller/agent_kibana.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ func AddAgentKibana(mgr manager.Manager, accessReviewer rbac.AccessReviewer, par
AssociationConfAnnotationNameBase: commonv1.KibanaConfigAnnotationNameBase,
AssociationResourceNameLabelName: kibana.KibanaNameLabelName,
AssociationResourceNamespaceLabelName: kibana.KibanaNamespaceLabelName,

ElasticsearchUserCreation: &association.ElasticsearchUserCreation{
ElasticsearchRef: getElasticsearchFromKibana,
UserSecretSuffix: "agent-kb-user",
Expand Down
1 change: 0 additions & 1 deletion pkg/controller/association/controller/apm_es.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ func AddApmES(mgr manager.Manager, accessReviewer rbac.AccessReviewer, params op
AssociationConfAnnotationNameBase: commonv1.ElasticsearchConfigAnnotationNameBase,
AssociationResourceNameLabelName: eslabel.ClusterNameLabelName,
AssociationResourceNamespaceLabelName: eslabel.ClusterNamespaceLabelName,

ElasticsearchUserCreation: &association.ElasticsearchUserCreation{
ElasticsearchRef: func(c k8s.Client, association commonv1.Association) (bool, commonv1.ObjectSelector, error) {
return true, association.AssociationRef(), nil
Expand Down
1 change: 0 additions & 1 deletion pkg/controller/association/controller/apm_kibana.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ func AddApmKibana(mgr manager.Manager, accessReviewer rbac.AccessReviewer, param
AssociationConfAnnotationNameBase: commonv1.KibanaConfigAnnotationNameBase,
AssociationResourceNameLabelName: kibana.KibanaNameLabelName,
AssociationResourceNamespaceLabelName: kibana.KibanaNamespaceLabelName,

ElasticsearchUserCreation: &association.ElasticsearchUserCreation{
ElasticsearchRef: getElasticsearchFromKibana,
UserSecretSuffix: "apm-kb-user",
Expand Down
1 change: 0 additions & 1 deletion pkg/controller/association/controller/beat_es.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ func AddBeatES(mgr manager.Manager, accessReviewer rbac.AccessReviewer, params o
AssociationConfAnnotationNameBase: commonv1.ElasticsearchConfigAnnotationNameBase,
AssociationResourceNameLabelName: eslabel.ClusterNameLabelName,
AssociationResourceNamespaceLabelName: eslabel.ClusterNamespaceLabelName,

ElasticsearchUserCreation: &association.ElasticsearchUserCreation{
ElasticsearchRef: func(c k8s.Client, association commonv1.Association) (bool, commonv1.ObjectSelector, error) {
return true, association.AssociationRef(), nil
Expand Down
1 change: 0 additions & 1 deletion pkg/controller/association/controller/beat_kibana.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ func AddBeatKibana(mgr manager.Manager, accessReviewer rbac.AccessReviewer, para
AssociationConfAnnotationNameBase: commonv1.KibanaConfigAnnotationNameBase,
AssociationResourceNameLabelName: kibana.KibanaNameLabelName,
AssociationResourceNamespaceLabelName: kibana.KibanaNamespaceLabelName,

ElasticsearchUserCreation: &association.ElasticsearchUserCreation{
ElasticsearchRef: getElasticsearchFromKibana,
UserSecretSuffix: "beat-kb-user",
Expand Down
1 change: 0 additions & 1 deletion pkg/controller/association/controller/ent_es.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ func AddEntES(mgr manager.Manager, accessReviewer rbac.AccessReviewer, params op
AssociationConfAnnotationNameBase: commonv1.ElasticsearchConfigAnnotationNameBase,
AssociationResourceNameLabelName: eslabel.ClusterNameLabelName,
AssociationResourceNamespaceLabelName: eslabel.ClusterNamespaceLabelName,

ElasticsearchUserCreation: &association.ElasticsearchUserCreation{
ElasticsearchRef: func(c k8s.Client, association commonv1.Association) (bool, commonv1.ObjectSelector, error) {
return true, association.AssociationRef(), nil
Expand Down
1 change: 0 additions & 1 deletion pkg/controller/association/controller/es_monitoring.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ func esMonitoringAssociationInfo() association.AssociationInfo {
AssociationConfAnnotationNameBase: commonv1.ElasticsearchConfigAnnotationNameBase,
AssociationResourceNameLabelName: eslabel.ClusterNameLabelName,
AssociationResourceNamespaceLabelName: eslabel.ClusterNamespaceLabelName,

ElasticsearchUserCreation: &association.ElasticsearchUserCreation{
ElasticsearchRef: func(c k8s.Client, association commonv1.Association) (bool, commonv1.ObjectSelector, error) {
return true, association.AssociationRef(), nil
Expand Down
1 change: 0 additions & 1 deletion pkg/controller/association/controller/kb_monitoring.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ func AddKbMonitoring(mgr manager.Manager, accessReviewer rbac.AccessReviewer, pa
AssociationConfAnnotationNameBase: commonv1.ElasticsearchConfigAnnotationNameBase,
AssociationResourceNameLabelName: eslabel.ClusterNameLabelName,
AssociationResourceNamespaceLabelName: eslabel.ClusterNamespaceLabelName,

ElasticsearchUserCreation: &association.ElasticsearchUserCreation{
ElasticsearchRef: func(c k8s.Client, association commonv1.Association) (bool, commonv1.ObjectSelector, error) {
return true, association.AssociationRef(), nil
Expand Down
1 change: 0 additions & 1 deletion pkg/controller/association/controller/kibana_es.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ func AddKibanaES(mgr manager.Manager, accessReviewer rbac.AccessReviewer, params
AssociationConfAnnotationNameBase: commonv1.ElasticsearchConfigAnnotationNameBase,
AssociationResourceNameLabelName: eslabel.ClusterNameLabelName,
AssociationResourceNamespaceLabelName: eslabel.ClusterNamespaceLabelName,

ElasticsearchUserCreation: &association.ElasticsearchUserCreation{
ElasticsearchRef: func(c k8s.Client, association commonv1.Association) (bool, commonv1.ObjectSelector, error) {
return true, association.AssociationRef(), nil
Expand Down
1 change: 0 additions & 1 deletion pkg/controller/association/controller/maps_es.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,6 @@ func AddMapsES(mgr manager.Manager, accessReviewer rbac.AccessReviewer, params o
AssociationConfAnnotationNameBase: commonv1.ElasticsearchConfigAnnotationNameBase,
AssociationResourceNameLabelName: eslabel.ClusterNameLabelName,
AssociationResourceNamespaceLabelName: eslabel.ClusterNamespaceLabelName,

ElasticsearchUserCreation: &association.ElasticsearchUserCreation{
ElasticsearchRef: func(c k8s.Client, association commonv1.Association) (bool, commonv1.ObjectSelector, error) {
return true, association.AssociationRef(), nil
Expand Down
1 change: 0 additions & 1 deletion pkg/controller/association/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ type AssociationInfo struct {
// namespace of the associated resource (eg. user secret allowing to connect Beat to Kibana will have this label
// pointing to the Beat resource).
AssociationResourceNamespaceLabelName string

// ElasticsearchUserCreation specifies settings to create an Elasticsearch user as part of the association.
// May be nil if no user creation is required.
ElasticsearchUserCreation *ElasticsearchUserCreation
Expand Down
50 changes: 49 additions & 1 deletion pkg/controller/common/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,66 @@ import (
"sync/atomic"

"go.elastic.co/apm"
"k8s.io/utils/strings/slices"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/manager"
"sigs.k8s.io/controller-runtime/pkg/predicate"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/source"

"github.com/elastic/cloud-on-k8s/pkg/controller/common/operator"
"github.com/elastic/cloud-on-k8s/pkg/controller/common/predicates"
"github.com/elastic/cloud-on-k8s/pkg/controller/common/tracing"
logconf "github.com/elastic/cloud-on-k8s/pkg/utils/log"
)

// NewController creates a new controller with the given name, reconciler and parameters and registers it with the manager.
func NewController(mgr manager.Manager, name string, r reconcile.Reconciler, p operator.Parameters) (controller.Controller, error) {
return controller.New(name, mgr, controller.Options{Reconciler: r, MaxConcurrentReconciles: p.MaxConcurrentReconciles})
c, err := controller.New(name, mgr, controller.Options{Reconciler: r, MaxConcurrentReconciles: p.MaxConcurrentReconciles})
if err != nil {
return nil, err
}
return newNamespaceAwareWatchersController(c, p.ManagedNamespaces, p.OperatorNamespace), nil
}

// NewControllerWithOptions creates a new controller with the given name, reconciler, parameters, options and registers it with the manager.
func NewControllerWithOptions(mgr manager.Manager, name string, p operator.Parameters, options controller.Options) (controller.Controller, error) {
c, err := controller.New(name, mgr, options)
if err != nil {
return nil, err
}
return newNamespaceAwareWatchersController(c, p.ManagedNamespaces, p.OperatorNamespace), nil
}

var _ controller.Controller = &namespaceAwareController{}

// namespaceAwareController implements the controller.Controller interface and automatically include a predicate to filter events
// which are not in a managed namespace.
type namespaceAwareController struct {
controller.Controller
namespacePredicate predicate.Predicate
}

// newNamespaceAwareWatchersController creates a new namespaceAwareController, ensuring that a predicate exists to ignore any
// namespaced events outside of managed namespaces, and the operator namespace.
func newNamespaceAwareWatchersController(c controller.Controller, managedNamespaces []string, operatorNamespace string) controller.Controller {
watchedNamespaces := managedNamespaces
// if the length of watchedNamespaces is 0, then we're watching all namespaces, and shouldn't append anything to the slice, as
// it will just cause issues wth the managed namespaces predicate.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// it will just cause issues wth the managed namespaces predicate.
// it will just cause issues with the managed namespaces predicate.

if len(watchedNamespaces) > 0 && !slices.Contains(watchedNamespaces, operatorNamespace) {
watchedNamespaces = append(watchedNamespaces, operatorNamespace)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really required ? The operatorNamespace is supposed to be already added to the managedNamespaces: https://github.com/elastic/cloud-on-k8s/pull/5058/files#diff-435eecb1d2af40ee96747f88ab71eb2758da989c92f76dcb1f714fc1b300c633R479-R480

}
return &namespaceAwareController{
Controller: c,
namespacePredicate: predicates.NewManagedNamespacesPredicate(watchedNamespaces),
}
}

// Watch implements controller.Controller interface, and calls the underlying controller's
// watch method, ensuring that the namespace predicate exists.
func (n *namespaceAwareController) Watch(src source.Source, eventhandler handler.EventHandler, predicates ...predicate.Predicate) error {
return n.Controller.Watch(src, eventhandler, append(predicates, n.namespacePredicate)...)
}

// NewReconciliationContext increments iteration, creates an apm transaction and initiates the logger. Returns context
Expand Down
2 changes: 2 additions & 0 deletions pkg/controller/common/operator/parameters.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import (
type Parameters struct {
// ExposedNodeLabels holds regular expressions of node labels which are allowed to be automatically set as annotations on Elasticsearch Pods.
ExposedNodeLabels esvalidation.NodeLabels
// ManagedNamespaces are the list of namespaces that the operator manages.
ManagedNamespaces []string
// OperatorNamespace is the control plane namespace of the operator.
OperatorNamespace string
// OperatorInfo is information about the operator
Expand Down
Loading