Move to operator sdk version 1.24.0 #729

ShyamsundarR · 2023-02-23T01:21:32Z

Including update to gomega for golang.org/x/net
dependency update

ShyamsundarR · 2023-02-23T01:37:38Z

Operator SDK related version upgrade change as per documentation here are made in series.

Updated gomega in mod, which indirectly upgrades the dependent x/net to 0.7.0 (and other indirect dependents).

Changes to test are due to better error reporting, which as a result failed the tests that were updated.

There is one pending test failure, where a VRG is left behind and hence the API server does not stop, which will be fixed in a subsequent commit.

nirs · 2023-02-23T16:12:35Z

This is pretty big single commit pr. Can we split the work to smaller commits or multiple prs
to make this change incrementally?

ShyamsundarR · 2023-02-23T23:49:59Z

This is pretty big single commit pr. Can we split the work to smaller commits or multiple prs to make this change incrementally?

Posted multiple commits...

ShyamsundarR · 2023-02-23T23:52:34Z

Operator SDK related version upgrade change as per documentation here are made in series.

Updated gomega in mod, which indirectly upgrades the dependent x/net to 0.7.0 (and other indirect dependents).

Changes to test are due to better error reporting, which as a result failed the tests that were updated.

There is one pending test failure, where a VRG is left behind and hence the API server does not stop, which will be fixed in a subsequent commit.

Most of the above are now in commit messages, but note that make test still has the above failure and KUBEBUILDER_ASSETS is not set right from the Makefile on a new environment (i.e when run manually on the shell and then executing make test works, but on a fresh system it is empty)

nirs · 2023-02-28T23:52:24Z

Much nicer! The first commits updating the version looks trivial and
hopefully can be merge now, unless they are the reason for the test
failure.

Regarding gomega - if I understand this correctly, we update it for
fixing the security issue in /x/net. This looks fragile, for example
if we stop using this package, another package can bring older /x/net.

If we want to ensure that we have safe version of /x/net, why not make
the requirement explicit?

Changes to test are due to better error reporting, which as a result failed the tests that were updated.
There is one pending test failure, where a VRG is left behind and hence the API server does not stop, which will be fixed in a subsequent commit.

I don't understand the problem - the test failure are result of upgaraing
the versions or incomplete change, using new features available in the
new verson?

I think it would be useful to merge what we can merge now from this PR,
deferring the incomplete changes to later PR if possible.

ShyamsundarR · 2023-03-01T01:16:15Z

Much nicer! The first commits updating the version looks trivial and hopefully can be merge now, unless they are the reason for the test failure.

I would like to merge this as a batch and not in parts. Having moved the SDK version from somewhere around 1.11 till here and picking changes that are intermediate, even though bisect-able, is preferred that we merge this in one go.

Regarding gomega - if I understand this correctly, we update it for fixing the security issue in /x/net. This looks fragile, for example if we stop using this package, another package can bring older /x/net.

Yes it can, go mod graph helps track down dependencies and address the same. Further security scans help check and report any new regressions.

If we want to ensure that we have safe version of /x/net, why not make the requirement explicit?

We do not use this package directly, it comes in as a dependency as an indirect package, so adding it directly does not serve the purpose, for similar reasons as above (i.e if we stop using say gomega, there would be no reason to add this to the module, although quite a few packages that we depend on also depend on /x/net).

Changes to test are due to better error reporting, which as a result failed the tests that were updated.
There is one pending test failure, where a VRG is left behind and hence the API server does not stop, which will be fixed in a subsequent commit.

I don't understand the problem - the test failure are result of upgaraing the versions or incomplete change, using new features available in the new verson?

Upgrade of gomega now ends up returning status errors and compares them more correctly, causing test failures, requiring changes to this file

If you run the change in the commit without those changes the error reported by the tests are visible.

I think it would be useful to merge what we can merge now from this PR, deferring the incomplete changes to later PR if possible.

The changes are incomplete only from the perspective of why KUBEBUILDER_ASSETS is an empty string when run on a fresh setup, the "other" issue due to an existing issue in the VRG tests that was not considered a failure till now, but is from here on, which hence needs a fix.

