Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

diagnostics: AggregatedLogging ClusterRoleBindings false negative fix #18641

Merged
merged 1 commit into from
Mar 7, 2018
Merged

diagnostics: AggregatedLogging ClusterRoleBindings false negative fix #18641

merged 1 commit into from
Mar 7, 2018

Conversation

wozniakjan
Copy link
Contributor

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

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 16, 2018
@wozniakjan wozniakjan force-pushed the diagnostics/aggregatedlogging/clusterrolebinding_falsepositive branch from 72d68a3 to 847a3af Compare February 16, 2018 13:02
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 16, 2018
@wozniakjan wozniakjan changed the title diagnostics: AggregatedLogging ClusterRoleBindings false positive fix diagnostics: AggregatedLogging ClusterRoleBindings false negative fix Feb 16, 2018
diagnostics: AggregatedLogging ClusterRoleBindings false negative fix
2d54ca3 
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`
@wozniakjan wozniakjan force-pushed the diagnostics/aggregatedlogging/clusterrolebinding_falsepositive branch from 847a3af to 2d54ca3 Compare February 16, 2018 13:21
@jcantrill
Copy link
Contributor

/test unit

@wozniakjan
Copy link
Contributor Author

/test gcp

@wozniakjan
Copy link
Contributor Author

/assign @liggitt

@anpingli
Copy link

https://bugzilla.redhat.com/show_bug.cgi?id=1550018 have been filed to trace this issue.

@sosiouxme
Copy link
Member

As a replacement for the logic, looks fine.
/lgtm

It would probably be more stable to use a SAR to determine that the service account can read *

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 28, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sosiouxme, wozniakjan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2018
@jcantrill
Copy link
Contributor

/test gcp
/test extended_conformance_install

1 similar comment
@jcantrill
Copy link
Contributor

/test gcp
/test extended_conformance_install

@jcantrill
Copy link
Contributor

/test gcp

1 similar comment
@jcantrill
Copy link
Contributor

/test gcp

@sosiouxme
Copy link
Member

infra bork
/retest

@wozniakjan
Copy link
Contributor Author

/retest

@wozniakjan
Copy link
Contributor Author

Flake - Get https://registry.svc.ci.openshift.org/v2/ci/clonerefs/manifests/latest: dial tcp 35.184.16.217:443: i/o timeout.
/retest

@wozniakjan
Copy link
Contributor Author

flake failures in unrelated code

[Fail] [Feature:Builds][Conformance] s2i build with a root user image  [It] should create a root build and pass with a privileged SCC [Suite:openshift/conformance/parallel] 
/tmp/openshift/build-rpms/rpm/BUILD/origin-3.10.0/_output/local/go/src/github.com/openshift/origin/test/extended/builds/s2i_root.go:95

[Fail] [Feature:Builds] forcePull should affect pulling builder images  [It] ForcePull test case execution s2i [Suite:openshift/conformance/parallel] 
/tmp/openshift/build-rpms/rpm/BUILD/origin-3.10.0/_output/local/go/src/github.com/openshift/origin/test/extended/builds/forcepull.go:25

/retest

@wozniakjan
Copy link
Contributor Author

flake Timed out waiting for build "nodejspass-1" to complete
/retest

@sosiouxme
Copy link
Member

https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/18641/test_pull_request_origin_extended_conformance_install/8549/ was #17883

@wozniakjan
Copy link
Contributor Author

flake in ########## STARTING STAGE: SYNC REPOSITORIES ##########
/retest

@sosiouxme
Copy link
Member

/cherrypick release-3.9

@openshift-cherrypick-robot
Copy link

@sosiouxme: once the present PR merges, I will cherry-pick it on top of release-3.9 in a new PR and assign it to you.

In response to this:

/cherrypick release-3.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sosiouxme
Copy link
Member

sosiouxme commented Mar 7, 2018 •
edited
Loading

https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/18641/test_pull_request_origin_extended_conformance_install/8624/ = #17695 (?) and #18301 / #17330
/retest

@sosiouxme
Copy link
Member

FFS
https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended_conformance_install/8646/consoleFull#101797057758b6e51eb7608a5981914356 is infra flake
/retest

@wozniakjan
Copy link
Contributor Author

@sosiouxme looks like finally passing all the tests, is anything else missing to merge it?

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 18835, 18857, 18641, 18656, 18837).

@openshift-merge-robot openshift-merge-robot merged commit 8f431a0 into openshift:master Mar 7, 2018
@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Mar 7, 2018
Merged
@openshift-cherrypick-robot
Copy link

@sosiouxme: new pull request created: #18888

In response to this:

/cherrypick release-3.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sosiouxme
Copy link
Member

@wozniakjan just had to clear the queue :)

openshift-merge-robot added a commit that referenced this pull request Mar 8, 2018
@openshift-merge-robot
Merge pull request #18888 from openshift-cherrypick-robot/cherry-pick…
60c4264 
…-18641-to-release-3.9

Automatic merge from submit-queue.

[release-3.9] diagnostics: AggregatedLogging ClusterRoleBindings false negative fix

This is an automated cherry-pick of #18641

/assign sosiouxme
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcantrill jcantrill jcantrill approved these changes

@pravisankar pravisankar Awaiting requested review from pravisankar

Assignees

@sosiouxme sosiouxme

@liggitt liggitt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/diagnostics lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@wozniakjan @jcantrill @anpingli @sosiouxme @openshift-ci-robot @openshift-cherrypick-robot @openshift-merge-robot @liggitt