From d556573f1b03a392d742f57fc4465ea64f20eb1e Mon Sep 17 00:00:00 2001 From: btaani Date: Wed, 24 Apr 2024 13:40:11 +0200 Subject: [PATCH 01/17] Add support for logging agent health checking --- internal/addon/addon.go | 63 +++++++++++++++++++++++ internal/addon/addon_test.go | 98 ++++++++++++++++++++++++++++++++++++ main.go | 1 + 3 files changed, 162 insertions(+) create mode 100644 internal/addon/addon_test.go diff --git a/internal/addon/addon.go b/internal/addon/addon.go index 3d61c71..33bd500 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -1,12 +1,20 @@ package addon import ( + "fmt" + "open-cluster-management.io/addon-framework/pkg/agent" "open-cluster-management.io/addon-framework/pkg/utils" addonapiv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" + workapiv1 "open-cluster-management.io/api/work/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) +const ( + clusterLoggingNS = "openshift-logging" + collectorNS = "spoke-otelcol" +) + func NewRegistrationOption(agentName string) *agent.RegistrationOption { return &agent.RegistrationOption{ CSRConfigurations: agent.KubeClientSignerConfigurations(Name, agentName), @@ -30,3 +38,58 @@ func GetObjectKey(configRef []addonapiv1alpha1.ConfigReference, group, resource } return key } + +// agentHealthProber returns an instance of the agent's health prober. It is used for +// probing and checking the health status of the agent. +func AgentHealthProber() *agent.HealthProber { + return &agent.HealthProber{ + Type: agent.HealthProberTypeDeploymentAvailability, + WorkProber: &agent.WorkHealthProber{ + ProbeFields: []agent.ProbeField{ + { + ResourceIdentifier: workapiv1.ResourceIdentifier{ + Group: "apps", + Resource: "deployments", + Name: "cluster-logging-operator", + Namespace: clusterLoggingNS, + }, + ProbeRules: []workapiv1.FeedbackRule{ + { + Type: workapiv1.WellKnownStatusType, + }, + }, + }, + { + ResourceIdentifier: workapiv1.ResourceIdentifier{ + Group: "apps", + Resource: "deployments", + Name: "spoke-otelcol-collector", + Namespace: collectorNS, + }, + ProbeRules: []workapiv1.FeedbackRule{ + { + Type: workapiv1.WellKnownStatusType, + }, + }, + }, + }, + HealthCheck: func(identifier workapiv1.ResourceIdentifier, result workapiv1.StatusFeedbackResult) error { + if len(result.Values) == 0 { + return fmt.Errorf("no values are probed for deployment %s/%s", identifier.Namespace, identifier.Name) + } + for _, value := range result.Values { + if value.Name != "readyReplicas" { + continue + } + + if *value.Value.Integer >= 1 { + return nil + } + + return fmt.Errorf("readyReplica is %d for deployement %s/%s", *value.Value.Integer, identifier.Namespace, identifier.Name) + } + return fmt.Errorf("readyReplica is not probed") + }, + }, + } +} diff --git a/internal/addon/addon_test.go b/internal/addon/addon_test.go new file mode 100644 index 0000000..b4d202b --- /dev/null +++ b/internal/addon/addon_test.go @@ -0,0 +1,98 @@ +package addon + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/require" + appsv1 "k8s.io/api/apps/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + v1 "open-cluster-management.io/api/work/v1" + + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func Test_AgentHealthProber_Healthy(t *testing.T) { + fakeKubeClient := fake.NewClientBuilder().Build() + colDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "spoke-otelcol-collector", + Namespace: "spoke-otelcol", + }, + Status: appsv1.DeploymentStatus{ + ReadyReplicas: 1, + }, + } + + err := fakeKubeClient.Create(context.TODO(), colDeployment, &client.CreateOptions{}) + require.NoError(t, err) + + readyReplicas := int64(colDeployment.Status.ReadyReplicas) + + healthProber := AgentHealthProber() + + err = healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ + Group: colDeployment.APIVersion, + Resource: colDeployment.Kind, + Name: colDeployment.Name, + Namespace: colDeployment.Namespace, + }, v1.StatusFeedbackResult{ + Values: []v1.FeedbackValue{ + { + Name: "readyReplicas", + Value: v1.FieldValue{ + Type: v1.Integer, + Integer: &readyReplicas, + }, + }, + }, + }) + + require.NoError(t, err) + +} + +func Test_AgentHealthProber(t *testing.T) { + + fakeKubeClient := fake.NewClientBuilder().Build() + + cloDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cluster-logging-operator", + Namespace: "openshift-logging", + }, + Status: appsv1.DeploymentStatus{ + ReadyReplicas: 0, + }, + } + + err := fakeKubeClient.Create(context.TODO(), cloDeployment, &client.CreateOptions{}) + require.NoError(t, err) + + readyReplicas := int64(cloDeployment.Status.ReadyReplicas) + + healthProber := AgentHealthProber() + + err = healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ + Group: cloDeployment.APIVersion, + Resource: cloDeployment.Kind, + Name: cloDeployment.Name, + Namespace: cloDeployment.Namespace, + }, v1.StatusFeedbackResult{ + Values: []v1.FeedbackValue{ + { + Name: "readyReplicas", + Value: v1.FieldValue{ + Type: v1.Integer, + Integer: &readyReplicas, + }, + }, + }, + }) + + expectedErr := fmt.Errorf("readyReplica is %d for deployement %s/%s", readyReplicas, cloDeployment.Namespace, cloDeployment.Name) + require.EqualError(t, err, expectedErr.Error()) + +} diff --git a/main.go b/main.go index f96e87a..c7243ba 100644 --- a/main.go +++ b/main.go @@ -158,6 +158,7 @@ func runController(ctx context.Context, kubeConfig *rest.Config) error { WithGetValuesFuncs(addonConfigValuesFn, addonhelm.GetValuesFunc(k8sClient)). WithAgentRegistrationOption(registrationOption). WithScheme(scheme.Scheme). + WithAgentHealthProber(addon.AgentHealthProber()). BuildHelmAgentAddon() if err != nil { klog.Errorf("failed to build agent %v", err) From 30ea3d4c7919313a3a59eac6a3d45ce16749cd06 Mon Sep 17 00:00:00 2001 From: btaani Date: Wed, 24 Apr 2024 13:45:20 +0200 Subject: [PATCH 02/17] remove comment --- internal/addon/addon.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index 33bd500..e36376d 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -39,8 +39,6 @@ func GetObjectKey(configRef []addonapiv1alpha1.ConfigReference, group, resource return key } -// agentHealthProber returns an instance of the agent's health prober. It is used for -// probing and checking the health status of the agent. func AgentHealthProber() *agent.HealthProber { return &agent.HealthProber{ Type: agent.HealthProberTypeDeploymentAvailability, From d2a71b18478f95d0ce518df77e5be98d8e0620fe Mon Sep 17 00:00:00 2001 From: btaani Date: Thu, 25 Apr 2024 14:51:15 +0200 Subject: [PATCH 03/17] some refactoring --- internal/addon/addon.go | 23 ++++++++++++----------- internal/addon/var.go | 3 +++ main.go | 2 +- 3 files changed, 16 insertions(+), 12 deletions(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index e36376d..7725a80 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -10,11 +10,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -const ( - clusterLoggingNS = "openshift-logging" - collectorNS = "spoke-otelcol" -) - func NewRegistrationOption(agentName string) *agent.RegistrationOption { return &agent.RegistrationOption{ CSRConfigurations: agent.KubeClientSignerConfigurations(Name, agentName), @@ -49,7 +44,7 @@ func AgentHealthProber() *agent.HealthProber { Group: "apps", Resource: "deployments", Name: "cluster-logging-operator", - Namespace: clusterLoggingNS, + Namespace: ClusterLoggingNS, }, ProbeRules: []workapiv1.FeedbackRule{ { @@ -62,7 +57,7 @@ func AgentHealthProber() *agent.HealthProber { Group: "apps", Resource: "deployments", Name: "spoke-otelcol-collector", - Namespace: collectorNS, + Namespace: CollectorNS, }, ProbeRules: []workapiv1.FeedbackRule{ { @@ -72,21 +67,27 @@ func AgentHealthProber() *agent.HealthProber { }, }, HealthCheck: func(identifier workapiv1.ResourceIdentifier, result workapiv1.StatusFeedbackResult) error { + if identifier.Resource != "deployments" { + return fmt.Errorf("unsupported resource type %s", identifier.Resource) + } + if identifier.Group != "apps" { + return fmt.Errorf("unsupported resource group %s", identifier.Group) + } if len(result.Values) == 0 { return fmt.Errorf("no values are probed for deployment %s/%s", identifier.Namespace, identifier.Name) } for _, value := range result.Values { - if value.Name != "readyReplicas" { + if value.Name != "ReadyReplicas" { continue } - if *value.Value.Integer >= 1 { + if *value.Value.Integer >= 0 { return nil } - return fmt.Errorf("readyReplica is %d for deployement %s/%s", *value.Value.Integer, identifier.Namespace, identifier.Name) + return fmt.Errorf("readyReplicas is %d for deployement %s/%s", *value.Value.Integer, identifier.Namespace, identifier.Name) } - return fmt.Errorf("readyReplica is not probed") + return fmt.Errorf("readyReplicas is not probed") }, }, } diff --git a/internal/addon/var.go b/internal/addon/var.go index 79be9db..fe8408f 100644 --- a/internal/addon/var.go +++ b/internal/addon/var.go @@ -16,6 +16,9 @@ const ( AdcLoggingDisabledKey = "loggingDisabled" AdcTracingisabledKey = "tracingDisabled" + + ClusterLoggingNS = "openshift-logging" + CollectorNS = "spoke-otelcol" ) //go:embed manifests diff --git a/main.go b/main.go index c7243ba..d2d4a1e 100644 --- a/main.go +++ b/main.go @@ -156,9 +156,9 @@ func runController(ctx context.Context, kubeConfig *rest.Config) error { utils.AddOnDeploymentConfigGVR, ). WithGetValuesFuncs(addonConfigValuesFn, addonhelm.GetValuesFunc(k8sClient)). + WithAgentHealthProber(addon.AgentHealthProber()). WithAgentRegistrationOption(registrationOption). WithScheme(scheme.Scheme). - WithAgentHealthProber(addon.AgentHealthProber()). BuildHelmAgentAddon() if err != nil { klog.Errorf("failed to build agent %v", err) From 1d58714ec88ead7631b9fb21847ff8b3b05d410d Mon Sep 17 00:00:00 2001 From: btaani Date: Thu, 25 Apr 2024 15:00:14 +0200 Subject: [PATCH 04/17] fix unit test --- internal/addon/addon.go | 2 +- internal/addon/addon_test.go | 28 ++++++++++++++++------------ 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index 7725a80..119505c 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -81,7 +81,7 @@ func AgentHealthProber() *agent.HealthProber { continue } - if *value.Value.Integer >= 0 { + if *value.Value.Integer >= 1 { return nil } diff --git a/internal/addon/addon_test.go b/internal/addon/addon_test.go index b4d202b..7d2e667 100644 --- a/internal/addon/addon_test.go +++ b/internal/addon/addon_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/require" appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/klog/v2" v1 "open-cluster-management.io/api/work/v1" "sigs.k8s.io/controller-runtime/pkg/client" @@ -17,6 +18,10 @@ import ( func Test_AgentHealthProber_Healthy(t *testing.T) { fakeKubeClient := fake.NewClientBuilder().Build() colDeployment := &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps", + Kind: "deployments", + }, ObjectMeta: metav1.ObjectMeta{ Name: "spoke-otelcol-collector", Namespace: "spoke-otelcol", @@ -41,7 +46,7 @@ func Test_AgentHealthProber_Healthy(t *testing.T) { }, v1.StatusFeedbackResult{ Values: []v1.FeedbackValue{ { - Name: "readyReplicas", + Name: "ReadyReplicas", Value: v1.FieldValue{ Type: v1.Integer, Integer: &readyReplicas, @@ -54,11 +59,12 @@ func Test_AgentHealthProber_Healthy(t *testing.T) { } -func Test_AgentHealthProber(t *testing.T) { - - fakeKubeClient := fake.NewClientBuilder().Build() - +func Test_AgentHealthProber_Unhealthy(t *testing.T) { cloDeployment := &appsv1.Deployment{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "apps", + Kind: "deployments", + }, ObjectMeta: metav1.ObjectMeta{ Name: "cluster-logging-operator", Namespace: "openshift-logging", @@ -67,15 +73,11 @@ func Test_AgentHealthProber(t *testing.T) { ReadyReplicas: 0, }, } - - err := fakeKubeClient.Create(context.TODO(), cloDeployment, &client.CreateOptions{}) - require.NoError(t, err) - readyReplicas := int64(cloDeployment.Status.ReadyReplicas) healthProber := AgentHealthProber() - err = healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ + err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ Group: cloDeployment.APIVersion, Resource: cloDeployment.Kind, Name: cloDeployment.Name, @@ -83,7 +85,7 @@ func Test_AgentHealthProber(t *testing.T) { }, v1.StatusFeedbackResult{ Values: []v1.FeedbackValue{ { - Name: "readyReplicas", + Name: "ReadyReplicas", Value: v1.FieldValue{ Type: v1.Integer, Integer: &readyReplicas, @@ -92,7 +94,9 @@ func Test_AgentHealthProber(t *testing.T) { }, }) - expectedErr := fmt.Errorf("readyReplica is %d for deployement %s/%s", readyReplicas, cloDeployment.Namespace, cloDeployment.Name) + klog.Info(err) + + expectedErr := fmt.Errorf("readyReplicas is %d for deployement %s/%s", readyReplicas, cloDeployment.Namespace, cloDeployment.Name) require.EqualError(t, err, expectedErr.Error()) } From e385d146f95bfacdd157175e04af4f4c00634625 Mon Sep 17 00:00:00 2001 From: Bayan Taani Date: Fri, 17 May 2024 13:10:16 +0200 Subject: [PATCH 05/17] fix probe fields --- internal/addon/addon.go | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index 119505c..ed1da7e 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -41,18 +41,24 @@ func AgentHealthProber() *agent.HealthProber { ProbeFields: []agent.ProbeField{ { ResourceIdentifier: workapiv1.ResourceIdentifier{ - Group: "apps", - Resource: "deployments", - Name: "cluster-logging-operator", - Namespace: ClusterLoggingNS, + Group: "opentelemetry.io", + Resource: "opentelemetrycollectors", + Name: "spoke-otelcol", + Namespace: CollectorNS, }, ProbeRules: []workapiv1.FeedbackRule{ { - Type: workapiv1.WellKnownStatusType, + Type: workapiv1.JSONPathsType, + JsonPaths: []workapiv1.JsonPath{ + { + Name: "replicas", + Path: ".spec.replicas", + }, + }, }, }, }, - { + /* { ResourceIdentifier: workapiv1.ResourceIdentifier{ Group: "apps", Resource: "deployments", @@ -64,20 +70,14 @@ func AgentHealthProber() *agent.HealthProber { Type: workapiv1.WellKnownStatusType, }, }, - }, + }, */ }, HealthCheck: func(identifier workapiv1.ResourceIdentifier, result workapiv1.StatusFeedbackResult) error { - if identifier.Resource != "deployments" { - return fmt.Errorf("unsupported resource type %s", identifier.Resource) - } - if identifier.Group != "apps" { - return fmt.Errorf("unsupported resource group %s", identifier.Group) - } if len(result.Values) == 0 { - return fmt.Errorf("no values are probed for deployment %s/%s", identifier.Namespace, identifier.Name) + return fmt.Errorf("no values are probed for %s/%s", identifier.Namespace, identifier.Name) } for _, value := range result.Values { - if value.Name != "ReadyReplicas" { + if value.Name != "replicas" { continue } @@ -85,9 +85,9 @@ func AgentHealthProber() *agent.HealthProber { return nil } - return fmt.Errorf("readyReplicas is %d for deployement %s/%s", *value.Value.Integer, identifier.Namespace, identifier.Name) + return fmt.Errorf("replicas is %d for %s/%s", *value.Value.Integer, identifier.Namespace, identifier.Name) } - return fmt.Errorf("readyReplicas is not probed") + return fmt.Errorf("replicas is not probed") }, }, } From 076501f77561d3da501b1b2745a21f374036db0e Mon Sep 17 00:00:00 2001 From: btaani Date: Wed, 22 May 2024 19:33:30 +0200 Subject: [PATCH 06/17] add clf and otelcol resource probes to health check --- internal/addon/addon.go | 71 ++++++++++++++++++++--------- internal/addon/addon_test.go | 88 +++++++++++++++++------------------- internal/addon/var.go | 11 ++++- 3 files changed, 99 insertions(+), 71 deletions(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index ed1da7e..de9e93c 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -1,6 +1,7 @@ package addon import ( + "errors" "fmt" "open-cluster-management.io/addon-framework/pkg/agent" @@ -10,6 +11,11 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) +var ( + ErrWrongType = errors.New("probe condition is not satisfied") + ErrValueNotProbed = errors.New("value not probed") +) + func NewRegistrationOption(agentName string) *agent.RegistrationOption { return &agent.RegistrationOption{ CSRConfigurations: agent.KubeClientSignerConfigurations(Name, agentName), @@ -41,10 +47,10 @@ func AgentHealthProber() *agent.HealthProber { ProbeFields: []agent.ProbeField{ { ResourceIdentifier: workapiv1.ResourceIdentifier{ - Group: "opentelemetry.io", - Resource: "opentelemetrycollectors", - Name: "spoke-otelcol", - Namespace: CollectorNS, + Group: OtelcolGroup, + Resource: OtelcolResource, + Name: OtelcolName, + Namespace: OtelcolNS, }, ProbeRules: []workapiv1.FeedbackRule{ { @@ -58,36 +64,57 @@ func AgentHealthProber() *agent.HealthProber { }, }, }, - /* { + { ResourceIdentifier: workapiv1.ResourceIdentifier{ - Group: "apps", - Resource: "deployments", - Name: "spoke-otelcol-collector", - Namespace: CollectorNS, + Group: ClfGroup, + Resource: ClfResource, + Name: ClfName, + Namespace: ClusterLoggingNS, }, ProbeRules: []workapiv1.FeedbackRule{ { - Type: workapiv1.WellKnownStatusType, + Type: workapiv1.JSONPathsType, + JsonPaths: []workapiv1.JsonPath{ + { + Name: "type", + Path: ".status.conditions[0].type", + }, + }, }, }, - }, */ + }, }, HealthCheck: func(identifier workapiv1.ResourceIdentifier, result workapiv1.StatusFeedbackResult) error { - if len(result.Values) == 0 { - return fmt.Errorf("no values are probed for %s/%s", identifier.Namespace, identifier.Name) - } for _, value := range result.Values { - if value.Name != "replicas" { - continue - } + if identifier.Resource == ClfResource { + if value.Name != "type" { + continue + } - if *value.Value.Integer >= 1 { - return nil - } + if *value.Value.String == "Ready" { + return nil + } + + return fmt.Errorf("%w: status condition type is %s for %s/%s", ErrWrongType, *value.Value.String, identifier.Namespace, identifier.Name) + } else if identifier.Resource == OtelcolResource { + if value.Name != "replicas" { + continue + } - return fmt.Errorf("replicas is %d for %s/%s", *value.Value.Integer, identifier.Namespace, identifier.Name) + if *value.Value.Integer >= 1 { + return nil + } + + return fmt.Errorf("%w: replicas is %d for %s/%s", ErrWrongType, *value.Value.Integer, identifier.Namespace, identifier.Name) + } else { + continue + } + } + if identifier.Resource == ClfResource || identifier.Resource == OtelcolResource { + return fmt.Errorf("%w: for %s/%s", ErrValueNotProbed, identifier.Namespace, identifier.Name) + } else { + return nil } - return fmt.Errorf("replicas is not probed") }, }, } diff --git a/internal/addon/addon_test.go b/internal/addon/addon_test.go index 7d2e667..ad5e70d 100644 --- a/internal/addon/addon_test.go +++ b/internal/addon/addon_test.go @@ -1,55 +1,55 @@ package addon import ( - "context" "fmt" "testing" + //"github.com/openshift/cluster-logging-operator/internal/status" "github.com/stretchr/testify/require" - appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/klog/v2" + "k8s.io/client-go/kubernetes/scheme" v1 "open-cluster-management.io/api/work/v1" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" + otelv1alpha1 "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" + loggingapis "github.com/openshift/cluster-logging-operator/apis" + operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" + operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" +) + +var ( + _ = loggingapis.AddToScheme(scheme.Scheme) + _ = operatorsv1.AddToScheme(scheme.Scheme) + _ = operatorsv1alpha1.AddToScheme(scheme.Scheme) ) func Test_AgentHealthProber_Healthy(t *testing.T) { - fakeKubeClient := fake.NewClientBuilder().Build() - colDeployment := &appsv1.Deployment{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "apps", - Kind: "deployments", - }, + replicas := int32(1) + otelcol := &otelv1alpha1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Name: "spoke-otelcol-collector", - Namespace: "spoke-otelcol", + Name: OtelcolName, + Namespace: OtelcolNS, }, - Status: appsv1.DeploymentStatus{ - ReadyReplicas: 1, + Spec: otelv1alpha1.OpenTelemetryCollectorSpec{ + Replicas: &replicas, }, } - err := fakeKubeClient.Create(context.TODO(), colDeployment, &client.CreateOptions{}) - require.NoError(t, err) - - readyReplicas := int64(colDeployment.Status.ReadyReplicas) - healthProber := AgentHealthProber() - err = healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ - Group: colDeployment.APIVersion, - Resource: colDeployment.Kind, - Name: colDeployment.Name, - Namespace: colDeployment.Namespace, + replicas64 := int64(replicas) + + err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ + Group: otelcol.APIVersion, + Resource: OtelcolResource, + Name: otelcol.Name, + Namespace: otelcol.Namespace, }, v1.StatusFeedbackResult{ Values: []v1.FeedbackValue{ { - Name: "ReadyReplicas", + Name: "replicas", Value: v1.FieldValue{ Type: v1.Integer, - Integer: &readyReplicas, + Integer: &replicas64, }, }, }, @@ -60,43 +60,37 @@ func Test_AgentHealthProber_Healthy(t *testing.T) { } func Test_AgentHealthProber_Unhealthy(t *testing.T) { - cloDeployment := &appsv1.Deployment{ - TypeMeta: metav1.TypeMeta{ - APIVersion: "apps", - Kind: "deployments", - }, + replicas := int32(0) + otelcol := &otelv1alpha1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Name: "cluster-logging-operator", - Namespace: "openshift-logging", + Name: OtelcolName, + Namespace: OtelcolNS, }, - Status: appsv1.DeploymentStatus{ - ReadyReplicas: 0, + Spec: otelv1alpha1.OpenTelemetryCollectorSpec{ + Replicas: &replicas, }, } - readyReplicas := int64(cloDeployment.Status.ReadyReplicas) - healthProber := AgentHealthProber() + replicas64 := int64(replicas) err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ - Group: cloDeployment.APIVersion, - Resource: cloDeployment.Kind, - Name: cloDeployment.Name, - Namespace: cloDeployment.Namespace, + Group: otelcol.APIVersion, + Resource: OtelcolResource, + Name: otelcol.Name, + Namespace: otelcol.Namespace, }, v1.StatusFeedbackResult{ Values: []v1.FeedbackValue{ { - Name: "ReadyReplicas", + Name: "replicas", Value: v1.FieldValue{ Type: v1.Integer, - Integer: &readyReplicas, + Integer: &replicas64, }, }, }, }) - klog.Info(err) - - expectedErr := fmt.Errorf("readyReplicas is %d for deployement %s/%s", readyReplicas, cloDeployment.Namespace, cloDeployment.Name) + expectedErr := fmt.Errorf("%w: replicas is %d for %s/%s", ErrWrongType, replicas, otelcol.Namespace, otelcol.Name) require.EqualError(t, err, expectedErr.Error()) } diff --git a/internal/addon/var.go b/internal/addon/var.go index fe8408f..a5739ee 100644 --- a/internal/addon/var.go +++ b/internal/addon/var.go @@ -17,8 +17,15 @@ const ( AdcLoggingDisabledKey = "loggingDisabled" AdcTracingisabledKey = "tracingDisabled" - ClusterLoggingNS = "openshift-logging" - CollectorNS = "spoke-otelcol" + ClfGroup = "logging.openshift.io" + ClfResource = "clusterlogforwarders" + ClfName = "instance" + ClusterLoggingNS = "openshift-logging" + + OtelcolGroup = "opentelemetry.io" + OtelcolResource = "opentelemetrycollectors" + OtelcolName = "spoke-otelcol" + OtelcolNS = "spoke-otelcol" ) //go:embed manifests From a5b5a274ec5d8a21ce5f69198ee1540b0cd5b9b4 Mon Sep 17 00:00:00 2001 From: Bayan Taani <86984560+btaani@users.noreply.github.com> Date: Thu, 23 May 2024 11:30:21 +0200 Subject: [PATCH 07/17] Update internal/addon/addon.go Co-authored-by: Joao Marcal --- internal/addon/addon.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index de9e93c..9c02030 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -12,8 +12,8 @@ import ( ) var ( - ErrWrongType = errors.New("probe condition is not satisfied") - ErrValueNotProbed = errors.New("value not probed") + errWrongType = errors.New("probe condition is not satisfied") + errValueNotProbed = errors.New("value not probed") ) func NewRegistrationOption(agentName string) *agent.RegistrationOption { From bafd7b235bbca53ae3a47fe261fe711b4f8c7201 Mon Sep 17 00:00:00 2001 From: Bayan Taani <86984560+btaani@users.noreply.github.com> Date: Thu, 23 May 2024 11:30:37 +0200 Subject: [PATCH 08/17] Update internal/addon/addon.go Co-authored-by: Joao Marcal --- internal/addon/addon.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index 9c02030..07a33b3 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -112,9 +112,7 @@ func AgentHealthProber() *agent.HealthProber { } if identifier.Resource == ClfResource || identifier.Resource == OtelcolResource { return fmt.Errorf("%w: for %s/%s", ErrValueNotProbed, identifier.Namespace, identifier.Name) - } else { - return nil - } + return nil }, }, } From 6447b61665256a0c59f2b3e0f79b1b1e3fd22997 Mon Sep 17 00:00:00 2001 From: Bayan Taani <86984560+btaani@users.noreply.github.com> Date: Thu, 23 May 2024 11:31:24 +0200 Subject: [PATCH 09/17] Update internal/addon/addon.go Co-authored-by: Joao Marcal --- internal/addon/addon.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index 07a33b3..b8db9a4 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -111,7 +111,7 @@ func AgentHealthProber() *agent.HealthProber { } } if identifier.Resource == ClfResource || identifier.Resource == OtelcolResource { - return fmt.Errorf("%w: for %s/%s", ErrValueNotProbed, identifier.Namespace, identifier.Name) + return fmt.Errorf("%w: for resource %s with key %s/%s", ErrValueNotProbed, identifier.Resource, identifier.Namespace, identifier.Name) return nil }, }, From ee9511ba14e4cf07d578ba2846f2349272d2d972 Mon Sep 17 00:00:00 2001 From: Bayan Taani <86984560+btaani@users.noreply.github.com> Date: Thu, 23 May 2024 11:31:34 +0200 Subject: [PATCH 10/17] Update internal/addon/addon_test.go Co-authored-by: Joao Marcal --- internal/addon/addon_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/addon/addon_test.go b/internal/addon/addon_test.go index ad5e70d..0ca8be4 100644 --- a/internal/addon/addon_test.go +++ b/internal/addon/addon_test.go @@ -49,7 +49,7 @@ func Test_AgentHealthProber_Healthy(t *testing.T) { Name: "replicas", Value: v1.FieldValue{ Type: v1.Integer, - Integer: &replicas64, + Integer: ptr.To(1), }, }, }, From 84a582e50cbb97bd8215a6e4b8d84118774cc1dc Mon Sep 17 00:00:00 2001 From: Joao Marcal Date: Wed, 29 May 2024 15:57:02 +0100 Subject: [PATCH 11/17] fixes tests and updates probe type to HealthProberTypeWork --- internal/addon/addon.go | 34 ++++++++++++++++++---------------- internal/addon/addon_test.go | 18 ++++++------------ 2 files changed, 24 insertions(+), 28 deletions(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index b8db9a4..a1c7b7f 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -12,7 +12,7 @@ import ( ) var ( - errWrongType = errors.New("probe condition is not satisfied") + errUnavailable = errors.New("probe condition is not satisfied") errValueNotProbed = errors.New("value not probed") ) @@ -42,7 +42,7 @@ func GetObjectKey(configRef []addonapiv1alpha1.ConfigReference, group, resource func AgentHealthProber() *agent.HealthProber { return &agent.HealthProber{ - Type: agent.HealthProberTypeDeploymentAvailability, + Type: agent.HealthProberTypeWork, WorkProber: &agent.WorkHealthProber{ ProbeFields: []agent.ProbeField{ { @@ -76,8 +76,8 @@ func AgentHealthProber() *agent.HealthProber { Type: workapiv1.JSONPathsType, JsonPaths: []workapiv1.JsonPath{ { - Name: "type", - Path: ".status.conditions[0].type", + Name: "isReady", + Path: ".status.conditions[?(@.type==\"Ready\")].status", }, }, }, @@ -86,33 +86,35 @@ func AgentHealthProber() *agent.HealthProber { }, HealthCheck: func(identifier workapiv1.ResourceIdentifier, result workapiv1.StatusFeedbackResult) error { for _, value := range result.Values { - if identifier.Resource == ClfResource { - if value.Name != "type" { + switch { + case identifier.Resource == ClfResource: + if value.Name != "isReady" { continue } - if *value.Value.String == "Ready" { - return nil + if *value.Value.String != "True" { + return fmt.Errorf("%w: status condition type is %s for %s/%s", errUnavailable, *value.Value.String, identifier.Namespace, identifier.Name) } - return fmt.Errorf("%w: status condition type is %s for %s/%s", ErrWrongType, *value.Value.String, identifier.Namespace, identifier.Name) - } else if identifier.Resource == OtelcolResource { + return nil + case identifier.Resource == OtelcolResource: if value.Name != "replicas" { continue } - if *value.Value.Integer >= 1 { - return nil + if *value.Value.Integer < 1 { + return fmt.Errorf("%w: replicas is %d for %s/%s", errUnavailable, *value.Value.Integer, identifier.Namespace, identifier.Name) } - return fmt.Errorf("%w: replicas is %d for %s/%s", ErrWrongType, *value.Value.Integer, identifier.Namespace, identifier.Name) - } else { + return nil + default: continue } } if identifier.Resource == ClfResource || identifier.Resource == OtelcolResource { - return fmt.Errorf("%w: for resource %s with key %s/%s", ErrValueNotProbed, identifier.Resource, identifier.Namespace, identifier.Name) - return nil + return fmt.Errorf("%w: for resource %s with key %s/%s", errValueNotProbed, identifier.Resource, identifier.Namespace, identifier.Name) + } + return nil }, }, } diff --git a/internal/addon/addon_test.go b/internal/addon/addon_test.go index 0ca8be4..2a33cd4 100644 --- a/internal/addon/addon_test.go +++ b/internal/addon/addon_test.go @@ -8,6 +8,7 @@ import ( "github.com/stretchr/testify/require" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/utils/ptr" v1 "open-cluster-management.io/api/work/v1" otelv1alpha1 "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" @@ -23,21 +24,18 @@ var ( ) func Test_AgentHealthProber_Healthy(t *testing.T) { - replicas := int32(1) otelcol := &otelv1alpha1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Name: OtelcolName, Namespace: OtelcolNS, }, Spec: otelv1alpha1.OpenTelemetryCollectorSpec{ - Replicas: &replicas, + Replicas: ptr.To[int32](1), }, } healthProber := AgentHealthProber() - replicas64 := int64(replicas) - err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ Group: otelcol.APIVersion, Resource: OtelcolResource, @@ -49,30 +47,27 @@ func Test_AgentHealthProber_Healthy(t *testing.T) { Name: "replicas", Value: v1.FieldValue{ Type: v1.Integer, - Integer: ptr.To(1), + Integer: ptr.To[int64](1), }, }, }, }) require.NoError(t, err) - } func Test_AgentHealthProber_Unhealthy(t *testing.T) { - replicas := int32(0) otelcol := &otelv1alpha1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ Name: OtelcolName, Namespace: OtelcolNS, }, Spec: otelv1alpha1.OpenTelemetryCollectorSpec{ - Replicas: &replicas, + Replicas: ptr.To[int32](0), }, } healthProber := AgentHealthProber() - replicas64 := int64(replicas) err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ Group: otelcol.APIVersion, Resource: OtelcolResource, @@ -84,13 +79,12 @@ func Test_AgentHealthProber_Unhealthy(t *testing.T) { Name: "replicas", Value: v1.FieldValue{ Type: v1.Integer, - Integer: &replicas64, + Integer: ptr.To[int64](0), }, }, }, }) - expectedErr := fmt.Errorf("%w: replicas is %d for %s/%s", ErrWrongType, replicas, otelcol.Namespace, otelcol.Name) + expectedErr := fmt.Errorf("%w: replicas is %d for %s/%s", errUnavailable, 0, otelcol.Namespace, otelcol.Name) require.EqualError(t, err, expectedErr.Error()) - } From aa25f9b693645714796165eb7a880933ff710ae8 Mon Sep 17 00:00:00 2001 From: Joao Marcal Date: Wed, 29 May 2024 18:30:00 +0100 Subject: [PATCH 12/17] updates health check function --- internal/addon/addon.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index a1c7b7f..fab1512 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -111,10 +111,7 @@ func AgentHealthProber() *agent.HealthProber { continue } } - if identifier.Resource == ClfResource || identifier.Resource == OtelcolResource { - return fmt.Errorf("%w: for resource %s with key %s/%s", errValueNotProbed, identifier.Resource, identifier.Namespace, identifier.Name) - } - return nil + return fmt.Errorf("%w: for resource %s with key %s/%s", errValueNotProbed, identifier.Resource, identifier.Namespace, identifier.Name) }, }, } From c148205c8574fd8801b862e14c9560eeff5c25fd Mon Sep 17 00:00:00 2001 From: Joao Marcal Date: Fri, 31 May 2024 11:26:29 +0100 Subject: [PATCH 13/17] fix naming of variables --- internal/addon/addon.go | 26 ++++++++++++++------------ internal/addon/addon_test.go | 12 ++++++------ internal/addon/var.go | 16 +++++++--------- internal/logging/handlers/handler.go | 6 +----- internal/tracing/handlers/handler.go | 6 +----- 5 files changed, 29 insertions(+), 37 deletions(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index fab1512..34364d3 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -4,6 +4,8 @@ import ( "errors" "fmt" + otelv1alpha1 "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" + loggingv1 "github.com/openshift/cluster-logging-operator/apis/logging/v1" "open-cluster-management.io/addon-framework/pkg/agent" "open-cluster-management.io/addon-framework/pkg/utils" addonapiv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" @@ -47,10 +49,10 @@ func AgentHealthProber() *agent.HealthProber { ProbeFields: []agent.ProbeField{ { ResourceIdentifier: workapiv1.ResourceIdentifier{ - Group: OtelcolGroup, - Resource: OtelcolResource, - Name: OtelcolName, - Namespace: OtelcolNS, + Group: otelv1alpha1.GroupVersion.Group, + Resource: OpenTelemetryCollectorsResource, + Name: spokeOTELColName, + Namespace: spokeOTELColNamespace, }, ProbeRules: []workapiv1.FeedbackRule{ { @@ -66,10 +68,10 @@ func AgentHealthProber() *agent.HealthProber { }, { ResourceIdentifier: workapiv1.ResourceIdentifier{ - Group: ClfGroup, - Resource: ClfResource, - Name: ClfName, - Namespace: ClusterLoggingNS, + Group: loggingv1.GroupVersion.Group, + Resource: ClusterLogForwardersResource, + Name: spokeCLFName, + Namespace: spokeLoggingNamespace, }, ProbeRules: []workapiv1.FeedbackRule{ { @@ -87,23 +89,23 @@ func AgentHealthProber() *agent.HealthProber { HealthCheck: func(identifier workapiv1.ResourceIdentifier, result workapiv1.StatusFeedbackResult) error { for _, value := range result.Values { switch { - case identifier.Resource == ClfResource: + case identifier.Resource == ClusterLogForwardersResource: if value.Name != "isReady" { continue } if *value.Value.String != "True" { - return fmt.Errorf("%w: status condition type is %s for %s/%s", errUnavailable, *value.Value.String, identifier.Namespace, identifier.Name) + return fmt.Errorf("%w: clusterlogforwarder status condition type is %s for %s/%s", errUnavailable, *value.Value.String, identifier.Namespace, identifier.Name) } return nil - case identifier.Resource == OtelcolResource: + case identifier.Resource == OpenTelemetryCollectorsResource: if value.Name != "replicas" { continue } if *value.Value.Integer < 1 { - return fmt.Errorf("%w: replicas is %d for %s/%s", errUnavailable, *value.Value.Integer, identifier.Namespace, identifier.Name) + return fmt.Errorf("%w: opentelemetrycollector replicas is %d for %s/%s", errUnavailable, *value.Value.Integer, identifier.Namespace, identifier.Name) } return nil diff --git a/internal/addon/addon_test.go b/internal/addon/addon_test.go index 2a33cd4..3916857 100644 --- a/internal/addon/addon_test.go +++ b/internal/addon/addon_test.go @@ -26,8 +26,8 @@ var ( func Test_AgentHealthProber_Healthy(t *testing.T) { otelcol := &otelv1alpha1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Name: OtelcolName, - Namespace: OtelcolNS, + Name: spokeOTELColName, + Namespace: spokeOTELColNamespace, }, Spec: otelv1alpha1.OpenTelemetryCollectorSpec{ Replicas: ptr.To[int32](1), @@ -38,7 +38,7 @@ func Test_AgentHealthProber_Healthy(t *testing.T) { err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ Group: otelcol.APIVersion, - Resource: OtelcolResource, + Resource: OpenTelemetryCollectorsResource, Name: otelcol.Name, Namespace: otelcol.Namespace, }, v1.StatusFeedbackResult{ @@ -59,8 +59,8 @@ func Test_AgentHealthProber_Healthy(t *testing.T) { func Test_AgentHealthProber_Unhealthy(t *testing.T) { otelcol := &otelv1alpha1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Name: OtelcolName, - Namespace: OtelcolNS, + Name: spokeOTELColName, + Namespace: spokeOTELColNamespace, }, Spec: otelv1alpha1.OpenTelemetryCollectorSpec{ Replicas: ptr.To[int32](0), @@ -70,7 +70,7 @@ func Test_AgentHealthProber_Unhealthy(t *testing.T) { err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ Group: otelcol.APIVersion, - Resource: OtelcolResource, + Resource: OpenTelemetryCollectorsResource, Name: otelcol.Name, Namespace: otelcol.Namespace, }, v1.StatusFeedbackResult{ diff --git a/internal/addon/var.go b/internal/addon/var.go index a5739ee..6057fe5 100644 --- a/internal/addon/var.go +++ b/internal/addon/var.go @@ -17,15 +17,13 @@ const ( AdcLoggingDisabledKey = "loggingDisabled" AdcTracingisabledKey = "tracingDisabled" - ClfGroup = "logging.openshift.io" - ClfResource = "clusterlogforwarders" - ClfName = "instance" - ClusterLoggingNS = "openshift-logging" - - OtelcolGroup = "opentelemetry.io" - OtelcolResource = "opentelemetrycollectors" - OtelcolName = "spoke-otelcol" - OtelcolNS = "spoke-otelcol" + ClusterLogForwardersResource = "clusterlogforwarders" + spokeCLFName = "instance" + spokeLoggingNamespace = "openshift-logging" + + OpenTelemetryCollectorsResource = "opentelemetrycollectors" + spokeOTELColName = "spoke-otelcol" + spokeOTELColNamespace = "spoke-otelcol" ) //go:embed manifests diff --git a/internal/logging/handlers/handler.go b/internal/logging/handlers/handler.go index 6d2998e..e83abff 100644 --- a/internal/logging/handlers/handler.go +++ b/internal/logging/handlers/handler.go @@ -11,16 +11,12 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -const ( - clusterLogForwarderResource = "clusterlogforwarders" -) - func BuildOptions(k8s client.Client, mcAddon *addonapiv1alpha1.ManagedClusterAddOn, adoc *addonapiv1alpha1.AddOnDeploymentConfig) (manifests.Options, error) { resources := manifests.Options{ AddOnDeploymentConfig: adoc, } - key := addon.GetObjectKey(mcAddon.Status.ConfigReferences, loggingv1.GroupVersion.Group, clusterLogForwarderResource) + key := addon.GetObjectKey(mcAddon.Status.ConfigReferences, loggingv1.GroupVersion.Group, addon.ClusterLogForwardersResource) clf := &loggingv1.ClusterLogForwarder{} if err := k8s.Get(context.Background(), key, clf, &client.GetOptions{}); err != nil { return resources, err diff --git a/internal/tracing/handlers/handler.go b/internal/tracing/handlers/handler.go index 56799d6..da18024 100644 --- a/internal/tracing/handlers/handler.go +++ b/internal/tracing/handlers/handler.go @@ -16,10 +16,6 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" ) -const ( - opentelemetryCollectorResource = "opentelemetrycollectors" -) - var ( errNoExportersFound = fmt.Errorf("no exporters found") errNoMountPathFound = fmt.Errorf("mountpath not found in any secret") @@ -32,7 +28,7 @@ func BuildOptions(k8s client.Client, mcAddon *addonapiv1alpha1.ManagedClusterAdd ClusterName: mcAddon.Namespace, } - key := addon.GetObjectKey(mcAddon.Status.ConfigReferences, otelv1alpha1.GroupVersion.Group, opentelemetryCollectorResource) + key := addon.GetObjectKey(mcAddon.Status.ConfigReferences, otelv1alpha1.GroupVersion.Group, addon.OpenTelemetryCollectorsResource) otelCol := &otelv1alpha1.OpenTelemetryCollector{} if err := k8s.Get(context.Background(), key, otelCol, &client.GetOptions{}); err != nil { return resources, err From e245bee401f8415ab5358d45a3f6b82b9115510f Mon Sep 17 00:00:00 2001 From: Joao Marcal Date: Fri, 31 May 2024 12:18:06 +0100 Subject: [PATCH 14/17] add tests for logging and move variables to consts --- internal/addon/addon.go | 28 +++---- internal/addon/addon_test.go | 148 ++++++++++++++++++---------------- internal/addon/var.go | 12 ++- internal/logging/helm_test.go | 4 + internal/tracing/helm_test.go | 6 +- 5 files changed, 109 insertions(+), 89 deletions(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index 34364d3..69bfda8 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -49,18 +49,18 @@ func AgentHealthProber() *agent.HealthProber { ProbeFields: []agent.ProbeField{ { ResourceIdentifier: workapiv1.ResourceIdentifier{ - Group: otelv1alpha1.GroupVersion.Group, - Resource: OpenTelemetryCollectorsResource, - Name: spokeOTELColName, - Namespace: spokeOTELColNamespace, + Group: loggingv1.GroupVersion.Group, + Resource: ClusterLogForwardersResource, + Name: SpokeCLFName, + Namespace: SpokeCLFNamespace, }, ProbeRules: []workapiv1.FeedbackRule{ { Type: workapiv1.JSONPathsType, JsonPaths: []workapiv1.JsonPath{ { - Name: "replicas", - Path: ".spec.replicas", + Name: clfProbeKey, + Path: clfProbePath, }, }, }, @@ -68,18 +68,18 @@ func AgentHealthProber() *agent.HealthProber { }, { ResourceIdentifier: workapiv1.ResourceIdentifier{ - Group: loggingv1.GroupVersion.Group, - Resource: ClusterLogForwardersResource, - Name: spokeCLFName, - Namespace: spokeLoggingNamespace, + Group: otelv1alpha1.GroupVersion.Group, + Resource: OpenTelemetryCollectorsResource, + Name: SpokeOTELColName, + Namespace: SpokeOTELColNamespace, }, ProbeRules: []workapiv1.FeedbackRule{ { Type: workapiv1.JSONPathsType, JsonPaths: []workapiv1.JsonPath{ { - Name: "isReady", - Path: ".status.conditions[?(@.type==\"Ready\")].status", + Name: otelColProbeKey, + Path: otelColProbePath, }, }, }, @@ -90,7 +90,7 @@ func AgentHealthProber() *agent.HealthProber { for _, value := range result.Values { switch { case identifier.Resource == ClusterLogForwardersResource: - if value.Name != "isReady" { + if value.Name != clfProbeKey { continue } @@ -100,7 +100,7 @@ func AgentHealthProber() *agent.HealthProber { return nil case identifier.Resource == OpenTelemetryCollectorsResource: - if value.Name != "replicas" { + if value.Name != otelColProbeKey { continue } diff --git a/internal/addon/addon_test.go b/internal/addon/addon_test.go index 3916857..d7a9577 100644 --- a/internal/addon/addon_test.go +++ b/internal/addon/addon_test.go @@ -4,87 +4,97 @@ import ( "fmt" "testing" - //"github.com/openshift/cluster-logging-operator/internal/status" "github.com/stretchr/testify/require" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/client-go/kubernetes/scheme" - "k8s.io/utils/ptr" v1 "open-cluster-management.io/api/work/v1" otelv1alpha1 "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" - loggingapis "github.com/openshift/cluster-logging-operator/apis" - operatorsv1 "github.com/operator-framework/api/pkg/operators/v1" - operatorsv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1" + loggingv1 "github.com/openshift/cluster-logging-operator/apis/logging/v1" ) -var ( - _ = loggingapis.AddToScheme(scheme.Scheme) - _ = operatorsv1.AddToScheme(scheme.Scheme) - _ = operatorsv1alpha1.AddToScheme(scheme.Scheme) -) - -func Test_AgentHealthProber_Healthy(t *testing.T) { - otelcol := &otelv1alpha1.OpenTelemetryCollector{ - ObjectMeta: metav1.ObjectMeta{ - Name: spokeOTELColName, - Namespace: spokeOTELColNamespace, +func Test_AgentHealthProber_CLF(t *testing.T) { + unhealthyError := fmt.Errorf("%w: clusterlogforwarder status condition type is %s for %s/%s", errUnavailable, "False", SpokeCLFNamespace, SpokeCLFName) + for _, tc := range []struct { + name string + status string + expectedErr string + }{ + { + name: "healthy", + status: "True", }, - Spec: otelv1alpha1.OpenTelemetryCollectorSpec{ - Replicas: ptr.To[int32](1), + { + name: "unhealthy", + status: "False", + expectedErr: unhealthyError.Error(), }, - } - - healthProber := AgentHealthProber() - - err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ - Group: otelcol.APIVersion, - Resource: OpenTelemetryCollectorsResource, - Name: otelcol.Name, - Namespace: otelcol.Namespace, - }, v1.StatusFeedbackResult{ - Values: []v1.FeedbackValue{ - { - Name: "replicas", - Value: v1.FieldValue{ - Type: v1.Integer, - Integer: ptr.To[int64](1), + } { + t.Run(tc.name, func(t *testing.T) { + healthProber := AgentHealthProber() + err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ + Group: loggingv1.GroupVersion.Group, + Resource: ClusterLogForwardersResource, + Name: SpokeCLFName, + Namespace: SpokeCLFNamespace, + }, v1.StatusFeedbackResult{ + Values: []v1.FeedbackValue{ + { + Name: "isReady", + Value: v1.FieldValue{ + Type: v1.String, + String: &tc.status, + }, + }, }, - }, - }, - }) - - require.NoError(t, err) + }) + if tc.expectedErr != "" { + require.EqualError(t, err, tc.expectedErr) + return + } + require.NoError(t, err) + }) + } } -func Test_AgentHealthProber_Unhealthy(t *testing.T) { - otelcol := &otelv1alpha1.OpenTelemetryCollector{ - ObjectMeta: metav1.ObjectMeta{ - Name: spokeOTELColName, - Namespace: spokeOTELColNamespace, +func Test_AgentHealthProber_OTELCol(t *testing.T) { + unhealthyError := fmt.Errorf("%w: opentelemetrycollector replicas is %d for %s/%s", errUnavailable, 0, SpokeOTELColNamespace, SpokeOTELColName) + for _, tc := range []struct { + name string + replicas int64 + expectedErr string + }{ + { + name: "healthy", + replicas: 1, }, - Spec: otelv1alpha1.OpenTelemetryCollectorSpec{ - Replicas: ptr.To[int32](0), + { + name: "unhealthy", + replicas: 0, + expectedErr: unhealthyError.Error(), }, - } - healthProber := AgentHealthProber() - - err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ - Group: otelcol.APIVersion, - Resource: OpenTelemetryCollectorsResource, - Name: otelcol.Name, - Namespace: otelcol.Namespace, - }, v1.StatusFeedbackResult{ - Values: []v1.FeedbackValue{ - { - Name: "replicas", - Value: v1.FieldValue{ - Type: v1.Integer, - Integer: ptr.To[int64](0), + } { + t.Run(tc.name, func(t *testing.T) { + healthProber := AgentHealthProber() + err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ + Group: otelv1alpha1.GroupVersion.Group, + Resource: OpenTelemetryCollectorsResource, + Name: SpokeOTELColName, + Namespace: SpokeOTELColNamespace, + }, v1.StatusFeedbackResult{ + Values: []v1.FeedbackValue{ + { + Name: "replicas", + Value: v1.FieldValue{ + Type: v1.Integer, + Integer: &tc.replicas, + }, + }, }, - }, - }, - }) - - expectedErr := fmt.Errorf("%w: replicas is %d for %s/%s", errUnavailable, 0, otelcol.Namespace, otelcol.Name) - require.EqualError(t, err, expectedErr.Error()) + }) + if tc.expectedErr != "" { + require.EqualError(t, err, tc.expectedErr) + return + } + require.NoError(t, err) + }) + } } diff --git a/internal/addon/var.go b/internal/addon/var.go index 6057fe5..6bb2e61 100644 --- a/internal/addon/var.go +++ b/internal/addon/var.go @@ -18,12 +18,16 @@ const ( AdcTracingisabledKey = "tracingDisabled" ClusterLogForwardersResource = "clusterlogforwarders" - spokeCLFName = "instance" - spokeLoggingNamespace = "openshift-logging" + SpokeCLFName = "instance" + SpokeCLFNamespace = "openshift-logging" + clfProbeKey = "isReady" + clfProbePath = ".status.conditions[?(@.type==\"Ready\")].status" OpenTelemetryCollectorsResource = "opentelemetrycollectors" - spokeOTELColName = "spoke-otelcol" - spokeOTELColNamespace = "spoke-otelcol" + SpokeOTELColName = "spoke-otelcol" + SpokeOTELColNamespace = "spoke-otelcol" + otelColProbeKey = "replicas" + otelColProbePath = ".spec.replicas" ) //go:embed manifests diff --git a/internal/logging/helm_test.go b/internal/logging/helm_test.go index 51c855b..d52b4e3 100644 --- a/internal/logging/helm_test.go +++ b/internal/logging/helm_test.go @@ -225,6 +225,10 @@ func Test_Logging_AllConfigsTogether_AllResources(t *testing.T) { require.NotNil(t, obj.Spec.Outputs[1].Secret) require.Equal(t, "static-authentication", obj.Spec.Outputs[0].Secret.Name) require.Equal(t, "static-authentication", obj.Spec.Outputs[1].Secret.Name) + // Check name and namespace to make usre that if we change the helm + // manifests that we don't break the addon probes + require.Equal(t, addon.SpokeCLFName, obj.Name) + require.Equal(t, addon.SpokeCLFNamespace, obj.Namespace) case *corev1.Secret: if obj.Name == "static-authentication" { require.Equal(t, staticCred.Data, obj.Data) diff --git a/internal/tracing/helm_test.go b/internal/tracing/helm_test.go index e193068..f498cee 100644 --- a/internal/tracing/helm_test.go +++ b/internal/tracing/helm_test.go @@ -188,8 +188,10 @@ func Test_Tracing_AllConfigsTogether_AllResources(t *testing.T) { for _, obj := range objects { switch obj := obj.(type) { case *otelv1alpha1.OpenTelemetryCollector: - require.Equal(t, "spoke-otelcol", obj.ObjectMeta.Name) - require.Equal(t, "spoke-otelcol", obj.ObjectMeta.Namespace) + // Check name and namespace to make usre that if we change the helm + // manifests that we don't break the addon probes + require.Equal(t, addon.SpokeOTELColName, obj.Name) + require.Equal(t, addon.SpokeOTELColNamespace, obj.Namespace) require.NotEmpty(t, obj.Spec.Config) case *corev1.Secret: if obj.Name == "tracing-otlphttp-auth" { From 6a24ada1b71a02ea04372eb204f84aa7bd59854a Mon Sep 17 00:00:00 2001 From: Joao Marcal Date: Mon, 3 Jun 2024 17:24:27 +0100 Subject: [PATCH 15/17] address reviews --- internal/addon/addon.go | 39 ++++++++++++------- internal/addon/addon_test.go | 26 ++++++------- .../templates/clusterlogforwarder.yaml | 2 +- .../charts/tracing/templates/namespace.yaml | 2 +- .../templates/opentelemetrycollector.yaml | 4 +- .../mcoa/charts/tracing/templates/secret.yaml | 2 +- internal/addon/var.go | 6 +-- internal/logging/helm_test.go | 6 +-- internal/logging/manifests/logging_test.go | 4 +- internal/tracing/helm_test.go | 6 +-- 10 files changed, 54 insertions(+), 43 deletions(-) diff --git a/internal/addon/addon.go b/internal/addon/addon.go index 69bfda8..8a9f9db 100644 --- a/internal/addon/addon.go +++ b/internal/addon/addon.go @@ -9,13 +9,14 @@ import ( "open-cluster-management.io/addon-framework/pkg/agent" "open-cluster-management.io/addon-framework/pkg/utils" addonapiv1alpha1 "open-cluster-management.io/api/addon/v1alpha1" - workapiv1 "open-cluster-management.io/api/work/v1" + workv1 "open-cluster-management.io/api/work/v1" "sigs.k8s.io/controller-runtime/pkg/client" ) var ( - errUnavailable = errors.New("probe condition is not satisfied") - errValueNotProbed = errors.New("value not probed") + errProbeConditionNotSatisfied = errors.New("probe condition is not satisfied") + errProbeValueIsNil = errors.New("probe value is nil") + errValueNotProbed = errors.New("value not probed") ) func NewRegistrationOption(agentName string) *agent.RegistrationOption { @@ -42,22 +43,24 @@ func GetObjectKey(configRef []addonapiv1alpha1.ConfigReference, group, resource return key } +// AgentHealthProber returns a HealthProber struct that contains the necessary +// information to assert if an addon deployment is ready or not. func AgentHealthProber() *agent.HealthProber { return &agent.HealthProber{ Type: agent.HealthProberTypeWork, WorkProber: &agent.WorkHealthProber{ ProbeFields: []agent.ProbeField{ { - ResourceIdentifier: workapiv1.ResourceIdentifier{ + ResourceIdentifier: workv1.ResourceIdentifier{ Group: loggingv1.GroupVersion.Group, Resource: ClusterLogForwardersResource, Name: SpokeCLFName, Namespace: SpokeCLFNamespace, }, - ProbeRules: []workapiv1.FeedbackRule{ + ProbeRules: []workv1.FeedbackRule{ { - Type: workapiv1.JSONPathsType, - JsonPaths: []workapiv1.JsonPath{ + Type: workv1.JSONPathsType, + JsonPaths: []workv1.JsonPath{ { Name: clfProbeKey, Path: clfProbePath, @@ -67,16 +70,16 @@ func AgentHealthProber() *agent.HealthProber { }, }, { - ResourceIdentifier: workapiv1.ResourceIdentifier{ + ResourceIdentifier: workv1.ResourceIdentifier{ Group: otelv1alpha1.GroupVersion.Group, Resource: OpenTelemetryCollectorsResource, Name: SpokeOTELColName, Namespace: SpokeOTELColNamespace, }, - ProbeRules: []workapiv1.FeedbackRule{ + ProbeRules: []workv1.FeedbackRule{ { - Type: workapiv1.JSONPathsType, - JsonPaths: []workapiv1.JsonPath{ + Type: workv1.JSONPathsType, + JsonPaths: []workv1.JsonPath{ { Name: otelColProbeKey, Path: otelColProbePath, @@ -86,7 +89,7 @@ func AgentHealthProber() *agent.HealthProber { }, }, }, - HealthCheck: func(identifier workapiv1.ResourceIdentifier, result workapiv1.StatusFeedbackResult) error { + HealthCheck: func(identifier workv1.ResourceIdentifier, result workv1.StatusFeedbackResult) error { for _, value := range result.Values { switch { case identifier.Resource == ClusterLogForwardersResource: @@ -94,8 +97,12 @@ func AgentHealthProber() *agent.HealthProber { continue } + if value.Value.String == nil { + return fmt.Errorf("%w: clusterlogforwarder with key %s/%s", errProbeValueIsNil, identifier.Namespace, identifier.Name) + } + if *value.Value.String != "True" { - return fmt.Errorf("%w: clusterlogforwarder status condition type is %s for %s/%s", errUnavailable, *value.Value.String, identifier.Namespace, identifier.Name) + return fmt.Errorf("%w: clusterlogforwarder status condition type is %s for %s/%s", errProbeConditionNotSatisfied, *value.Value.String, identifier.Namespace, identifier.Name) } return nil @@ -104,8 +111,12 @@ func AgentHealthProber() *agent.HealthProber { continue } + if value.Value.Integer == nil { + return fmt.Errorf("%w: opentelemetrycollector with key %s/%s", errProbeValueIsNil, identifier.Namespace, identifier.Name) + } + if *value.Value.Integer < 1 { - return fmt.Errorf("%w: opentelemetrycollector replicas is %d for %s/%s", errUnavailable, *value.Value.Integer, identifier.Namespace, identifier.Name) + return fmt.Errorf("%w: opentelemetrycollector replicas is %d for %s/%s", errProbeConditionNotSatisfied, *value.Value.Integer, identifier.Namespace, identifier.Name) } return nil diff --git a/internal/addon/addon_test.go b/internal/addon/addon_test.go index d7a9577..c8b097e 100644 --- a/internal/addon/addon_test.go +++ b/internal/addon/addon_test.go @@ -5,14 +5,14 @@ import ( "testing" "github.com/stretchr/testify/require" - v1 "open-cluster-management.io/api/work/v1" + workv1 "open-cluster-management.io/api/work/v1" otelv1alpha1 "github.com/open-telemetry/opentelemetry-operator/apis/v1alpha1" loggingv1 "github.com/openshift/cluster-logging-operator/apis/logging/v1" ) func Test_AgentHealthProber_CLF(t *testing.T) { - unhealthyError := fmt.Errorf("%w: clusterlogforwarder status condition type is %s for %s/%s", errUnavailable, "False", SpokeCLFNamespace, SpokeCLFName) + unhealthyError := fmt.Errorf("%w: clusterlogforwarder status condition type is %s for %s/%s", errProbeConditionNotSatisfied, "False", SpokeCLFNamespace, SpokeCLFName) for _, tc := range []struct { name string status string @@ -30,17 +30,17 @@ func Test_AgentHealthProber_CLF(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { healthProber := AgentHealthProber() - err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ + err := healthProber.WorkProber.HealthCheck(workv1.ResourceIdentifier{ Group: loggingv1.GroupVersion.Group, Resource: ClusterLogForwardersResource, Name: SpokeCLFName, Namespace: SpokeCLFNamespace, - }, v1.StatusFeedbackResult{ - Values: []v1.FeedbackValue{ + }, workv1.StatusFeedbackResult{ + Values: []workv1.FeedbackValue{ { Name: "isReady", - Value: v1.FieldValue{ - Type: v1.String, + Value: workv1.FieldValue{ + Type: workv1.String, String: &tc.status, }, }, @@ -56,7 +56,7 @@ func Test_AgentHealthProber_CLF(t *testing.T) { } func Test_AgentHealthProber_OTELCol(t *testing.T) { - unhealthyError := fmt.Errorf("%w: opentelemetrycollector replicas is %d for %s/%s", errUnavailable, 0, SpokeOTELColNamespace, SpokeOTELColName) + unhealthyError := fmt.Errorf("%w: opentelemetrycollector replicas is %d for %s/%s", errProbeConditionNotSatisfied, 0, SpokeOTELColNamespace, SpokeOTELColName) for _, tc := range []struct { name string replicas int64 @@ -74,17 +74,17 @@ func Test_AgentHealthProber_OTELCol(t *testing.T) { } { t.Run(tc.name, func(t *testing.T) { healthProber := AgentHealthProber() - err := healthProber.WorkProber.HealthCheck(v1.ResourceIdentifier{ + err := healthProber.WorkProber.HealthCheck(workv1.ResourceIdentifier{ Group: otelv1alpha1.GroupVersion.Group, Resource: OpenTelemetryCollectorsResource, Name: SpokeOTELColName, Namespace: SpokeOTELColNamespace, - }, v1.StatusFeedbackResult{ - Values: []v1.FeedbackValue{ + }, workv1.StatusFeedbackResult{ + Values: []workv1.FeedbackValue{ { Name: "replicas", - Value: v1.FieldValue{ - Type: v1.Integer, + Value: workv1.FieldValue{ + Type: workv1.Integer, Integer: &tc.replicas, }, }, diff --git a/internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterlogforwarder.yaml b/internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterlogforwarder.yaml index 1fa49c6..be3b4c1 100644 --- a/internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterlogforwarder.yaml +++ b/internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterlogforwarder.yaml @@ -2,7 +2,7 @@ apiVersion: logging.openshift.io/v1 kind: ClusterLogForwarder metadata: - name: instance + name: mcoa-instance namespace: openshift-logging labels: app: {{ template "logginghelm.name" . }} diff --git a/internal/addon/manifests/charts/mcoa/charts/tracing/templates/namespace.yaml b/internal/addon/manifests/charts/mcoa/charts/tracing/templates/namespace.yaml index b86b6ce..61ed4b7 100644 --- a/internal/addon/manifests/charts/mcoa/charts/tracing/templates/namespace.yaml +++ b/internal/addon/manifests/charts/mcoa/charts/tracing/templates/namespace.yaml @@ -11,7 +11,7 @@ metadata: apiVersion: v1 kind: Namespace metadata: - name: spoke-otelcol + name: mcoa-opentelemetry labels: app: {{ template "tracinghelm.name" . }} chart: {{ template "tracinghelm.chart" . }} diff --git a/internal/addon/manifests/charts/mcoa/charts/tracing/templates/opentelemetrycollector.yaml b/internal/addon/manifests/charts/mcoa/charts/tracing/templates/opentelemetrycollector.yaml index 6766115..db606ba 100644 --- a/internal/addon/manifests/charts/mcoa/charts/tracing/templates/opentelemetrycollector.yaml +++ b/internal/addon/manifests/charts/mcoa/charts/tracing/templates/opentelemetrycollector.yaml @@ -2,8 +2,8 @@ apiVersion: opentelemetry.io/v1alpha1 kind: OpenTelemetryCollector metadata: - name: spoke-otelcol - namespace: spoke-otelcol + name: mcoa-instance + namespace: mcoa-opentelemetry labels: app: {{ template "tracinghelm.name" . }} chart: {{ template "tracinghelm.chart" . }} diff --git a/internal/addon/manifests/charts/mcoa/charts/tracing/templates/secret.yaml b/internal/addon/manifests/charts/mcoa/charts/tracing/templates/secret.yaml index c589902..bc874f5 100644 --- a/internal/addon/manifests/charts/mcoa/charts/tracing/templates/secret.yaml +++ b/internal/addon/manifests/charts/mcoa/charts/tracing/templates/secret.yaml @@ -4,7 +4,7 @@ apiVersion: v1 kind: Secret metadata: name: {{ $secret_config.name }} - namespace: spoke-otelcol + namespace: mcoa-opentelemetry labels: app: {{ template "tracinghelm.name" $ }} chart: {{ template "tracinghelm.chart" $ }} diff --git a/internal/addon/var.go b/internal/addon/var.go index 6bb2e61..01f9199 100644 --- a/internal/addon/var.go +++ b/internal/addon/var.go @@ -18,14 +18,14 @@ const ( AdcTracingisabledKey = "tracingDisabled" ClusterLogForwardersResource = "clusterlogforwarders" - SpokeCLFName = "instance" + SpokeCLFName = "mcoa-instance" SpokeCLFNamespace = "openshift-logging" clfProbeKey = "isReady" clfProbePath = ".status.conditions[?(@.type==\"Ready\")].status" OpenTelemetryCollectorsResource = "opentelemetrycollectors" - SpokeOTELColName = "spoke-otelcol" - SpokeOTELColNamespace = "spoke-otelcol" + SpokeOTELColName = "mcoa-instance" + SpokeOTELColNamespace = "mcoa-opentelemetry" otelColProbeKey = "replicas" otelColProbePath = ".spec.replicas" ) diff --git a/internal/logging/helm_test.go b/internal/logging/helm_test.go index d52b4e3..185e27e 100644 --- a/internal/logging/helm_test.go +++ b/internal/logging/helm_test.go @@ -91,7 +91,7 @@ func Test_Logging_AllConfigsTogether_AllResources(t *testing.T) { }, ConfigReferent: addonapiv1alpha1.ConfigReferent{ Namespace: "open-cluster-management", - Name: "instance", + Name: "mcoa-instance", }, }, } @@ -99,7 +99,7 @@ func Test_Logging_AllConfigsTogether_AllResources(t *testing.T) { // Setup configuration resources: ClusterLogForwarder, AddOnDeploymentConfig, Secrets, ConfigMaps clf = &loggingv1.ClusterLogForwarder{ ObjectMeta: metav1.ObjectMeta{ - Name: "instance", + Name: "mcoa-instance", Namespace: "open-cluster-management", Annotations: map[string]string{ "authentication.mcoa.openshift.io/app-logs": "SecretReference", @@ -225,7 +225,7 @@ func Test_Logging_AllConfigsTogether_AllResources(t *testing.T) { require.NotNil(t, obj.Spec.Outputs[1].Secret) require.Equal(t, "static-authentication", obj.Spec.Outputs[0].Secret.Name) require.Equal(t, "static-authentication", obj.Spec.Outputs[1].Secret.Name) - // Check name and namespace to make usre that if we change the helm + // Check name and namespace to make sure that if we change the helm // manifests that we don't break the addon probes require.Equal(t, addon.SpokeCLFName, obj.Name) require.Equal(t, addon.SpokeCLFNamespace, obj.Namespace) diff --git a/internal/logging/manifests/logging_test.go b/internal/logging/manifests/logging_test.go index 1c6f9ae..8b8cd58 100644 --- a/internal/logging/manifests/logging_test.go +++ b/internal/logging/manifests/logging_test.go @@ -119,7 +119,7 @@ func Test_BuildCLFSpec(t *testing.T) { }, ConfigReferent: addonapiv1alpha1.ConfigReferent{ Namespace: "open-cluster-management", - Name: "instance", + Name: "mcoa-instance", }, }, { @@ -147,7 +147,7 @@ func Test_BuildCLFSpec(t *testing.T) { // Setup configuration resources: ClusterLogForwarder, AddOnDeploymentConfig, Secrets, ConfigMaps clf = &loggingv1.ClusterLogForwarder{ ObjectMeta: metav1.ObjectMeta{ - Name: "instance", + Name: "mcoa-instance", Namespace: "open-cluster-management", }, Spec: loggingv1.ClusterLogForwarderSpec{ diff --git a/internal/tracing/helm_test.go b/internal/tracing/helm_test.go index f498cee..7c45435 100644 --- a/internal/tracing/helm_test.go +++ b/internal/tracing/helm_test.go @@ -102,7 +102,7 @@ func Test_Tracing_AllConfigsTogether_AllResources(t *testing.T) { }, ConfigReferent: addonapiv1alpha1.ConfigReferent{ Namespace: "open-cluster-management", - Name: "spoke-otelcol", + Name: "mcoa-instance", }, }, } @@ -114,7 +114,7 @@ func Test_Tracing_AllConfigsTogether_AllResources(t *testing.T) { otelCol = &otelv1alpha1.OpenTelemetryCollector{ ObjectMeta: metav1.ObjectMeta{ - Name: "spoke-otelcol", + Name: "mcoa-instance", Namespace: "open-cluster-management", }, Spec: otelv1alpha1.OpenTelemetryCollectorSpec{ @@ -188,7 +188,7 @@ func Test_Tracing_AllConfigsTogether_AllResources(t *testing.T) { for _, obj := range objects { switch obj := obj.(type) { case *otelv1alpha1.OpenTelemetryCollector: - // Check name and namespace to make usre that if we change the helm + // Check name and namespace to make sure that if we change the helm // manifests that we don't break the addon probes require.Equal(t, addon.SpokeOTELColName, obj.Name) require.Equal(t, addon.SpokeOTELColNamespace, obj.Namespace) From 11640583eb141afb999494b11e68bdf44bbf27f8 Mon Sep 17 00:00:00 2001 From: Joao Marcal Date: Wed, 5 Jun 2024 14:53:42 +0100 Subject: [PATCH 16/17] logging: added SA & clusterrole binding to allow for CLF not nameded instance --- .../templates/clusterrole-binding.yaml | 52 +++++++++++++++++++ .../logging/templates/service-account.yaml | 11 ++++ internal/logging/manifests/logging.go | 1 + 3 files changed, 64 insertions(+) create mode 100644 internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterrole-binding.yaml create mode 100644 internal/addon/manifests/charts/mcoa/charts/logging/templates/service-account.yaml diff --git a/internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterrole-binding.yaml b/internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterrole-binding.yaml new file mode 100644 index 0000000..8a3c1a0 --- /dev/null +++ b/internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterrole-binding.yaml @@ -0,0 +1,52 @@ +{{- if .Values.enabled }} +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: openshift-logging:mcoa-logcollector:application-logs + labels: + app: {{ template "mcoahelm.name" . }} + chart: {{ template "mcoahelm.chart" . }} + release: {{ .Release.Name }} +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: collect-application-logs +subjects: + - kind: ServiceAccount + name: mcoa-logcollector + namespace: openshift-logging +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: openshift-logging:mcoa-logcollector:audit-logs + labels: + app: {{ template "mcoahelm.name" . }} + chart: {{ template "mcoahelm.chart" . }} + release: {{ .Release.Name }} +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: collect-audit-logs +subjects: + - kind: ServiceAccount + name: mcoa-logcollector + namespace: openshift-logging +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: openshift-logging:mcoa-logcollector:infrastructure-logs + labels: + app: {{ template "mcoahelm.name" . }} + chart: {{ template "mcoahelm.chart" . }} + release: {{ .Release.Name }} +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: ClusterRole + name: collect-infrastructure-logs +subjects: + - kind: ServiceAccount + name: mcoa-logcollector + namespace: openshift-logging +{{- end }} \ No newline at end of file diff --git a/internal/addon/manifests/charts/mcoa/charts/logging/templates/service-account.yaml b/internal/addon/manifests/charts/mcoa/charts/logging/templates/service-account.yaml new file mode 100644 index 0000000..2356d10 --- /dev/null +++ b/internal/addon/manifests/charts/mcoa/charts/logging/templates/service-account.yaml @@ -0,0 +1,11 @@ +{{- if .Values.enabled }} +apiVersion: v1 +kind: ServiceAccount +metadata: + name: mcoa-logcollector + namespace: openshift-logging + labels: + app: {{ template "logginghelm.name" . }} + chart: {{ template "logginghelm.chart" . }} + release: {{ .Release.Name }} +{{- end }} \ No newline at end of file diff --git a/internal/logging/manifests/logging.go b/internal/logging/manifests/logging.go index d694208..c7b70bf 100644 --- a/internal/logging/manifests/logging.go +++ b/internal/logging/manifests/logging.go @@ -54,6 +54,7 @@ func buildClusterLogForwarderSpec(resources Options) (*loggingv1.ClusterLogForwa return nil, err } } + clf.Spec.ServiceAccountName = "mcoa-logcollector" return &clf.Spec, nil } From f6ffd3af28e5469494fa27e1f16fb9b31c4636c7 Mon Sep 17 00:00:00 2001 From: Joao Marcal Date: Thu, 6 Jun 2024 11:12:35 +0100 Subject: [PATCH 17/17] logging: fix tests --- .../logging/templates/clusterrole-binding.yaml | 12 ++++++------ internal/logging/helm_test.go | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterrole-binding.yaml b/internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterrole-binding.yaml index 8a3c1a0..f9fe8b0 100644 --- a/internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterrole-binding.yaml +++ b/internal/addon/manifests/charts/mcoa/charts/logging/templates/clusterrole-binding.yaml @@ -4,8 +4,8 @@ kind: ClusterRoleBinding metadata: name: openshift-logging:mcoa-logcollector:application-logs labels: - app: {{ template "mcoahelm.name" . }} - chart: {{ template "mcoahelm.chart" . }} + app: {{ template "logginghelm.name" . }} + chart: {{ template "logginghelm.chart" . }} release: {{ .Release.Name }} roleRef: apiGroup: rbac.authorization.k8s.io @@ -21,8 +21,8 @@ kind: ClusterRoleBinding metadata: name: openshift-logging:mcoa-logcollector:audit-logs labels: - app: {{ template "mcoahelm.name" . }} - chart: {{ template "mcoahelm.chart" . }} + app: {{ template "logginghelm.name" . }} + chart: {{ template "logginghelm.chart" . }} release: {{ .Release.Name }} roleRef: apiGroup: rbac.authorization.k8s.io @@ -38,8 +38,8 @@ kind: ClusterRoleBinding metadata: name: openshift-logging:mcoa-logcollector:infrastructure-logs labels: - app: {{ template "mcoahelm.name" . }} - chart: {{ template "mcoahelm.chart" . }} + app: {{ template "logginghelm.name" . }} + chart: {{ template "logginghelm.chart" . }} release: {{ .Release.Name }} roleRef: apiGroup: rbac.authorization.k8s.io diff --git a/internal/logging/helm_test.go b/internal/logging/helm_test.go index 185e27e..9303359 100644 --- a/internal/logging/helm_test.go +++ b/internal/logging/helm_test.go @@ -214,7 +214,7 @@ func Test_Logging_AllConfigsTogether_AllResources(t *testing.T) { // Render manifests and return them as k8s runtime objects objects, err := loggingAgentAddon.Manifests(managedCluster, managedClusterAddOn) require.NoError(t, err) - require.Equal(t, 7, len(objects)) + require.Equal(t, 11, len(objects)) for _, obj := range objects { switch obj := obj.(type) {