I will fix these subsequently and update this PR for a merge, not spending time splitting this up.

ShyamsundarR · 2023-03-02T00:48:53Z

Operator SDK related version upgrade change as per documentation here are made in series.
Updated gomega in mod, which indirectly upgrades the dependent x/net to 0.7.0 (and other indirect dependents).
Changes to test are due to better error reporting, which as a result failed the tests that were updated.
There is one pending test failure, where a VRG is left behind and hence the API server does not stop, which will be fixed in a subsequent commit.

Most of the above are now in commit messages, but note that make test still has the above failure and KUBEBUILDER_ASSETS is not set right from the Makefile on a new environment (i.e when run manually on the shell and then executing make test works, but on a fresh system it is empty)

KUBEBUILDER_ASSETS issue in Makefile is fixed by splitting up the Makefile targets to envtest and setup-envtest, unsure why this works though.

envtests failure to shutdown the instance is addressed based on https://github.com/kubernetes-sigs/controller-runtime/issues/1571

All changes are updated and ready for review/merge as appropriate.

nirs · 2023-03-07T14:38:23Z

config/crd/bases/ramendr.openshift.io_drclusters.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
  annotations:
-    controller-gen.kubebuilder.io/version: v0.9.0
+    controller-gen.kubebuilder.io/version: v0.9.2


Why is this needed? the commit message explains only about using
gcr.io/kubebuilder/kube-rbac-proxy:v0.13.0".

Looks like I did 2 things in one commit, this updates controller-gen version as per this, causing the CRD changes that are auto generated when running make generate.

Ok, would be nice to mention this in the commit message.

nirs · 2023-03-07T14:39:24Z

config/crd/bases/ramendr.openshift.io_drplacementcontrols.yaml

@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
  annotations:
-    controller-gen.kubebuilder.io/version: v0.9.0
+    controller-gen.kubebuilder.io/version: v0.9.2


I hope we can eliminate the duplication by seting this tag in the makefile and updating
the config crds sing kustomization.

These files are auto generated during make generate, so not changing it to kustomize as such.

It the files are auto generated, maybe they should not be part of git, and generated
when running make? I don't suggest to do this right now.

nirs · 2023-03-07T14:39:45Z

config/crd/bases/ramendr.openshift.io_drplacementcontrols.yaml

@@ -113,6 +113,7 @@ spec:
                    description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids'
                    type: string
                type: object
+                x-kubernetes-map-type: atomic


Why is this needed?

Again auto-generated, but to satisfy ObjectRef as an atomic merge strategy.

nirs · 2023-03-07T14:42:59Z

Makefile

@@ -38,7 +38,7 @@ IMAGE_NAME ?= ramen
 IMAGE_TAG ?= latest
 PLATFORM ?= k8s
 IMAGE_TAG_BASE = $(IMAGE_REGISTRY)/$(IMAGE_REPOSITORY)/$(IMAGE_NAME)
-RBAC_PROXY_IMG ?= "gcr.io/kubebuilder/kube-rbac-proxy:v0.8.0"
+RBAC_PROXY_IMG ?= "gcr.io/kubebuilder/kube-rbac-proxy:v0.13.0"


Does this really work? we have kustomization replacing the version to 0.13.0 later,
which is not upadted if you use antoher RBAC_PROXY_IMG value at build time.

This setting and changing things in the kustomize is to keep changed to committed sources to a minimum when deploying (which is where this image is used). IOW, this allows someone to use a different image, and when deployed would change that is in the deployment manifest via kustomize, if not changed then those manifests remain as is and not report any diffs when doing git diff etc.

Or, I am missing the question

So if you change this in the makefile, some autogenerated files are updated automatically
or you need to do the same change in multiple files (like we see in this commit)?

nirs · 2023-03-07T14:45:17Z

Makefile

@@ -338,7 +338,7 @@ ifeq (,$(shell which opm 2>/dev/null))
 	set -e ;\
 	mkdir -p $(dir $(OPM)) ;\
 	OS=$(shell go env GOOS) && ARCH=$(shell go env GOARCH) && \
