Webhook injection no kubelet (#1573)

This pull request includes several changes to enhance the debugging
setup, improve environment variable handling, and refactor code for
better readability and maintainability. The most important changes
include updates to the debugging configuration, injection of environment
variables, and refactoring of workload handling.

### Debugging Enhancements:
*
[`.vscode/launch.json`](diffhunk://#diff-bd5430ee7c51dc892a67b3f2829d1f5b6d223f0fd48b82322cfd45baf9f5e945L20-R23):
Added environment variables for local mutating webhook certificate
directory to facilitate local debugging.
*
[`CONTRIBUTING.md`](diffhunk://#diff-eca12c0a30e25b4b46522ebf89465a03ba72a03f540796c979137931d8f92055R170-R202):
Added detailed instructions for debugging the Instrumentor when the
Mutating Webhook is enabled.

### Environment Variable Injection:
*
[`instrumentor/controllers/instrumentationdevice/pods_webhook.go`](diffhunk://#diff-09f9653d9c6c1a8bb7d52e7f3bf0060e0176369df0ee04d04bfb6b4d620794ddL10-L21):
Introduced constants for ODIGOS environment variables and added a
function to inject these variables into all containers.
[[1]](diffhunk://#diff-09f9653d9c6c1a8bb7d52e7f3bf0060e0176369df0ee04d04bfb6b4d620794ddL10-L21)
[[2]](diffhunk://#diff-09f9653d9c6c1a8bb7d52e7f3bf0060e0176369df0ee04d04bfb6b4d620794ddL31-R89)
*
[`instrumentor/main.go`](diffhunk://#diff-e36937e3510a3c05ef5f17e89c712712f4a500188cfc4818fba7e5015ebe47b2L101-R102):
Added logic to check for a local webhook certificate directory and
configure the WebhookServer accordingly for local development.
[[1]](diffhunk://#diff-e36937e3510a3c05ef5f17e89c712712f4a500188cfc4818fba7e5015ebe47b2L101-R102)
[[2]](diffhunk://#diff-e36937e3510a3c05ef5f17e89c712712f4a500188cfc4818fba7e5015ebe47b2R177-R190)

### Refactoring:
*
[`k8sutils/pkg/workload/ownerreference.go`](diffhunk://#diff-183ad6413c2a5e4a1aad9e9566e87fc06f88ed51afe6c6fffcb521d9f74ee556L4-R47):
Refactored workload handling functions to separate concerns and improve
readability.
*
[`opampserver/pkg/server/handlers.go`](diffhunk://#diff-944e905a1aea43bd5f8a4c5ad17b404e63ea772321096a15829fb2951723a8d8L14-R43):
Refactored the `OnNewConnection` method to extract and handle agent
attributes more efficiently and added helper functions for resolving
Kubernetes attributes.
[[1]](diffhunk://#diff-944e905a1aea43bd5f8a4c5ad17b404e63ea772321096a15829fb2951723a8d8L14-R43)
[[2]](diffhunk://#diff-944e905a1aea43bd5f8a4c5ad17b404e63ea772321096a15829fb2951723a8d8L56-R79)
[[3]](diffhunk://#diff-944e905a1aea43bd5f8a4c5ad17b404e63ea772321096a15829fb2951723a8d8L86-R91)
[[4]](diffhunk://#diff-944e905a1aea43bd5f8a4c5ad17b404e63ea772321096a15829fb2951723a8d8L99-R104)
[[5]](diffhunk://#diff-944e905a1aea43bd5f8a4c5ad17b404e63ea772321096a15829fb2951723a8d8R199-R263)

### Minor Imports Adjustments:
*
[`instrumentor/main.go`](diffhunk://#diff-e36937e3510a3c05ef5f17e89c712712f4a500188cfc4818fba7e5015ebe47b2R36):
Added missing import for `webhook` package.
*
[`opampserver/pkg/server/handlers.go`](diffhunk://#diff-944e905a1aea43bd5f8a4c5ad17b404e63ea772321096a15829fb2951723a8d8L14-R43):
Aliased the `deviceid` import for consistency.

These changes collectively improve the development experience and code
maintainability.

---------

Co-authored-by: Tamir David <[email protected]>

.gitignore

@@ -6,4 +6,5 @@ go.work.sum
 cli/odigos
 .venv
 **/__pycache__/
-**/*.pyc
+**/*.pyc
+serving-certs/

.vscode/launch.json

@@ -17,7 +17,10 @@
             "request": "launch",
             "mode": "debug",
             "program": "${workspaceFolder}/instrumentor",
-            "cwd": "${workspaceFolder}/instrumentor"
+            "cwd": "${workspaceFolder}/instrumentor",
+            "env": {
+                "LOCAL_MUTATING_WEBHOOK_CERT_DIR": "${workspaceFolder}/serving-certs"
+            }            
         },
         {
             "name": "frontend",

CONTRIBUTING.md

@@ -167,3 +167,36 @@ make debug-odiglet
 
 Then, you can attach a debugger to the Odiglet pod. For example, if you are using Goland, you can follow the instructions [here](https://www.jetbrains.com/help/go/attach-to-running-go-processes-with-debugger.html#step-3-create-the-remote-run-debug-configuration-on-the-client-computer) to attach to a remote process.
 For Visual Studio Code, you can use the `.vscode/launch.json` file in this repo to attach to the Odiglet pod.
+
+
+
+## Instrumentor
+
+### Debugging
+If the Mutating Webhook is enabled, follow these steps:
+
+1. Copy the TLS certificate and key:
+Create a local directory and extract the certificate and key by running the following command:  
+```
+mkdir -p serving-certs && kubectl get secret instrumentor-webhook-cert -n odigos-system -o jsonpath='{.data.tls\.crt}' | base64 -d > serving-certs/tls.crt && kubectl get secret instrumentor-webhook-cert -n odigos-system -o jsonpath='{.data.tls\.key}' | base64 -d > serving-certs/tls.key
+```
+
+
+2. Apply this service to the cluster, it will replace the existing `odigos-instrumentor` service:
+
+```
+apiVersion: v1
+kind: Service
+metadata:
+  name: odigos-instrumentor
+  namespace: odigos-system
+spec:
+  type: ExternalName
+  externalName: host.docker.internal
+  ports:
+    - name: webhook-server
+      port: 9443
+      protocol: TCP
+```
+
+Once this is done, you can use the .vscode/launch.json configuration and run instrumentor local for debugging.

instrumentor/controllers/instrumentationdevice/pods_webhook.go

@@ -3,22 +3,26 @@ package instrumentationdevice
 import (
 	"context"
 	"fmt"
+	"strings"
 
+	common "github.com/odigos-io/odigos/common"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 
 	corev1 "k8s.io/api/core/v1"
-	logf "sigs.k8s.io/controller-runtime/pkg/log"
-
 	"k8s.io/apimachinery/pkg/runtime"
 )
 
+const (
+	EnvVarNamespace     = "ODIGOS_WORKLOAD_NAMESPACE"
+	EnvVarContainerName = "ODIGOS_CONTAINER_NAME"
+	EnvVarPodName       = "ODIGOS_POD_NAME"
+)
+
 type PodsWebhook struct{}
 
 var _ webhook.CustomDefaulter = &PodsWebhook{}
 
 func (p *PodsWebhook) Default(ctx context.Context, obj runtime.Object) error {
-	// TODO(edenfed): add object selector to mutatingwebhookconfiguration
-	log := logf.FromContext(ctx)
 	pod, ok := obj.(*corev1.Pod)
 	if !ok {
 		return fmt.Errorf("expected a Pod but got a %T", obj)
@@ -28,7 +32,71 @@ func (p *PodsWebhook) Default(ctx context.Context, obj runtime.Object) error {
 		pod.Annotations = map[string]string{}
 	}
 
-	//pod.Annotations["odigos.io/instrumented-webhook"] = "true"
-	log.V(0).Info("Defaulted Pod", "name", pod.Name)
+	// Inject ODIGOS environment variables into all containers
+	injectOdigosEnvVars(pod)
+
 	return nil
 }
+
+func injectOdigosEnvVars(pod *corev1.Pod) {
+	namespace := pod.Namespace
+
+	// Common environment variables that do not change across containers
+	commonEnvVars := []corev1.EnvVar{
+		{
+			Name:  EnvVarNamespace,
+			Value: namespace,
+		},
+		{
+			Name: EnvVarPodName,
+			ValueFrom: &corev1.EnvVarSource{
+				FieldRef: &corev1.ObjectFieldSelector{
+					FieldPath: "metadata.name",
+				},
+			},
+		},
+	}
+
+	for i := range pod.Spec.Containers {
+		container := &pod.Spec.Containers[i]
+
+		// Check if the container does NOT have device in conatiner limits. If so, skip the environment injection.
+		if !hasOdigosInstrumentationInLimits(container.Resources) {
+			continue
+		}
+
+		// Check if the environment variables are already present, if so skip inject them again.
+		if envVarsExist(container.Env, commonEnvVars) {
+			continue
+		}
+
+		container.Env = append(container.Env, append(commonEnvVars, corev1.EnvVar{
+			Name:  EnvVarContainerName,
+			Value: container.Name,
+		})...)
+	}
+}
+
+func envVarsExist(containerEnv []corev1.EnvVar, commonEnvVars []corev1.EnvVar) bool {
+	envMap := make(map[string]struct{})
+	for _, envVar := range containerEnv {
+		envMap[envVar.Name] = struct{}{} // Inserting empty struct as value
+	}
+
+	for _, commonEnvVar := range commonEnvVars {
+		if _, exists := envMap[commonEnvVar.Name]; exists { // Checking if key exists
+			return true
+		}
+	}
+	return false
+}
+
+// Helper function to check if a container's resource limits have a key starting with the specified namespace
+func hasOdigosInstrumentationInLimits(resources corev1.ResourceRequirements) bool {
+	for resourceName := range resources.Limits {
+		if strings.HasPrefix(string(resourceName), common.OdigosResourceNamespace) {
+			return true
+		}
+	}
+	return false
+}

instrumentor/main.go

@@ -35,6 +35,7 @@ import (
 
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
 
 	metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
 
@@ -100,7 +101,7 @@ func main() {
 	logger := zapr.NewLogger(zapLogger)
 	ctrl.SetLogger(logger)
 
-	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
+	mgrOptions := ctrl.Options{
 		Scheme: scheme,
 		Metrics: metricsserver.Options{
 			BindAddress: metricsAddr,
@@ -178,7 +179,20 @@ func main() {
 				},
 			},
 		},
-	})
+	}
+
+	// Check if the environment variable `LOCAL_WEBHOOK_CERT_DIR` is set.
+	// If defined, add WebhookServer options with the specified certificate directory.
+	// This is used primarily for local development environments to provide a custom path for serving TLS certificates.
+	localCertDir := os.Getenv("LOCAL_MUTATING_WEBHOOK_CERT_DIR")
+	if localCertDir != "" {
+		mgrOptions.WebhookServer = webhook.NewServer(webhook.Options{
+			CertDir: localCertDir,
+		})
+	}
+
+	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), mgrOptions)
+
 	if err != nil {
 		setupLog.Error(err, "unable to start manager")
 		os.Exit(1)

k8sutils/pkg/workload/ownerreference.go

@@ -1,36 +1,43 @@
 package workload
 
 import (
-	"errors"
+	"fmt"
 	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+// GetWorkloadFromOwnerReference retrieves both the workload name and workload kind
+// from the provided owner reference.
 func GetWorkloadFromOwnerReference(ownerReference metav1.OwnerReference) (workloadName string, workloadKind WorkloadKind, err error) {
-	ownerName := ownerReference.Name
-	ownerKind := ownerReference.Kind
+
+	return GetWorkloadNameAndKind(ownerReference.Name, ownerReference.Kind)
+}
+
+func GetWorkloadNameAndKind(ownerName, ownerKind string) (string, WorkloadKind, error) {
 	if ownerKind == "ReplicaSet" {
-		// ReplicaSet name is in the format <deployment-name>-<random-string>
-		hyphenIndex := strings.LastIndex(ownerName, "-")
-		if hyphenIndex == -1 {
-			// It is possible for a user to define a bare ReplicaSet without a deployment, currently not supporting this
-			err = errors.New("replicaset name does not contain a hyphen")
-			return
-		}
-		// Extract deployment name from ReplicaSet name
-		workloadName = ownerName[:hyphenIndex]
-		workloadKind = WorkloadKindDeployment
-		return
+		return extractDeploymentInfo(ownerName)
+	}
+	return handleNonReplicaSet(ownerName, ownerKind)
+}
+
+// extractDeploymentInfo extracts deployment information from a ReplicaSet name
+func extractDeploymentInfo(replicaSetName string) (string, WorkloadKind, error) {
+	hyphenIndex := strings.LastIndex(replicaSetName, "-")
+	if hyphenIndex == -1 {
+		return "", "", fmt.Errorf("replicaset name '%s' does not contain a hyphen", replicaSetName)
 	}
 
-	workloadKind = WorkloadKindFromString(ownerKind)
+	deploymentName := replicaSetName[:hyphenIndex]
+	return deploymentName, WorkloadKindDeployment, nil
+}
+
+// handleNonReplicaSet processes non-ReplicaSet workload types
+func handleNonReplicaSet(ownerName, ownerKind string) (string, WorkloadKind, error) {
+	workloadKind := WorkloadKindFromString(ownerKind)
 	if workloadKind == "" {
-		err = ErrKindNotSupported
-		return
+		return "", "", ErrKindNotSupported
 	}
-	workloadName = ownerName
 
-	err = nil
-	return
+	return ownerName, workloadKind, nil
 }