Fix vulnerabilities #165

roypaulin · 2022-03-02T16:44:05Z

This PR follows security vulnerabilities that have been detected inside operator pods.
the highlights are:

upgrade operator-sdk which leads to upgrade of go (v1.17), controller-gen (v0.8) and other packages as you can see here https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.17.0
In this version of controller-gen trivialVersions and preserveUnknownFields are not supported anymore
To install package we use go install instead of go get as using go get to build and install packages is deprecated and will not be possible from v1.18.
setup-envtest.sh does not exist anymore in recent releases and has been replaced by setup-envtest package.

Closes #30

roypaulin · 2022-03-02T17:12:48Z

Makefile

+	mkdir -p ${KUBEBUILDER_ASSETS}
+	$(SETUP_ENVTEST) use --bin-dir $(ENVTEST_ASSETS_BIN_DIR)
+	find $(ENVTEST_ASSETS_BIN_DIR)/ -exec cp {} $(KUBEBUILDER_ASSETS)/ \;
+	sudo rm -r $(ENVTEST_ASSETS_BIN_DIR)


I do not like this sudo. First I did not intend to add it but the github checks were failing. Using setup-envtest, the binaries are generated in <your-dir>/k8s/<version>. So I generated them in testbin/tmp/k8s/<version> them copy them to testbin/bin to then delete testbin/tmp.

From the operator-sdk changelog, it looks like it is advising this change instead:

+# ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary. +ENVTEST_K8S_VERSION = 1.21 test: manifests generate fmt vet envtest ## Run tests. - go test ./... -coverprofile cover.out + KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test ./... -coverprofile cover.out

From https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.11.0/

Did you try this approach instead?

There is also a separate target to download envtest. It might be easier to create a new project with operator-sdk then compare the Makefile it generates.

spilchen

I didn't see any evidence that we applied these upgrade steps. We should apply them:
https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.11.0/#gov3-ansiblev1-helmv1-add-containerport-protocol-field-in-manifests
https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.16.0/#add-annotation-to-specify-the-default-container
https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.18.0/#support-image-digests-instead-of-tags

This upgrade was partially done:
https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.14.0/#upgrade-k8s-versions-to-use-122-golangv3

I didn't see any evidence that these ones were added

ENVTEST_K8S_VERSION (although the version has been bumped since then)
Replace your admissionReviewVersions={v1,v1beta1} markers with admissionReviewVersions=v1

spilchen · 2022-03-02T18:26:37Z

Makefile

+	mkdir -p ${KUBEBUILDER_ASSETS}
+	$(SETUP_ENVTEST) use --bin-dir $(ENVTEST_ASSETS_BIN_DIR)
+	find $(ENVTEST_ASSETS_BIN_DIR)/ -exec cp {} $(KUBEBUILDER_ASSETS)/ \;
+	sudo rm -r $(ENVTEST_ASSETS_BIN_DIR)


From the operator-sdk changelog, it looks like it is advising this change instead:

+# ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary. +ENVTEST_K8S_VERSION = 1.21 test: manifests generate fmt vet envtest ## Run tests. - go test ./... -coverprofile cover.out + KUBEBUILDER_ASSETS="$(shell $(ENVTEST) use $(ENVTEST_K8S_VERSION) -p path)" go test ./... -coverprofile cover.out

From https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.11.0/

Did you try this approach instead?

There is also a separate target to download envtest. It might be easier to create a new project with operator-sdk then compare the Makefile it generates.

spilchen · 2022-03-02T19:10:59Z

Makefile

@@ -421,7 +427,11 @@ KUBERNETES_SPLIT_YAML = $(shell pwd)/bin/kubernetes-split-yaml
 kubernetes-split-yaml: ## Download kubernetes-split-yaml locally if necessary.
 	$(call go-get-tool,$(KUBERNETES_SPLIT_YAML),github.com/mogensen/kubernetes-split-yaml@v0.3.0)

-# go-get-tool will 'go get' any package $2 and install it to $1.
+SETUP_ENVTEST = $(shell pwd)/bin/setup-envtest


We should follow what is done in the generated Makefile. So this becomes ENVTEST = and the make target below becomes envtest.

Yes it is definitely simpler like that. I will do that.

scripts/gen-csv.sh

spilchen · 2022-03-02T19:17:47Z

We should add a changie entry too to indicate we moved to operator-sdk version 1.18.0

roypaulin · 2022-03-02T19:58:54Z

I didn't see any evidence that we applied these upgrade steps. We should apply them: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.11.0/#gov3-ansiblev1-helmv1-add-containerport-protocol-field-in-manifests https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.16.0/#add-annotation-to-specify-the-default-container https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.18.0/#support-image-digests-instead-of-tags

This upgrade was partially done: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.14.0/#upgrade-k8s-versions-to-use-122-golangv3

I didn't see any evidence that these ones were added
* ENVTEST_K8S_VERSION  (although the version has been bumped since then)

* Replace your admissionReviewVersions={v1,v1beta1} markers with admissionReviewVersions=v1

