Chart optionally installs CRD / CR Manager configurable for more strict clusters #344

onelapahead · 2020-04-10T13:39:46Z

Changes

Controller

Adds DISABLE_CUSTOM_RESOURCE_MANAGER env var to the daemon config
Modifies the customResourceManager to use this new flag to determine whether or it not it should attempt to upsert the CRD

Chart

CRD is moved to chart and sourced from charts/kubernetes-external-secrets/crds/kubernetes-client.io_externalsecrets_crd.yaml
Adds customResourceManagerDisabled option to the Helm chart. This controls the respective env var, the scope of the cluster role, and determines whether or not to install the CRD as part of the chart.
- Defaults to false to remain backwards compat
Chart is prepared for future upgrade from v1 to v2 for Helm V3 compatibility only if that is desired for future 4.x releases.
Chart docs updated to assume Helm V3, V2 instructions are still provided
Sets AWS_DEFAULT_REGION env var in the helm chart

E2E Tests

Script adjusted to work with Helm V2/V3 and toggling the custom resource manager
GH Actions workflow uses a build matrix to test all combinations of Helm V2/V3 and disabling the custom resource manager

Justification

Closes #300
Also closes #249

Coupling the CRD creation to a controller requires an unnecessarily privileged cluster role.

In our OpenShift clusters, our admins would not be willing to grant the controller service account that role. We require the chart itself to install the CRD, and the service account or admin installing the chart would have the crd:create access while the controller be more tightly scoped. We have been using this fork in our lab clusters for a few weeks now with no issue.

This essentially deprecates the custom resource manager so that it can removed entirely in future 4.x releases.

onelapahead · 2020-04-11T01:59:01Z

charts/kubernetes-external-secrets/templates/crd.yaml

+metadata:
+  name: externalsecrets.kubernetes-client.io
+  annotations:
+    app.kubernetes.io/managed-by: helm


Not sure how to address the two different copies of the CRD... I'm also leveraging this annotation in the E2E tests which is a bit hacky.

Perhaps in 4.x the custom resource manager can be removed in favor of the chart CRD install?

So for helm v3 we should probably put the CRD in crds folder, these cant be templated though IIRC.
We could probably source this file in the code as well to avoid copies, and overrwrite metadata.annotations["app.kubernetes.io/managed-by"] in the code before upserting it.

For helm v2 backwards compat this file could be changed to something like

{{- if .Values.customResourceManagerDisabled }} {{- if .Values.crds.create }} {{- range $path, $bytes := .Files.Glob "crds/*.yaml" }} {{ $.Files.Get $path }} --- {{- end }} {{- end }} {{- end }}

Probably we should also set DISABLE_CUSTOM_RESOURCE_MANAGER to true by default if running helm v3.

Maybe this is all overkill 😅 either way we should support both helm v2 and v3, I'll try to check with silas if he has any opinion on removing the CRD management from the runtime. I think it makes sense to favor the chart installing the CRD.

So perhaps we can go straight for 4.x solution :)

Great suggestions for sourcing the CRD and v2 backwards compat, I'll give them a shot.

Alright I got the CRD sourced from one location in the repo now, and added your template for helm v2 support. However, I couldn't conditionally set a value depending on whether or not helm v2/v3 was being used so DISABLE_CUSTOM_RESOURCE_MANAGER will have to default to false for now to remain backwards compat.

The helm docs will need to be updated as a result to include --skip-crds and explain how to install the CRD via the chart using v2 and v3.

e2e/run-e2e-suite.sh

onelapahead · 2020-04-26T19:33:21Z

charts/kubernetes-external-secrets/README.md

