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

Move to operator sdk version 1.24.0 #729

Merged
merged 7 commits into from
Mar 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 11 additions & 8 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)?

OPERATOR_SUGGESTED_NAMESPACE ?= ramen-system
AUTO_CONFIGURE_DR_CLUSTER ?= true
KUBE_OBJECT_PROTECTION_DISABLED ?= false
Expand Down Expand Up @@ -136,11 +136,14 @@ lint: golangci-bin ## Run configured golangci-lint and pre-commit.sh linters aga
testbin/golangci-lint run ./... --config=./.golangci.yaml
hack/pre-commit.sh

ENVTEST_K8S_VERSION = 1.25.0
ENVTEST_ASSETS_DIR=$(shell pwd)/testbin
setup-envtest:
mkdir -p ${ENVTEST_ASSETS_DIR}
test -f ${ENVTEST_ASSETS_DIR}/setup-envtest.sh || curl -sSLo ${ENVTEST_ASSETS_DIR}/setup-envtest.sh https://raw.githubusercontent.com/kubernetes-sigs/controller-runtime/v0.8.3/hack/setup-envtest.sh
source ${ENVTEST_ASSETS_DIR}/setup-envtest.sh; fetch_envtest_tools $(ENVTEST_ASSETS_DIR); setup_envtest_env $(ENVTEST_ASSETS_DIR)
nirs marked this conversation as resolved.
Show resolved Hide resolved
envtest:
mkdir -p $(ENVTEST_ASSETS_DIR)
test -s $(ENVTEST_ASSETS_DIR)/setup-envtest || GOBIN=$(ENVTEST_ASSETS_DIR) go install sigs.k8s.io/controller-runtime/tools/setup-envtest@latest

setup-envtest: envtest
KUBEBUILDER_ASSETS="$(shell $(ENVTEST_ASSETS_DIR)/setup-envtest use $(ENVTEST_K8S_VERSION) --bin-dir $(ENVTEST_ASSETS_DIR) -p path)"
nirs marked this conversation as resolved.
Show resolved Hide resolved

test: generate manifests setup-envtest ## Run tests.
go test ./... -coverprofile cover.out $(GO_TEST_GINKGO_ARGS)
Expand Down Expand Up @@ -232,7 +235,7 @@ undeploy-dr-cluster: kustomize ## Undeploy dr-cluster controller from the K8s cl
##@ Tools

CONTROLLER_GEN = $(shell pwd)/bin/controller-gen
controller_gen_version=v0.9.0
controller_gen_version=v0.9.2
controller-gen: ## Download controller-gen locally if necessary.
@test '$(shell $(CONTROLLER_GEN) --version)' = 'Version: $(controller_gen_version)' ||\
$(call go-get-tool,sigs.k8s.io/controller-tools/cmd/controller-gen@$(controller_gen_version))
Expand Down Expand Up @@ -267,7 +270,7 @@ ifeq (,$(shell which operator-sdk 2>/dev/null))
set -e ;\
mkdir -p $(dir $(OSDK)) ;\
OS=$(shell go env GOOS) && ARCH=$(shell go env GOARCH) && \
curl -sSLo $(OSDK) https://github.com/operator-framework/operator-sdk/releases/download/v1.11.0/operator-sdk_$${OS}_$${ARCH} ;\
curl -sSLo $(OSDK) https://github.com/operator-framework/operator-sdk/releases/download/v1.24.0/operator-sdk_$${OS}_$${ARCH} ;\
chmod +x $(OSDK) ;\
}
else
Expand Down Expand Up @@ -338,7 +341,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 ;\
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

chmod +x $(OPM) ;\
}
else
Expand Down
2 changes: 1 addition & 1 deletion config/crd/bases/ramendr.openshift.io_drclusters.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

creationTimestamp: null
name: drclusters.ramendr.openshift.io
spec:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

creationTimestamp: null
name: drplacementcontrols.ramendr.openshift.io
spec:
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

