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

Allow Agent and Elastic stack in different namespaces. #7382

Merged
merged 15 commits into from
Dec 19, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
13 changes: 7 additions & 6 deletions pkg/apis/common/v1/association.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,12 +199,13 @@ func FormatNameWithID(template string, id string) string {

// AssociationConf holds the association configuration of a referenced resource in an association.
type AssociationConf struct {
AuthSecretName string `json:"authSecretName"`
AuthSecretKey string `json:"authSecretKey"`
IsServiceAccount bool `json:"isServiceAccount"`
CACertProvided bool `json:"caCertProvided"`
CASecretName string `json:"caSecretName"`
URL string `json:"url"`
AuthSecretName string `json:"authSecretName"`
AuthSecretKey string `json:"authSecretKey"`
IsServiceAccount bool `json:"isServiceAccount"`
CACertProvided bool `json:"caCertProvided"`
CASecretName string `json:"caSecretName"`
AdditionalSecretsHash string `json:"additionalSecretsHash"`
naemono marked this conversation as resolved.
Show resolved Hide resolved
naemono marked this conversation as resolved.
Show resolved Hide resolved
URL string `json:"url"`
// Version of the referenced resource. If a version upgrade is in progress,
// matches the lowest running version. May be empty if unknown.
Version string `json:"version"`
Expand Down
1 change: 0 additions & 1 deletion pkg/controller/association/ca.go
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ func (r *Reconciler) ReconcileCASecret(ctx context.Context, association commonv1
}

labels := r.AssociationResourceLabels(k8s.ExtractNamespacedName(association), association.AssociationRef().NamespacedName())

// Certificate data should be copied over a secret in the association namespace
expectedSecret := corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Expand Down
13 changes: 9 additions & 4 deletions pkg/controller/association/controller/agent_fleetserver.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/elastic/cloud-on-k8s/v2/pkg/controller/association"
"github.com/elastic/cloud-on-k8s/v2/pkg/controller/common/operator"
"github.com/elastic/cloud-on-k8s/v2/pkg/utils/k8s"
ulog "github.com/elastic/cloud-on-k8s/v2/pkg/utils/log"
"github.com/elastic/cloud-on-k8s/v2/pkg/utils/rbac"
)

Expand All @@ -30,7 +31,7 @@ func AddAgentFleetServer(mgr manager.Manager, accessReviewer rbac.AccessReviewer
AssociationName: "agent-fleetserver",
AssociatedShortName: "agent",
AssociationType: commonv1.FleetServerAssociationType,
AdditionalSecrets: addtionalSecrets,
AdditionalSecrets: additionalSecrets,
Labels: func(associated types.NamespacedName) map[string]string {
return map[string]string{
AgentAssociationLabelName: associated.Name,
Expand All @@ -46,19 +47,20 @@ func AddAgentFleetServer(mgr manager.Manager, accessReviewer rbac.AccessReviewer
})
}

func addtionalSecrets(c k8s.Client, assoc commonv1.Association) ([]types.NamespacedName, error) {
func additionalSecrets(ctx context.Context, c k8s.Client, assoc commonv1.Association) ([]types.NamespacedName, error) {
log := ulog.FromContext(ctx)
associated := assoc.Associated()
var agent agentv1alpha1.Agent
nsn := types.NamespacedName{Namespace: associated.GetNamespace(), Name: associated.GetName()}
if err := c.Get(context.Background(), nsn, &agent); err != nil {
if err := c.Get(ctx, nsn, &agent); err != nil {
return nil, err
}
fleetServerRef := assoc.AssociationRef()
if !fleetServerRef.IsDefined() {
return nil, nil
}
fleetServer := agentv1alpha1.Agent{}
if err := c.Get(context.Background(), fleetServerRef.NamespacedName(), &fleetServer); err != nil {
if err := c.Get(ctx, fleetServerRef.NamespacedName(), &fleetServer); err != nil {
return nil, err
}

Expand All @@ -74,14 +76,17 @@ func addtionalSecrets(c k8s.Client, assoc commonv1.Association) ([]types.Namespa

// if both agent and ES are same namespace no copying needed
if agent.GetNamespace() == esAssociation.GetNamespace() {
log.V(1).Info("no additional secrets because same namespace")
return nil, nil
}

conf, err := esAssociation.AssociationConf()
if err != nil {
log.V(1).Info("no additional secrets because no assoc conf")
return nil, err
}
if conf == nil || !conf.CACertProvided {
log.V(1).Info("no additional secrets because conf nil or no CA provided")
return nil, nil
}
return []types.NamespacedName{{
Expand Down
5 changes: 3 additions & 2 deletions pkg/controller/association/dynamic_watches.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package association

import (
"context"
"fmt"

"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -49,7 +50,7 @@ func additionalSecretWatchName(associated types.NamespacedName) string {
// * if there's an ES user to create, watch the user Secret in ES namespace
// All watches for all given associations are set under the same watch name and replaced with each reconciliation.
// The given associations are expected to be of the same type (e.g. Kibana -> Elasticsearch, not Kibana -> Enterprise Search).
func (r *Reconciler) reconcileWatches(associated types.NamespacedName, associations []commonv1.Association) error {
func (r *Reconciler) reconcileWatches(ctx context.Context, associated types.NamespacedName, associations []commonv1.Association) error {
managedElasticRef := filterManagedElasticRef(associations)
unmanagedElasticRef := filterUnmanagedElasticRef(associations)

Expand Down Expand Up @@ -103,7 +104,7 @@ func (r *Reconciler) reconcileWatches(associated types.NamespacedName, associati
if err := ReconcileGenericWatch(associated, managedElasticRef, r.watches.Secrets, additionalSecretWatchName(associated), func() ([]types.NamespacedName, error) {
var toWatch []types.NamespacedName
for _, association := range associations {
secs, err := r.AdditionalSecrets(r.Client, association)
secs, err := r.AdditionalSecrets(ctx, r.Client, association)
if err != nil {
return nil, err
}
Expand Down
19 changes: 11 additions & 8 deletions pkg/controller/association/reconciler.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package association
import (
"context"
"fmt"
"hash/fnv"
"reflect"
"time"

Expand Down Expand Up @@ -64,7 +65,7 @@ type AssociationInfo struct { //nolint:revive
// AdditionalSecrets are additional secrets to copy from an association's namespace to the associated resource namespace.
// Currently this is only used for copying the CA from an Elasticsearch association to the same namespace as
// an Agent referencing a Fleet Server.
AdditionalSecrets func(c k8s.Client, assoc commonv1.Association) ([]types.NamespacedName, error)
AdditionalSecrets func(context.Context, k8s.Client, commonv1.Association) ([]types.NamespacedName, error)
// Labels are labels set on all resources created for association purpose. Note that some resources will be also
// labelled with AssociationResourceNameLabelName and AssociationResourceNamespaceLabelName in addition to any
// labels provided here.
Expand Down Expand Up @@ -191,7 +192,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
}

// reconcile watches for all associations of this type
if err := r.reconcileWatches(associatedKey, associations); err != nil {
if err := r.reconcileWatches(ctx, associatedKey, associations); err != nil {
return reconcile.Result{}, tracing.CaptureError(ctx, err)
}

Expand Down Expand Up @@ -263,13 +264,14 @@ func (r *Reconciler) reconcileAssociation(ctx context.Context, association commo
return commonv1.AssociationPending, err // maybe not created yet
}

secretsHash := fnv.New32a()
if r.AdditionalSecrets != nil {
additionalSecrets, err := r.AdditionalSecrets(r.Client, association)
additionalSecrets, err := r.AdditionalSecrets(ctx, r.Client, association)
if err != nil {
return commonv1.AssociationPending, err // maybe not created yet
}
for _, sec := range additionalSecrets {
if err := copySecret(ctx, r.Client, r.AssociationInfo, k8s.ExtractNamespacedName(association.Associated()), association.GetNamespace(), sec); err != nil {
if err := copySecret(ctx, r.Client, secretsHash, association.GetNamespace(), sec); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we supposed to watch this Secret? It's not reconciled if I delete it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, it doesn't return, but the agent-es association controller is triggered on delete. I wonder if this:
https://github.com/naemono/cloud-on-k8s/blob/5dd7bb85550fd0d0b23af1762803025e725bf2d3/pkg/controller/association/dynamic_watches.go#L103-L117
is associating the secret incorrectly? I'll investigate and update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We were only watching the source secrets (the secrets we were copying into the target ns) and not the target/created secrets. It seems we would need to watch both:

  1. if source secret changes, update target secret.
  2. if target secret is changed/deleted, reconcile the secret.

I have updated and verified that both the target and source are re-created upon deletion.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed we have a similar issue with the fleet server es user/service token secret. Probably worth addressing in a separate PR though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just realised we have captured my observation in an issue already #7170

return commonv1.AssociationPending, err
}
}
Expand All @@ -294,10 +296,11 @@ func (r *Reconciler) reconcileAssociation(ctx context.Context, association commo

// construct the expected association configuration
expectedAssocConf := &commonv1.AssociationConf{
CACertProvided: caSecret.CACertProvided,
CASecretName: caSecret.Name,
URL: url,
Version: ver,
CACertProvided: caSecret.CACertProvided,
CASecretName: caSecret.Name,
AdditionalSecretsHash: fmt.Sprint(secretsHash.Sum32()),
naemono marked this conversation as resolved.
Show resolved Hide resolved
URL: url,
Version: ver,
}

if r.ElasticsearchUserCreation == nil {
Expand Down
Loading