@@ -6,7 +6,7 @@

 ```bash
 $ helm repo add external-secrets https://godaddy.github.io/kubernetes-external-secrets/
-$ helm install external-secrets/kubernetes-external-secrets
+$ helm install --skip-crds external-secrets/kubernetes-external-secrets


Should it remain:

$ helm install --name my-release external-secrets/kubernetes-external-secrets

?

Which version of Helm are the docs targeting? I cannot think of a way to set the defaults so that the easy install works for both V3 and V2 without one of the versions requiring an additional --skip-crds or --set.

I'd say they've been targeting helm v2 but with EOL of v2 around the corner, I'd say it makes sense to target v3 as a default.

unit tests for manageCrd; always adding CRD manifest to client setting AWS_DEFAULT_REGION in the helm chart e2e tests pass locally first pass at e2e test fixing e2e test; running tests for both settings e2e tests work Helm 2 and 3 friedly solution w/ sane default externalsecrets and localstack have to be deleted btwn runs to avoid flakiness sourcing from one location; proper helm v3 and v2 support fixing crds template for v2 helm version and custom resource manager build matrix client only template syntax differs between v2 and v3 matching operator crd naming scheme; fixing e2e args chart docs

onelapahead · 2020-05-14T15:19:48Z

Sorry for the delay, finally got around to testing on our lab OpenShift clusters, looks good! This controller is great. I'll rebase off of master and update the docs, and then let's try to get this merged?

Sytten · 2020-05-27T19:56:32Z

If that correct to assume that currently you cant install the service with helm v3? I get an error on startup about not finding /openapi.

onelapahead · 2020-05-27T20:08:08Z

@Sytten can you provide more details? Are you saying master currently does not work with helm v3 or this branch specifically? What is erroring on startup? It might also help to provide which Kubernetes distro and version you are running on.

I have not tried master honestly, but we are using helm v3 in argocd to deploy this version of the chart currently without issue.

Sytten · 2020-05-27T20:09:17Z

@hfuss I created #392

onelapahead · 2020-06-01T18:15:26Z

@Flydiverny @silasbw hey folks, any chance I could get a review this week?

Flydiverny

Sorry for the delays on this!
So from what I can tell by the default settings this is not a breaking change, right? :)

Flydiverny · 2020-06-02T00:35:21Z

lib/custom-resource-manager.js

@@ -104,6 +104,20 @@ class CustomResourceManager {

    throw new Error(`Unable to update resource ${resourceName}`)
  }
+
+  async manageCrd ({ customResourceManifest }) {
+    this._kubeClient.addCustomResourceDefinition(customResourceManifest)


Does this always need to be added, perhaps only do this if its not disabled? I'm thinking it would be discovered by await kubeClient.loadSpec() in daemon above if CRDs are already in the cluster.

Flydiverny · 2020-06-02T00:36:26Z

.github/workflows/workflow.yml

    steps:
      - uses: actions/checkout@v2
      - name: Setup node
        uses: actions/setup-node@v1
        with:
          node-version: 12
+      - uses: azure/setup-helm@v1
+        with:
+          version: v2.16.1


Suggested change

version: v2.16.1

version: v2.16.7

Flydiverny · 2020-06-02T00:39:27Z

I can't merge this since the E2E status check is stuck as expected. I assume as thats whats on master, but on this branch its changed (as it should be).

I'll try to poke @silasbw

silasbw · 2020-06-02T01:27:12Z

Thanks @hfuss (and @Flydiverny for pinging me on slack)! 🥂

onelapahead · 2020-06-02T13:49:01Z

@Flydiverny yessir! I wrote the changes so that it would be backwards compatible for users who previously installed the chart using Helm V2 and relied on the CR manager. Helm V3 users will relying on the CR manager will need --skip-crds moving forward but the documentation addresses that as best it can. The documentation is also now tailored towards Helm V3 rather than Helm V2. Thanks again for the early feedback as it was very helpful.

And thanks @silasbw for merging this! 🍻

onelapahead commented Apr 11, 2020

View reviewed changes

onelapahead force-pushed the configurable-cr-manager branch from 40cc74c to 8bf9789 Compare April 11, 2020 12:58

onelapahead marked this pull request as ready for review April 11, 2020 13:00

onelapahead force-pushed the configurable-cr-manager branch 2 times, most recently from d412409 to 54f1fcd Compare April 26, 2020 15:50

onelapahead commented Apr 26, 2020

View reviewed changes

e2e/run-e2e-suite.sh Outdated Show resolved Hide resolved

onelapahead force-pushed the configurable-cr-manager branch from efd9891 to 75e039a Compare April 26, 2020 19:21

onelapahead commented Apr 26, 2020

View reviewed changes

onelapahead force-pushed the configurable-cr-manager branch from 75e039a to 7b194c7 Compare April 26, 2020 19:38

onelapahead added 2 commits May 13, 2020 23:38

values for argocd testing

9a10b4c

fixing values file

844c4e4

onelapahead added 3 commits May 23, 2020 11:29

merge from master

708512e

fixing values file

1fb7e57

Removing sandbox values file

2c93d96

onelapahead force-pushed the configurable-cr-manager branch from 6d5b91f to 2c93d96 Compare May 23, 2020 15:39

Final README changes

d7e872a

onelapahead requested a review from Flydiverny May 23, 2020 19:20

Sytten mentioned this pull request May 27, 2020

Failed to get /openapi/v2 and /swagger.json #392

Closed

Flydiverny approved these changes Jun 2, 2020

View reviewed changes

silasbw merged commit 131e201 into external-secrets:master Jun 2, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chart optionally installs CRD / CR Manager configurable for more strict clusters #344

Chart optionally installs CRD / CR Manager configurable for more strict clusters #344

onelapahead commented Apr 10, 2020 •

edited

Loading

onelapahead Apr 11, 2020

Flydiverny Apr 26, 2020 •

edited

Loading

onelapahead Apr 26, 2020

onelapahead Apr 26, 2020

onelapahead Apr 26, 2020

Flydiverny Apr 27, 2020

onelapahead commented May 14, 2020

Sytten commented May 27, 2020

onelapahead commented May 27, 2020 •

edited

Loading

Sytten commented May 27, 2020

onelapahead commented Jun 1, 2020

Flydiverny left a comment

Flydiverny Jun 2, 2020

Flydiverny Jun 2, 2020

Flydiverny commented Jun 2, 2020

silasbw commented Jun 2, 2020

onelapahead commented Jun 2, 2020

Chart optionally installs CRD / CR Manager configurable for more strict clusters #344

Chart optionally installs CRD / CR Manager configurable for more strict clusters #344

Conversation

onelapahead commented Apr 10, 2020 • edited Loading

Changes

Controller

Chart

E2E Tests

Justification

onelapahead Apr 11, 2020

Choose a reason for hiding this comment

Flydiverny Apr 26, 2020 • edited Loading

Choose a reason for hiding this comment

onelapahead Apr 26, 2020

Choose a reason for hiding this comment

onelapahead Apr 26, 2020

Choose a reason for hiding this comment

onelapahead Apr 26, 2020

Choose a reason for hiding this comment

Flydiverny Apr 27, 2020

Choose a reason for hiding this comment

onelapahead commented May 14, 2020

Sytten commented May 27, 2020

onelapahead commented May 27, 2020 • edited Loading

Sytten commented May 27, 2020

onelapahead commented Jun 1, 2020

Flydiverny left a comment

Choose a reason for hiding this comment

Flydiverny Jun 2, 2020

Choose a reason for hiding this comment

Flydiverny Jun 2, 2020

Choose a reason for hiding this comment

Flydiverny commented Jun 2, 2020

silasbw commented Jun 2, 2020

onelapahead commented Jun 2, 2020

onelapahead commented Apr 10, 2020 •

edited

Loading

Flydiverny Apr 26, 2020 •

edited

Loading

onelapahead commented May 27, 2020 •

edited

Loading