Merge pull request #957 from aminesnow/oauth_cookie_bp_54

[release-5.4] LOG-3306: Fix kibana oauth cookie expiration issue

apis/logging/v1/groupversion_info.go

@@ -32,7 +32,7 @@ var (
 // +kubebuilder:rbac:groups=authentication.k8s.io,resources=tokenreviews;subjectaccessreviews,verbs=create
 // +kubebuilder:rbac:groups=authorization.k8s.io,resources=subjectaccessreviews,verbs=create
 // +kubebuilder:rbac:groups=rbac.authorization.k8s.io,resources=roles;rolebindings,verbs=*
-// +kubebuilder:rbac:groups=config.openshift.io,resources=proxies,verbs=get;list;watch
+// +kubebuilder:rbac:groups=config.openshift.io,resources=proxies;oauths,verbs=get;list;watch
 // +kubebuilder:rbac:groups=networking.k8s.io,resources=networkpolicies,verbs=create;delete
 // +kubebuilder:rbac:groups=apps,resourceNames=elasticsearch-operator,resources=deployments/finalizers,verbs=update
 // +kubebuilder:rbac:groups=coordination.k8s.io,resources=leases,verbs=get;create;update

bundle/manifests/elasticsearch-operator.clusterserviceversion.yaml

@@ -362,6 +362,7 @@ spec:
         - apiGroups:
           - config.openshift.io
           resources:
+          - oauths
           - proxies
           verbs:
           - get

config/rbac/role.yaml

@@ -49,6 +49,7 @@ rules:
 - apiGroups:
   - config.openshift.io
   resources:
+  - oauths
   - proxies
   verbs:
   - get

controllers/logging/kibana_controller.go

@@ -296,6 +296,14 @@ func (r *KibanaReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		GenericFunc: func(e event.GenericEvent) bool { return false },
 	}
 
+	// Watch for updates to the global oauth config only
+	oauthPred := predicate.Funcs{
+		UpdateFunc:  func(e event.UpdateEvent) bool { return true },
+		DeleteFunc:  func(e event.DeleteEvent) bool { return true },
+		CreateFunc:  func(e event.CreateEvent) bool { return true },
+		GenericFunc: func(e event.GenericEvent) bool { return false },
+	}
+
 	// TODO: replace the watches with For and Own
 	return ctrl.NewControllerManagedBy(mgr).
 		Named("kibana-controller").
@@ -309,5 +317,6 @@ func (r *KibanaReconciler) SetupWithManager(mgr ctrl.Manager) error {
 			IsController: true,
 		}, builder.WithPredicates(routePred)).
 		Watches(&source.Kind{Type: &imagev1.ImageStream{}}, globalMapHandler, builder.WithPredicates(isPred)).
+		Watches(&source.Kind{Type: &configv1.OAuth{}}, globalMapHandler, builder.WithPredicates(oauthPred)).
 		Complete(r)
 }

internal/constants/constants.go

