feat: support for instrumentation status fields without the health sidecar (where applicable)

.github/ct.yaml

@@ -4,7 +4,7 @@
 target-branch: main
 
 chart-repos:
-  - newrelic=https://helm-charts.newrelic.com
+  - newrelic=https://newrelic.github.io/helm-charts/
 
 # Charts will be released manually.
 check-version-increment: false

.github/workflows/release-charts.yml

@@ -26,7 +26,7 @@ jobs:
           git config user.email "$GITHUB_ACTOR@users.noreply.github.com"
 
       - name: Add newrelic repository
-        run: helm repo add newrelic https://helm-charts.newrelic.com
+        run: helm repo add newrelic https://newrelic.github.io/helm-charts/
 
       - name: Run chart-releaser
         uses: helm/chart-releaser-action@cae68fefc6b5f367a0275617c9f83181ba54714f # v1.7.0

api/v1alpha2/instrumentation_webhook.go

@@ -118,17 +118,18 @@ func (r *InstrumentationValidator) validate(inst *Instrumentation) (admission.Wa
 		return nil, fmt.Errorf("instrumentation must be in operator namespace")
 	}
 
+	// Check if agent is empty first, before validating individual fields
+	if inst.Spec.Agent.IsEmpty() {
+		return nil, fmt.Errorf("instrumentation %q agent is empty", inst.Name)
+	}
+
 	if agentLang := inst.Spec.Agent.Language; !slices.Contains(acceptableLangs, agentLang) {
 		return nil, fmt.Errorf("instrumentation agent language %q must be one of the accepted languages (%s)", agentLang, strings.Join(acceptableLangs, ", "))
 	}
 
 	if err := r.validateEnv(inst.Spec.Agent.Env); err != nil {
 		return nil, err
 	}
-
-	if inst.Spec.Agent.IsEmpty() {
-		return nil, fmt.Errorf("instrumentation %q agent is empty", inst.Name)
-	}
 	if _, err := metav1.LabelSelectorAsSelector(&inst.Spec.PodLabelSelector); err != nil {
 		return nil, err
 	}
@@ -141,6 +142,14 @@ func (r *InstrumentationValidator) validate(inst *Instrumentation) (admission.Wa
 
 // validateEnv to validate the environment variables used all start with the required prefixes
 func (r *InstrumentationValidator) validateEnv(envs []corev1.EnvVar) error {
+	// First, check that NEW_RELIC_LICENSE_KEY is not set (it should only be in the secret)
+	for _, env := range envs {
+		if env.Name == "NEW_RELIC_LICENSE_KEY" {
+			return fmt.Errorf("NEW_RELIC_LICENSE_KEY should not be set in agent.env; the license key should be set via the licenseKeySecret field")
+		}
+	}
+
+	// Then validate that all env vars start with valid prefixes
 	var invalidNames []string
 	for _, env := range envs {
 		var valid bool

api/v1alpha2/instrumentation_webhook_test.go

@@ -0,0 +1,202 @@
+package v1alpha2
+
+import (
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestValidateLicenseKeyInEnv(t *testing.T) {
+	operatorNamespace := "operator-ns"
+	validator := &InstrumentationValidator{
+		OperatorNamespace: operatorNamespace,
+	}
+
+	tests := []struct {
+		name    string
+		inst    *Instrumentation
+		wantErr bool
+		errMsg  string
+	}{
+		{
+			name: "license key in env should fail",
+			inst: &Instrumentation{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-inst",
+					Namespace: operatorNamespace,
+				},
+				Spec: InstrumentationSpec{
+					Agent: Agent{
+						Language: "java",
+						Image:    "newrelic/java-agent:latest",
+						Env: []corev1.EnvVar{
+							{
+								Name:  "NEW_RELIC_LICENSE_KEY",
+								Value: "secret-key",
+							},
+						},
+					},
+					PodLabelSelector:       metav1.LabelSelector{},
+					NamespaceLabelSelector: metav1.LabelSelector{},
+				},
+			},
+			wantErr: true,
+			errMsg:  "NEW_RELIC_LICENSE_KEY should not be set in agent.env",
+		},
+		{
+			name: "other NEW_RELIC env vars should pass",
+			inst: &Instrumentation{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-inst",
+					Namespace: operatorNamespace,
+				},
+				Spec: InstrumentationSpec{
+					Agent: Agent{
+						Language: "java",
+						Image:    "newrelic/java-agent:latest",
+						Env: []corev1.EnvVar{
+							{
+								Name:  "NEW_RELIC_APP_NAME",
+								Value: "my-app",
+							},
+							{
+								Name:  "NEW_RELIC_DISTRIBUTED_TRACING_ENABLED",
+								Value: "true",
+							},
+						},
+					},
+					PodLabelSelector:       metav1.LabelSelector{},
+					NamespaceLabelSelector: metav1.LabelSelector{},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "NEWRELIC prefix env vars should pass",
+			inst: &Instrumentation{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-inst",
+					Namespace: operatorNamespace,
+				},
+				Spec: InstrumentationSpec{
+					Agent: Agent{
+						Language: "python",
+						Image:    "newrelic/python-agent:latest",
+						Env: []corev1.EnvVar{
+							{
+								Name:  "NEWRELIC_LOG_LEVEL",
+								Value: "debug",
+							},
+						},
+					},
+					PodLabelSelector:       metav1.LabelSelector{},
+					NamespaceLabelSelector: metav1.LabelSelector{},
+				},
+			},
+			wantErr: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := validator.validate(tt.inst)
+
+			if tt.wantErr {
+				if err == nil {
+					t.Errorf("validate() expected error, got nil")
+				} else if tt.errMsg != "" && !contains(err.Error(), tt.errMsg) {
+					t.Errorf("validate() error = %v, want error containing %q", err, tt.errMsg)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("validate() unexpected error = %v", err)
+				}
+			}
+		})
+	}
+}
+
+func TestValidationOrder(t *testing.T) {
+	operatorNamespace := "operator-ns"
+	validator := &InstrumentationValidator{
+		OperatorNamespace: operatorNamespace,
+	}
+
+	tests := []struct {
+		name    string
+		inst    *Instrumentation
+		wantErr bool
+		errMsg  string
+	}{
+		{
+			name: "empty agent should fail with empty agent error, not language error",
+			inst: &Instrumentation{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-inst",
+					Namespace: operatorNamespace,
+				},
+				Spec: InstrumentationSpec{
+					Agent: Agent{
+						Language: "", // Empty language
+						Image:    "", // Empty image
+					},
+					PodLabelSelector:       metav1.LabelSelector{},
+					NamespaceLabelSelector: metav1.LabelSelector{},
+				},
+			},
+			wantErr: true,
+			errMsg:  "agent is empty", // Should get this error, not language error
+		},
+		{
+			name: "agent with image but invalid language should fail with language error",
+			inst: &Instrumentation{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-inst",
+					Namespace: operatorNamespace,
+				},
+				Spec: InstrumentationSpec{
+					Agent: Agent{
+						Language: "invalid-lang",
+						Image:    "some-image:latest",
+					},
+					PodLabelSelector:       metav1.LabelSelector{},
+					NamespaceLabelSelector: metav1.LabelSelector{},
+				},
+			},
+			wantErr: true,
+			errMsg:  "must be one of the accepted languages",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			_, err := validator.validate(tt.inst)
+
+			if tt.wantErr {
+				if err == nil {
+					t.Errorf("validate() expected error, got nil")
+				} else if tt.errMsg != "" && !contains(err.Error(), tt.errMsg) {
+					t.Errorf("validate() error = %v, want error containing %q", err, tt.errMsg)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("validate() unexpected error = %v", err)
+				}
+			}
+		})
+	}
+}
+
+// contains is a helper to check if a string contains a substring
+func contains(s, substr string) bool {
+	if len(s) == 0 || len(substr) == 0 {
+		return false
+	}
+	for i := 0; i <= len(s)-len(substr); i++ {
+		if s[i:i+len(substr)] == substr {
+			return true
+		}
+	}
+	return false
+}