failoverCluster:
description: FailoverCluster is the cluster name that the user wants
to failover the application to. If not sepcified, then the DRPC
Expand Down Expand Up @@ -181,6 +182,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
preferredCluster:
description: PreferredCluster is the cluster name that the user preferred
to run the application on
Expand Down Expand Up @@ -232,6 +234,7 @@ spec:
are ANDed.
type: object
type: object
x-kubernetes-map-type: atomic
required:
- drPolicyRef
- placementRef
Expand Down
4 changes: 3 additions & 1 deletion config/crd/bases/ramendr.openshift.io_drpolicies.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
creationTimestamp: null
name: drpolicies.ramendr.openshift.io
spec:
Expand Down Expand Up @@ -87,6 +87,7 @@ spec:
are ANDed.
type: object
type: object
x-kubernetes-map-type: atomic
schedulingInterval:
description: scheduling Interval for replicating Persistent Volume
data to a peer cluster. Interval is typically in the form <num><m,h,d>.
Expand Down Expand Up @@ -140,6 +141,7 @@ spec:
are ANDed.
type: object
type: object
x-kubernetes-map-type: atomic
required:
- drClusters
type: object
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
creationTimestamp: null
name: protectedvolumereplicationgrouplists.ramendr.openshift.io
spec:
Expand Down Expand Up @@ -159,6 +159,7 @@ spec:
requirements are ANDed.
type: object
type: object
x-kubernetes-map-type: atomic
schedulingInterval:
description: scheduling Interval for replicating Persistent
Volume data to a peer cluster. Interval is typically
Expand Down Expand Up @@ -216,6 +217,7 @@ spec:
requirements are ANDed.
type: object
type: object
x-kubernetes-map-type: atomic
required:
- schedulingInterval
type: object
Expand Down Expand Up @@ -299,6 +301,7 @@ spec:
contains only "value". The requirements are ANDed.
type: object
type: object
x-kubernetes-map-type: atomic
replicationState:
description: Desired state of all volumes [primary or secondary]
in this replication group; this value is propagated to
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
creationTimestamp: null
name: volumereplicationgroups.ramendr.openshift.io
spec:
Expand Down Expand Up @@ -110,6 +110,7 @@ spec:
"value". The requirements are ANDed.
type: object
type: object
x-kubernetes-map-type: atomic
schedulingInterval:
description: scheduling Interval for replicating Persistent Volume
data to a peer cluster. Interval is typically in the form <num><m,h,d>.
Expand Down Expand Up @@ -163,6 +164,7 @@ spec:
"value". The requirements are ANDed.
type: object
type: object
x-kubernetes-map-type: atomic
required:
- schedulingInterval
type: object
Expand Down Expand Up @@ -242,6 +244,7 @@ spec:
are ANDed.
type: object
type: object
x-kubernetes-map-type: atomic
replicationState:
description: Desired state of all volumes [primary or secondary] in
this replication group; this value is propagated to children VolumeReplication
Expand Down
8 changes: 7 additions & 1 deletion config/default/manager_auth_proxy_patch.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,10 @@ spec:
ports:
- containerPort: 9289
name: https

resources:
limits:
cpu: 500m
memory: 128Mi
requests:
cpu: 5m
memory: 64Mi
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this change? the link does not give any reason and it points to
operator-framework/operator-sdk#5505
which also does not give any info on why this is needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is good to have resource limits on the containers, so that they do not run wild and get capped. I would assume it is added by the SDK as a good practice, hence the (suggested) updates.

