Protect auth delegator behind a mutex (jaegertracing#2318)

Signed-off-by: Israel Blancas <[email protected]>

apis/v1/jaeger_types.go

@@ -54,6 +54,9 @@ const (
 	// FlagKafkaProvision represents the 'kafka-provision' flag.
 	FlagKafkaProvision = "kafka-provision"
 
+	// FlagAuthDelegatorAvailability represents the 'auth-delegator-available' flag.
+	FlagAuthDelegatorAvailability = "auth-delegator-available"
+
 	// IngressSecurityNone disables any form of security for ingress objects (default)
 	IngressSecurityNone IngressSecurityType = ""
 

pkg/autodetect/main.go

@@ -356,32 +356,32 @@ func (b *Background) detectClusterRoles(ctx context.Context) {
 			Token: "TEST",
 		},
 	}
-	currentAuthDelegator := viper.GetBool("auth-delegator-available")
-	var newAuthDelegator bool
+	currentAuthDelegator := OperatorConfiguration.GetAuthDelegator()
+	var newAuthDelegator AuthDelegatorAvailability
 	if err := b.cl.Create(ctx, tr); err != nil {
-		if !viper.IsSet("auth-delegator-available") || (viper.IsSet("auth-delegator-available") && currentAuthDelegator) {
+		if !OperatorConfiguration.IsAuthDelegatorSet() || OperatorConfiguration.IsAuthDelegatorAvailable() {
 			// for the first run, we log this info, or when the previous value was true
 			log.Log.Info(
 				"The service account running this operator does not have the role 'system:auth-delegator', consider granting it for additional capabilities",
 			)
 		}
-		newAuthDelegator = false
+		newAuthDelegator = AuthDelegatorAvailabilityNo
 	} else {
 		// this isn't technically correct, as we only ensured that we can create token reviews (which is what the OAuth Proxy does)
 		// but it might be the case that we have *another* cluster role that includes this access and still not have
 		// the "system:auth-delegator". This is an edge case, and it's more complicated to check that, so, we'll keep it simple for now
 		// and deal with the edge case if it ever manifests in the real world
-		if !viper.IsSet("auth-delegator-available") || (viper.IsSet("auth-delegator-available") && !currentAuthDelegator) {
+		if !OperatorConfiguration.IsAuthDelegatorSet() || (OperatorConfiguration.IsAuthDelegatorSet() && !OperatorConfiguration.IsAuthDelegatorAvailable()) {
 			// for the first run, we log this info, or when the previous value was 'false'
 			log.Log.Info(
 				"The service account running this operator has the role 'system:auth-delegator', enabling OAuth Proxy's 'delegate-urls' option",
 			)
 		}
-		newAuthDelegator = true
+		newAuthDelegator = AuthDelegatorAvailabilityYes
 	}
 
-	if currentAuthDelegator != newAuthDelegator || !viper.IsSet("auth-delegator-available") {
-		viper.Set("auth-delegator-available", newAuthDelegator)
+	if currentAuthDelegator != newAuthDelegator || !OperatorConfiguration.IsAuthDelegatorSet() {
+		OperatorConfiguration.SetAuthDelegatorAvailability(newAuthDelegator)
 	}
 
 	if err := b.cl.Delete(ctx, tr); err != nil {

pkg/autodetect/main_test.go

@@ -25,7 +25,7 @@ func TestStart(t *testing.T) {
 	defer viper.Reset()
 
 	// sanity check
-	assert.False(t, viper.IsSet("auth-delegator-available"))
+	assert.False(t, OperatorConfiguration.IsAuthDelegatorAvailable())
 
 	// prepare
 	dcl := &fakeDiscoveryClient{}
@@ -35,7 +35,7 @@ func TestStart(t *testing.T) {
 	done := make(chan bool)
 	go func() {
 		for {
-			if viper.IsSet("auth-delegator-available") {
+			if OperatorConfiguration.IsAuthDelegatorSet() {
 				break
 			}
 			// it would typically take less than 10ms to get the first result already, so, it should wait only once
@@ -50,7 +50,7 @@ func TestStart(t *testing.T) {
 	// verify
 	select {
 	case <-done:
-		assert.True(t, viper.GetBool("auth-delegator-available"))
+		assert.True(t, OperatorConfiguration.IsAuthDelegatorAvailable())
 	case <-time.After(1 * time.Second):
 		assert.Fail(t, "timed out waiting for the start process to detect the capabilities")
 	}
@@ -71,7 +71,7 @@ func TestStartContinuesInBackground(t *testing.T) {
 	done := make(chan bool)
 	go func() {
 		for {
-			if viper.IsSet("auth-delegator-available") {
+			if OperatorConfiguration.IsAuthDelegatorSet() {
 				break
 			}
 			// it would typically take less than 10ms to get the first result already, so, it should wait only once
@@ -84,7 +84,7 @@ func TestStartContinuesInBackground(t *testing.T) {
 
 	select {
 	case <-done:
-		assert.False(t, viper.GetBool("auth-delegator-available"))
+		assert.False(t, OperatorConfiguration.IsAuthDelegatorAvailable())
 	case <-time.After(1 * time.Second):
 		assert.Fail(t, "timed out waiting for the start process to detect the capabilities")
 	}
@@ -94,7 +94,7 @@ func TestStartContinuesInBackground(t *testing.T) {
 
 	go func() {
 		for {
-			if viper.GetBool("auth-delegator-available") {
+			if OperatorConfiguration.IsAuthDelegatorAvailable() {
 				break
 			}
 			time.Sleep(500 * time.Millisecond)
@@ -105,7 +105,7 @@ func TestStartContinuesInBackground(t *testing.T) {
 	// verify
 	select {
 	case <-done:
-		assert.True(t, viper.GetBool("auth-delegator-available"))
+		assert.True(t, OperatorConfiguration.IsAuthDelegatorAvailable())
 	case <-time.After(6 * time.Second): // this one might take up to 5 seconds to run again + processing time
 		assert.Fail(t, "timed out waiting for the start process to detect the new capabilities")
 	}
@@ -510,7 +510,7 @@ func TestSkipAuthDelegatorNonOpenShift(t *testing.T) {
 	b.detectClusterRoles(context.Background())
 
 	// verify
-	assert.False(t, viper.IsSet("auth-delegator-available"))
+	assert.False(t, OperatorConfiguration.IsAuthDelegatorAvailable())
 }
 
 func TestNoAuthDelegatorAvailable(t *testing.T) {
@@ -529,7 +529,7 @@ func TestNoAuthDelegatorAvailable(t *testing.T) {
 	b.detectClusterRoles(context.Background())
 
 	// verify
-	assert.False(t, viper.GetBool("auth-delegator-available"))
+	assert.False(t, OperatorConfiguration.IsAuthDelegatorAvailable())
 }
 
 func TestAuthDelegatorBecomesAvailable(t *testing.T) {
@@ -546,11 +546,11 @@ func TestAuthDelegatorBecomesAvailable(t *testing.T) {
 
 	// test
 	b.detectClusterRoles(context.Background())
-	assert.False(t, viper.GetBool("auth-delegator-available"))
+	assert.False(t, OperatorConfiguration.IsAuthDelegatorAvailable())
 
 	cl.CreateFunc = cl.Client.Create
 	b.detectClusterRoles(context.Background())
-	assert.True(t, viper.GetBool("auth-delegator-available"))
+	assert.True(t, OperatorConfiguration.IsAuthDelegatorAvailable())
 }
 
 func TestAuthDelegatorBecomesUnavailable(t *testing.T) {
@@ -564,13 +564,13 @@ func TestAuthDelegatorBecomesUnavailable(t *testing.T) {
 
 	// test
 	b.detectClusterRoles(context.Background())
-	assert.True(t, viper.GetBool("auth-delegator-available"))
+	assert.True(t, OperatorConfiguration.IsAuthDelegatorAvailable())
 
 	cl.CreateFunc = func(ctx context.Context, obj client.Object, opts ...client.CreateOption) error {
 		return fmt.Errorf("faked error")
 	}
 	b.detectClusterRoles(context.Background())
-	assert.False(t, viper.GetBool("auth-delegator-available"))
+	assert.False(t, OperatorConfiguration.IsAuthDelegatorAvailable())
 }
 
 type fakeClient struct {

pkg/autodetect/operatorconfig.go

@@ -54,6 +54,24 @@ func (p KafkaOperatorIntegration) String() string {
 	return [...]string{"Yes", "No"}[p]
 }
 
+// AuthDelegatorAvailability holds the if the AuthDelegator available.
+type AuthDelegatorAvailability int
+
+const (
+	// AuthDelegatorAvailabilityYes represents the AuthDelegator is available.
+	AuthDelegatorAvailabilityYes AuthDelegatorAvailability = iota
+
+	// AuthDelegatorAvailabilityNo represents the AuthDelegator is not available.
+	AuthDelegatorAvailabilityNo
+
+	// AuthDelegatorAvailabilityUnknown represents the AuthDelegator availability is not known.
+	AuthDelegatorAvailabilityUnknown
+)
+
+func (p AuthDelegatorAvailability) String() string {
+	return [...]string{"Yes", "No", "Unknown"}[p]
+}
+
 var OperatorConfiguration operatorConfigurationWrapper
 
 type operatorConfigurationWrapper struct {
@@ -160,3 +178,46 @@ func (c *operatorConfigurationWrapper) GetKafkaIntegration() KafkaOperatorIntegr
 func (c *operatorConfigurationWrapper) IsKafkaOperatorIntegrationEnabled() bool {
 	return c.GetKafkaIntegration() == KafkaOperatorIntegrationYes
 }
+
+func (c *operatorConfigurationWrapper) SetAuthDelegatorAvailability(e interface{}) {
+	var availability string
+	switch v := e.(type) {
+	case string:
+		availability = v
+	case AuthDelegatorAvailability:
+		availability = v.String()
+	default:
+		availability = AuthDelegatorAvailabilityUnknown.String()
+	}
+
+	c.mu.Lock()
+	viper.Set(v1.FlagAuthDelegatorAvailability, availability)
+	c.mu.Unlock()
+}
+
+func (c *operatorConfigurationWrapper) GetAuthDelegator() AuthDelegatorAvailability {
+	c.mu.RLock()
+	e := viper.GetString(v1.FlagAuthDelegatorAvailability)
+	c.mu.RUnlock()
+
+	var available AuthDelegatorAvailability
+	switch strings.ToLower(e) {
+	case "yes":
+		available = AuthDelegatorAvailabilityYes
+	case "no":
+		available = AuthDelegatorAvailabilityNo
+	default:
+		available = AuthDelegatorAvailabilityUnknown
+	}
+	return available
+}
+
+// IsAuthDelegatorAvailable returns true if the AuthDelegator is available
+func (c *operatorConfigurationWrapper) IsAuthDelegatorAvailable() bool {
+	return c.GetAuthDelegator() == AuthDelegatorAvailabilityYes
+}
+
+// IsAuthDelegatorAvailable returns true if the AuthDelegator is set
+func (c *operatorConfigurationWrapper) IsAuthDelegatorSet() bool {
+	return c.GetAuthDelegator() != AuthDelegatorAvailabilityUnknown
+}

pkg/clusterrolebinding/main.go

@@ -3,19 +3,19 @@ package clusterrolebinding
 import (
 	"fmt"
 
-	"github.com/spf13/viper"
 	rbac "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	v1 "github.com/jaegertracing/jaeger-operator/apis/v1"
 	"github.com/jaegertracing/jaeger-operator/pkg/account"
+	"github.com/jaegertracing/jaeger-operator/pkg/autodetect"
 	"github.com/jaegertracing/jaeger-operator/pkg/util"
 )
 
 // Get returns all the service accounts to be created for this Jaeger instance
 func Get(jaeger *v1.Jaeger) []rbac.ClusterRoleBinding {
 	if jaeger.Spec.Ingress.Security == v1.IngressSecurityOAuthProxy && len(jaeger.Spec.Ingress.Openshift.DelegateUrls) > 0 {
-		if viper.GetBool("auth-delegator-available") {
+		if autodetect.OperatorConfiguration.IsAuthDelegatorAvailable() {
 			return []rbac.ClusterRoleBinding{oauthProxyAuthDelegator(jaeger)}
 		}
 

pkg/clusterrolebinding/main_test.go

@@ -9,14 +9,15 @@ import (
 
 	v1 "github.com/jaegertracing/jaeger-operator/apis/v1"
 	"github.com/jaegertracing/jaeger-operator/pkg/account"
+	"github.com/jaegertracing/jaeger-operator/pkg/autodetect"
 )
 
 func TestGetClusterRoleBinding(t *testing.T) {
 	// prepare
 	name := "TestGetClusterRoleBinding"
 	trueVar := true
 
-	viper.Set("auth-delegator-available", true)
+	autodetect.OperatorConfiguration.SetAuthDelegatorAvailability(autodetect.AuthDelegatorAvailabilityYes)
 	defer viper.Reset()
 
 	jaeger := v1.NewJaeger(types.NamespacedName{Name: name})
@@ -77,7 +78,7 @@ func TestAuthDelegatorNotAvailable(t *testing.T) {
 	name := "TestAuthDelegatorNotAvailable"
 	trueVar := true
 
-	viper.Set("auth-delegator-available", false)
+	autodetect.OperatorConfiguration.SetAuthDelegatorAvailability(autodetect.AuthDelegatorAvailabilityNo)
 	defer viper.Reset()
 
 	jaeger := v1.NewJaeger(types.NamespacedName{Name: name})

pkg/inject/oauth_proxy.go

@@ -11,6 +11,7 @@ import (
 
 	v1 "github.com/jaegertracing/jaeger-operator/apis/v1"
 	"github.com/jaegertracing/jaeger-operator/pkg/account"
+	"github.com/jaegertracing/jaeger-operator/pkg/autodetect"
 	"github.com/jaegertracing/jaeger-operator/pkg/config/ca"
 	"github.com/jaegertracing/jaeger-operator/pkg/service"
 	"github.com/jaegertracing/jaeger-operator/pkg/util"
@@ -90,7 +91,7 @@ func getOAuthProxyContainer(jaeger *v1.Jaeger) corev1.Container {
 		args = append(args, fmt.Sprintf("--openshift-sar=%s", *jaeger.Spec.Ingress.Openshift.SAR))
 	}
 
-	if len(jaeger.Spec.Ingress.Openshift.DelegateUrls) > 0 && viper.GetBool("auth-delegator-available") {
+	if len(jaeger.Spec.Ingress.Openshift.DelegateUrls) > 0 && autodetect.OperatorConfiguration.IsAuthDelegatorAvailable() {
 		args = append(args, fmt.Sprintf("--openshift-delegate-urls=%s", jaeger.Spec.Ingress.Openshift.DelegateUrls))
 	}
 

pkg/inject/oauth_proxy_test.go

@@ -131,7 +131,7 @@ func TestDoNotMountWhenNotNeeded(t *testing.T) {
 }
 
 func TestOAuthProxyWithCustomDelegateURLs(t *testing.T) {
-	viper.Set("auth-delegator-available", true)
+	autodetect.OperatorConfiguration.SetAuthDelegatorAvailability(autodetect.AuthDelegatorAvailabilityYes)
 	defer viper.Reset()
 
 	jaeger := v1.NewJaeger(types.NamespacedName{Name: "my-instance"})
@@ -149,7 +149,7 @@ func TestOAuthProxyWithCustomDelegateURLs(t *testing.T) {
 }
 
 func TestOAuthProxyWithCustomDelegateURLsWithoutProperClusterRole(t *testing.T) {
-	viper.Set("auth-delegator-available", false)
+	autodetect.OperatorConfiguration.SetAuthDelegatorAvailability(autodetect.AuthDelegatorAvailabilityNo)
 	defer func() {
 		viper.Reset()
 		setDefaults()