NE-199 Phase 1: Add initial Canary Controller and Canary Resources #476
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
approved
sgreene570
force-pushed
the
NE-199
branch
4 times, most recently
from
096df7b to
f4f40a8
Compare
sgreene570
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
Not sure if there's registry issues again, or if those test failures were one offs |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
removed
the
needs-rebase
|
rebased. |
|
/retest |
1 similar comment
|
/retest |
| - containerPort: 8080 | ||
| protocol: TCP | ||
| - containerPort: 8888 | ||
| protocol: TCP |
There was a problem hiding this comment.
Might add deployment liveliness / readiness probes in a future PR.
|
/test e2e-aws |
/retest |
| verbs: | ||
| - list | ||
| - watch | ||
| - '*' |
There was a problem hiding this comment.
We could define a new role and rolebinding in the openshift-ingress-canary namespace to avoid granting such broad permissions.
There was a problem hiding this comment.
I might be missing something, but how would the operator be able to take advantage of a new role + role binding in the openshift-ingress-canary namespace? The canary controller (which resides within the ingress operator) needs to be able to perform all (well, most at least, so maybe the "*" isn't necessary) actions on routes.
There was a problem hiding this comment.
The role would be in the openshift-ingress-canary namespace and allow full control over routes, and the role binding would bind that role to the operator's service account. Because it would be a role (not a clusterrole) in the openshift-ingress-canary namespace, the role binding should only grant access to routes within that namespace, if I understand correctly. Something like the following should work (but I have not verified that it does):
---
kind: Role
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: ingress-operator-canary-controller
namespace: openshift-ingress-canary
annotations:
include.release.openshift.io/self-managed-high-availability: "true"
rules:
- apiGroups:
- route.openshift.io
resources:
- routes
verbs:
- *
---
kind: RoleBinding
apiVersion: rbac.authorization.k8s.io/v1
metadata:
name: ingress-operator-canary-controller
namespace: openshift-ingress-canary
annotations:
include.release.openshift.io/self-managed-high-availability: "true"
subjects:
- kind: ServiceAccount
name: ingress-operator
namespace: openshift-ingress-operator
roleRef:
kind: Role
apiGroup: rbac.authorization.k8s.io
name: ingress-operator-canary-controllerThere was a problem hiding this comment.
Ill include these resources in the manifests in my next push and verify that they work. Thanks!
There was a problem hiding this comment.
@Miciah the CVO does not (and in my opinion, probably should not) create the openshift-ingress-canary namespace, so the CVO cannot create a Role in the openshift-ingress-canary namespace. Additionally, the operator would need cluster-wide route RBAC permissions to be able to create a role in the openshift-ingress-canary namespace for routes.
So I see 2 solutions:
- The CVO creates the
openshift-ingress-canarynamespace, and then creates the ingress canary controller role.
(This seems like an awkward thing to do especially if the canary controller isnt a mandatory component of the operator) - We fall back to just giving the operator '*' permissions on routes in it's
cluster-role.
Thoughts?
There was a problem hiding this comment.
Note, ive reverted this PR to use * permissions for routes in the operator's cluster role
There was a problem hiding this comment.
~ @Miciah https://kubernetes.io/docs/reference/access-authn-authz/rbac/#privilege-escalation-prevention-and-bootstrapping is the solution we are looking for! I amended this PR so that the ingress operator is granted bind and escalate permissions on roles and rolebindings in it's cluster role. I also added the role.yaml and role-binding.yaml assets for the ingress canary. With these new changes, the ingress operator's route permissions are extended to * for the openshift-ingress-canary namespace only. PTAL when you can. ~
There was a problem hiding this comment.
Going with route '*' permissions after a discussion with miciah. While these permissions are broad, this is the "least bad" approach. The above mentioned escalate/bind approach introduces far more concerning privilege escalation issues to the operator (despite work around efforts to mitigate this).
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
Not sure where the retest bot went. /retest |
|
/retest |
|
Whoops, accidentally clicked the "close" button instead of "comment" 😢 |
|
/retest |
|
/test e2e-aws |
|
/retest |
1 similar comment
|
/retest |
|
/refresh |
|
/test e2e-aws |
|
Not sure why the retest bot is absent still |
|
@sgreene570: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
/retest |
3 similar comments
|
/retest |
|
/retest |
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
NE-199 Phase 1: Add initial Canary Controller and Canary Resources
7 participants
This PR adds an initial canary controller in
pkg/operator/controller/canary/that manages Ingress canary resources in theopenshift-ingress-canarynamespace. The canary controller reconciles a daemonset, service, and route to expose thehello-openshiftcontainer. Phase 2 of NE-199 will implement the network checks and metrics/status reporting in the canary controller.