From 5970a8930b5e368c94f49ecfabadd0d41585d7fe Mon Sep 17 00:00:00 2001 From: Mohamed-Amine Bouqsimi Date: Wed, 9 Nov 2022 14:46:23 +0100 Subject: [PATCH] fix kibana oauth cookie expiration --- apis/logging/v1/groupversion_info.go | 2 +- ...search-operator.clusterserviceversion.yaml | 1 + config/rbac/role.yaml | 1 + controllers/logging/kibana_controller.go | 9 +++++ internal/constants/constants.go | 1 + internal/elasticsearch/common.go | 27 +++++++------ internal/kibana/kibana_test.go | 3 ++ internal/kibana/reconciler.go | 30 +++++++++++++- internal/kibana/reconciler_test.go | 39 +++++++++++++------ internal/utils/comparators/envvars.go | 6 ++- internal/utils/utils.go | 6 ++- 11 files changed, 94 insertions(+), 31 deletions(-) diff --git a/apis/logging/v1/groupversion_info.go b/apis/logging/v1/groupversion_info.go index 773b69d1b..46ae6bfef 100644 --- a/apis/logging/v1/groupversion_info.go +++ b/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 diff --git a/bundle/manifests/elasticsearch-operator.clusterserviceversion.yaml b/bundle/manifests/elasticsearch-operator.clusterserviceversion.yaml index cbcc054b0..af7590c03 100644 --- a/bundle/manifests/elasticsearch-operator.clusterserviceversion.yaml +++ b/bundle/manifests/elasticsearch-operator.clusterserviceversion.yaml @@ -362,6 +362,7 @@ spec: - apiGroups: - config.openshift.io resources: + - oauths - proxies verbs: - get diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 80fc78134..21d55c2cb 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -49,6 +49,7 @@ rules: - apiGroups: - config.openshift.io resources: + - oauths - proxies verbs: - get diff --git a/controllers/logging/kibana_controller.go b/controllers/logging/kibana_controller.go index 9a8c8b218..ffa3bfab9 100644 --- a/controllers/logging/kibana_controller.go +++ b/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) } diff --git a/internal/constants/constants.go b/internal/constants/constants.go index 601b6e9cd..f09a3c312 100644 --- a/internal/constants/constants.go +++ b/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" diff --git a/internal/elasticsearch/common.go b/internal/elasticsearch/common.go index bef947d14..645aa556d 100644 --- a/internal/elasticsearch/common.go +++ b/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 diff --git a/internal/kibana/kibana_test.go b/internal/kibana/kibana_test.go index 62ae8daff..201a2aba5 100644 --- a/internal/kibana/kibana_test.go +++ b/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) }) diff --git a/internal/kibana/reconciler.go b/internal/kibana/reconciler.go index c440b899d..1379b3d73 100644 --- a/internal/kibana/reconciler.go +++ b/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", diff --git a/internal/kibana/reconciler_test.go b/internal/kibana/reconciler_test.go index 7f4472614..9f78db4c2 100644 --- a/internal/kibana/reconciler_test.go +++ b/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 diff --git a/internal/utils/comparators/envvars.go b/internal/utils/comparators/envvars.go index a22d01918..d248e0e32 100644 --- a/internal/utils/comparators/envvars.go +++ b/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 diff --git a/internal/utils/utils.go b/internal/utils/utils.go index 132c1a455..d4760dc6b 100644 --- a/internal/utils/utils.go +++ b/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) {