@@ -4,6 +4,7 @@ import "github.com/openshift/elasticsearch-operator/internal/utils"
 
 const (
 	ProxyName                   = "cluster"
+	OAuthName                   = "cluster"
 	TrustedCABundleKey          = "ca-bundle.crt"
 	TrustedCABundleMountDir     = "/etc/pki/ca-trust/extracted/pem/"
 	TrustedCABundleMountFile    = "tls-ca-bundle.pem"

internal/elasticsearch/common.go

@@ -661,19 +661,22 @@ func newVolumeSource(clusterName, nodeName, namespace string, node api.Elasticse
 kind: NetworkPolicy
 apiVersion: networking.k8s.io/v1
 metadata:
-  name: restricted-es-access
+
+	name: restricted-es-access
+
 spec:
-  podSelector:
-    matchLabels:
-      component: elasticsearch
-  ingress:
-  - from:
-    - podSelector:
-        matchLabels:
-          name: elasticsearch-operator
-    ports:
-    - protocol: TCP
-      port: 9200
+
+	podSelector:
+	  matchLabels:
+	    component: elasticsearch
+	ingress:
+	- from:
+	  - podSelector:
+	      matchLabels:
+	        name: elasticsearch-operator
+	  ports:
+	  - protocol: TCP
+	    port: 9200
 */
 func newNetworkPolicy(namespace string) networking.NetworkPolicy {
 	protocol := v1.ProtocolTCP

internal/kibana/kibana_test.go

@@ -30,6 +30,7 @@ var _ = Describe("Reconciling", func() {
 	_ = consolev1.AddToScheme(scheme.Scheme)
 	_ = loggingv1.SchemeBuilder.AddToScheme(scheme.Scheme)
 	_ = imagev1.AddToScheme(scheme.Scheme)
+	_ = configv1.AddToScheme(scheme.Scheme)
 
 	var (
 		cluster = &loggingv1.Kibana{
@@ -137,6 +138,7 @@ var _ = Describe("Reconciling", func() {
 				},
 			},
 		}
+		oauth = &configv1.OAuth{}
 	)
 
 	Describe("Kibana", func() {
@@ -197,6 +199,7 @@ var _ = Describe("Reconciling", func() {
 					kibanaSecret,
 					kibanaProxySecret,
 					proxySourceImage,
+					oauth,
 				)
 				esClient = newFakeEsClient(client, fakeResponses)
 			})

internal/kibana/reconciler.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"reflect"
+	"time"
 
 	"github.com/ViaQ/logerr/kverrors"
 	"k8s.io/apimachinery/pkg/types"
@@ -38,6 +39,7 @@ const (
 	oauthProxyImageName          = "oauth-proxy"
 	oauthProxyNamespace          = "openshift"
 	oauthProxyTag                = "v4.4"
+	oauthTimeout                 = 24 * time.Hour
 )
 
 var kibanaServiceAccountAnnotations = map[string]string{
@@ -102,6 +104,19 @@ func Reconcile(requestCluster *kibana.Kibana, requestClient client.Client, esCli
 	return clusterKibanaRequest.UpdateStatus()
 }
 
+func getOAuthConfig(r client.Client) (*configv1.OAuth, error) {
+	oauthNamespacedName := types.NamespacedName{Name: constants.OAuthName}
+	oauthConfig := &configv1.OAuth{}
+	if err := r.Get(context.TODO(), oauthNamespacedName, oauthConfig); err != nil {
+		if !apierrors.IsNotFound(err) {
+			return nil, kverrors.Wrap(err, "encountered unexpected error getting oauth",
+				"oauth", oauthNamespacedName,
+			)
+		}
+	}
+	return oauthConfig, nil
+}
+
 func GetProxyConfig(r client.Client) (*configv1.Proxy, error) {
 	proxyNamespacedName := types.NamespacedName{Name: constants.ProxyName}
 	proxyConfig := &configv1.Proxy{}
@@ -227,10 +242,21 @@ func (clusterRequest *KibanaRequest) createOrUpdateKibanaDeployment(proxyConfig
 		return kverrors.Wrap(err, "Failed to get oauth-proxy image")
 	}
 
+	oauthConfig, err := getOAuthConfig(clusterRequest.client)
+	if err != nil {
+		return kverrors.Wrap(err, "Failed to get oauth config")
+	}
+
+	cookieTimeout := oauthTimeout
+	if oauthConfig != nil && oauthConfig.Spec.TokenConfig.AccessTokenInactivityTimeout != nil {
+		cookieTimeout = oauthConfig.Spec.TokenConfig.AccessTokenInactivityTimeout.Duration
+	}
+
 	kibanaPodSpec := newKibanaPodSpec(
 		clusterRequest,
 		fmt.Sprintf("%s.%s.svc", clusterName, clusterRequest.cluster.Namespace),
 		proxyConfig,
+		cookieTimeout,
 		kibanaTrustBundle,
 		oauthProxyImage,
 	)
@@ -500,7 +526,7 @@ func findTagReferencePolicy(tags []imagev1.TagReference, name string) imagev1.Ta
 	return ""
 }
 
-func newKibanaPodSpec(cluster *KibanaRequest, elasticsearchName string, proxyConfig *configv1.Proxy,
+func newKibanaPodSpec(cluster *KibanaRequest, elasticsearchName string, proxyConfig *configv1.Proxy, oauthTimeout time.Duration,
 	trustedCABundleCM *v1.ConfigMap, oauthProxyImage string) v1.PodSpec {
 	visSpec := kibana.KibanaSpec{}
 	if cluster.cluster != nil {
@@ -583,7 +609,7 @@ func newKibanaPodSpec(cluster *KibanaRequest, elasticsearchName string, proxyCon
 		fmt.Sprintf("-client-id=system:serviceaccount:%s:kibana", cluster.cluster.Namespace),
 		"-client-secret-file=/var/run/secrets/kubernetes.io/serviceaccount/token",
 		"-cookie-secret-file=/secret/session-secret",
-		"-cookie-expire=24h",
+		fmt.Sprintf("-cookie-expire=%s", oauthTimeout),
 		"-skip-provider-button",
 		"-upstream=http://localhost:5601",
 		"-scope=user:info user:check-access user:list-projects",

internal/kibana/reconciler_test.go

@@ -5,6 +5,7 @@ import (
 	"reflect"
 	"strings"
 	"testing"
+	"time"
 
 	kibana "github.com/openshift/elasticsearch-operator/apis/logging/v1"
 
@@ -25,7 +26,7 @@ func TestNewKibanaPodSpecSetsProxyToUseServiceAccountAsOAuthClient(t *testing.T)
 			},
 		},
 	}
-	spec := newKibanaPodSpec(cluster, "kibana", nil, nil, "")
+	spec := newKibanaPodSpec(cluster, "kibana", nil, oauthTimeout, nil, "")
 	for _, arg := range spec.Containers[1].Args {
 		keyValue := strings.Split(arg, "=")
 		if len(keyValue) >= 2 && keyValue[0] == "-client-id" {
@@ -49,7 +50,7 @@ func TestNewKibanaPodSpecWhenFieldsAreUndefined(t *testing.T) {
 			},
 		},
 	}
-	podSpec := newKibanaPodSpec(cluster, "test-app-name", nil, nil, "")
+	podSpec := newKibanaPodSpec(cluster, "test-app-name", nil, oauthTimeout, nil, "")
 
 	if len(podSpec.Containers) != 2 {
 		t.Error("Exp. there to be 2 container")
@@ -99,7 +100,7 @@ func TestNewKibanaPodSpecWhenResourcesAreDefined(t *testing.T) {
 		},
 	}
 
-	podSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, nil, "")
+	podSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, oauthTimeout, nil, "")
 
 	limitMemory := resource.MustParse("100Gi")
 	requestMemory := resource.MustParse("120Gi")
@@ -154,7 +155,7 @@ func TestNewKibanaPodSpecWhenNodeSelectorIsDefined(t *testing.T) {
 		},
 	}
 
-	podSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, nil, "")
+	podSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, oauthTimeout, nil, "")
 
 	// check kibana
 	if !reflect.DeepEqual(podSpec.NodeSelector, expSelector) {
@@ -175,7 +176,7 @@ func TestNewKibanaPodNoTolerations(t *testing.T) {
 		},
 	}
 
-	podSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, nil, "")
+	podSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, oauthTimeout, nil, "")
 	tolerations := podSpec.Tolerations
 
 	if !utils.AreTolerationsSame(tolerations, expTolerations) {
@@ -204,7 +205,7 @@ func TestNewKibanaPodWithTolerations(t *testing.T) {
 		},
 	}
 
-	podSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, nil, "")
+	podSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, oauthTimeout, nil, "")
 	tolerations := podSpec.Tolerations
 
 	if !utils.AreTolerationsSame(tolerations, expTolerations) {
@@ -244,6 +245,7 @@ func TestNewKibanaPodSpecWhenProxyConfigExists(t *testing.T) {
 				NoProxy:    noproxy,
 			},
 		},
