-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
image: transform api groups to legacy resources in admission #13421
Conversation
@smarterclayton i think I missed your comment :) this is the actual PR, can you please comment with the another hole you think this might expose? |
Don't try transforming in the controller itself. Transform when admission is called. That preserves legacy behavior (user defines "builds") as well as prevents accidental slip ups (user only defines "builds.builds.openshift.io"). Have you reviewed all the other admission controllers for config that has resources in it? |
@smarterclayton gotcha |
@smarterclayton i briefly went trough other admission controller (origin) and unless I'm missing something, only image policy admission actually have list of resources it applies to. If that is true and we fix this, we should be safe, but perhaps another person should go trough them as well to be 100% confident ( @soltysh @enj ?) |
9a9032f
to
741030b
Compare
[test] @smarterclayton PTAL |
741030b
to
4ee2145
Compare
Flake: #13386 [test] |
Evaluated for origin test up to 4ee2145 |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/295/) (Base Commit: e6a87ad) |
Lgtm [merge] |
Evaluated for origin merge up to 4ee2145 |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/135/) (Base Commit: 52c9149) (Image: devenv-rhel7_6086) |
Ref: #13412