api/v1beta1/instrumentation_webhook.go

@@ -120,6 +120,11 @@ func (r *InstrumentationValidator) validate(inst *Instrumentation) (admission.Wa
 		return nil, fmt.Errorf("instrumentation must be in operator namespace")
 	}
 
+	// Check if agent is empty first, before validating individual fields
+	if inst.Spec.Agent.IsEmpty() {
+		return nil, fmt.Errorf("instrumentation %q agent is empty", inst.Name)
+	}
+
 	agentLang := inst.Spec.Agent.Language
 	if !slices.Contains(acceptableLangs, agentLang) {
 		return nil, fmt.Errorf("instrumentation agent language %q must be one of the accepted languages (%s)", agentLang, strings.Join(acceptableLangs, ", "))
@@ -129,15 +134,17 @@ func (r *InstrumentationValidator) validate(inst *Instrumentation) (admission.Wa
 		if !slices.Contains(acceptLangsForAgentConfigMap, agentLang) {
 			return nil, fmt.Errorf("instrumentation agent language %q does not support an agentConfigMap, agentConfigMap can only be configured with one of these languages (%q)", agentLang, strings.Join(acceptLangsForAgentConfigMap, ", "))
 		}
+		// Check that NEWRELIC_FILE is not set when using agentConfigMap (Java only)
+		for _, env := range inst.Spec.Agent.Env {
+			if env.Name == "NEWRELIC_FILE" {
+				return nil, fmt.Errorf("%q is already set by the agentConfigMap", env.Name)
+			}
+		}
 	}
 
 	if err := r.validateEnv(inst.Spec.Agent.Env); err != nil {
 		return nil, err
 	}
-
-	if inst.Spec.Agent.IsEmpty() {
-		return nil, fmt.Errorf("instrumentation %q agent is empty", inst.Name)
-	}
 	if len(inst.Spec.HealthAgent.Env) > 0 && inst.Spec.HealthAgent.Image == "" {
 		return nil, fmt.Errorf("instrumentation %q healthAgent.image is empty, meanwhile the environment is not", inst.Name)
 	}
@@ -154,6 +161,14 @@ func (r *InstrumentationValidator) validate(inst *Instrumentation) (admission.Wa
 
 // validateEnv to validate the environment variables used all start with the required prefixes
 func (r *InstrumentationValidator) validateEnv(envs []corev1.EnvVar) error {
+	// First, check that NEW_RELIC_LICENSE_KEY is not set (it should only be in the secret)
+	for _, env := range envs {
+		if env.Name == "NEW_RELIC_LICENSE_KEY" {
+			return fmt.Errorf("NEW_RELIC_LICENSE_KEY should not be set in agent.env; the license key should be set via the licenseKeySecret field")
+		}
+	}
+
+	// Then validate that all env vars start with valid prefixes
 	var invalidNames []string
 	for _, env := range envs {
 		var valid bool