Changed the operator to gracefully degrade when not on cluster-wide scope

[jpkrohling]

This is a first draft in getting the operator to work in a single namespace, without cluster roles. This supersedes #830 and fixes #791. It also fixes #905.

This PR also includes instrumentation of the reconciliation for deployments and namespaces, as I added them in order to better understand what needed fixing.

Signed-off-by: Juraci Paixão Kröhling juraci@kroehling.de

[codecov]

Codecov Report

Merging #916 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #916      +/-   ##
==========================================
- Coverage   64.04%   64.01%   -0.03%     
==========================================
  Files          79       79              
  Lines        6427     6423       -4     
==========================================
- Hits         4116     4112       -4     
  Misses       2173     2173              
  Partials      138      138

Impacted Files	Coverage Δ
pkg/inject/sidecar.go	96.75% <0%> (-0.09%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d80bd24...b1d31e1. Read the comment docs.

[jpkrohling]

I'm marking this as WIP, as I want to confirm whether the ClusterRole as we have it here is compatible with what OLM expects.

[jpkrohling]

Testing this, I found out something interesting that I kinda mentioned on #791 / #830 but wasn't able to find definitive documentation about it: the WATCH_NAMESPACE can be a comma-separated list of values:

Excerpt:

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  name: jaeger-operator
  namespace: other-observability
spec:
  template:
    metadata:
      annotations:
        olm.operatorGroup: other-observability-ntvzv
        olm.operatorNamespace: other-observability
        olm.targetNamespaces: observability,other-observability
    spec:
      containers:
      - args:
        - start
        env:
        - name: WATCH_NAMESPACE
          valueFrom:
            fieldRef:
              apiVersion: v1
              fieldPath: metadata.annotations['olm.targetNamespaces']

I couldn't find a way to do it via the web interface, but I can provision an operator via OLM's UI and then later manually change the operator group (kubectl edit operatorgroup other-observability-ntvzv) so that it looks like this:

apiVersion: operators.coreos.com/v1
kind: OperatorGroup
metadata:
  annotations:
    olm.providedAPIs: Jaeger.v1.jaegertracing.io
  creationTimestamp: "2020-02-20T16:27:24Z"
  generateName: other-observability-
  generation: 2
  name: other-observability-ntvzv
  namespace: other-observability
  resourceVersion: "129022"
  selfLink: /apis/operators.coreos.com/v1/namespaces/other-observability/operatorgroups/other-observability-ntvzv
  uid: a05ae72f-e405-4728-b01c-45deb2387e9a
spec:
  targetNamespaces:
  - other-observability
  - observability
status:
  lastUpdated: "2020-02-20T16:29:23Z"
  namespaces:
  - other-observability
  - observability

[jpkrohling]

Wanted to record another finding here: OLM will create a new ClusterRoleBinding between the operator's service account and every ClusterRole defined in the CSV. The Role and ClusterRole present in the deploy/role.yaml are the sources for this. So, we really can't use ClusterRole definitions as @pavolloffay mentioned before in this PR. This also means that, in the current iteration, we can't have an operator instance installed via OLM optionally without ClusterRole permissions: we either remove all cluster roles, or we keep it and they will always be requested.

I also tried to create an operator with a regular user, but looks like not all ordinary users can even list operators. I need to do some more research on this part.

[pavolloffay]

So, we really can't use ClusterRole definitions as @pavolloffay mentioned before in this PR

do you mean in #916 (comment)?

I would be interested to know the motivation behind this PR. There might be two user bases. The OpenShift user base using the OLM might not care much about the permission as it seems OLM is using the ClusterRoles anyways. However the vanilla k8s user base might care more about the RBAC as they are not using the OLM and they install the operator explicitly.

[jpkrohling]

Yes, that's the one I meant.

There might be two user bases. The OpenShift user base using the OLM might not care much about the permission as it seems OLM is using the ClusterRoles anyways. However the vanilla k8s user base might care more about the RBAC as they are not using the OLM and they install the operator explicitly.

#791 is an example of this second case, but there's a third as well: corporations that are using OpenShift+OLM but are hesitant in using operators that require cluster-wide permissions.

[jpkrohling]

This is now ready for the next review.

cc @objectiser, @pavolloffay, @rubenvp8510

[objectiser]

@jpkrohling Would it be possible to summarise the implications of this PR, e.g. in the context of the CR used by Service Mesh with oauth proxy config for Kiali?

[jpkrohling]

Would it be possible to summarise the implications of this PR, e.g. in the context of the CR used by Service Mesh with oauth proxy config for Kiali?

There should be no changes to consumers of the operator via OLM, as it will install the cluster roles as well. A separate CSV could be provided without the cluster permissions, but it's not part of this PR.

There are also no changes to the CR, everything that worked before should work now.

[jpkrohling]

Merged, as both @objectiser and @pavolloffay were OK, with restrictions only around the CI passing (it did) and docs (jaegertracing/documentation#370)