Allow route spec.host to be controlled by permission #13905
Conversation
|
[test] also fixed for ingress at the same time |
smarterclayton
force-pushed
the
make_route_set_require_permission
branch
from
9c24314 to
f6b0681
Compare
|
@deads2k @sttts rebase flake, test panics, I *thought* we had a cherry pick
for this but not sure:
https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1012/testReport/junit/(root)/Extended/_k8s_io__StatefulSet__k8s_io__Basic_StatefulSet_functionality__StatefulSetBasic__Scaling_down_before_scale_up_is_finished_should_wait_until_current_pod_will_be_running_and_ready_before_it_will_be_removed/
|
|
@smarterclayton seeing the same flake upstream as well: We have only whitelisted issues with |
|
Silly github |
| @@ -22,19 +22,26 @@ import ( | |||
| // HostGeneratedAnnotationKey is the key for an annotation set to "true" if the route's host was generated | |||
| const HostGeneratedAnnotationKey = "openshift.io/host.generated" | |||
|
|
|||
| // Registry is an interface for performing subject access reviews | |||
There was a problem hiding this comment.
s/Registry/SubjectAccessReview
|
Open question - do @Miciah do you want to gate setting custom external certs on this permission? In practice I don't need a custom cert if I can't customize the route, UNLESS there is no default cert, in which case someone needs to generate one cert per endpoint. How common is that? |
In my (unsubstantiated) view, very uncommon. |
|
For Online, we want to allow either both custom host and certificate or neither. @twiest, are you aware of a use-case in Dedicated for prohibiting a user to create a route with a custom host while allowing that user to create a route with a custom certificate? |
In order to simplify control over custom routes (routes with a non-default spec.host), make using a custom host field require the "create" verb on "routes/custom-host". By default, admin and edit within a project will get this permission. Users are also prohibited from setting or altering certificates without this permission. Integrators who want to prohibit this can drop the permission from the role and only allow cluster administrators to fill that field.
smarterclayton
force-pushed
the
make_route_set_require_permission
branch
from
f6b0681 to
bef5a17
Compare
|
Evaluated for origin test up to bef5a17 |
|
Gated setting destinationCACertificate, caCertificate, key, and certificate by the permission as well. Justification:
Custom certs aren't useful unless you have custom hosts, or your admin won't configure the router to use a default wildcard. If they don't use a wildcard, it's usually because they want you to generate your own cert. If they do that, they can still ignore the default hostname on the router side. Backwards compatible for existing routes, and for clusters that run reconcile on upgrade for new routes. |
|
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1213/) (Base Commit: 34cda05) |
|
Any other review comments from anyone? |
|
The latest comments from you on this PR satisfy the Online use cases perfectly - no additional comments from me. |
|
[merge] based on review comments. |
|
Evaluated for origin merge up to bef5a17 |
|
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/648/) (Base Commit: 7e3e3c7) (Image: devenv-rhel7_6228) |
| res, err := s.sarClient.CreateSubjectAccessReview( | ||
| ctx, | ||
| &authorizationapi.SubjectAccessReview{ | ||
| User: user.GetName(), |
There was a problem hiding this comment.
@deads2k just noticed this wasn't populating group and scope info... there's a helper for populating a subject access review with the correct user info from the context
There was a problem hiding this comment.
I cribbed this from somewhere else as well. Helper I need?
There was a problem hiding this comment.
There was a problem hiding this comment.
nm, @deads2k picked it up in 26cd8fd#diff-13cdc93be4a264426a1f7c00583e996dR82
There was a problem hiding this comment.
looks like it needs fixing in imagestreamimport.go
| User: user.GetName(), | ||
| Action: authorizationapi.Action{ | ||
| Verb: "create", | ||
| Group: api.GroupName, |
There was a problem hiding this comment.
@liggitt will this work for legacy api?
A new virtual resource
routes/custom-hostcontrols whether spec.host can be set a usercreateis required to set a spec.host on creationupdateis required to change spec.host at any subsequent changeProject admins and editors by default get
create. Only cluster admin hasupdateby default.API compatibility is preserved for the error message returned by immutable fields.