Skip to content

Commit

Permalink
diagnostics: AggregatedLogging ClusterRoleBindings false negative fix
Browse files Browse the repository at this point in the history
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`
  • Loading branch information
Jan Wozniak authored and openshift-cherrypick-robot committed Mar 7, 2018
1 parent a9ce967 commit 59a7133
Show file tree
Hide file tree
Showing 4 changed files with 26 additions and 13 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import (
"k8s.io/kubernetes/pkg/apis/rbac"
)

const clusterReaderRoleBindingName = "cluster-readers"
const clusterReaderRoleBindingRoleName = "cluster-reader"

var clusterReaderRoleBindingNames = sets.NewString(fluentdServiceAccountName)

Expand All @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,28 +13,36 @@ 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{
Name: name,
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 59a7133

Please sign in to comment.