From 2d54ca3303a9858543d231dc4573af41f10eb390 Mon Sep 17 00:00:00 2001 From: Jan Wozniak Date: Fri, 16 Feb 2018 10:50:17 +0100 Subject: [PATCH] diagnostics: AggregatedLogging ClusterRoleBindings false negative fix The clusterRoleBindings check can output false alarm even if `cluster-reader` role is assigned to fluentd service account and logging works as expected. The check used to query for `cluster-readers` CRB, but the common command `oc adm policy add-cluster-role-to-user cluster-reader system:serviceaccount:logging:aggregated-logging-fluentd` no longer appers to add the SA into `cluster-readers` group but instead creates cluster-reader-1 CRB. ``` $ oc get clusterrolebindings cluster-readers -o yaml ... roleRef: name: cluster-reader ... userNames: null $ oc get clusterrolebindings NAME ROLE USERS GROUPS SERVICE ACCOUNTS SUBJECTS ... cluster-reader /cluster-reader management-infra/management-admin cluster-reader-0 /cluster-reader default/router cluster-reader-1 /cluster-reader logging/aggregated-logging-fluentd cluster-readers /cluster-reader system:cluster-readers ... ``` This fix queries all clusterrolebindings, iterates over those, that have role `cluster-reader` and then validates there is a `cluster-reader` entry for `system:serviceaccount:logging:aggregated-logging-fluentd` --- .../aggregated_logging/clusterrolebindings.go | 15 ++++++++++----- .../clusterrolebindings_test.go | 18 +++++++++++++----- .../cluster/aggregated_logging/diagnostic.go | 4 ++-- .../cluster/aggregated_logging/interfaces.go | 2 +- 4 files changed, 26 insertions(+), 13 deletions(-) diff --git a/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/clusterrolebindings.go b/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/clusterrolebindings.go index d75cfa3d35dc..1ba823d7c634 100644 --- a/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/clusterrolebindings.go +++ b/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/clusterrolebindings.go @@ -7,7 +7,7 @@ import ( "k8s.io/kubernetes/pkg/apis/rbac" ) -const clusterReaderRoleBindingName = "cluster-readers" +const clusterReaderRoleBindingRoleName = "cluster-reader" var clusterReaderRoleBindingNames = sets.NewString(fluentdServiceAccountName) @@ -22,15 +22,20 @@ the following: func checkClusterRoleBindings(r diagnosticReporter, adapter clusterRoleBindingsAdapter, project string) { r.Debug("AGL0600", "Checking ClusterRoleBindings...") - crb, err := adapter.getClusterRoleBinding(clusterReaderRoleBindingName) + crbs, err := adapter.listClusterRoleBindings() if err != nil { r.Error("AGL0605", err, fmt.Sprintf("There was an error while trying to retrieve the ClusterRoleBindings for the logging stack: %s", err)) return } boundServiceAccounts := sets.NewString() - for _, subject := range crb.Subjects { - if subject.Kind == rbac.ServiceAccountKind && subject.Namespace == project { - boundServiceAccounts.Insert(subject.Name) + for _, crb := range crbs.Items { + if crb.RoleRef.Name != clusterReaderRoleBindingRoleName { + continue + } + for _, subject := range crb.Subjects { + if subject.Kind == rbac.ServiceAccountKind && subject.Namespace == project { + boundServiceAccounts.Insert(subject.Name) + } } } for _, name := range clusterReaderRoleBindingNames.List() { diff --git a/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/clusterrolebindings_test.go b/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/clusterrolebindings_test.go index 92a795293833..5b343dd47bfc 100644 --- a/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/clusterrolebindings_test.go +++ b/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/clusterrolebindings_test.go @@ -13,20 +13,21 @@ import ( type fakeRoleBindingDiagnostic struct { fakeDiagnostic - fakeClusterRoleBinding authapi.ClusterRoleBinding + fakeClusterRoleBindingList authapi.ClusterRoleBindingList } func newFakeRoleBindingDiagnostic(t *testing.T) *fakeRoleBindingDiagnostic { return &fakeRoleBindingDiagnostic{ - fakeDiagnostic: *newFakeDiagnostic(t), + fakeDiagnostic: *newFakeDiagnostic(t), + fakeClusterRoleBindingList: authapi.ClusterRoleBindingList{}, } } -func (f *fakeRoleBindingDiagnostic) getClusterRoleBinding(name string) (*authapi.ClusterRoleBinding, error) { +func (f *fakeRoleBindingDiagnostic) listClusterRoleBindings() (*authapi.ClusterRoleBindingList, error) { if f.err != nil { return nil, f.err } - return &f.fakeClusterRoleBinding, nil + return &f.fakeClusterRoleBindingList, nil } func (f *fakeRoleBindingDiagnostic) addBinding(name string, namespace string) { ref := kapi.ObjectReference{ @@ -34,7 +35,14 @@ func (f *fakeRoleBindingDiagnostic) addBinding(name string, namespace string) { Kind: rbac.ServiceAccountKind, Namespace: namespace, } - f.fakeClusterRoleBinding.Subjects = append(f.fakeClusterRoleBinding.Subjects, ref) + if len(f.fakeClusterRoleBindingList.Items) == 0 { + f.fakeClusterRoleBindingList.Items = append(f.fakeClusterRoleBindingList.Items, authapi.ClusterRoleBinding{ + RoleRef: kapi.ObjectReference{ + Name: clusterReaderRoleBindingRoleName, + }, + }) + } + f.fakeClusterRoleBindingList.Items[0].Subjects = append(f.fakeClusterRoleBindingList.Items[0].Subjects, ref) } // Test error when client error diff --git a/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/diagnostic.go b/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/diagnostic.go index 6c46c9e0a856..0d86d7ae08be 100644 --- a/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/diagnostic.go +++ b/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/diagnostic.go @@ -86,8 +86,8 @@ func (d *AggregatedLogging) getScc(name string) (*securityapi.SecurityContextCon return d.SCCClient.SecurityContextConstraints().Get(name, metav1.GetOptions{}) } -func (d *AggregatedLogging) getClusterRoleBinding(name string) (*authapi.ClusterRoleBinding, error) { - return d.CRBClient.ClusterRoleBindings().Get(name, metav1.GetOptions{}) +func (d *AggregatedLogging) listClusterRoleBindings() (*authapi.ClusterRoleBindingList, error) { + return d.CRBClient.ClusterRoleBindings().List(metav1.ListOptions{}) } func (d *AggregatedLogging) routes(project string, options metav1.ListOptions) (*routesapi.RouteList, error) { diff --git a/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/interfaces.go b/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/interfaces.go index 1f66e0333424..d4bc193a7c75 100644 --- a/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/interfaces.go +++ b/pkg/oc/admin/diagnostics/diagnostics/cluster/aggregated_logging/interfaces.go @@ -28,7 +28,7 @@ type sccAdapter interface { } type clusterRoleBindingsAdapter interface { - getClusterRoleBinding(name string) (*authapi.ClusterRoleBinding, error) + listClusterRoleBindings() (*authapi.ClusterRoleBindingList, error) } //deploymentConfigAdapter is an abstraction to retrieve resource for validating dcs