Skip to content

update addDeadClusterRole to add systemOnly annotation #15247

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
Merged
merged 1 commit into from
Jul 18, 2017
Merged

update addDeadClusterRole to add systemOnly annotation #15247

merged 1 commit into from
Jul 18, 2017

Conversation

benjaminapetersen
Copy link
Contributor

Fixes #15241 / web console issue 1858
Related to issue #14411, PR #14510

Roles that have been replaced by kube controller roles should receive
the systemOnly annotation to ensure they are not visible to typical
end users (such as those who would use the web console).

  • updates bootstrappolicy/dead addDeadClusterRole
  • updates bootstrappolicy/web_console_role_test TestSystemOnlyRoles
    • still skips controller roles (maintenance simplicity), but will throw an error if the annotation is missing

Change to web_console_role_test.go results in:

=== RUN   TestSystemOnlyRoles
--- FAIL: TestSystemOnlyRoles (0.00s)
	web_console_role_test.go:66: Controller role "system:replication-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:endpoint-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:replicaset-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:garbage-collector-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:job-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:hpa-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:daemonset-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:disruption-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:namespace-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:gc-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:certificate-signing-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:statefulset-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:build-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:deploymentconfig-controller" is missing the system only annotation
	web_console_role_test.go:66: Controller role "system:deployment-controller" is missing the system only annotation
FAIL
exit status 1
FAIL	github.com/openshift/origin/pkg/cmd/server/bootstrappolicy	0.117s

Followed by the updates to dead.go to return the test to passing.

@enj @jwforres

@benjaminapetersen benjaminapetersen requested a review from enj July 17, 2017 20:14
@benjaminapetersen benjaminapetersen force-pushed the bpeterse/issue/15241/system-only-roles branch from c2fdaeb to 15fd425 Compare July 17, 2017 20:16
@enj
Copy link
Contributor

enj commented Jul 17, 2017

[test]

enj
enj approved these changes Jul 17, 2017
View reviewed changes
Copy link
Contributor

@enj enj left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @benjaminapetersen can you verify the behavior in the web console?

pkg/cmd/server/bootstrappolicy/dead.go Outdated
ObjectMeta: metav1.ObjectMeta{
Name: name,
Annotations: map[string]string{
// typical users should not see dead cluster roles
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one should ever see this really since they are just placeholders for reconciliation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just update the comment or do we want to do anything else?

@benjaminapetersen
Copy link
Contributor Author

Looks like a flake:

Recording test results
[BFA] Scanning build for known causes...
[BFA] Found failure cause(s):
[BFA] dind provisioning failure from category flake
[BFA] Command Failure from category failure
[BFA] Job Stage Failed
[BFA] Done. 14s
Finished: FAILURE

[test]

@benjaminapetersen benjaminapetersen mentioned this pull request Jul 18, 2017
Closed
@benjaminapetersen
update bootstrappolicy/dead addDeadClusterRole to include systemOnly …
66e9970 
…annotation

Roles that have been replaced by kube controller roles should receive
the systemOnly annotation to ensure they are not visible to typical
end users (such as those who would use the web console).

- updates bootstrappolicy/dead addDeadClusterRole
- updates bootstrappolicy/web_console_role_test TestSystemOnlyRoles
	- still skips controller roles (maintenance simplicity), but will throw an error if the annotation is missing
@benjaminapetersen benjaminapetersen force-pushed the bpeterse/issue/15241/system-only-roles branch from 15fd425 to 66e9970 Compare July 18, 2017 16:14
@benjaminapetersen
Copy link
Contributor Author

benjaminapetersen commented Jul 18, 2017 •
edited
Loading

flake #15311
[test]

@benjaminapetersen
Copy link
Contributor Author

flake #10773
[test]

@openshift openshift deleted a comment from openshift-bot Jul 18, 2017
@openshift openshift deleted a comment from openshift-bot Jul 18, 2017
@openshift openshift deleted a comment from openshift-bot Jul 18, 2017
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 66e9970

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3237/) (Base Commit: 20e72f7) (PR Branch Commit: 66e9970)

@enj
Copy link
Contributor

enj commented Jul 18, 2017

[merge][severity:blocker] @jwforres really wanted this in 3.6

@enj enj added this to the 3.6.0 milestone Jul 18, 2017
@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 18, 2017 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/3237/) (Base Commit: 20e72f7) (PR Branch Commit: 66e9970) (Image: devenv-rhel7_6448)

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 66e9970

@openshift-bot openshift-bot merged commit c70c652 into openshift:master Jul 18, 2017
@liggitt liggitt mentioned this pull request Jul 18, 2017
Merged
@liggitt
Copy link
Contributor

liggitt commented Jul 18, 2017

picked into origin 3.6 in #15320

@benjaminapetersen benjaminapetersen deleted the bpeterse/issue/15241/system-only-roles branch July 19, 2017 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enj enj enj approved these changes

Assignees

@enj enj

Labels
None yet
Projects
None yet
Milestone
3.6.0
Development

Successfully merging this pull request may close these issues.
4 participants
@benjaminapetersen @enj @openshift-bot @liggitt