Yes I oversimplified this upgrade. With such a big jump in version it is clear that intermediary upgrades should not be skipped. I will change that.

roypaulin · 2022-03-02T20:00:15Z

We should add a changie entry too to indicate we moved to operator-sdk version 1.18.0

Yes I will do a last commit for that once the changes will be approved.

spilchen · 2022-03-03T18:59:33Z

config/webhook/manifests.yaml

  clientConfig:
    service:
      name: webhook-service
      namespace: system
      path: /mutate-vertica-com-v1beta1-verticadb
  failurePolicy: Fail
  name: mverticadb.kb.io
-  namespaceSelector:


This file is no longer generated. So, I think we still need this. This allows the webhoook to work.

Sorry what do you mean by is no longer generated? because with controller-gen crd webhook .... it automatically generates the file you see right now. Or do you mean we should still keep the part you highlighted and prevent this file to be re-generated each time?

We changed this file to not be generated anymore. We did this because we needed to add the namespace selector to it in order to get the webhook to work. So, I think we still need that selector added.

spilchen · 2022-03-03T19:53:59Z

Makefile

-	$(call go-get-tool,$(SETUP_ENVTEST),sigs.k8s.io/controller-runtime/tools/setup-envtest@latest)
+ENVTEST = $(shell pwd)/bin/setup-envtest
+.PHONY: envtest
+envtest: ## Download envtest-setup locally if necessary.


The comment should be "Download setup-envtest-setup locally if necessary."

spilchen

I think we can get rid of scripts/wait-for-webhook.sh. The update to controller-runtime should have fixed that as it adds a true health check that helm --wait can depend on.

config/webhook/manifests.yaml

spilchen · 2022-03-04T17:14:02Z

The changes look good. Lets create the changie entry. And I will push it sometime next week once we have upgraded Go on our internal build machines.

roypaulin · 2022-03-04T17:22:56Z

I guess the Kind of changie is "Changed".

spilchen · 2022-03-07T12:15:17Z

changes/unreleased/Changed-20220304-221506.yaml

+body: Move to operator-sdk v1.18.0
+time: 2022-03-04T22:15:06.6959129+01:00
+custom:
+  Issue: "30"


We should use 165 as the issue number. I typically try to link with the PR that pushed the change as that gives more context.

roypaulin added 7 commits February 27, 2022 19:18

Upgrade packages to resolve security vulnerabilities

eca9178

Set go version to 1.17 in github actions

e2ff7ca

Set kustomize version to 4.5.2

dae345c

Add version to install for go-junit-report

1eb530f

Fix setup-envtest install issue

5c4eaa4

Fix permission issue

2ae650e

Remove useless kustomize settings during minio setup

3081aa6

roypaulin commented Mar 2, 2022

View reviewed changes

roypaulin requested review from spilchen and davidmathi March 2, 2022 18:40

spilchen reviewed Mar 2, 2022

View reviewed changes

Apply upgrade steps 1.11 1.14 1.16 1.18

5a04b67

spilchen reviewed Mar 3, 2022

View reviewed changes

Apply review comments

88851fb

spilchen mentioned this pull request Mar 4, 2022

Webhook server could not answer in time #30

Closed

spilchen reviewed Mar 4, 2022

View reviewed changes

config/webhook/manifests.yaml Show resolved Hide resolved

Delete wait-for-webhook.sh

d51e4b4

spilchen reviewed Mar 7, 2022

View reviewed changes

roypaulin force-pushed the fix-vulnerabilities branch from fcfa192 to d51e4b4 Compare March 7, 2022 12:30

Add Changie entry for upgraded version of operator-sdk

6c0fe65

spilchen merged commit 48650ea into vertica:main Mar 7, 2022

roypaulin deleted the fix-vulnerabilities branch March 23, 2022 15:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix vulnerabilities #165

Fix vulnerabilities #165

roypaulin commented Mar 2, 2022 •

edited by spilchen

Loading

roypaulin Mar 2, 2022 •

edited

Loading

spilchen Mar 2, 2022

spilchen left a comment

spilchen Mar 2, 2022

spilchen Mar 2, 2022

roypaulin Mar 2, 2022

spilchen commented Mar 2, 2022

roypaulin commented Mar 2, 2022

roypaulin commented Mar 2, 2022

spilchen Mar 3, 2022

roypaulin Mar 3, 2022

spilchen Mar 3, 2022

spilchen Mar 3, 2022

spilchen left a comment

spilchen commented Mar 4, 2022

roypaulin commented Mar 4, 2022

spilchen Mar 7, 2022

Fix vulnerabilities #165

Fix vulnerabilities #165

Conversation

roypaulin commented Mar 2, 2022 • edited by spilchen Loading

roypaulin Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spilchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spilchen commented Mar 2, 2022

roypaulin commented Mar 2, 2022

roypaulin commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spilchen left a comment

Choose a reason for hiding this comment

spilchen commented Mar 4, 2022

roypaulin commented Mar 4, 2022

Choose a reason for hiding this comment

roypaulin commented Mar 2, 2022 •

edited by spilchen

Loading

roypaulin Mar 2, 2022 •

edited

Loading