-	curl -sSLo $(OPM) https://github.com/operator-framework/operator-registry/releases/download/v1.15.1/$${OS}-$${ARCH}-opm ;\
+	curl -sSLo $(OPM) https://github.com/operator-framework/operator-registry/releases/download/v1.23.2/$${OS}-$${ARCH}-opm ;\


Would be nicer to extract the tag to a separate variable later.

True, this and other version strings within various tool downloads/use in the Makefile.

I think we can do this later, maybe open an isue for this? this looks like a good first change.

nirs · 2023-03-07T14:51:23Z

config/manager/manager.yaml

@@ -21,6 +21,8 @@ spec:
    metadata:
      labels:
        control-plane: controller-manager
+      annotations:
+        kubectl.kubernetes.io/default-container: manager


Again not clear why this is needed, and the link points to another uhelpful link.

This adds a notion of a default container, which is useful when certain commands are run that need a container to be specified to omit the same (for example kubectl logs comes to mind). Again, a suggested practice as per the SDK, which we adopt.

Sure it makes sense, just not explained in the commit message.

Makefile

controllers/drpolicy_controller_test.go

ShyamsundarR · 2023-03-07T16:51:43Z

@nirs some of the changes are easier to follow if you run through a sample operator build exercise using operator-sdk. What I typically do is have a fresh sample created from the version we are moving to, to look at the changes/auto-generated files etc. and pull back changes to our code as per the upgrade documentation.

For example the envtest KUBEBUILDER_ASSETTS or the CRD annotations are explained away more easily.

Ref: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.23.0/#upgrade-kube-proxy-image-gcriokubebuilderkube-rbac-proxy-from-v0110-to-v0130 Signed-off-by: Shyamsundar Ranganathan <[email protected]>

ref: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.22.0/#for-go-gov3-gov2-ansible-ansiblev1-helm-helmv1-and-helm-hybrid-hybridhelmv1-alpha-language-based-operators---bump-opm-version-from-v1191-to-v1230-more-infohttpsgithubcomoperator-frameworkoperator-registryreleases Signed-off-by: Shyamsundar Ranganathan <[email protected]>

Ref: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.24.0/ Signed-off-by: Shyamsundar Ranganathan <[email protected]>

Ref: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.17.0/#add-resource-requests-and-limits-to-kube-rbac-proxy Signed-off-by: Shyamsundar Ranganathan <[email protected]>

Ref: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.16.0/#add-annotation-to-specify-the-default-container Signed-off-by: Shyamsundar Ranganathan <[email protected]>

Change as per operator-sdk current practices, - Exact change as per SDK upgrade process is not known, but has changed since 1.8.0 Also update envtest to cancel the controller context before shutdown Change triggered due to investigating: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.14.0/#upgrade-k8s-versions-to-use-122-golangv3 Signed-off-by: Shyamsundar Ranganathan <[email protected]>

To address indirect module dependency for, - golang.org/x/net - golang.org/x/sys - golang.org/x/term - golang.org/x/text Signed-off-by: Shyamsundar Ranganathan <[email protected]>

nirs · 2023-03-09T16:23:12Z

@nirs some of the changes are easier to follow if you run through a sample operator build exercise using operator-sdk. What I typically do is have a fresh sample created from the version we are moving to, to look at the changes/auto-generated files etc. and pull back changes to our code as per the upgrade documentation.

For example the envtest KUBEBUILDER_ASSETTS or the CRD annotations are explained away more easily.

Sounds like we need a maintainer.md document explaning this process for the next upgrade.

ShyamsundarR · 2023-03-09T17:05:56Z

@nirs Can we merge this now?

nirs · 2023-03-09T17:12:08Z

Sure, lets merge.

ShyamsundarR force-pushed the modupdates branch from ef6bf41 to 7a95350 Compare February 23, 2023 23:49

ShyamsundarR force-pushed the modupdates branch 3 times, most recently from 9309acb to 76f824e Compare March 2, 2023 00:35

BenamarMk approved these changes Mar 2, 2023

