Upgrade operator-sdk #83

koikonom · 2020-12-02T12:55:44Z

This PR upgrades the operator to the latest version of the operator SDK used.

Unlike other upgrades this one comes is quite invasive because of the SDK's attempts to
move to a structure like the one used in kubebuilder and eventually at some point in the
future merge the two projects.

In order to make this easier to review I've kept all original commits instead of squashing
them. Once the review is done I will sqash and merge to master.

Also note that this is based on the agent_support branch which is also pending review, instead
of master.

koikonom · 2020-12-02T14:19:53Z

In order to run make test locally you need to have kubebuilder installed on your machine and set the KUBEBUILDER_ASSETS environment variable to point at kubebuilder's bin/ directory.

dak1n1

Wow! This a huge change. Thanks for putting in all this effort!

I haven't successfully run this yet, because I was trying to deploy it using the helm chart, which is not compatible with these changes. However, we should have a chat with Phil before deciding on the next steps for the helm chart. At first glance, it appears that we can replace the helm chart with all these new Kustomize changes.

I wanted to submit my initial review now, so I can unblock your work. I'll see if I can get this new version of the operator deployed on my machine on Monday.

dak1n1 · 2020-12-11T18:00:11Z

Dockerfile

@@ -0,0 +1,29 @@
+# Build the terraform-k8s binary
+FROM golang:1.13 as builder


In order to match go.mod, we should use 1.15 here. Optionally, we could also use the alpine version of the image, to save some time downloading: golang:1.15-alpine

Suggested change

FROM golang:1.13 as builder

FROM golang:1.15 as builder

Makefile

Dockerfile

dak1n1 · 2020-12-12T00:15:13Z

Dockerfile

+COPY api/ api/
+COPY controllers/ controllers/
+COPY version/ version/
+COPY workspacehelper/ workspacehelper/


We can save some build time and disk space if you replace all the COPY statements with a single copy statement:

COPY . .

That will grab the entire directory and save you from having to create 6 layers.

Dockerfile

dak1n1 · 2020-12-12T23:13:00Z

Makefile

+ go get sigs.k8s.io/controller-tools/cmd/[email protected] ;\
+ rm -rf $$CONTROLLER_GEN_TMP_DIR ;\
+ }
+CONTROLLER_GEN=$(GOBIN)/controller-gen


This section could be replaced with go install sigs.k8s.io/controller-tools/cmd/controller-gen

Makefile

config/manager/manager.yaml

koikonom · 2020-12-13T16:56:15Z

Thanks for the massive review ! I just adopted the Makefile generated by the SDK, but your suggestions make sense. I will get back with changes :) .

In order for our helm template to work I had to do the following changes locally (after copying the latest CRD from hashicorp/terraform-k8s)

diff --git a/templates/sync-workspace-deployment.yaml b/templates/sync-workspace-deployment.yaml
index 6d3027e..c67ab84 100644
--- a/templates/sync-workspace-deployment.yaml
+++ b/templates/sync-workspace-deployment.yaml
@@ -59,14 +59,10 @@ spec:
             mountPath: "/tmp/secrets"
             readOnly: true
           command:
-            - "/bin/sh"
-            - "-ec"
-            - |
-              terraform-k8s sync-workspace \
-                --k8s-watch-namespace="{{ default .Release.Namespace .Values.syncWorkspace.k8WatchNamespace}}" \
-                {{- if .Values.syncWorkspace.logLevel }}
-                --zap-level={{ .Values.syncWorkspace.logLevel }} \
-                {{- end }}
+            - /terraform-k8s
+          args:
+            - --enable-leader-election
+            - --k8s-watch-namespace={{ default .Release.Namespace .Values.syncWorkspace.k8WatchNamespace}}
           livenessProbe:
             httpGet:
               path: /metrics
diff --git a/templates/sync-workspace-role.yaml b/templates/sync-workspace-role.yaml
index b46d631..7c81967 100644
--- a/templates/sync-workspace-role.yaml
+++ b/templates/sync-workspace-role.yaml
@@ -23,6 +23,14 @@ rules:
   - secrets
   verbs:
   - '*'
+- apiGroups:
+  - ""
+  resources:
+  - configmaps/status
+  verbs:
+  - get
+  - update
+  - patch
 - apiGroups:
   - apps
   resources:

koikonom · 2020-12-13T17:01:04Z

The change above also stops us from needing /bin/sh

dak1n1 · 2020-12-14T20:16:04Z

Thanks, that's good to know that you were able to get the operator running using the helm repo. Those changes you gave me will help to test it. Although we will need a PR to get those changes into the helm repo, if that's the path we're going to take.

