use const for labels, include timeout and fix race condition

Author: pixelsoccupied

internal/controllers/inventory_controller.go

@@ -76,7 +76,6 @@ import (
 //+kubebuilder:rbac:urls="/o2ims-infrastructureInventory/v1/resourceTypes/*",verbs=get
 //+kubebuilder:rbac:urls="/hardware-manager/inventory/*",verbs=get;list
 //+kubebuilder:rbac:groups="batch",resources=cronjobs,verbs=get;list;watch;create;update;patch;delete
-//+kubebuilder:rbac:groups=route.openshift.io,resources=routes,verbs=get;list;watch
 //+kubebuilder:rbac:groups="",resources=events,verbs=create;patch;update
 //+kubebuilder:rbac:groups=coordination.k8s.io,resources=leases,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=clcm.openshift.io,resources=hardwareplugins,verbs=get;list;watch;create;update;patch;delete
@@ -1346,22 +1345,6 @@ func (t *reconcilerTask) createAlarmServerClusterRole(ctx context.Context) error
 					"patch",
 				},
 			},
-			{
-				APIGroups: []string{
-					"route.openshift.io",
-				},
-				Resources: []string{
-					"routes",
-				},
-				Verbs: []string{
-					"get",
-					"list",
-					"watch",
-				},
-				ResourceNames: []string{
-					"alertmanager",
-				},
-			},
 			{
 				NonResourceURLs: []string{
 					"/o2ims-infrastructureCluster/v1/nodeClusterTypes",

internal/service/alarms/internal/alertmanager/api.go

@@ -13,21 +13,21 @@ import (
 	"log/slog"
 	"net/http"
 	"net/url"
+	"os"
 	"time"
 
+	"github.com/openshift-kni/oran-o2ims/internal/constants"
+	ctlrutils "github.com/openshift-kni/oran-o2ims/internal/controllers/utils"
 	"github.com/openshift-kni/oran-o2ims/internal/service/alarms/internal/db/repo"
 	"github.com/openshift-kni/oran-o2ims/internal/service/alarms/internal/infrastructure"
-	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 const (
-	// ACMObsAMRouteName route to collect ACM's AM API host
-	ACMObsAMRouteName = "alertmanager"
-	// ACMObsAMAuthSecretName secret to collect ACM's AM API BEARER and ca.Crt
-	ACMObsAMAuthSecretName = "observability-alertmanager-accessor-token" // nolint: gosec
+	// ACMObsAMServiceName is the alertmanager service name in ACM observability
+	ACMObsAMServiceName = "alertmanager"
+	// ACMObsAMServicePort is the HTTPS port for alertmanager service
+	ACMObsAMServicePort = 9095
 )
 
 // APIAlert represents the alert structure returned by the Alertmanager API.
@@ -110,16 +110,21 @@ func (c *AMClient) SyncAlerts(ctx context.Context) error {
 
 // getAlerts retrieves all alerts from Alertmanager API
 func (c *AMClient) getAlerts(ctx context.Context) ([]APIAlert, error) {
-	// Get the alertmanager route
-	alertmanagerHost, err := c.GetAlertmanagerRoute(ctx)
+	// Initialize a new client each time to pick up the latest token
+	httpClient, token, err := c.createAlertmanagerClient()
 	if err != nil {
-		return nil, fmt.Errorf("failed to get alertmanager route: %w", err)
+		return nil, fmt.Errorf("failed to create alertmanager client: %w", err)
 	}
 
-	// Initialize a new client each time to pick up the latest secrets
-	httpClient, token, err := c.createAlertmanagerClient(ctx)
-	if err != nil {
-		return nil, fmt.Errorf("failed to create alertmanager client: %w", err)
+	// Build service URL for alertmanager
+	// Format: alertmanager.open-cluster-management-observability.svc:9095
+	// Allow override for testing
+	alertmanagerHost := os.Getenv("ALARMS_SERVER_AM_HOST")
+	if alertmanagerHost == "" {
+		alertmanagerHost = fmt.Sprintf("%s.%s.svc:%d",
+			ACMObsAMServiceName,
+			ctlrutils.OpenClusterManagementObservabilityNamespace,
+			ACMObsAMServicePort)
 	}
 
 	// Create request
@@ -138,7 +143,7 @@ func (c *AMClient) getAlerts(ctx context.Context) ([]APIAlert, error) {
 	q.Set("silenced", "true")
 
 	u.RawQuery = q.Encode()
-	req, err := http.NewRequest(http.MethodGet, u.String(), nil)
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil)
 	if err != nil {
 		return nil, fmt.Errorf("error creating request: %w", err)
 	}
@@ -179,78 +184,32 @@ func (c *AMClient) getAlerts(ctx context.Context) ([]APIAlert, error) {
 	return alerts, nil
 }
 
-// GetAlertmanagerRoute gets the hostname for the alertmanager route using unstructured type
-func (c *AMClient) GetAlertmanagerRoute(ctx context.Context) (string, error) {
-	// Define the route GVK (Group, Version, Kind)
-	routeGVK := schema.GroupVersionKind{
-		Group:   "route.openshift.io",
-		Version: "v1",
-		Kind:    "Route",
-	}
-
-	// Create an unstructured object
-	route := &unstructured.Unstructured{}
-	route.SetGroupVersionKind(routeGVK)
-
-	// Get the route
-	if err := c.k8sClient.Get(ctx, client.ObjectKey{
-		Namespace: ACMObsAMNamespace,
-		Name:      ACMObsAMRouteName,
-	}, route); err != nil {
-		return "", fmt.Errorf("error getting '%s' route: %w", ACMObsAMRouteName, err)
-	}
-
-	// Try to get host from spec
-	specHost, found, err := unstructured.NestedString(route.Object, "spec", "host")
-	if err != nil {
-		return "", fmt.Errorf("error accessing spec.host: %w", err)
-	}
-	if found && specHost != "" {
-		return specHost, nil
+// createAlertmanagerClient creates a new HTTP client with the service account token and service CA certificate.
+// The token comes from the pod's service account and CA cert from the service CA file.
+func (c *AMClient) createAlertmanagerClient() (*http.Client, string, error) {
+	// Determine token file path (allow override for testing/local development)
+	tokenPath := constants.DefaultBackendTokenFile
+	if envPath := os.Getenv("ALARMS_SERVER_TOKEN_FILE"); envPath != "" {
+		tokenPath = envPath
 	}
 
-	// Fallback to status if spec host is empty
-	statusIngress, found, err := unstructured.NestedSlice(route.Object, "status", "ingress")
+	// Read token from service account token file
+	token, err := os.ReadFile(tokenPath)
 	if err != nil {
-		return "", fmt.Errorf("error accessing status.ingress: %w", err)
+		return nil, "", fmt.Errorf("error reading service account token: %w", err)
 	}
 
-	if found && len(statusIngress) > 0 {
-		ingressMap, ok := statusIngress[0].(map[string]interface{})
-		if !ok {
-			return "", fmt.Errorf("invalid ingress format")
-		}
-
-		host, found := ingressMap["host"]
-		if found && host != "" {
-			return host.(string), nil
-		}
-	}
-
-	return "", fmt.Errorf("no host found in alertmanager route")
-}
-
-// createAlertmanagerClient creates a new HTTP client with the latest token
-// TODO: we can be more robust by reading our `/var/run/secrets/kubernetes.io/serviceaccount/token` for token instead of relying on ACMObsAMAuthSecretName data.
-func (c *AMClient) createAlertmanagerClient(ctx context.Context) (*http.Client, string, error) {
-	var secret corev1.Secret
-	if err := c.k8sClient.Get(ctx, client.ObjectKey{
-		Namespace: ACMObsAMNamespace,
-		Name:      ACMObsAMAuthSecretName,
-	}, &secret); err != nil {
-		return nil, "", fmt.Errorf("error getting token secret from '%s': %w", ACMObsAMAuthSecretName, err)
+	// Determine service CA file path (allow override for testing/local development)
+	caPath := constants.DefaultServiceCAFile
+	if envPath := os.Getenv("ALARMS_SERVER_CA_FILE"); envPath != "" {
+		caPath = envPath
 	}
 
-	// Extract token
-	token, ok := secret.Data["token"]
-	if !ok {
-		return nil, "", fmt.Errorf("token not found in secret")
-	}
-
-	// Extract CA certificate
-	caCrt, ok := secret.Data["ca.crt"]
-	if !ok {
-		return nil, "", fmt.Errorf("ca.crt not found in secret")
+	// Read service CA certificate
+	// This is automatically mounted by Kubernetes for service-to-service TLS
+	caCrt, err := os.ReadFile(caPath)
+	if err != nil {
+		return nil, "", fmt.Errorf("error reading service CA certificate: %w", err)
 	}
 
 	// Create certificate pool with the CA cert

internal/service/alarms/internal/alertmanager/api_test.go

@@ -10,21 +10,18 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"os"
 	"time"
 
 	"github.com/jackc/pgx/v5"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
-	"github.com/openshift-kni/oran-o2ims/internal/constants"
 	"github.com/openshift-kni/oran-o2ims/internal/service/alarms/internal/alertmanager"
 	"github.com/openshift-kni/oran-o2ims/internal/service/alarms/internal/db/repo/generated"
 	"github.com/openshift-kni/oran-o2ims/internal/service/alarms/internal/infrastructure"
 	"go.uber.org/mock/gomock"
 	corev1 "k8s.io/api/core/v1"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
@@ -46,10 +43,11 @@ var _ = Describe("Alertmanager API Client", func() {
 		mockRepo      *generated.MockAlarmRepositoryInterface
 		infra         *infrastructure.Infrastructure
 		amClient      *alertmanager.AMClient
-		fakeRoute     *unstructured.Unstructured
-		fakeSecret    *corev1.Secret
 		mockAMServer  *httptest.Server
 		testAPIAlerts []alertmanager.APIAlert
+		tempTokenFile string
+		tempCAFile    string
+		originalHost  string
 	)
 
 	BeforeEach(func() {
@@ -70,46 +68,41 @@ var _ = Describe("Alertmanager API Client", func() {
 			Expect(err).To(BeNil())
 		}))
 
-		// Set up fake k8s client with the route and secret
-		scheme := runtime.NewScheme()
-		Expect(corev1.AddToScheme(scheme)).To(Succeed())
+		// Create temporary token file
+		tempToken, err := os.CreateTemp("", "token-*")
+		Expect(err).To(BeNil())
+		tempTokenFile = tempToken.Name()
+		err = os.WriteFile(tempTokenFile, []byte("fake-token"), 0o600)
+		Expect(err).To(BeNil())
+		tempToken.Close()
 
-		// Create a fake Route resource
-		fakeRoute = &unstructured.Unstructured{}
-		fakeRoute.SetGroupVersionKind(schema.GroupVersionKind(metav1.GroupVersionKind{
-			Group:   "route.openshift.io",
-			Version: "v1",
-			Kind:    "Route",
-		}))
-		fakeRoute.SetNamespace(alertmanager.ACMObsAMNamespace)
-		fakeRoute.SetName(alertmanager.ACMObsAMRouteName)
-		// Extract server hostname from test server URL without the scheme
-		serverHost := mockAMServer.URL[8:] // Skip "https://"
-		Expect(unstructured.SetNestedField(fakeRoute.Object, serverHost, "spec", "host")).To(Succeed())
-
-		// Extract server certificate for use in our fake secret
-		// This gets the actual certificate from our test server
+		// Create temporary CA file with the test server's certificate
 		certPEM := pem.EncodeToMemory(&pem.Block{
 			Type:  "CERTIFICATE",
 			Bytes: mockAMServer.Certificate().Raw,
 		})
+		tempCA, err := os.CreateTemp("", "ca-*")
+		Expect(err).To(BeNil())
+		tempCAFile = tempCA.Name()
+		err = os.WriteFile(tempCAFile, certPEM, 0o600)
+		Expect(err).To(BeNil())
+		tempCA.Close()
+
+		// Set environment variables to use temp files
+		os.Setenv("ALARMS_SERVER_TOKEN_FILE", tempTokenFile)
+		os.Setenv("ALARMS_SERVER_CA_FILE", tempCAFile)
+
+		// Override the alertmanager host to point to our mock server
+		// Extract server hostname from test server URL without the scheme
+		originalHost = mockAMServer.URL[8:] // Skip "https://"
+		os.Setenv("ALARMS_SERVER_AM_HOST", originalHost)
 
-		// Create a fake Secret
-		fakeSecret = &corev1.Secret{
-			ObjectMeta: metav1.ObjectMeta{
-				Namespace: alertmanager.ACMObsAMNamespace,
-				Name:      alertmanager.ACMObsAMAuthSecretName,
-			},
-			Data: map[string][]byte{
-				"token":  []byte("fake-token"),
-				"ca.crt": certPEM,
-			},
-		}
+		// Set up minimal fake k8s client
+		scheme := runtime.NewScheme()
+		Expect(corev1.AddToScheme(scheme)).To(Succeed())
 
 		fakeClient = fake.NewClientBuilder().
 			WithScheme(scheme).
-			WithObjects(fakeSecret).
-			WithRuntimeObjects(fakeRoute).
 			Build()
 
 		infra = &infrastructure.Infrastructure{
@@ -145,6 +138,19 @@ var _ = Describe("Alertmanager API Client", func() {
 	AfterEach(func() {
 		mockAMServer.Close()
 		ctrl.Finish()
+
+		// Clean up temporary files
+		if tempTokenFile != "" {
+			os.Remove(tempTokenFile)
+		}
+		if tempCAFile != "" {
+			os.Remove(tempCAFile)
+		}
+
+		// Unset environment variables
+		os.Unsetenv("ALARMS_SERVER_TOKEN_FILE")
+		os.Unsetenv("ALARMS_SERVER_CA_FILE")
+		os.Unsetenv("ALARMS_SERVER_AM_HOST")
 	})
 
 	Describe("SyncAlerts", func() {
@@ -203,32 +209,6 @@ var _ = Describe("Alertmanager API Client", func() {
 		})
 	})
 
-	Describe("GetAlertmanagerRoute", func() {
-		It("should retrieve the route from Kubernetes API", func() {
-			// This is using the fake client with our route already set up
-			host, err := amClient.GetAlertmanagerRoute(ctx)
-
-			// Assert
-			Expect(err).NotTo(HaveOccurred())
-			Expect(host).NotTo(BeEmpty())
-			Expect(host).To(ContainSubstring(constants.Localhost))
-		})
-
-		It("should return error when route not found", func() {
-			// Create a client without the route
-			emptyClient := fake.NewClientBuilder().
-				WithScheme(runtime.NewScheme()).
-				Build()
-
-			clientWithoutRoute := alertmanager.NewAlertmanagerClient(emptyClient, mockRepo, infra)
-
-			_, err := clientWithoutRoute.GetAlertmanagerRoute(ctx)
-
-			// Assert
-			Expect(err).To(HaveOccurred())
-		})
-	})
-
 	Describe("RunAlertSyncScheduler", func() {
 		It("should schedule alert syncs at regular intervals", func() {
 			// Given a short interval for testing

internal/service/alarms/internal/alertmanager/converter.go

@@ -16,6 +16,7 @@ import (
 	api "github.com/openshift-kni/oran-o2ims/internal/service/alarms/api/generated"
 	"github.com/openshift-kni/oran-o2ims/internal/service/alarms/internal/db/models"
 	"github.com/openshift-kni/oran-o2ims/internal/service/alarms/internal/infrastructure"
+	"github.com/openshift-kni/oran-o2ims/internal/service/common"
 )
 
 // ConvertAmToAlarmEventRecordModels get alarmEventRecords based on the alertmanager notification and AlarmDefinition
@@ -133,9 +134,9 @@ func getClusterID(labels map[string]string) *uuid.UUID {
 
 // isHardwareAlert checks if an alert is a hardware alert by checking for type=hardware AND component=ironic labels
 func isHardwareAlert(labels map[string]string) bool {
-	alertType, hasType := labels["type"]
-	component, hasComponent := labels["component"]
-	return hasType && alertType == "hardware" && hasComponent && component == "ironic"
+	alertType, hasType := labels[common.HardwareAlertTypeLabel]
+	component, hasComponent := labels[common.HardwareAlertComponentLabel]
+	return hasType && alertType == common.HardwareAlertTypeValue && hasComponent && component == common.HardwareAlertComponentValue
 }
 
 // getResourceID extracts resource ID from instance_uuid label for hardware alerts