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

Add Namespace Source instrumentation #2105

Merged
merged 22 commits into from
Jan 8, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
7ef4d94
Include Namespace Sources in GetSourceListForWorkload
damemi Dec 30, 2024
e0320fa
Add Namespace instrumentation to startlangdetection
damemi Dec 30, 2024
e0a96bc
Add Namespace uninstrumentation to deletinstrumentedapplication
damemi Dec 30, 2024
2bf63a8
Add Namespace instrumentation e2e + uninstrumentation e2e
damemi Dec 30, 2024
57b61c5
e2e: wait for instrumentation to take effect
damemi Jan 2, 2025
c228058
Don't delete explicitly instrumented sources when NS is uninstrumented
damemi Jan 2, 2025
ea8ac3c
Fix instEffectEnabled check in deleteinstrumentedapplication
damemi Jan 2, 2025
8676015
Fix inheritance check for individual workload sources
damemi Jan 3, 2025
253d622
Retry traffic generator -- check for minimum spans
damemi Jan 3, 2025
35ea009
Remove sleeps and increase traffic generation timeouts
damemi Jan 3, 2025
ba56913
Remove wait for conditions
damemi Jan 6, 2025
0bc6c26
Merge remote-tracking branch 'upstream/feature/source-crd' into sourc…
damemi Jan 6, 2025
1b26c5a
React to merge
damemi Jan 6, 2025
f1ac129
Update exclude label, update comment, use WorkloadSources object
damemi Jan 6, 2025
ef48c59
Update source e2e to not use InstrumentedApplication
damemi Jan 6, 2025
4277faa
Got the delete instrumentationconfig check wrong again
damemi Jan 6, 2025
1ea0bb2
Move namespace pre-processing into namespace path
damemi Jan 6, 2025
7295ac9
Support creation/deletion of excluded Source in instrumented namespace
damemi Jan 7, 2025
eeefef5
Deduplicate some code in controllers
damemi Jan 7, 2025
626e042
Use request.Namespace when injecting env vars in webhook
damemi Jan 4, 2025
5bffb1e
Use reported name annotation in workload-lifecycle tests
damemi Jan 4, 2025
27a767c
Feedback
damemi Jan 8, 2025
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
72 changes: 62 additions & 10 deletions api/odigos/v1alpha1/source_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,19 @@ package v1alpha1

import (
"context"
"errors"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"sigs.k8s.io/controller-runtime/pkg/client"

commonconsts "github.com/odigos-io/odigos/common/consts"
"github.com/odigos-io/odigos/k8sutils/pkg/consts"
"github.com/odigos-io/odigos/k8sutils/pkg/workload"
)

var ErrorTooManySources = errors.New("too many Sources found for workload")

// Source configures an application for auto-instrumentation.
// +genclient
// +kubebuilder:object:root=true
Expand Down Expand Up @@ -67,18 +71,66 @@ type SourceList struct {
Items []Source `json:"items"`
}