View reviewed changes

nirs reviewed Mar 7, 2023

View reviewed changes

nirs mentioned this pull request Mar 7, 2023

Bump golang.org/x/net from 0.4.0 to 0.7.0 #721

Closed

ShyamsundarR added 7 commits March 7, 2023 12:01

Update kube-rbac-proxy image to v0.13.0

9cc8304

Ref: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.23.0/#upgrade-kube-proxy-image-gcriokubebuilderkube-rbac-proxy-from-v0110-to-v0130 Signed-off-by: Shyamsundar Ranganathan <[email protected]>

Update operator-sdk to v1.24.0

1cb4ac5

Ref: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.24.0/ Signed-off-by: Shyamsundar Ranganathan <[email protected]>

Add resource limits to rbac proxy container

9734f1c

Ref: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.17.0/#add-resource-requests-and-limits-to-kube-rbac-proxy Signed-off-by: Shyamsundar Ranganathan <[email protected]>

Add default container to operator pod

570e7b9

Ref: https://sdk.operatorframework.io/docs/upgrading-sdk-version/v1.16.0/#add-annotation-to-specify-the-default-container Signed-off-by: Shyamsundar Ranganathan <[email protected]>

Update gomega to 1.27.1

becd1a8

To address indirect module dependency for, - golang.org/x/net - golang.org/x/sys - golang.org/x/term - golang.org/x/text Signed-off-by: Shyamsundar Ranganathan <[email protected]>

ShyamsundarR force-pushed the modupdates branch from 76f824e to becd1a8 Compare March 7, 2023 17:05

nirs approved these changes Mar 9, 2023

View reviewed changes

ShyamsundarR merged commit 8a09ff0 into RamenDR:main Mar 9, 2023

ShyamsundarR deleted the modupdates branch March 9, 2023 17:21

ShyamsundarR mentioned this pull request Mar 9, 2023

Ramen catalog fails to report healthy in drenv, potentially due to olm installation differences #745

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move to operator sdk version 1.24.0 #729

Move to operator sdk version 1.24.0 #729

ShyamsundarR commented Feb 23, 2023

ShyamsundarR commented Feb 23, 2023

nirs commented Feb 23, 2023

ShyamsundarR commented Feb 23, 2023

ShyamsundarR commented Feb 23, 2023 •

edited

Loading

nirs commented Feb 28, 2023

ShyamsundarR commented Mar 1, 2023

ShyamsundarR commented Mar 2, 2023

nirs Mar 7, 2023

ShyamsundarR Mar 7, 2023

nirs Mar 9, 2023

nirs Mar 7, 2023

ShyamsundarR Mar 7, 2023

nirs Mar 9, 2023

nirs Mar 7, 2023

ShyamsundarR Mar 7, 2023

nirs Mar 7, 2023

ShyamsundarR Mar 7, 2023

nirs Mar 9, 2023

nirs Mar 7, 2023

ShyamsundarR Mar 7, 2023

nirs Mar 9, 2023

nirs Mar 7, 2023

ShyamsundarR Mar 7, 2023

nirs Mar 9, 2023

ShyamsundarR commented Mar 7, 2023

nirs commented Mar 9, 2023

ShyamsundarR commented Mar 9, 2023

nirs commented Mar 9, 2023

Move to operator sdk version 1.24.0 #729

Move to operator sdk version 1.24.0 #729

Conversation

ShyamsundarR commented Feb 23, 2023

ShyamsundarR commented Feb 23, 2023

nirs commented Feb 23, 2023

ShyamsundarR commented Feb 23, 2023

ShyamsundarR commented Feb 23, 2023 • edited Loading

nirs commented Feb 28, 2023

ShyamsundarR commented Mar 1, 2023

ShyamsundarR commented Mar 2, 2023

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

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

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

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

Choose a reason for hiding this comment

ShyamsundarR commented Mar 7, 2023

nirs commented Mar 9, 2023

ShyamsundarR commented Mar 9, 2023

nirs commented Mar 9, 2023

ShyamsundarR commented Feb 23, 2023 •

edited

Loading