+		1*time.Hour,
 		&v1.ConfigMap{
 			ObjectMeta: metav1.ObjectMeta{
 				Namespace: "openshift-logging",
@@ -260,6 +262,8 @@ func TestNewKibanaPodSpecWhenProxyConfigExists(t *testing.T) {
 		t.Error("Exp. there to be 2 kibana container")
 	}
 
+	checkKibanaCookieExpire(t, podSpec, "-cookie-expire=1h0m0s")
+
 	checkKibanaProxyEnvVar(t, podSpec, "HTTP_PROXY", httpproxy)
 	checkKibanaProxyEnvVar(t, podSpec, "HTTPS_PROXY", httpproxy)
 	checkKibanaProxyEnvVar(t, podSpec, "NO_PROXY", noproxy)
@@ -279,7 +283,7 @@ func TestDeploymentDifferentWithKibanaEnvVar(t *testing.T) {
 		},
 	}
 
-	lhsPodSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, nil, "")
+	lhsPodSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, oauthTimeout, nil, "")
 
 	lhsDeployment := NewDeployment(
 		"kibana",
@@ -290,7 +294,7 @@ func TestDeploymentDifferentWithKibanaEnvVar(t *testing.T) {
 		lhsPodSpec,
 	)
 
-	rhsPodSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, nil, "")
+	rhsPodSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, oauthTimeout, nil, "")
 
 	index := -1
 	for k, v := range rhsPodSpec.Containers {
@@ -335,7 +339,7 @@ func TestDeploymentDifferentWithKibanaReplicas(t *testing.T) {
 			},
 		},
 	}