// GetSourceListForWorkload returns a SourceList of all Sources that have matching
// workload name, namespace, and kind labels for an object. In theory, this should only
// ever return a list with 0 or 1 items, but due diligence should handle unexpected cases.
func GetSourceListForWorkload(ctx context.Context, kubeClient client.Client, obj client.Object) (SourceList, error) {
sourceList := SourceList{}
selector := labels.SelectorFromSet(labels.Set{
consts.WorkloadNameLabel: obj.GetName(),
// +kubebuilder:object:generate=false

type WorkloadSources struct {
Workload *Source
Namespace *Source
}

// GetWorkloadSources returns a WorkloadSources listing the Workload and Namespace Source
// that currently apply to the given object. In theory, this should only ever return at most
// 1 Namespace and/or 1 Workload Source for an object. If more are found, an error is returned.
func GetWorkloadSources(ctx context.Context, kubeClient client.Client, obj client.Object) (*WorkloadSources, error) {
var err error
workloadSources := &WorkloadSources{}

if obj.GetObjectKind().GroupVersionKind().Kind != "Namespace" {
damemi marked this conversation as resolved.
Show resolved Hide resolved
sourceList := SourceList{}
selector := labels.SelectorFromSet(labels.Set{
consts.WorkloadNameLabel: obj.GetName(),
consts.WorkloadNamespaceLabel: obj.GetNamespace(),
consts.WorkloadKindLabel: obj.GetObjectKind().GroupVersionKind().Kind,
})
err := kubeClient.List(ctx, &sourceList, &client.ListOptions{LabelSelector: selector})
if err != nil {
return nil, err
}
if len(sourceList.Items) > 1 {
return nil, ErrorTooManySources
}
if len(sourceList.Items) == 1 {
workloadSources.Workload = &sourceList.Items[0]
}
}

namespaceSourceList := SourceList{}
namespaceSelector := labels.SelectorFromSet(labels.Set{
consts.WorkloadNameLabel: obj.GetNamespace(),
consts.WorkloadNamespaceLabel: obj.GetNamespace(),
consts.WorkloadKindLabel: obj.GetObjectKind().GroupVersionKind().Kind,
consts.WorkloadKindLabel: "Namespace",
})
err := kubeClient.List(ctx, &sourceList, &client.ListOptions{LabelSelector: selector})
return sourceList, err
err = kubeClient.List(ctx, &namespaceSourceList, &client.ListOptions{LabelSelector: namespaceSelector})
if err != nil {
return nil, err
}
if len(namespaceSourceList.Items) > 1 {
return nil, ErrorTooManySources
}
if len(namespaceSourceList.Items) == 1 {
workloadSources.Namespace = &namespaceSourceList.Items[0]
}

return workloadSources, nil
}

// IsWorkloadExcludedSource returns true if the Source is used to exclude a workload.
// Otherwise, it returns false.
func IsWorkloadExcludedSource(source *Source) bool {
if val, exists := source.Labels[commonconsts.OdigosWorkloadExcludedLabel]; exists && val == "true" {
return true
}
return false
}

func init() {
Expand Down
1 change: 1 addition & 0 deletions common/consts/consts.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const (
OdigosNamespaceAnnotation = "odigos.io/workload-namespace"
OdigosWorkloadKindAnnotation = "odigos.io/workload-kind"
OdigosWorkloadNameAnnotation = "odigos.io/workload-name"
OdigosWorkloadExcludedLabel = "odigos.io/workload-excluded"
OdigosReportedNameAnnotation = "odigos.io/reported-name"

// GatewayMaxConnectionAge and GatewayMaxConnectionAgeGrace are the default values for the gateway collector.
Expand Down
30 changes: 17 additions & 13 deletions instrumentor/controllers/deleteinstrumentationconfig/common.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package deleteinstrumentationconfig
import (
"context"

"github.com/odigos-io/odigos/api/odigos/v1alpha1"
odigosv1 "github.com/odigos-io/odigos/api/odigos/v1alpha1"
"github.com/odigos-io/odigos/common/consts"
"github.com/odigos-io/odigos/k8sutils/pkg/workload"
Expand All @@ -22,14 +21,18 @@ func reconcileWorkloadObject(ctx context.Context, kubeClient client.Client, work
}

if instEffectiveEnabled {
// Check if a Source object exists for this workload
sourceList, err := v1alpha1.GetSourceListForWorkload(ctx, kubeClient, workloadObject)
if err != nil {
return err
}
if len(sourceList.Items) == 0 {
return nil
}
return nil
}

// Check if a Source object exists for this workload
sourceList, err := odigosv1.GetWorkloadSources(ctx, kubeClient, workloadObject)
if err != nil {
return err
}
if sourceList.Workload != nil || sourceList.Namespace != nil {
// Note that if a Source is being deleted, but still has the finalizer, it will still show up in this List
// So we can't use this check to trigger un-instrumentation via Source deletion
return nil
}

if err := deleteWorkloadInstrumentationConfig(ctx, kubeClient, workloadObject); err != nil {
Expand All @@ -46,24 +49,25 @@ func reconcileWorkloadObject(ctx context.Context, kubeClient client.Client, work
}

func deleteWorkloadInstrumentationConfig(ctx context.Context, kubeClient client.Client, workloadObject client.Object) error {
logger := log.FromContext(ctx)
ns := workloadObject.GetNamespace()
name := workloadObject.GetName()
kind := workload.WorkloadKindFromClientObject(workloadObject)
instrumentedApplicationName := workload.CalculateWorkloadRuntimeObjectName(name, kind)
instrumentationConfigName := workload.CalculateWorkloadRuntimeObjectName(name, kind)
logger.V(1).Info("deleting instrumentationconfig", "name", instrumentationConfigName, "kind", kind)

instConfigErr := kubeClient.Delete(ctx, &odigosv1.InstrumentationConfig{
ObjectMeta: metav1.ObjectMeta{
Namespace: ns,
Name: instrumentedApplicationName,
Name: instrumentationConfigName,
},
})

if instConfigErr != nil {
return client.IgnoreNotFound(instConfigErr)
}

logger := log.FromContext(ctx)
logger.V(1).Info("instrumented application deleted", "namespace", ns, "name", name, "kind", kind)
logger.V(1).Info("instrumentationconfig deleted", "namespace", ns, "name", name, "kind", kind)
return nil
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,11 +92,11 @@ func (r *InstrumentationConfigReconciler) Reconcile(ctx context.Context, req ctr

if !instEffectiveEnabled {
// Check if a Source object exists for this workload
sourceList, err := v1alpha1.GetSourceListForWorkload(ctx, r.Client, workloadObject)
sourceList, err := v1alpha1.GetWorkloadSources(ctx, r.Client, workloadObject)
if err != nil {
return ctrl.Result{}, err
}
if len(sourceList.Items) == 0 {
if sourceList.Workload == nil && sourceList.Namespace == nil {
logger.Info("Deleting instrumented application for non-enabled workload")
err := r.Client.Delete(ctx, &instrumentationConfig)
return ctrl.Result{}, client.IgnoreNotFound(err)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package deleteinstrumentationconfig

import (
odigosv1 "github.com/odigos-io/odigos/api/odigos/v1alpha1"
k8sutils "github.com/odigos-io/odigos/k8sutils/pkg/predicate"
odigospredicate "github.com/odigos-io/odigos/k8sutils/pkg/predicate"

appsv1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -81,7 +80,7 @@ func SetupWithManager(mgr ctrl.Manager) error {
err = builder.
ControllerManagedBy(mgr).
Named("deleteinstrumentationconfig-source").
WithEventFilter(&k8sutils.OnlyUpdatesPredicate{}).
WithEventFilter(predicate.Or(&odigospredicate.CreationPredicate{}, &odigospredicate.OnlyUpdatesPredicate{})).
For(&odigosv1.Source{}).
Complete(&SourceReconciler{
Client: mgr.GetClient(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"errors"

"github.com/odigos-io/odigos/api/odigos/v1alpha1"
"github.com/odigos-io/odigos/common/consts"
"github.com/odigos-io/odigos/k8sutils/pkg/workload"

Expand Down Expand Up @@ -137,7 +138,6 @@ func (r *NamespaceReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
}

func syncGenericWorkloadListToNs(ctx context.Context, c client.Client, kind workload.WorkloadKind, key client.ObjectKey) error {

// it is very important that we make the changes based on a fresh copy of the workload object
// if a list operation pulled in state and is now slowly iterating over it, we might be working with stale data
freshWorkloadCopy := workload.ClientObjectFromWorkloadKind(kind)
Expand All @@ -151,11 +151,15 @@ func syncGenericWorkloadListToNs(ctx context.Context, c client.Client, kind work
}
}

if !isInheritingInstrumentationFromNs(freshWorkloadCopy) {
var err error
inheriting, err := isInheritingInstrumentationFromNs(ctx, c, freshWorkloadCopy)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this check only says if a workload instrumentation is inherited from ns. not if it's effectively instrumented or not.

It used to be ok because of this which guaranteed that this function is called after we already know the namespace is not labeled.

I think that the changes in this PR renders this assumption invalid, and we can now deleteWorkloadInstrumentationConfig and removeReportedNameAnnotation below even for instrumented workloads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as #2105 (comment)

if err != nil {
return err
}
if !inheriting {
return nil
}

var err error
err = errors.Join(err, deleteWorkloadInstrumentationConfig(ctx, c, freshWorkloadCopy))
err = errors.Join(err, removeReportedNameAnnotation(ctx, c, freshWorkloadCopy))
return err
Expand All @@ -165,11 +169,24 @@ func syncGenericWorkloadListToNs(ctx context.Context, c client.Client, kind work
// when reconciling the namespace, the usecase is to delete instrumentation for workloads that were only
// instrumented due to the label on the namespace. These are workloads with the label missing.
// (they inherit the instrumentation from the namespace this way)
func isInheritingInstrumentationFromNs(obj client.Object) bool {
func isInheritingInstrumentationFromNs(ctx context.Context, c client.Client, obj client.Object) (bool, error) {
sourceList, err := v1alpha1.GetWorkloadSources(ctx, c, obj)
if err != nil {
return false, err
}

if sourceList.Namespace != nil && sourceList.Namespace.DeletionTimestamp.IsZero() {
return true, nil
}
Comment on lines +178 to +180
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the workload has a source CR, then it's not inheriting it's instrumentation from the ns, but this function can return true here instead of false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

like we talked about offline, this function and its use will be refactored in the backend label clean-up. Keeping this open as a reminder to do that


if sourceList.Workload != nil && sourceList.Workload.DeletionTimestamp.IsZero() {
return false, nil
}

labels := obj.GetLabels()
if labels == nil {
return true
return true, nil
}
_, exists := labels[consts.OdigosInstrumentationLabel]
return !exists
return !exists, nil
}
Loading
Loading