Skip to content
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

Remove operator roles #2530

Merged
merged 10 commits into from
Feb 13, 2020
Merged

Remove operator roles #2530

merged 10 commits into from
Feb 13, 2020

Conversation

pebrc
Copy link
Collaborator

@pebrc pebrc commented Feb 6, 2020

What this PR does:

  • remove the concept of operator roles as flags
  • remove the global operator manifests
  • keep/modify the namespace operator manifests to allow deployments restricted to one or more namespaces
  • allow the parameterization of the operator name to allow multiple (namespaced) operators to be deployed in a single namespace
  • make the operator logs target work for both variants
  • run the e2e tests with a all-in-one operator
  • add a dedicated webhook toggle flag
  • disable the webhook by default in the namespaced variant, enable it in the all-in-one variant

What this PR does not:

  • introduce new CLI/kustomize/Helm tooling to craft different versions of the manifests
  • replace the various manifest variants with just one that is customized as need
  • remove the need to update versions and RBAC permissions in various places
  • remove the separate e2e manifests

Fixes #2254

Removes the roles flag. Adjusts the manifest templates for all-in-one
and e2e tests. Removes the concept of a global operator but keeps the
namespaced version for now.
@pebrc
Copy link
Collaborator Author

pebrc commented Feb 6, 2020

Jenkins test this please

@pebrc pebrc marked this pull request as ready for review February 7, 2020 13:29
Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

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

Overall lgtm, just left few nits, I'm just wondering if we should also update https://github.com/elastic/cloud-on-k8s/blob/master/docs/design/0005-configurable-operator.md#decision-outcome

(Also I ran make ci-e2e TESTS_MATCH='^TestCrossNS' successfully 👍)

config/operator/README.md Outdated Show resolved Hide resolved
config/operator/Makefile Show resolved Hide resolved
test/e2e/cmd/run/run.go Outdated Show resolved Hide resolved
test/e2e/test/context.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Co-Authored-By: Michael Morello <michael.morello@gmail.com>
Copy link
Contributor

@david-kow david-kow left a comment

Choose a reason for hiding this comment

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

I think docs/operator-config.asciidoc also needs changes.

Makefile Outdated Show resolved Hide resolved
cmd/manager/main.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@thbkrkr thbkrkr left a comment

Choose a reason for hiding this comment

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

LGTM!
Glad to see this simplification without losing flexibility.

One little thing tickles me but this can be discussed and handled later.
In the config/operator/README.md, we present two different deployment modes: All-in-one and Namespaced. And we provide some examples of the manifests to deploy the operator in config/all-in-one and config/namespace.

I would like to be consistent with the names between the README and the config examples directories.
I'm not very comfortable with namespace or Namespaced. It is not very clear what it is. It makes me think that the operator is in a namespace... nothing more.

But one more time, it's about naming and it is hard to find the good words.

Some suggestions:

  • namespaces: with the s
  • multi-namespaces: a bit long but more explicit
  • cross-namespace
  • cluster-wide: this tends to suggest it is for all namespaces

Note: I like the etcd operator approach where the default mode is pretty restricted and users need to opt-in to deploy the operator cluster-wide:

Default etcd operator behavior is to only manage etcd clusters created in the same namespace. It is possible to deploy an etcd operator with special option to manage clusterwide etcd clusters.

config/operator/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@barkbay barkbay left a comment

Choose a reason for hiding this comment

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

LGTM

@pebrc pebrc merged commit 3819530 into elastic:master Feb 13, 2020
pebrc added a commit that referenced this pull request Feb 14, 2020
)

The trial license e2e test was failing since #2530 because we are now running the e2e with a operator restricted to the managed namespaces via --namespaces, but we did not include the operator namespace itself in the list of namespaces.

As the list of namespaces controls which resources will be cached by the k8s client the operator was not able to access the license related secrets in the operator namespace and trial mode could not be activated.

This commit makes sure the operator namespace is always included in the client cache even when running in namespace restricted mode.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deprecate the "global + namespace" operator concept
4 participants