I didn't make those changes to my local helm repo yet, because I didn't think it would be ok to have a specific version of the helm repo tied to a specific version of the operator. (I'm concerned about what our responsibility might be regarding backwards compatibility between the helm repo and the operator).

This morning I was close to getting the operator to deploy using just kustomize, which I think is a neat option, since it's built into kubectl (kubectl apply -k can apply your kustomize configs! how cool!). This makes it a really native way to apply these configs. I read this post that kind of helped sway me toward using it.

Before continuing with this operator PR, I wanted to get an answer this question:
Is it acceptable to tie the operator to a specific version of the helm chart (and not guarantee backwards-compatibility)?

Because depending on the answer to that question, we might either deploy this change using kustomize, or make some changes to the helm repo, or make some other changes to this PR which would ensure backwards-compatibility.

Due to the prioritization of a different project, I won't be able to take another look at this PR until January. But maybe by then we'll have the answer to that question and know which direction to take it.

dak1n1 · 2021-01-25T18:20:04Z

Dockerfile

+FROM alpine:3.12.1
+WORKDIR /
+COPY --from=builder /workspace/terraform-k8s .
+USER nonroot:nonroot


I tried to run this image today. It built successfully, but in my cluster it gave me this error:

Warning Failed 0s (x4 over 53s) kubelet, minikube Error: failed to start container "terraform-sync-workspace": Error response from daemon: unable to find user nonroot: no matching entries in passwd file

I tried running it manually too. Looks like we'll have to choose another user, since the one specified in this Dockerfile does not exist.

$ docker images REPOSITORY TAG IMAGE ID CREATED SIZE localhost/terraform-k8s-dev latest 586fbea58e79 4 minutes ago 54.8 MB $ docker run -ti --rm 586fbea58e79 /bin/bash Error: unable to find user nonroot: no matching entries in passwd file

Yeah this is probably due to the change from distroless to alpine. I will switch it to nobody that I know for a fact it exists in the alpine image.

dak1n1 · 2021-01-25T19:57:57Z

Dockerfile

+COPY main.go main.go
+COPY api/ api/
+COPY controllers/ controllers/
+COPY version/ version/
+COPY workspacehelper/ workspacehelper/


Suggested change

COPY main.go main.go

COPY api/ api/

COPY controllers/ controllers/

COPY version/ version/

COPY workspacehelper/ workspacehelper/

COPY . .

dak1n1

I finally got it all running on TFC and TFE using the branch you gave me from the terraform-helm repo. (new_sdk) Let's get a PR for that branch next, since it's all tested and working. This looks good! Thanks again.

koikonom · 2021-01-28T11:08:20Z

Squashed and rebased.

Unlike other upgrades this one comes is quite invasive because of the SDK's attempts to move to a structure like the one used in kubebuilder and eventually at some point in the future merge the two projects.

koikonom · 2021-01-28T11:13:30Z

I've also pushed a branch called new_sdk_historical that contains the individual commits I made while working on this PR just in case we need them in the future.

koikonom requested a review from a team December 2, 2020 12:55

koikonom changed the base branch from master to agent_support December 2, 2020 17:11

aareet linked an issue Dec 7, 2020 that may be closed by this pull request

Upgrade Operator SDK to v1.x #76

Closed

dak1n1 reviewed Dec 13, 2020

View reviewed changes

Base automatically changed from agent_support to master December 13, 2020 16:39

koikonom force-pushed the new_sdk branch from e20ee70 to 819fa5f Compare December 13, 2020 16:47

aareet mentioned this pull request Jan 6, 2021

List this project on operatorhub #57

Open

dak1n1 reviewed Jan 25, 2021

View reviewed changes

koikonom requested a review from dak1n1 January 27, 2021 10:18

dak1n1 approved these changes Jan 28, 2021

View reviewed changes

koikonom force-pushed the new_sdk branch from 2e1798f to 1233dbd Compare January 28, 2021 11:07

Upgrade the operator SDK to version 1.2.0

69e3b7b

Unlike other upgrades this one comes is quite invasive because of the SDK's attempts to move to a structure like the one used in kubebuilder and eventually at some point in the future merge the two projects.

koikonom force-pushed the new_sdk branch from 1233dbd to 69e3b7b Compare January 28, 2021 11:09

koikonom merged commit cdc5af2 into master Jan 28, 2021

koikonom deleted the new_sdk branch January 28, 2021 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade operator-sdk #83

Upgrade operator-sdk #83

koikonom commented Dec 2, 2020

koikonom commented Dec 2, 2020

dak1n1 left a comment

dak1n1 Dec 11, 2020

dak1n1 Jan 25, 2021

dak1n1 Dec 12, 2020

dak1n1 Dec 12, 2020

koikonom commented Dec 13, 2020

koikonom commented Dec 13, 2020

dak1n1 commented Dec 14, 2020

dak1n1 Jan 25, 2021

koikonom Jan 25, 2021

dak1n1 Jan 25, 2021

dak1n1 left a comment

koikonom commented Jan 28, 2021

koikonom commented Jan 28, 2021

		@@ -0,0 +1,29 @@
		# Build the terraform-k8s binary
		FROM golang:1.13 as builder

Upgrade operator-sdk #83

Upgrade operator-sdk #83

Conversation

koikonom commented Dec 2, 2020

koikonom commented Dec 2, 2020

dak1n1 left a comment

Choose a reason for hiding this comment

dak1n1 Dec 11, 2020

Choose a reason for hiding this comment

dak1n1 Jan 25, 2021

Choose a reason for hiding this comment

dak1n1 Dec 12, 2020

Choose a reason for hiding this comment

dak1n1 Dec 12, 2020

Choose a reason for hiding this comment

koikonom commented Dec 13, 2020

koikonom commented Dec 13, 2020

dak1n1 commented Dec 14, 2020

dak1n1 Jan 25, 2021

Choose a reason for hiding this comment

koikonom Jan 25, 2021

Choose a reason for hiding this comment

dak1n1 Jan 25, 2021

Choose a reason for hiding this comment

dak1n1 left a comment

Choose a reason for hiding this comment

koikonom commented Jan 28, 2021

koikonom commented Jan 28, 2021