-	lhsPodSpec := newKibanaPodSpec(ClusterRequest, "test-app-name", nil, nil, "")
+	lhsPodSpec := newKibanaPodSpec(ClusterRequest, "test-app-name", nil, oauthTimeout, nil, "")
 	lhsDeployment := NewDeployment(
 		"kibana",
 		ClusterRequest.cluster.Namespace,
@@ -346,7 +350,7 @@ func TestDeploymentDifferentWithKibanaReplicas(t *testing.T) {
 	)
 
 	newReplicas := int32(2)
-	rhsPodSpec := newKibanaPodSpec(ClusterRequest, "test-app-name", nil, nil, "")
+	rhsPodSpec := newKibanaPodSpec(ClusterRequest, "test-app-name", nil, oauthTimeout, nil, "")
 	rhsDeployment := NewDeployment(
 		"kibana",
 		ClusterRequest.cluster.Namespace,
@@ -374,7 +378,7 @@ func TestDeploymentDifferentWithProxyEnvVar(t *testing.T) {
 		},
 	}
 
-	lhsPodSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, nil, "")
+	lhsPodSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, oauthTimeout, nil, "")
 
 	lhsDeployment := NewDeployment(
 		"kibana",
@@ -385,7 +389,7 @@ func TestDeploymentDifferentWithProxyEnvVar(t *testing.T) {
 		lhsPodSpec,
 	)
 
-	rhsPodSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, nil, "")
+	rhsPodSpec := newKibanaPodSpec(clusterRequest, "test-app-name", nil, oauthTimeout, nil, "")
 
 	index := -1
 	for k, v := range rhsPodSpec.Containers {
@@ -418,6 +422,17 @@ func TestDeploymentDifferentWithProxyEnvVar(t *testing.T) {
 	}
 }
 
+func checkKibanaCookieExpire(t *testing.T, podSpec v1.PodSpec, cookieArg string) {
+	args := podSpec.Containers[1].Args
+	for _, arg := range args {
+		if arg == cookieArg {
+			return
+		}
+	}
+
+	t.Errorf("Cookie arg %q not found", cookieArg)
+}
+
 func checkKibanaProxyEnvVar(t *testing.T, podSpec v1.PodSpec, name string, value string) {
 	env := podSpec.Containers[1].Env
 	found := false

internal/utils/comparators/envvars.go

@@ -6,12 +6,14 @@ import (
 	v1 "k8s.io/api/core/v1"
 )
 
-/**
+/*
+*
 EnvValueEqual - check if 2 EnvValues are equal or not
 Notes:
 - reflect.DeepEqual does not return expected results if the to-be-compared value is a pointer.
 - needs to adjust with k8s.io/api/core/v#/types.go when the types are updated.
-**/
+*
+*/
 func EnvValueEqual(lhs, rhs []v1.EnvVar) bool {
 	if len(lhs) != len(rhs) {
 		return false

internal/utils/utils.go

@@ -186,12 +186,14 @@ func RemoveString(slice []string, s string) (result []string) {
 	return
 }
 
-/**
+/*
+*
 EnvValueEqual - check if 2 EnvValues are equal or not
 Notes:
 - reflect.DeepEqual does not return expected results if the to-be-compared value is a pointer.
 - needs to adjust with k8s.io/api/core/v#/types.go when the types are updated.
-**/
+*
+*/
 func EnvValueEqual(env1, env2 []v1.EnvVar) bool {
 	var found bool
 	if len(env1) != len(env2) {