Protect the platform behind a mutex (jaegertracing#2278)

* Refactor the autodetect module to reduce the number of writes/reads in viper Signed-off-by: Israel Blancas <[email protected]> * Fix linting Signed-off-by: Israel Blancas <[email protected]> * Move the cleaning tasks outside the autodetection Signed-off-by: Israel Blancas <[email protected]> * Increase timeotus Signed-off-by: Israel Blancas <[email protected]> --------- Signed-off-by: Israel Blancas <[email protected]>
ishaqkhattana · Aug 23, 2023 · 3c5aec6 · 3c5aec6
1 parent f3125c8
commit 3c5aec6
Show file tree

Hide file tree

Showing 30 changed files with 166 additions and 81 deletions.
diff --git a/apis/v1/jaeger_types.go b/apis/v1/jaeger_types.go
@@ -36,14 +36,8 @@ const (
 	// FlagAutoscalingVersionV2Beta2 represents the v2beta2 version of the Kubernetes Autoscaling API, no longer available as of 1.26
 	FlagAutoscalingVersionV2Beta2 = "autoscaling/v2beta2"
 
-	// FlagPlatformKubernetes represents the value for the 'platform' flag for Kubernetes
-	FlagPlatformKubernetes = "kubernetes"
-
-	// FlagPlatformOpenShift represents the value for the 'platform' flag for OpenShift
-	FlagPlatformOpenShift = "openshift"
-
-	// FlagPlatformAutoDetect represents the "auto-detect" value for the platform flag
-	FlagPlatformAutoDetect = "auto-detect"
+	// FlagPlatform represents the flag to set the platform
+	FlagPlatform = "platform"
 
 	// FlagProvisionElasticsearchAuto represents the 'auto' value for the 'es-provision' flag
 	FlagProvisionElasticsearchAuto = "auto"

diff --git a/controllers/appsv1/deployment_webhook_test.go b/controllers/appsv1/deployment_webhook_test.go
@@ -24,6 +24,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	v1 "github.com/jaegertracing/jaeger-operator/apis/v1"
+	"github.com/jaegertracing/jaeger-operator/pkg/autodetect"
 	"github.com/jaegertracing/jaeger-operator/pkg/inject"
 )
 
@@ -93,8 +94,7 @@ func TestReconcileConfigMaps(t *testing.T) {
 				errors:    tC.errors,
 			}
 
-			viper.Set("platform", v1.FlagPlatformOpenShift)
-			defer viper.Reset()
+			autodetect.OperatorConfiguration.SetPlatform(autodetect.OpenShiftPlatform)
 
 			// test
 			err := reconcileConfigMaps(context.Background(), cl, jaeger, &dep)

diff --git a/pkg/autodetect/main.go b/pkg/autodetect/main.go
@@ -192,10 +192,10 @@ func AvailableAPIs(discovery discovery.DiscoveryInterface, groups map[string]boo
 
 func (b *Background) detectPlatform(ctx context.Context, apiList []*metav1.APIResourceList) {
 	// detect the platform, we run this only once, as the platform can't change between runs ;)
-	platform := viper.GetString("platform")
+	platform := OperatorConfiguration.GetPlatform()
 	detectedPlatform := ""
 
-	if !strings.EqualFold(platform, v1.FlagPlatformAutoDetect) {
+	if !OperatorConfiguration.IsPlatformAutodetectionEnabled() {
 		log.Log.V(-1).Info(
 			"The 'platform' option is explicitly set",
 			"platform", platform,
@@ -205,12 +205,12 @@ func (b *Background) detectPlatform(ctx context.Context, apiList []*metav1.APIRe
 
 	log.Log.V(-1).Info("Attempting to auto-detect the platform")
 	if isOpenShift(apiList) {
-		detectedPlatform = v1.FlagPlatformOpenShift
+		detectedPlatform = OpenShiftPlatform.String()
 	} else {
-		detectedPlatform = v1.FlagPlatformKubernetes
+		detectedPlatform = KubernetesPlatform.String()
 	}
 
-	viper.Set("platform", detectedPlatform)
+	OperatorConfiguration.SetPlatform(detectedPlatform)
 	log.Log.Info(
 		"Auto-detected the platform",
 		"platform", detectedPlatform,
@@ -222,7 +222,7 @@ func (b *Background) detectOAuthProxyImageStream(ctx context.Context) {
 	ctx, span := tracer.Start(ctx, "detectOAuthProxyImageStream")
 	defer span.End()
 
-	if viper.GetString("platform") != v1.FlagPlatformOpenShift {
+	if OperatorConfiguration.GetPlatform() == OpenShiftPlatform {
 		log.Log.V(-1).Info(
 			"Not running on OpenShift, so won't configure OAuthProxy imagestream.",
 		)
@@ -347,7 +347,7 @@ func (b *Background) detectKafka(_ context.Context, apiList []*metav1.APIResourc
 }
 
 func (b *Background) detectClusterRoles(ctx context.Context) {
-	if viper.GetString("platform") != v1.FlagPlatformOpenShift {
+	if OperatorConfiguration.GetPlatform() != OpenShiftPlatform {
 		return
 	}
 	tr := &authenticationapi.TokenReview{

diff --git a/pkg/autodetect/main_test.go b/pkg/autodetect/main_test.go
@@ -21,7 +21,7 @@ import (
 )
 
 func TestStart(t *testing.T) {
-	viper.Set("platform", v1.FlagPlatformOpenShift)
+	OperatorConfiguration.SetPlatform(OpenShiftPlatform)
 	defer viper.Reset()
 
 	// sanity check
@@ -57,7 +57,7 @@ func TestStart(t *testing.T) {
 }
 
 func TestStartContinuesInBackground(t *testing.T) {
-	viper.Set("platform", v1.FlagPlatformOpenShift)
+	OperatorConfiguration.SetPlatform(OpenShiftPlatform)
 	defer viper.Reset()
 
 	// prepare
@@ -68,6 +68,9 @@ func TestStartContinuesInBackground(t *testing.T) {
 	}
 	b := WithClients(cl, dcl, cl)
 
+	fmt.Println(viper.IsSet("auth-delegator-available"))
+	fmt.Println(viper.GetBool("auth-delegator-available"))
+
 	done := make(chan bool)
 	go func() {
 		for {
@@ -85,7 +88,7 @@ func TestStartContinuesInBackground(t *testing.T) {
 	select {
 	case <-done:
 		assert.False(t, viper.GetBool("auth-delegator-available"))
-	case <-time.After(1 * time.Second):
+	case <-time.After(5 * time.Second):
 		assert.Fail(t, "timed out waiting for the start process to detect the capabilities")
 	}
 
@@ -97,7 +100,7 @@ func TestStartContinuesInBackground(t *testing.T) {
 			if viper.GetBool("auth-delegator-available") {
 				break
 			}
-			time.Sleep(500 * time.Millisecond)
+			time.Sleep(10 * time.Millisecond)
 		}
 		done <- true
 	}()
@@ -176,7 +179,7 @@ func TestAutoDetectWithServerResourcesForGroupVersionError(t *testing.T) {
 
 func TestAutoDetectOpenShift(t *testing.T) {
 	// prepare
-	viper.Set("platform", v1.FlagPlatformAutoDetect)
+	viper.Set("platform", "auto-detect")
 	defer viper.Reset()
 
 	dcl := &fakeDiscoveryClient{}
@@ -197,7 +200,7 @@ func TestAutoDetectOpenShift(t *testing.T) {
 	b.autoDetectCapabilities()
 
 	// verify
-	assert.Equal(t, v1.FlagPlatformOpenShift, viper.GetString("platform"))
+	assert.Equal(t, OpenShiftPlatform, OperatorConfiguration.GetPlatform())
 
 	// set the error
 	dcl.ServerResourcesForGroupVersionFunc = func(_ string) (apiGroupList *metav1.APIResourceList, err error) {
@@ -208,12 +211,12 @@ func TestAutoDetectOpenShift(t *testing.T) {
 	b.autoDetectCapabilities()
 
 	// verify again
-	assert.Equal(t, v1.FlagPlatformOpenShift, viper.GetString("platform"))
+	assert.Equal(t, OpenShiftPlatform, OperatorConfiguration.GetPlatform())
 }
 
 func TestAutoDetectKubernetes(t *testing.T) {
 	// prepare
-	viper.Set("platform", v1.FlagPlatformAutoDetect)
+	viper.Set("platform", "auto-detect")
 	defer viper.Reset()
 
 	dcl := &fakeDiscoveryClient{}
@@ -224,12 +227,12 @@ func TestAutoDetectKubernetes(t *testing.T) {
 	b.autoDetectCapabilities()
 
 	// verify
-	assert.Equal(t, v1.FlagPlatformKubernetes, viper.GetString("platform"))
+	assert.Equal(t, KubernetesPlatform, OperatorConfiguration.GetPlatform())
 }
 
 func TestExplicitPlatform(t *testing.T) {
 	// prepare
-	viper.Set("platform", v1.FlagPlatformOpenShift)
+	OperatorConfiguration.SetPlatform(OpenShiftPlatform)
 	defer viper.Reset()
 
 	dcl := &fakeDiscoveryClient{}
@@ -240,7 +243,7 @@ func TestExplicitPlatform(t *testing.T) {
 	b.autoDetectCapabilities()
 
 	// verify
-	assert.Equal(t, v1.FlagPlatformOpenShift, viper.GetString("platform"))
+	assert.Equal(t, OpenShiftPlatform, OperatorConfiguration.GetPlatform())
 }
 
 func TestAutoDetectEsProvisionNoEsOperator(t *testing.T) {
@@ -499,7 +502,7 @@ func TestAutoDetectAutoscalingVersion(t *testing.T) {
 
 func TestSkipAuthDelegatorNonOpenShift(t *testing.T) {
 	// prepare
-	viper.Set("platform", v1.FlagPlatformKubernetes)
+	OperatorConfiguration.SetPlatform(KubernetesPlatform)
 	defer viper.Reset()
 
 	dcl := &fakeDiscoveryClient{}
@@ -515,7 +518,7 @@ func TestSkipAuthDelegatorNonOpenShift(t *testing.T) {
 
 func TestNoAuthDelegatorAvailable(t *testing.T) {
 	// prepare
-	viper.Set("platform", v1.FlagPlatformOpenShift)
+	OperatorConfiguration.SetPlatform(OpenShiftPlatform)
 	defer viper.Reset()
 
 	dcl := &fakeDiscoveryClient{}
@@ -534,7 +537,7 @@ func TestNoAuthDelegatorAvailable(t *testing.T) {
 
 func TestAuthDelegatorBecomesAvailable(t *testing.T) {
 	// prepare
-	viper.Set("platform", v1.FlagPlatformOpenShift)
+	OperatorConfiguration.SetPlatform(OpenShiftPlatform)
 	defer viper.Reset()
 
 	dcl := &fakeDiscoveryClient{}
@@ -555,7 +558,7 @@ func TestAuthDelegatorBecomesAvailable(t *testing.T) {
 
 func TestAuthDelegatorBecomesUnavailable(t *testing.T) {
 	// prepare
-	viper.Set("platform", v1.FlagPlatformOpenShift)
+	OperatorConfiguration.SetPlatform(OpenShiftPlatform)
 	defer viper.Reset()
 
 	dcl := &fakeDiscoveryClient{}

diff --git a/pkg/autodetect/operatorconfig.go b/pkg/autodetect/operatorconfig.go
@@ -0,0 +1,72 @@
+package autodetect
+
+import (
+	"strings"
+	"sync"
+
+	"github.com/spf13/viper"
+
+	v1 "github.com/jaegertracing/jaeger-operator/apis/v1"
+)
+
+// Platform holds the auto-detected running platform.
+type Platform int
+
+const (
+	// KubernetesPlatform represents the cluster is Kubernetes.
+	KubernetesPlatform Platform = iota
+
+	// OpenShiftPlatform represents the cluster is OpenShift.
+	OpenShiftPlatform
+)
+
+func (p Platform) String() string {
+	return [...]string{"Kubernetes", "OpenShift"}[p]
+}
+
+var OperatorConfiguration operatorConfigurationWrapper
+
+type operatorConfigurationWrapper struct {
+	mu sync.RWMutex
+}
+
+func (c *operatorConfigurationWrapper) SetPlatform(p interface{}) {
+	var platform string
+	switch v := p.(type) {
+	case string:
+		platform = v
+	case Platform:
+		platform = v.String()
+	default:
+		platform = KubernetesPlatform.String()
+	}
+
+	c.mu.Lock()
+	viper.Set(v1.FlagPlatform, platform)
+	c.mu.Unlock()
+}
+
+func (c *operatorConfigurationWrapper) GetPlatform() Platform {
+	c.mu.RLock()
+	p := viper.GetString(v1.FlagPlatform)
+	c.mu.RUnlock()
+
+	p = strings.ToLower(p)
+
+	var platform Platform
+	switch p {
+	case "openshift":
+		platform = OpenShiftPlatform
+	default:
+		platform = KubernetesPlatform
+	}
+	return platform
+}
+
+func (c *operatorConfigurationWrapper) IsPlatformAutodetectionEnabled() bool {
+	c.mu.RLock()
+	p := viper.GetString(v1.FlagPlatform)
+	c.mu.RUnlock()
+
+	return strings.EqualFold(p, "auto-detect")
+}
diff --git a/pkg/cmd/start/bootstrap.go b/pkg/cmd/start/bootstrap.go
@@ -104,7 +104,7 @@ func bootstrap(ctx context.Context) manager.Manager {
 		log.Log.V(6).Info("%s", err)
 	}
 
-	span.SetAttributes(otelattribute.String("Platform", viper.GetString("platform")))
+	span.SetAttributes(otelattribute.String("Platform", autodetect.OperatorConfiguration.GetPlatform().String()))
 	watchNamespace, found := os.LookupEnv("WATCH_NAMESPACE")
 	if found {
 		setupLog.Info("watching namespace(s)", "namespaces", watchNamespace)

diff --git a/pkg/config/ca/ca.go b/pkg/config/ca/ca.go
@@ -4,11 +4,11 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/spf13/viper"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	v1 "github.com/jaegertracing/jaeger-operator/apis/v1"
+	"github.com/jaegertracing/jaeger-operator/pkg/autodetect"
 	"github.com/jaegertracing/jaeger-operator/pkg/util"
 )
 
@@ -24,7 +24,7 @@ const (
 // GetTrustedCABundle returns a trusted CA bundle configmap if platform is OpenShift
 func GetTrustedCABundle(jaeger *v1.Jaeger) *corev1.ConfigMap {
 	// Only configure the trusted CA if running in OpenShift
-	if viper.GetString("platform") != v1.FlagPlatformOpenShift {
+	if autodetect.OperatorConfiguration.GetPlatform() != autodetect.OpenShiftPlatform {
 		return nil
 	}
 
@@ -59,7 +59,7 @@ func GetTrustedCABundle(jaeger *v1.Jaeger) *corev1.ConfigMap {
 // GetServiceCABundle returns a service CA configmap if platform is OpenShift
 func GetServiceCABundle(jaeger *v1.Jaeger) *corev1.ConfigMap {
 	// Only configure the service CA if running in OpenShift
-	if viper.GetString("platform") != v1.FlagPlatformOpenShift {
+	if autodetect.OperatorConfiguration.GetPlatform() != autodetect.OpenShiftPlatform {
 		return nil
 	}
 
@@ -93,7 +93,7 @@ func GetServiceCABundle(jaeger *v1.Jaeger) *corev1.ConfigMap {
 // trusted CA bundle volume and volumeMount, if running on OpenShift
 func Update(jaeger *v1.Jaeger, commonSpec *v1.JaegerCommonSpec) {
 	// Only configure the trusted CA if running in OpenShift
-	if viper.GetString("platform") != v1.FlagPlatformOpenShift {
+	if autodetect.OperatorConfiguration.GetPlatform() != autodetect.OpenShiftPlatform {
 		return
 	}
 
@@ -130,7 +130,7 @@ func Update(jaeger *v1.Jaeger, commonSpec *v1.JaegerCommonSpec) {
 // AddServiceCA will modify the supplied common spec, to include
 // the service CA volume and volumeMount, if running on OpenShift
 func AddServiceCA(jaeger *v1.Jaeger, commonSpec *v1.JaegerCommonSpec) {
-	if viper.GetString("platform") != v1.FlagPlatformOpenShift {
+	if autodetect.OperatorConfiguration.GetPlatform() != autodetect.OpenShiftPlatform {
 		return
 	}