2 changes: 1 addition & 1 deletion config/dr-cluster/default/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -69,4 +69,4 @@ resources:
images:
- name: kube-rbac-proxy
newName: gcr.io/kubebuilder/kube-rbac-proxy
newTag: v0.8.0
newTag: v0.13.0
2 changes: 1 addition & 1 deletion config/hub/default/k8s/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -89,4 +89,4 @@ resources:
images:
- name: kube-rbac-proxy
newName: gcr.io/kubebuilder/kube-rbac-proxy
newTag: v0.8.0
newTag: v0.13.0
2 changes: 1 addition & 1 deletion config/hub/default/ocp/kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -58,4 +58,4 @@ resources:
images:
- name: kube-rbac-proxy
newName: gcr.io/kubebuilder/kube-rbac-proxy
newTag: v0.8.0
newTag: v0.13.0
2 changes: 2 additions & 0 deletions config/manager/manager.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@ spec:
metadata:
labels:
control-plane: controller-manager
annotations:
kubectl.kubernetes.io/default-container: manager
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

spec:
securityContext:
runAsNonRoot: true
Expand Down
20 changes: 6 additions & 14 deletions controllers/drpolicy_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -266,9 +266,8 @@ var _ = Describe("DrpolicyController", func() {
})
When(`a drpolicy creation request contains an invalid scheduling interval`, func() {
It(`should fail`, func() {
err := func() *errors.StatusError {
err := func(value string) *errors.StatusError {
path := field.NewPath(`spec`, `schedulingInterval`)
value := ``

return errors.NewInvalid(
schema.GroupKind{
Expand All @@ -289,12 +288,12 @@ var _ = Describe("DrpolicyController", func() {
),
},
)
}()
}
drp := drpolicy.DeepCopy()
drp.Spec.SchedulingInterval = `3s`
Expect(k8sClient.Create(context.TODO(), drp)).To(MatchError(err))
Expect(k8sClient.Create(context.TODO(), drp)).To(MatchError(err(drp.Spec.SchedulingInterval)))
drp.Spec.SchedulingInterval = `0`
Expect(k8sClient.Create(context.TODO(), drp)).To(MatchError(err))
Expect(k8sClient.Create(context.TODO(), drp)).To(MatchError(err(drp.Spec.SchedulingInterval)))
nirs marked this conversation as resolved.
Show resolved Hide resolved
})
})

Expand All @@ -304,7 +303,6 @@ var _ = Describe("DrpolicyController", func() {
drp.Spec.DRClusters = nil
err := func() *errors.StatusError {
path := field.NewPath("spec", "drClusters")
value := "null"

return errors.NewInvalid(
schema.GroupKind{
Expand All @@ -313,15 +311,9 @@ var _ = Describe("DrpolicyController", func() {
},
drp.Name,
field.ErrorList{
field.Invalid(
field.Required(
path,
value,
validationErrors.InvalidType(
path.String(),
"body",
"array",
value,
).Error(),
"",
nirs marked this conversation as resolved.
Show resolved Hide resolved
),
},
)
Expand Down
13 changes: 7 additions & 6 deletions controllers/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
controller_runtime_config "sigs.k8s.io/controller-runtime/pkg/config/v1alpha1"
"sigs.k8s.io/controller-runtime/pkg/envtest"
"sigs.k8s.io/controller-runtime/pkg/envtest/printer"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
"sigs.k8s.io/controller-runtime/pkg/manager"
Expand Down Expand Up @@ -58,6 +57,8 @@ var (
apiReader client.Reader
k8sClient client.Client
testEnv *envtest.Environment
ctx context.Context
cancel context.CancelFunc
configMap *corev1.ConfigMap
ramenConfig *ramendrv1alpha1.RamenConfig
testLogger logr.Logger
Expand All @@ -79,9 +80,7 @@ var (
func TestAPIs(t *testing.T) {
RegisterFailHandler(Fail)

RunSpecsWithDefaultAndCustomReporters(t,
"Controller Suite",
[]Reporter{printer.NewlineReporter{}})
RunSpecs(t, "Controller Suite")
}

func namespaceCreate(name string) {
Expand Down Expand Up @@ -115,7 +114,7 @@ var _ = BeforeSuite(func() {
ramencontrollers.ControllerType = ramendrv1alpha1.DRHubType

if _, set := os.LookupEnv("KUBEBUILDER_ASSETS"); !set {
Expect(os.Setenv("KUBEBUILDER_ASSETS", "../testbin/bin")).To(Succeed())
Expect(os.Setenv("KUBEBUILDER_ASSETS", "../testbin/k8s/1.25.0-linux-amd64")).To(Succeed())
}

rNs, set := os.LookupEnv("POD_NAMESPACE")
Expand Down Expand Up @@ -341,8 +340,9 @@ var _ = BeforeSuite(func() {
err = drpcReconciler.SetupWithManager(k8sManager)
Expect(err).ToNot(HaveOccurred())

ctx, cancel = context.WithCancel(context.TODO())
go func() {
err = k8sManager.Start(ctrl.SetupSignalHandler())
err = k8sManager.Start(ctx)
Expect(err).ToNot(HaveOccurred())
}()

Expand All @@ -354,6 +354,7 @@ var _ = BeforeSuite(func() {
}, 60)

var _ = AfterSuite(func() {
cancel()
Expect(k8sClient.Delete(context.TODO(), configMap)).To(Succeed())
By("tearing down the test environment")
err := testEnv.Stop()
Expand Down
2 changes: 1 addition & 1 deletion controllers/util/util_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ var _ = BeforeSuite(func() {

By("Setting up KUBEBUILDER_ASSETS for envtest")
if _, set := os.LookupEnv("KUBEBUILDER_ASSETS"); !set {
Expect(os.Setenv("KUBEBUILDER_ASSETS", "../../testbin/bin")).To(Succeed())
Expect(os.Setenv("KUBEBUILDER_ASSETS", "../../testbin/k8s/1.25.0-linux-amd64")).To(Succeed())
}

By("Bootstrapping test environment")
Expand Down
2 changes: 1 addition & 1 deletion controllers/volsync/volsync_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ var _ = BeforeSuite(func() {

By("Setting up KUBEBUILDER_ASSETS for envtest")
if _, set := os.LookupEnv("KUBEBUILDER_ASSETS"); !set {
Expect(os.Setenv("KUBEBUILDER_ASSETS", "../../testbin/bin")).To(Succeed())
Expect(os.Setenv("KUBEBUILDER_ASSETS", "../../testbin/k8s/1.25.0-linux-amd64")).To(Succeed())
}

By("bootstrapping test environment")
Expand Down
10 changes: 5 additions & 5 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ require (
github.com/google/uuid v1.3.0
github.com/kubernetes-csi/external-snapshotter/client/v4 v4.2.0
github.com/onsi/ginkgo v1.16.5
github.com/onsi/gomega v1.24.2
github.com/onsi/gomega v1.27.1
github.com/open-cluster-management-io/api v0.0.0-00010101000000-000000000000
github.com/open-cluster-management/api v0.0.0-20210527013639-a6845f2ebcb1
github.com/operator-framework/api v0.15.1-0.20220624132056-decf74800a17
Expand Down Expand Up @@ -137,12 +137,12 @@ require (
go.uber.org/multierr v1.8.0 // indirect
golang.org/x/crypto v0.3.0 // indirect
golang.org/x/exp v0.0.0-20221114191408-850992195362 // indirect
golang.org/x/net v0.4.0 // indirect
golang.org/x/net v0.7.0 // indirect
golang.org/x/oauth2 v0.0.0-20221014153046-6fdb5e3db783 // indirect
golang.org/x/sync v0.1.0 // indirect
golang.org/x/sys v0.3.0 // indirect
golang.org/x/term v0.3.0 // indirect
golang.org/x/text v0.5.0 // indirect
golang.org/x/sys v0.5.0 // indirect
golang.org/x/term v0.5.0 // indirect
golang.org/x/text v0.7.0 // indirect
gomodules.xyz/jsonpatch/v2 v2.2.0 // indirect
google.golang.org/appengine v1.6.7 // indirect
google.golang.org/genproto v0.0.0-20221024183307-1bc688fe9f3e // indirect
Expand Down
Loading