From 117c3df4a923949f4d99ccd4513dd21c0c72b368 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Thu, 30 May 2024 17:18:19 +0000 Subject: [PATCH 01/13] Helm: add support for auto upgrading CRDs Introduces a pre-upgrade hook that will upgrade/create any of the CRD manifests that are bundled in the vso docker image. Other fixes: - make the component label test more reliable by checking all documents individually. - configure a backoff in the pre-delete "pdcc" Job. --- Dockerfile | 13 +- Makefile | 5 +- chart/templates/deployment.yaml | 20 ++- chart/templates/hook-upgrade-crds.yaml | 111 ++++++++++++ chart/templates/tests/test-runner.yaml | 1 + chart/values.yaml | 28 +++- go.mod | 6 +- internal/utils/utils.go | 98 +++++++++++ internal/utils/utils_test.go | 224 +++++++++++++++++++++++++ main.go | 51 +++++- test/unit/deployment.bats | 13 +- test/unit/helpers.bats | 13 +- 12 files changed, 557 insertions(+), 26 deletions(-) create mode 100644 chart/templates/hook-upgrade-crds.yaml diff --git a/Dockerfile b/Dockerfile index d135c81d7..9e47801d3 100644 --- a/Dockerfile +++ b/Dockerfile @@ -31,13 +31,20 @@ ARG LD_FLAGS # Build RUN CGO_ENABLED=0 GOOS=$GOOS GOARCH=$GOARCH go build -ldflags "$LD_FLAGS" -a -o $BIN_NAME main.go +# setup scripts directory needed for upgrading CRDs. +RUN mkdir scripts +COPY chart/crds scripts/crds +RUN ln -s ../$BIN_NAME scripts/upgrade-crds + # dev image # ----------------------------------- # Use distroless as minimal base image to package the manager binary # Refer to https://github.com/GoogleContainerTools/distroless for more details FROM gcr.io/distroless/static:nonroot as dev +ENV BIN_NAME=vault-secrets-operator WORKDIR / COPY --from=dev-builder /workspace/$BIN_NAME / +COPY --from=dev-builder /workspace/scripts /scripts USER 65532:65532 ENTRYPOINT ["/vault-secrets-operator"] @@ -59,7 +66,8 @@ LABEL revision=$PRODUCT_REVISION WORKDIR / -COPY dist/$TARGETOS/$TARGETARCH/$BIN_NAME / +COPY dist/$TARGETOS/$TARGETARCH/$BIN_NAME /$BIN_NAME +COPY dist/$TARGETOS/$TARGETARCH/scripts /scripts COPY LICENSE /licenses/copyright.txt USER 65532:65532 @@ -93,7 +101,8 @@ LABEL name="Vault Secrets Operator" \ WORKDIR / -COPY dist/$TARGETOS/$TARGETARCH/$BIN_NAME / +COPY dist/$TARGETOS/$TARGETARCH/$BIN_NAME /$BIN_NAME +COPY dist/$TARGETOS/$TARGETARCH/scripts /scripts COPY LICENSE /licenses/copyright.txt COPY --from=build-ubi /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem /etc/pki/ca-trust/extracted/pem/ diff --git a/Makefile b/Makefile index 280465bac..e72f1132a 100644 --- a/Makefile +++ b/Makefile @@ -274,7 +274,10 @@ docker-push: ## Push docker image with the manager. .PHONY: ci-build ci-build: ## Build operator binary (without generating assets). - mkdir -p $(BUILD_DIR)/$(GOOS)/$(GOARCH) + rm -rf $(BUILD_DIR)/$(GOOS)/$(GOARCH)/scripts + mkdir -p $(BUILD_DIR)/$(GOOS)/$(GOARCH)/scripts + cp -a chart/crds $(BUILD_DIR)/$(GOOS)/$(GOARCH)/scripts/. + ln -s ../$(BIN_NAME) $(BUILD_DIR)/$(GOOS)/$(GOARCH)/scripts/upgrade-crds CGO_ENABLED=0 GOOS=$(GOOS) GOARCH=$(GOARCH) go build \ -ldflags "${LD_FLAGS} $(shell GOOS=$(GOOS) GOARCH=$(GOARCH) ./scripts/ldflags-version.sh)" \ -a \ diff --git a/chart/templates/deployment.yaml b/chart/templates/deployment.yaml index 4c4ededf6..bce41c9c3 100644 --- a/chart/templates/deployment.yaml +++ b/chart/templates/deployment.yaml @@ -9,6 +9,7 @@ metadata: name: {{ include "vso.chart.fullname" . }}-controller-manager namespace: {{ .Release.Namespace }} labels: + app.kubernetes.io/component: controller-manager {{- include "vso.chart.labels" . | nindent 4 }} {{ include "vso.imagePullSecrets" .}} --- @@ -167,6 +168,7 @@ metadata: name: {{ printf "%s-%s" "pdcc" (include "vso.chart.fullname" .) | trunc 63 | trimSuffix "-" }} namespace: {{ .Release.Namespace }} labels: + app.kubernetes.io/component: controller-manager {{- include "vso.chart.labels" . | nindent 4 }} annotations: # This is what defines this resource as a hook. Without this line, the @@ -177,6 +179,7 @@ metadata: {{- toYaml .Values.controller.annotations | nindent 4 }} {{- end }} spec: + backoffLimit: 5 template: metadata: # This name is truncated because kubernetes applies labels to the job which contain the job and pod @@ -195,15 +198,20 @@ spec: - --pre-delete-hook-timeout-seconds={{ .Values.controller.preDeleteHookTimeoutSeconds }} command: - /vault-secrets-operator - resources: {{- toYaml .Values.controller.manager.resources | nindent 10 }} + {{- with .Values.hooks.resources }} + resources: + {{- toYaml . | nindent 10 }} + {{- end}} + {{- with .Values.controller.securityContext }} securityContext: - {{- toYaml .Values.controller.securityContext | nindent 10 }} + {{- toYaml .| nindent 10 }} + {{- end}} restartPolicy: Never - {{- if .Values.controller.nodeSelector }} + {{- with .Values.controller.nodeSelector }} nodeSelector: - {{- toYaml .Values.controller.nodeSelector | nindent 8 }} + {{- toYaml . | nindent 8 }} {{- end }} - {{- if .Values.controller.tolerations }} + {{- with .Values.controller.tolerations }} tolerations: - {{- toYaml .Values.controller.tolerations | nindent 8 }} + {{- toYaml .| nindent 8 }} {{- end }} diff --git a/chart/templates/hook-upgrade-crds.yaml b/chart/templates/hook-upgrade-crds.yaml new file mode 100644 index 000000000..2c46a2808 --- /dev/null +++ b/chart/templates/hook-upgrade-crds.yaml @@ -0,0 +1,111 @@ +{{- if .Values.hooks.upgradeCRDs.enabled -}} +--- +apiVersion: v1 +kind: ServiceAccount +metadata: + name: {{ template "vso.chart.fullname" . }}-upgrade-crds + namespace: {{ .Release.Namespace }} + labels: + app.kubernetes.io/component: controller-manager +{{ include "vso.chart.labels" . | indent 4 }} + annotations: + helm.sh/hook: pre-upgrade + helm.sh/hook-delete-policy: "hook-succeeded,before-hook-creation" + helm.sh/hook-weight: "1" +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRole +metadata: + name: {{ template "vso.chart.fullname" . }}-upgrade-crds + labels: + app.kubernetes.io/component: rbac +{{ include "vso.chart.labels" . | indent 4 }} + annotations: + helm.sh/hook: pre-upgrade + helm.sh/hook-delete-policy: "hook-succeeded,before-hook-creation" + helm.sh/hook-weight: "2" +rules: + - apiGroups: + - apiextensions.k8s.io + resources: + - customresourcedefinitions + verbs: + - create + - delete + - get + - list + - patch + - update +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: ClusterRoleBinding +metadata: + name: {{ template "vso.chart.fullname" . }}-upgrade-crds + labels: + app.kubernetes.io/component: rbac +{{ include "vso.chart.labels" . | indent 4 }} + annotations: + helm.sh/hook: pre-upgrade + helm.sh/hook-delete-policy: "hook-succeeded,before-hook-creation" + helm.sh/hook-weight: "2" +subjects: + - kind: ServiceAccount + name: {{ template "vso.chart.fullname" . }}-upgrade-crds + namespace: {{ .Release.Namespace }} +roleRef: + kind: ClusterRole + name: {{ template "vso.chart.fullname" . }}-upgrade-crds + apiGroup: rbac.authorization.k8s.io +--- +apiVersion: batch/v1 +kind: Job +metadata: + name: {{ printf "%s-%s" "upgrade-crds" (include "vso.chart.fullname" .) | trunc 63 | trimSuffix "-" }} + namespace: {{ .Release.Namespace }} + labels: + app.kubernetes.io/component: controller-manager + {{- include "vso.chart.labels" . | nindent 4 }} + annotations: + # This is what defines this resource as a hook. Without this line, the + # job is considered part of the release. + helm.sh/hook: pre-upgrade + helm.sh/hook-delete-policy: "hook-succeeded,before-hook-creation" + helm.sh/hook-weight: "99" + {{- if .Values.controller.annotations }} + {{- toYaml .Values.controller.annotations | nindent 4 }} + {{- end }} +spec: + backoffLimit: {{ .Values.hooks.upgradeCRDs.backoffLimit | default 5 }} + template: + metadata: + name: {{ printf "%s-%s" "upgrade-crds" (include "vso.chart.fullname" .) | trunc 63 | trimSuffix "-" }} + spec: + serviceAccountName: {{ template "vso.chart.fullname" . }}-upgrade-crds + securityContext: + {{- toYaml .Values.controller.podSecurityContext | nindent 8 }} + containers: + - name: pre-upgrade-crds + image: {{ .Values.controller.manager.image.repository }}:{{ .Values.controller.manager.image.tag }} + env: + - name: VSO_UPGRADE_CRDS_TIMEOUT + value: .Values.hooks.upgradeCRDs.executionTimeout + command: + - /scripts/upgrade-crds + {{- with .Values.hooks.resources }} + resources: + {{- toYaml . | nindent 10 }} + {{- end}} + {{- with .Values.controller.securityContext }} + securityContext: + {{- toYaml .| nindent 10 }} + {{- end}} + restartPolicy: Never + {{- with .Values.controller.nodeSelector }} + nodeSelector: + {{- toYaml . | nindent 8 }} + {{- end }} + {{- with .Values.controller.tolerations }} + tolerations: + {{- toYaml .| nindent 8 }} + {{- end }} +{{- end -}} diff --git a/chart/templates/tests/test-runner.yaml b/chart/templates/tests/test-runner.yaml index 05d591e1e..e83035720 100644 --- a/chart/templates/tests/test-runner.yaml +++ b/chart/templates/tests/test-runner.yaml @@ -9,6 +9,7 @@ metadata: name: {{ template "vso.chart.fullname" . }}-test namespace: {{ .Release.Namespace }} labels: + app.kubernetes.io/component: controller-manager app: {{ template "vso.chart.name" . }} chart: {{ template "vso.chart.chart" . }} heritage: {{ .Release.Service }} diff --git a/chart/values.yaml b/chart/values.yaml index 48cba89e8..036dab353 100644 --- a/chart/values.yaml +++ b/chart/values.yaml @@ -4,7 +4,6 @@ # Top level configuration for the vault secrets operator deployment. # This consists of a controller and a kube rbac proxy container. controller: - # Set the number of replicas for the operator. # @type: integer replicas: 1 @@ -753,6 +752,33 @@ telemetry: # @type: string scrapeTimeout: 10s +# Configure the behaviour of Helm hooks. +hooks: + # Resources common to all hooks. + resources: + limits: + cpu: 500m + memory: 128Mi + requests: + cpu: 10m + memory: 64Mi + # Configure the Helm pre-upgrade hook that handles custom resource definition (CRD) upgrades. + upgradeCRDs: + # Set to true to automatically upgrade the CRDs. + # Disabling this will require manual intervention to upgrade the CRDs, so it is recommended to + # always leave it enabled. + # @type: boolean + enabled: true + + # Limit the number of retries for the CRD upgrade. + # @type: integer + backoffLimit: 5 + + # Set the timeout for the CRD upgrade. The operation should typically take less than 5s + # to complete. + # @type: string + executionTimeout: 30s + ## Used by unit tests, and will not be rendered except when using `helm template`, this can be safely ignored. tests: # @type: boolean diff --git a/go.mod b/go.mod index dc7b7fbf1..e4ecf6307 100644 --- a/go.mod +++ b/go.mod @@ -29,12 +29,13 @@ require ( github.com/stretchr/testify v1.9.0 golang.org/x/crypto v0.23.0 google.golang.org/api v0.181.0 - gopkg.in/yaml.v3 v3.0.1 k8s.io/api v0.30.1 + k8s.io/apiextensions-apiserver v0.30.1 k8s.io/apimachinery v0.30.1 k8s.io/client-go v0.30.1 k8s.io/utils v0.0.0-20230726121419-3b25d923346b sigs.k8s.io/controller-runtime v0.18.3 + sigs.k8s.io/yaml v1.4.0 ) require ( @@ -167,10 +168,9 @@ require ( gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect - k8s.io/apiextensions-apiserver v0.30.1 // indirect + gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/klog/v2 v2.120.1 // indirect k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect - sigs.k8s.io/yaml v1.4.0 // indirect ) diff --git a/internal/utils/utils.go b/internal/utils/utils.go index 21ee67102..b16e5737e 100644 --- a/internal/utils/utils.go +++ b/internal/utils/utils.go @@ -5,15 +5,25 @@ package utils import ( "bytes" + "context" "errors" "fmt" + "io" "os" "path/filepath" + "strings" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/json" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/apiutil" + "sigs.k8s.io/controller-runtime/pkg/log/zap" + "sigs.k8s.io/yaml" + + "github.com/hashicorp/vault-secrets-operator/internal/version" ) var ( @@ -52,3 +62,91 @@ func GetOwnerRefFromObj(owner ctrlclient.Object, scheme *runtime.Scheme) (metav1 ownerRef.Kind = kind return ownerRef, nil } + +// UpgradeCRDs upgrades custom resource definitions a directory containing CRD +// YAML manifest files. It only supports +// apiextensionsv1.CustomResourceDefinition. If the CRD exists in the cluster, it +// will be patched from the contents from the corresponding manifest file. If the +// CRD does not exist, it will be created. The Client must have the +// apiextensionsv1.Scheme registered. +func UpgradeCRDs(ctx context.Context, c ctrlclient.Client, dir string) error { + logger := zap.New().WithName("UpgradeCRDs").WithValues( + "version", version.Version(), "dir", dir) + + d, err := os.ReadDir(dir) + if err != nil { + return err + } + + var crds []apiextensionsv1.CustomResourceDefinition + for _, f := range d { + if f.IsDir() { + continue + } + + if !strings.HasSuffix(f.Name(), ".yaml") { + continue + } + + fn := filepath.Join(dir, f.Name()) + fh, err := os.Open(fn) + if err != nil { + return err + } + + b, err := io.ReadAll(fh) + if err != nil { + return err + } + + jsonB, err := yaml.YAMLToJSONStrict(b) + if err != nil { + return err + } + + var crd apiextensionsv1.CustomResourceDefinition + if err := json.Unmarshal(jsonB, &crd); err != nil { + return err + } + + if crd.GroupVersionKind() != apiextensionsv1.SchemeGroupVersion.WithKind(crd.Kind) { + logger.Info("Skipping unsupported kind", "gvk", crd.GroupVersionKind(), + "name", crd.Name, "filename", fn) + continue + } + + crds = append(crds, crd) + } + + if len(crds) == 0 { + return fmt.Errorf("no CRDs found in directory %q", dir) + } + + // TODO(future): add support for optionally deleting obsolete CRDs + var errs error + for _, crd := range crds { + var cur apiextensionsv1.CustomResourceDefinition + if err := c.Get(ctx, ctrlclient.ObjectKey{Name: crd.Name}, &cur); err != nil { + if apierrors.IsNotFound(err) { + logger.Info("Creating", "name", crd.Name, "gvk", crd.GroupVersionKind()) + if err := c.Create(ctx, &crd); err != nil { + return err + } + } else { + errs = errors.Join(errs, err) + } + } else { + logger.Info("Patching", + "name", crd.Name, "gvk", crd.GroupVersionKind(), + "uid", cur.GetUID(), "resourceVersion", cur.GetResourceVersion(), + ) + patch := ctrlclient.MergeFrom(cur.DeepCopy()) + cur.Spec = crd.Spec + cur.ObjectMeta.Annotations = crd.ObjectMeta.Annotations + cur.ObjectMeta.Labels = crd.ObjectMeta.Labels + errors.Join(errs, c.Patch(ctx, &cur, patch)) + } + } + + return errs +} diff --git a/internal/utils/utils_test.go b/internal/utils/utils_test.go index a21a2af0d..2c5365f56 100644 --- a/internal/utils/utils_test.go +++ b/internal/utils/utils_test.go @@ -4,9 +4,19 @@ package utils import ( + "context" + "fmt" "os" "path/filepath" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) func TestGetCurrentNamespace(t *testing.T) { @@ -72,3 +82,217 @@ func TestGetCurrentNamespace(t *testing.T) { }) } } + +func TestUpgradeCRDs(t *testing.T) { + t.Parallel() + + manifests := []string{ + `apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: hcpauths.secrets.hashicorp.com +spec: + group: secrets.hashicorp.com + names: + kind: HCPAuth + listKind: HCPAuthList + plural: hcpauths + singular: hcpauth + scope: Namespaced + versions: + - name: v1beta1 +`, + } + + manifestsUpdated := []string{ + `apiVersion: apiextensions.k8s.io/v1 +kind: CustomResourceDefinition +metadata: + name: hcpauths.secrets.hashicorp.com +spec: + group: secrets.hashicorp.com + names: + kind: HCPAuth + listKind: HCPAuthList + plural: hcpauths + singular: hcpauth + scope: Cluster + versions: + - name: v1beta2 +`, + } + + others := []string{ + `apiVersion: v1 +kind: ConfigMap +metadata: + name: foo +`, + } + + wantCRD := &apiextensionsv1.CustomResourceDefinition{ + TypeMeta: metav1.TypeMeta{ + APIVersion: apiextensionsv1.SchemeGroupVersion.String(), + Kind: "CustomResourceDefinition", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "hcpauths.secrets.hashicorp.com", + }, + Spec: apiextensionsv1.CustomResourceDefinitionSpec{ + Group: "secrets.hashicorp.com", + Names: apiextensionsv1.CustomResourceDefinitionNames{ + Kind: "HCPAuth", + ListKind: "HCPAuthList", + Plural: "hcpauths", + Singular: "hcpauth", + }, + Scope: "Namespaced", + Versions: []apiextensionsv1.CustomResourceDefinitionVersion{ + { + Name: "v1beta1", + }, + }, + }, + } + + wantCRDUpdated := wantCRD.DeepCopy() + wantCRDUpdated.Spec.Scope = "Cluster" + wantCRDUpdated.Spec.Versions = []apiextensionsv1.CustomResourceDefinitionVersion{ + { + Name: "v1beta2", + }, + } + + scheme := runtime.NewScheme() + require.NoError(t, apiextensionsv1.AddToScheme(scheme)) + + root := t.TempDir() + ctx := context.Background() + builder := fake.NewClientBuilder().WithScheme(scheme) + tests := []struct { + name string + c client.Client + create []*apiextensionsv1.CustomResourceDefinition + createDir bool + manifests []string + otherManifests []string + subDirs int + canaries int + want []*apiextensionsv1.CustomResourceDefinition + wantErr assert.ErrorAssertionFunc + }{ + { + name: "create", + c: builder.Build(), + createDir: true, + subDirs: 2, + canaries: 2, + otherManifests: others, + manifests: manifests, + want: []*apiextensionsv1.CustomResourceDefinition{ + wantCRD.DeepCopy(), + }, + wantErr: assert.NoError, + }, + { + name: "upgrade", + c: builder.Build(), + createDir: true, + subDirs: 2, + canaries: 2, + manifests: manifestsUpdated, + otherManifests: others, + create: []*apiextensionsv1.CustomResourceDefinition{ + wantCRD.DeepCopy(), + }, + want: []*apiextensionsv1.CustomResourceDefinition{ + wantCRDUpdated.DeepCopy(), + }, + wantErr: assert.NoError, + }, + { + name: "invalid-scheme-not-registered", + c: fake.NewClientBuilder().WithScheme(runtime.NewScheme()).Build(), + createDir: true, + manifests: manifests, + wantErr: func(t assert.TestingT, err error, i ...interface{}) bool { + return assert.ErrorContains(t, err, + "no kind is registered for the type v1.CustomResourceDefinition in scheme") + }, + }, + { + name: "invalid-no-dir", wantErr: func(t assert.TestingT, err error, i ...interface{}) bool { + return assert.ErrorIs(t, err, os.ErrNotExist) + }, + }, + { + name: "invalid-no-crds-found", + otherManifests: others, + createDir: true, + subDirs: 2, + canaries: 2, + wantErr: func(t assert.TestingT, err error, i ...interface{}) bool { + return assert.ErrorContains(t, err, "no CRDs found in directory") + }, + }, + { + name: "invalid-yaml-parse-error", + otherManifests: []string{"foo:\n-bar"}, + createDir: true, + wantErr: func(t assert.TestingT, err error, i ...interface{}) bool { + return assert.ErrorContains(t, err, `yaml: line 3: could not find expected ':'`) + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + dir := filepath.Join(root, tt.name) + if tt.createDir { + require.NoError(t, os.MkdirAll(dir, 0o700)) + t.Cleanup(func() { + require.NoError(t, os.RemoveAll(dir)) + }) + } + + if len(tt.manifests) > 0 || len(tt.otherManifests) > 0 || tt.subDirs > 0 || tt.canaries > 0 { + require.True(t, tt.createDir, "createDir cannot be false") + } + + for idx, mft := range tt.otherManifests { + filename := filepath.Join(dir, fmt.Sprintf("other-%d.yaml", idx)) + require.NoError(t, os.WriteFile(filename, []byte(mft), 0o600)) + } + + for i := 0; i < tt.subDirs; i++ { + subDir := filepath.Join(dir, fmt.Sprintf("dir-%d", i)) + require.NoError(t, os.Mkdir(subDir, 0o700)) + } + + for i := 0; i < tt.subDirs; i++ { + filename := filepath.Join(dir, fmt.Sprintf("canary-%d", i)) + require.NoError(t, os.WriteFile(filename, []byte{}, 0o600)) + } + + for idx, mft := range tt.manifests { + filename := filepath.Join(dir, fmt.Sprintf("manifest-%d.yaml", idx)) + require.NoError(t, os.WriteFile(filename, []byte(mft), 0o600)) + } + + for _, crd := range tt.create { + require.NoError(t, tt.c.Create(ctx, crd)) + } + + if !tt.wantErr(t, UpgradeCRDs(ctx, tt.c, dir), fmt.Sprintf("UpgradeCRDs(%v, %v, %v)", + ctx, tt.c, dir)) { + return + } + + for _, w := range tt.want { + var crd apiextensionsv1.CustomResourceDefinition + require.NoError(t, tt.c.Get(ctx, client.ObjectKey{Name: w.Name}, &crd)) + assert.Equal(t, w.TypeMeta, crd.TypeMeta) + assert.Equal(t, w.Spec, crd.Spec) + } + }) + } +} diff --git a/main.go b/main.go index 0065225df..e86f64cac 100644 --- a/main.go +++ b/main.go @@ -10,6 +10,7 @@ import ( "flag" "fmt" "os" + "path/filepath" "strconv" "strings" "time" @@ -17,13 +18,14 @@ import ( argorolloutsv1alpha1 "github.com/argoproj/argo-rollouts/pkg/apis/rollouts/v1alpha1" "github.com/cenkalti/backoff/v4" "github.com/prometheus/client_golang/prometheus" - "gopkg.in/yaml.v3" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" "k8s.io/apimachinery/pkg/runtime" utilruntime "k8s.io/apimachinery/pkg/util/runtime" clientgoscheme "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/metrics/server" "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/yaml" // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC, etc.) // to ensure that exec-entrypoint and run can make use of them. @@ -38,6 +40,7 @@ import ( "github.com/hashicorp/vault-secrets-operator/internal/helpers" "github.com/hashicorp/vault-secrets-operator/internal/metrics" "github.com/hashicorp/vault-secrets-operator/internal/options" + "github.com/hashicorp/vault-secrets-operator/internal/utils" vclient "github.com/hashicorp/vault-secrets-operator/internal/vault" "github.com/hashicorp/vault-secrets-operator/internal/version" //+kubebuilder:scaffold:imports @@ -65,7 +68,53 @@ func init() { //+kubebuilder:scaffold:scheme } +// upgradeCRDs upgrades the CRDs in the cluster to the latest version. +func upgradeCRDs() error { + root, err := filepath.Abs(filepath.Dir(os.Args[0])) + if err != nil { + return err + } + + var c client.Client + config, err := ctrl.GetConfig() + if err != nil { + return err + } + + s := runtime.NewScheme() + if apiextensionsv1.AddToScheme(s) != nil { + return err + } + + c, err = client.New(config, client.Options{Scheme: s}) + if err != nil { + return err + } + + timeout := time.Second * 30 + if v := os.Getenv("VSO_UPGRADE_CRDS_TIMEOUT"); v != "" { + if to, err := time.ParseDuration(v); err == nil { + timeout = to + } + } + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + return utils.UpgradeCRDs(ctx, c, filepath.Join(root, "crds")) +} + func main() { + if filepath.Base(os.Args[0]) == "upgrade-crds" { + // If the binary is named "upgrade-crds" then we are running in a job to upgrade + // CRDs and exit. The docker image will contain a symlink to the binary with this + // name. Following this pattern allows us to need only one image for executing + // utility jobs like this. + var exitCode int + if err := upgradeCRDs(); err != nil { + os.Stderr.WriteString(fmt.Sprintf("failed to upgrade CRDs, err=%s\n", err)) + } + os.Exit(exitCode) + } + persistenceModelNone := "none" persistenceModelDirectUnencrypted := "direct-unencrypted" persistenceModelDirectEncrypted := "direct-encrypted" diff --git a/test/unit/deployment.bats b/test/unit/deployment.bats index f1d9ee89c..e2185908f 100755 --- a/test/unit/deployment.bats +++ b/test/unit/deployment.bats @@ -101,6 +101,10 @@ load _helpers --set 'controller.manager.resources.requests.cpu=100m' \ --set 'controller.manager.resources.limits.memory=200Mi' \ --set 'controller.manager.resources.limits.cpu=200m' \ + --set 'hooks.resources.requests.memory=256Mi' \ + --set 'hooks.resources.requests.cpu=200m' \ + --set 'hooks.resources.limits.memory=400Mi' \ + --set 'hooks.resources.limits.cpu=400m' \ . | tee /dev/stderr | yq '.' | tee /dev/stderr) @@ -115,14 +119,15 @@ load _helpers [ "${actual}" = "200m" ] actual=$(echo "$controller" | yq '.limits.memory' | tee /dev/stderr) [ "${actual}" = "200Mi" ] + actual=$(echo "$job" | yq '.requests.cpu' | tee /dev/stderr) - [ "${actual}" = "100m" ] + [ "${actual}" = "200m" ] actual=$(echo "$job" | yq '.requests.memory' | tee /dev/stderr) - [ "${actual}" = "100Mi" ] + [ "${actual}" = "256Mi" ] actual=$(echo "$job" | yq '.limits.cpu' | tee /dev/stderr) - [ "${actual}" = "200m" ] + [ "${actual}" = "400m" ] actual=$(echo "$job" | yq '.limits.memory' | tee /dev/stderr) - [ "${actual}" = "200Mi" ] + [ "${actual}" = "400Mi" ] } #-------------------------------------------------------------------- diff --git a/test/unit/helpers.bats b/test/unit/helpers.bats index 33e8c505c..1ae43c83c 100644 --- a/test/unit/helpers.bats +++ b/test/unit/helpers.bats @@ -65,13 +65,10 @@ load _helpers #-------------------------------------------------------------------- # component label # -# This test ensures that we set "component" labels in every file. -# If this test fails, you're likely missing setting that label somewhere. -# - -@test "helper/app.kubernetes.io/component label: used everywhere" { +@test "helper/app.kubernetes.io/component label: included in all resources" { cd `chart_dir` - # Grep for files that don't have 'component: ' in them - local actual=$(grep -L 'app.kubernetes.io/component: ' templates/*.yaml | tee /dev/stderr ) - [ "${actual}" = '' ] + local actual=$(helm template . | \ + yq '({"match": .metadata.labels | has("app.kubernetes.io/component"), "doc": document_index, "name": .metadata.name, "kind": .kind, "apiVersion": .apiVersion})' \ + | tee /dev/stderr | grep -c 'match: false') + [ "${actual}" = '0' ] } From 193d7b9a4ba006f40e642f8594f2a19e730b397c Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Thu, 30 May 2024 18:05:02 +0000 Subject: [PATCH 02/13] Preserve symlinks and directories --- .github/workflows/build.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index c9830a315..acc09ff71 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -108,7 +108,8 @@ jobs: run: | mkdir dist out make ci-build - zip -r -j out/${{ env.PKG_NAME }}_${{ needs.get-product-version.outputs.product-version }}_linux_${{ matrix.arch }}.zip dist/${{ env.GOOS }}/${{ env.GOARCH }}/ + cd dist/${{ env.GOOS }}/${{ env.GOARCH }}/. + zip -r --symlinks ../../../out/${{ env.PKG_NAME }}_${{ needs.get-product-version.outputs.product-version }}_linux_${{ matrix.arch }}.zip . - name: Upload binaries uses: actions/upload-artifact@65462800fd760344b1a7b4382951275a0abb4808 # v4.3.3 with: From 879d2713ee7641939f6c2591a5eab533e63bb4e6 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Thu, 30 May 2024 18:59:46 +0000 Subject: [PATCH 03/13] Work around limitations with actions-docker-build --- .github/workflows/build.yaml | 18 +++++++++++++----- Makefile | 7 +++++-- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index acc09ff71..e51f3d931 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -108,8 +108,7 @@ jobs: run: | mkdir dist out make ci-build - cd dist/${{ env.GOOS }}/${{ env.GOARCH }}/. - zip -r --symlinks ../../../out/${{ env.PKG_NAME }}_${{ needs.get-product-version.outputs.product-version }}_linux_${{ matrix.arch }}.zip . + zip -r -j out/${{ env.PKG_NAME }}_${{ needs.get-product-version.outputs.product-version }}_linux_${{ matrix.arch }}.zip dist/${{ env.GOOS }}/${{ env.GOARCH }}/${{ env.PKG_NAME }} - name: Upload binaries uses: actions/upload-artifact@65462800fd760344b1a7b4382951275a0abb4808 # v4.3.3 with: @@ -126,9 +125,12 @@ jobs: env: repo: ${{github.event.repository.name}} version: ${{needs.get-product-version.outputs.product-version}} - steps: - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v4.1.6 + - name: Setup scripts directory + shell: bash + run: | + make ci-build-scripts-dir GOARCH="${{ matrix.arch }}" - name: Docker Build (Action) uses: hashicorp/actions-docker-build@v2 env: @@ -164,9 +166,12 @@ jobs: repo: ${{github.event.repository.name}} version: ${{needs.get-product-version.outputs.product-version}} image_tag: ${{needs.get-product-version.outputs.product-version}}-ubi - steps: - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v4.1.6 + - name: Setup scripts directory + shell: bash + run: | + make ci-build-scripts-dir GOARCH="${{ matrix.arch }}" - name: Docker Build (Action) uses: hashicorp/actions-docker-build@v2 env: @@ -204,9 +209,12 @@ jobs: repo: ${{github.event.repository.name}} version: ${{needs.get-product-version.outputs.product-version}} image_tag: ${{needs.get-product-version.outputs.product-version}}-ubi - steps: - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v4.1.6 + - name: Setup scripts directory + shell: bash + run: | + make ci-build-scripts-dir GOARCH="${{ matrix.arch }}" - name: Docker Build (Action) uses: hashicorp/actions-docker-build@v2 env: diff --git a/Makefile b/Makefile index e72f1132a..e39ce6ca3 100644 --- a/Makefile +++ b/Makefile @@ -272,12 +272,15 @@ docker-push: ## Push docker image with the manager. ##@ CI -.PHONY: ci-build -ci-build: ## Build operator binary (without generating assets). +.PHONY: ci-build-scripts-dir +ci-build-scripts-dir: ## Build operator binary (without generating assets). rm -rf $(BUILD_DIR)/$(GOOS)/$(GOARCH)/scripts mkdir -p $(BUILD_DIR)/$(GOOS)/$(GOARCH)/scripts cp -a chart/crds $(BUILD_DIR)/$(GOOS)/$(GOARCH)/scripts/. ln -s ../$(BIN_NAME) $(BUILD_DIR)/$(GOOS)/$(GOARCH)/scripts/upgrade-crds + +.PHONY: ci-build +ci-build: ci-build-scripts-dir ## Build operator binary (without generating assets). CGO_ENABLED=0 GOOS=$(GOOS) GOARCH=$(GOARCH) go build \ -ldflags "${LD_FLAGS} $(shell GOOS=$(GOOS) GOARCH=$(GOARCH) ./scripts/ldflags-version.sh)" \ -a \ From 76aa42b78a354b5f71149df88e289f8dbc3fc910 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Thu, 30 May 2024 15:04:58 -0400 Subject: [PATCH 04/13] Cache go modules --- .github/workflows/build.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index e51f3d931..b2400e614 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -41,6 +41,8 @@ jobs: uses: actions/setup-go@cdcb36043654635271a94b9a6d1392de5bb323a7 # v5.0.1 with: go-version-file: .go-version + cache-dependency-path: | + go.sum - name: go fmt run: | make check-fmt From 09534f2b4362590ced24c3abf191faef6f4c1d4b Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Thu, 30 May 2024 15:11:10 -0400 Subject: [PATCH 05/13] Revert "Cache go modules" This reverts commit 76aa42b78a354b5f71149df88e289f8dbc3fc910. --- .github/workflows/build.yaml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index b2400e614..e51f3d931 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -41,8 +41,6 @@ jobs: uses: actions/setup-go@cdcb36043654635271a94b9a6d1392de5bb323a7 # v5.0.1 with: go-version-file: .go-version - cache-dependency-path: | - go.sum - name: go fmt run: | make check-fmt From 8eeff1ab14f82140488f905f9e8931408c088e70 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Thu, 30 May 2024 15:16:05 -0400 Subject: [PATCH 06/13] Set the exit code to 1 on error --- main.go | 1 + 1 file changed, 1 insertion(+) diff --git a/main.go b/main.go index e86f64cac..bfa7792df 100644 --- a/main.go +++ b/main.go @@ -110,6 +110,7 @@ func main() { // utility jobs like this. var exitCode int if err := upgradeCRDs(); err != nil { + exitCode = 1 os.Stderr.WriteString(fmt.Sprintf("failed to upgrade CRDs, err=%s\n", err)) } os.Exit(exitCode) From 92d88c9c61d55a34e2d448a7fe475abc8d726422 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Thu, 30 May 2024 19:42:41 +0000 Subject: [PATCH 07/13] Misc fixups --- chart/templates/hook-upgrade-crds.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/chart/templates/hook-upgrade-crds.yaml b/chart/templates/hook-upgrade-crds.yaml index 2c46a2808..e586155fc 100644 --- a/chart/templates/hook-upgrade-crds.yaml +++ b/chart/templates/hook-upgrade-crds.yaml @@ -88,7 +88,7 @@ spec: image: {{ .Values.controller.manager.image.repository }}:{{ .Values.controller.manager.image.tag }} env: - name: VSO_UPGRADE_CRDS_TIMEOUT - value: .Values.hooks.upgradeCRDs.executionTimeout + value: {{ .Values.hooks.upgradeCRDs.executionTimeout }} command: - /scripts/upgrade-crds {{- with .Values.hooks.resources }} From 9ebc4606f4e85b70a033f2f690c73683578516f0 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Thu, 30 May 2024 22:08:02 +0000 Subject: [PATCH 08/13] Add chart tests --- test/unit/hook-upgrade-crds.bats | 207 +++++++++++++++++++++++++++++++ 1 file changed, 207 insertions(+) create mode 100755 test/unit/hook-upgrade-crds.bats diff --git a/test/unit/hook-upgrade-crds.bats b/test/unit/hook-upgrade-crds.bats new file mode 100755 index 000000000..01a2583e8 --- /dev/null +++ b/test/unit/hook-upgrade-crds.bats @@ -0,0 +1,207 @@ +#!/usr/bin/env bats + +# +# Copyright (c) HashiCorp, Inc. +# SPDX-License-Identifier: BUSL-1.1 +# + +load _helpers + +#-------------------------------------------------------------------- +@test "hookUpgradeCRDs: disabled" { + cd "$(chart_dir)" + local actual + actual=$(helm template \ + -s templates/hook-upgrade-crds.yaml \ + --set hooks.upgradeCRDs.enabled=false \ + . | tee /dev/stderr | + yq '. | length' | tee /dev/stderr) + [ "${actual}" = "0" ] +} + +@test "hookUpgradeCRDs: enabled by default" { + pushd "$(chart_dir)" > /dev/stderr + local object + object=$(helm template \ + -s templates/hook-upgrade-crds.yaml \ + . | tee /dev/stderr) + + # assert that we have 4 documents using an base 0 index + [ "$(echo "${object}" | yq '. | di' | tee /dev/stderr | tail -n1)" = "3" ] + + local sa + sa="$(echo "${object}" | yq 'select(di == 0)' | tee /dev/stderr)" + [ "$(echo "${sa}" | \ + yq '.kind == "ServiceAccount"')" = "true" ] + [ "$(echo "${sa}" | \ + yq '.metadata.annotations | length')" = "3" ] + [ "$(echo "${sa}" | \ + yq '.metadata.annotations."helm.sh/hook" == "pre-upgrade"')" = "true" ] + [ "$(echo "${sa}" | \ + yq '.metadata.annotations."helm.sh/hook-delete-policy" == "hook-succeeded,before-hook-creation"')" = "true" ] + [ "$(echo "${sa}" | \ + yq '.metadata.annotations."helm.sh/hook-weight" == "1"')" = "true" ] + + local cr + cr="$(echo "${object}" | yq 'select(di == 1)' | tee /dev/stderr)" + [ "$(echo "${cr}" | yq '.kind == "ClusterRole"')" = "true" ] + [ "$(echo "${cr}" | \ + yq '.metadata.annotations | length')" = "3" ] + [ "$(echo "${cr}" | \ + yq '.metadata.annotations."helm.sh/hook" == "pre-upgrade"')" = "true" ] + [ "$(echo "${cr}" | \ + yq '.metadata.annotations."helm.sh/hook-delete-policy" == "hook-succeeded,before-hook-creation"')" = "true" ] + [ "$(echo "${cr}" | \ + yq '.metadata.annotations."helm.sh/hook-weight" == "2"')" = "true" ] + + local crb + crb="$(echo "${object}" | yq 'select(di == 2)' | tee /dev/stderr)" + [ "$(echo "${crb}" | yq '.kind == "ClusterRoleBinding"')" = "true" ] + [ "$(echo "${crb}" | \ + yq '.metadata.annotations | length')" = "3" ] + [ "$(echo "${crb}" | \ + yq '.metadata.annotations."helm.sh/hook" == "pre-upgrade"')" = "true" ] + [ "$(echo "${crb}" | \ + yq '.metadata.annotations."helm.sh/hook-delete-policy" == "hook-succeeded,before-hook-creation"')" = "true" ] + [ "$(echo "${crb}" | \ + yq '.metadata.annotations."helm.sh/hook-weight" == "2"')" = "true" ] + + local job + job="$(echo "${object}" | yq 'select(di == 3)' | tee /dev/stderr)" + [ "$(echo "${job}" | yq '.kind == "Job"')" = "true" ] + [ "$(echo "${job}" | \ + yq '.metadata.annotations | length')" = "3" ] + [ "$(echo "${job}" | \ + yq '.metadata.annotations."helm.sh/hook" == "pre-upgrade"')" = "true" ] + [ "$(echo "${job}" | \ + yq '.metadata.annotations."helm.sh/hook-delete-policy" == "hook-succeeded,before-hook-creation"')" = "true" ] + [ "$(echo "${job}" | \ + yq '.metadata.annotations."helm.sh/hook-weight" == "99"')" = "true" ] +} + + +@test "hookUpgradeCRDs: ClusterRole extended" { + pushd "$(chart_dir)" > /dev/stderr + local object + object=$(helm template \ + -s templates/hook-upgrade-crds.yaml \ + . ) + + # assert that we have 4 documents using an base 0 index + [ "$(echo "${object}" | yq '. | di' | tee /dev/stderr | tail -n1)" = "3" ] + + local cr + cr="$(echo "${object}" | yq 'select(di == 1)')" + [ "$(echo "${cr}" | yq '.kind == "ClusterRole"')" = "true" ] + [ "$(echo "${cr}" | yq '(.rules | length) == 1')" = "true" ] + [ "$(echo "${cr}" | yq '(.rules[0] | length) == 3')" = "true" ] + [ "$(echo "${cr}" | yq '.rules[0].apiGroups[0] == "apiextensions.k8s.io"')" = "true" ] + [ "$(echo "${cr}" | yq '(.rules[0].resources | length) == 1')" = "true" ] + [ "$(echo "${cr}" | yq '.rules[0].resources[0] == "customresourcedefinitions"')" = "true" ] + [ "$(echo "${cr}" | yq '(.rules[0].verbs | length) == 6')" = "true" ] + [ "$(echo "${cr}" | yq '.rules[0].verbs[0] == "create"')" = "true" ] + [ "$(echo "${cr}" | yq '.rules[0].verbs[1] == "delete"')" = "true" ] + [ "$(echo "${cr}" | yq '.rules[0].verbs[2] == "get"')" = "true" ] + [ "$(echo "${cr}" | yq '.rules[0].verbs[3] == "list"')" = "true" ] + [ "$(echo "${cr}" | yq '.rules[0].verbs[4] == "patch"')" = "true" ] + [ "$(echo "${cr}" | yq '.rules[0].verbs[5] == "update"')" = "true" ] +} + +@test "hookUpgradeCRDs: Job extended" { + pushd "$(chart_dir)" > /dev/stderr + local object + object=$(helm template \ + -s templates/hook-upgrade-crds.yaml \ + . ) + + # assert that we have 4 documents using an base 0 index + [ "$(echo "${object}" | yq '. | di' | tee /dev/stderr | tail -n1)" = "3" ] + + local job + job="$(echo "${object}" | yq 'select(di == 3)' | tee /dev/stderr)" + [ "$(echo "${job}" | yq '.kind == "Job"')" = "true" ] + [ "$(echo "${job}" | \ + yq '.spec.backoffLimit == 5')" = "true" ] + [ "$(echo "${job}" | \ + yq '(.spec.template.spec.containers | length) == "1"')" = "true" ] + [ "$(echo "${job}" | \ + yq '(.spec.template.spec.containers | length) == "1"')" = "true" ] + [ "$(echo "${job}" | \ + yq '.spec.template.spec.containers[0].command[0] == "/scripts/upgrade-crds"')" = "true" ] + [ "$(echo "${job}" | \ + yq '(.spec.template.spec.containers[0].resources | length) == 2')" = "true" ] + [ "$(echo "${job}" | \ + yq '(.spec.template.spec.containers[0].env | length == 1)')" = "true" ] +} + +@test "hookUpgradeCRDs: Job extended with defaults" { + pushd "$(chart_dir)" > /dev/stderr + local object + object=$(helm template \ + -s templates/hook-upgrade-crds.yaml \ + . ) + + # assert that we have 4 documents using an base 0 index + [ "$(echo "${object}" | yq '. | di' | tee /dev/stderr | tail -n1)" = "3" ] + + local job + job="$(echo "${object}" | yq 'select(di == 3)' | tee /dev/stderr)" + [ "$(echo "${job}" | yq '.kind == "Job"')" = "true" ] + [ "$(echo "${job}" | \ + yq '.spec.backoffLimit == 5')" = "true" ] + [ "$(echo "${job}" | \ + yq '(.spec.template.spec.containers | length) == "1"')" = "true" ] + [ "$(echo "${job}" | \ + yq '(.spec.template.spec.containers | length) == "1"')" = "true" ] + [ "$(echo "${job}" | \ + yq '.spec.template.spec.containers[0].command[0] == "/scripts/upgrade-crds"')" = "true" ] + [ "$(echo "${job}" | \ + yq '(.spec.template.spec.containers[0].resources | length) == 2')" = "true" ] + [ "$(echo "${job}" | \ + yq '(.spec.template.spec.containers[0].env | length == 1)')" = "true" ] +} + +@test "hookUpgradeCRDs: Job extended with custom values" { + pushd "$(chart_dir)" > /dev/stderr + local object + object=$(helm template \ + -s templates/hook-upgrade-crds.yaml \ + --set hooks.upgradeCRDs.backoffLimit=10 \ + --set hooks.upgradeCRDs.executionTimeout='64s' \ + --set hooks.resources.limits.cpu='501m' \ + --set hooks.resources.limits.memory='129Mi' \ + --set hooks.resources.requests.cpu='11m' \ + --set hooks.resources.requests.memory='65Mi' \ + . ) + + # assert that we have 4 documents using an base 0 index + [ "$(echo "${object}" | yq '. | di' | tee /dev/stderr | tail -n1)" = "3" ] + + local job + job="$(echo "${object}" | yq 'select(di == 3)' | tee /dev/stderr)" + [ "$(echo "${job}" | \ + yq '.spec.backoffLimit == 10')" = "true" ] + [ "$(echo "${job}" | yq '.kind == "Job"')" = "true" ] + [ "$(echo "${job}" | \ + yq '(.spec.template.spec.containers | length) == "1"')" = "true" ] + [ "$(echo "${job}" | \ + yq '(.spec.template.spec.containers | length) == "1"')" = "true" ] + [ "$(echo "${job}" | \ + yq '.spec.template.spec.containers[0].command[0] == "/scripts/upgrade-crds"')" = "true" ] + [ "$(echo "${job}" | \ + yq '(.spec.template.spec.containers[0].resources | length) == 2')" = "true" ] + [ "$(echo "${job}" | \ + yq '.spec.template.spec.containers[0].resources.limits.cpu == "501m"')" = "true" ] + [ "$(echo "${job}" | \ + yq '.spec.template.spec.containers[0].resources.limits.memory == "129Mi"')" = "true" ] + [ "$(echo "${job}" | \ + yq '.spec.template.spec.containers[0].resources.requests.cpu == "11m"')" = "true" ] + [ "$(echo "${job}" | \ + yq '.spec.template.spec.containers[0].resources.requests.memory == "65Mi"')" = "true" ] + [ "$(echo "${job}" | \ + yq '(.spec.template.spec.containers[0].env | length == 1)')" = "true" ] + [ "$(echo "${job}" | \ + yq '.spec.template.spec.containers[0].env[0].name == "VSO_UPGRADE_CRDS_TIMEOUT"')" = "true" ] + [ "$(echo "${job}" | \ + yq '.spec.template.spec.containers[0].env[0].value == "64s"')" = "true" ] +} From 9d98f240c59604ea53ab742d500dbaccba271e5d Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Fri, 31 May 2024 12:56:10 -0400 Subject: [PATCH 09/13] Add chart integration Go tests --- Makefile | 4 + chart/templates/cluster-role-binding.yaml | 5 + .../clusterrole-aggregated-editor.yaml | 5 + .../clusterrole-aggregated-viewer.yaml | 5 + chart/templates/hook-upgrade-crds.yaml | 5 + internal/utils/utils.go | 45 +-- test/chart/chart_test.go | 266 ++++++++++++++++++ 7 files changed, 318 insertions(+), 17 deletions(-) create mode 100644 test/chart/chart_test.go diff --git a/Makefile b/Makefile index e39ce6ca3..4fe305c89 100644 --- a/Makefile +++ b/Makefile @@ -338,6 +338,10 @@ integration-test-both: ## Run integration tests against Vault Enterprise and Vau $(MAKE) integration-test VAULT_ENTERPRISE=true ENT_TESTS=$(VAULT_ENTERPRISE) $(MAKE) integration-test +.PHONY: integration-test-chart +integration-test-chart: + INTEGRATION_TESTS=true go test github.com/hashicorp/vault-secrets-operator/test/chart/... $(TESTARGS) -timeout=10m + .PHONY: setup-kind setup-kind: ## create a kind cluster for running the acceptance tests locally kind get clusters | grep --silent "^$(KIND_CLUSTER_NAME)$$" || \ diff --git a/chart/templates/cluster-role-binding.yaml b/chart/templates/cluster-role-binding.yaml index d002c1e76..9ecfdee18 100644 --- a/chart/templates/cluster-role-binding.yaml +++ b/chart/templates/cluster-role-binding.yaml @@ -1,3 +1,8 @@ +{{- /* +# Copyright (c) HashiCorp, Inc. +# SPDX-License-Identifier: BUSL-1.1 +*/ -}} + --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding diff --git a/chart/templates/clusterrole-aggregated-editor.yaml b/chart/templates/clusterrole-aggregated-editor.yaml index 7ff7f3f94..fb2880f8d 100644 --- a/chart/templates/clusterrole-aggregated-editor.yaml +++ b/chart/templates/clusterrole-aggregated-editor.yaml @@ -1,3 +1,8 @@ +{{- /* +# Copyright (c) HashiCorp, Inc. +# SPDX-License-Identifier: BUSL-1.1 +*/ -}} + {{- if .Values.controller.rbac.clusterRoleAggregation.editorRoles -}} apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole diff --git a/chart/templates/clusterrole-aggregated-viewer.yaml b/chart/templates/clusterrole-aggregated-viewer.yaml index 0ee7de43e..a0098bb98 100644 --- a/chart/templates/clusterrole-aggregated-viewer.yaml +++ b/chart/templates/clusterrole-aggregated-viewer.yaml @@ -1,3 +1,8 @@ +{{- /* +# Copyright (c) HashiCorp, Inc. +# SPDX-License-Identifier: BUSL-1.1 +*/ -}} + {{- if .Values.controller.rbac.clusterRoleAggregation.viewerRoles -}} apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRole diff --git a/chart/templates/hook-upgrade-crds.yaml b/chart/templates/hook-upgrade-crds.yaml index e586155fc..804edc106 100644 --- a/chart/templates/hook-upgrade-crds.yaml +++ b/chart/templates/hook-upgrade-crds.yaml @@ -1,3 +1,8 @@ +{{- /* +# Copyright (c) HashiCorp, Inc. +# SPDX-License-Identifier: BUSL-1.1 +*/ -}} + {{- if .Values.hooks.upgradeCRDs.enabled -}} --- apiVersion: v1 diff --git a/internal/utils/utils.go b/internal/utils/utils.go index b16e5737e..854561b79 100644 --- a/internal/utils/utils.go +++ b/internal/utils/utils.go @@ -63,19 +63,14 @@ func GetOwnerRefFromObj(owner ctrlclient.Object, scheme *runtime.Scheme) (metav1 return ownerRef, nil } -// UpgradeCRDs upgrades custom resource definitions a directory containing CRD -// YAML manifest files. It only supports -// apiextensionsv1.CustomResourceDefinition. If the CRD exists in the cluster, it -// will be patched from the contents from the corresponding manifest file. If the -// CRD does not exist, it will be created. The Client must have the -// apiextensionsv1.Scheme registered. -func UpgradeCRDs(ctx context.Context, c ctrlclient.Client, dir string) error { - logger := zap.New().WithName("UpgradeCRDs").WithValues( - "version", version.Version(), "dir", dir) - +// LoadCRDsFromDir reads dir to find any CustomResourceDefinition YAML manifest +// files. It only supports apiextensionsv1.CustomResourceDefinition. Returns a +// slice of apiextensionsv1.CustomResourceDefinition objects or an error if any +// occurred. +func LoadCRDsFromDir(dir string) ([]apiextensionsv1.CustomResourceDefinition, error) { d, err := os.ReadDir(dir) if err != nil { - return err + return nil, err } var crds []apiextensionsv1.CustomResourceDefinition @@ -91,33 +86,49 @@ func UpgradeCRDs(ctx context.Context, c ctrlclient.Client, dir string) error { fn := filepath.Join(dir, f.Name()) fh, err := os.Open(fn) if err != nil { - return err + return nil, err } b, err := io.ReadAll(fh) if err != nil { - return err + return nil, err } jsonB, err := yaml.YAMLToJSONStrict(b) if err != nil { - return err + return nil, err } var crd apiextensionsv1.CustomResourceDefinition if err := json.Unmarshal(jsonB, &crd); err != nil { - return err + return nil, err } if crd.GroupVersionKind() != apiextensionsv1.SchemeGroupVersion.WithKind(crd.Kind) { - logger.Info("Skipping unsupported kind", "gvk", crd.GroupVersionKind(), - "name", crd.Name, "filename", fn) continue } crds = append(crds, crd) } + return crds, nil +} + +// UpgradeCRDs upgrades custom resource definitions a directory containing CRD +// YAML manifest files. It only supports +// apiextensionsv1.CustomResourceDefinition. If the CRD exists in the cluster, it +// will be patched from the contents from the corresponding manifest file. If the +// CRD does not exist, it will be created. The Client must have the +// apiextensionsv1.Scheme registered. +func UpgradeCRDs(ctx context.Context, c ctrlclient.Client, dir string) error { + logger := zap.New().WithName("UpgradeCRDs").WithValues( + "version", version.Version(), "dir", dir) + + crds, err := LoadCRDsFromDir(dir) + if err != nil { + return err + } + if len(crds) == 0 { return fmt.Errorf("no CRDs found in directory %q", dir) } diff --git a/test/chart/chart_test.go b/test/chart/chart_test.go new file mode 100644 index 000000000..b99c5f81c --- /dev/null +++ b/test/chart/chart_test.go @@ -0,0 +1,266 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package chart + +import ( + "context" + "fmt" + "log" + "os" + "os/exec" + "os/signal" + "path" + "path/filepath" + "runtime" + "strings" + "sync" + "syscall" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + ctrlruntime "k8s.io/apimachinery/pkg/runtime" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" + ctrl "sigs.k8s.io/controller-runtime" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/hashicorp/vault-secrets-operator/internal/utils" +) + +var ( + testRoot string + chartPath string + onlyOneSignalHandler = make(chan struct{}) + shutdownSignals = []os.Signal{os.Interrupt, syscall.SIGTERM} + vsoNamespace = "vault-secrets-operator-system" + // kindClusterName is set in TestMain + kindClusterName string + // set in TestMain + client ctrlclient.Client + scheme = ctrlruntime.NewScheme() +) + +func init() { + _, curFilePath, _, _ := runtime.Caller(0) + testRoot = path.Dir(curFilePath) + + var err error + chartPath, err = filepath.Abs(filepath.Join(testRoot, "..", "..", "chart")) + if err != nil { + panic(err) + } + + utilruntime.Must(apiextensionsv1.AddToScheme(scheme)) +} + +func TestMain(m *testing.M) { + if os.Getenv("INTEGRATION_TESTS") == "" { + os.Exit(0) + } + + kindClusterName = fmt.Sprintf("vso-chart-%d", time.Now().UnixNano()) + + var err error + var result int + + var tempDir string + tempDir, err = os.MkdirTemp(os.TempDir(), "MainTestChart") + if err != nil { + log.Printf("ERROR: Failed to create tempdir: %s", err) + os.Exit(1) + } + + kubeConfig := filepath.Join(tempDir, ".kube-config") + os.Setenv("KUBECONFIG", kubeConfig) + cleanupFunc := func() { + if tempDir != "" { + os.RemoveAll(tempDir) + } + + cmd := exec.Command("kind", + "delete", "cluster", "--name", kindClusterName, + ) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + if err = cmd.Run(); err != nil { + result = 1 + log.Printf("WARN: Failed to delete the kind cluster: %s", err) + } + } + + cmd := exec.Command("kind", + "create", "cluster", + "--name", kindClusterName, + "--kubeconfig", kubeConfig, + ) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + + wg := sync.WaitGroup{} + wg.Add(1) + ctx, cancel := setupSignalHandler() + { + go func() { + select { + case <-ctx.Done(): + cleanupFunc() + wg.Done() + } + }() + } + + err = cmd.Run() + if err != nil { + log.Printf("ERROR: Failed to create kind cluster: %s", err) + os.Exit(1) + } + + config := ctrl.GetConfigOrDie() + client, err = ctrlclient.New(config, ctrlclient.Options{Scheme: scheme}) + if err != nil { + log.Printf("ERROR: Failed to setup k8s client: %s", err) + os.Exit(1) + } + + result = m.Run() + + cancel() + wg.Wait() + os.Exit(result) +} + +func TestChart_upgradeCRDs(t *testing.T) { + releaseName := strings.Replace(strings.ToLower(t.Name()), "_", "-", -1) + ctx := context.Background() + t.Cleanup(func() { + assert.NoError(t, uninstallVSO(t, ctx, + "--wait", + "--namespace", vsoNamespace, + releaseName, + )) + }) + + require.NoError(t, runKind(t, ctx, + "load", "docker-image", "hashicorp/vault-secrets-operator:0.0.0-dev", + "--name", kindClusterName, + )) + + require.NoError(t, installVSO(t, ctx, + "--wait", + "--create-namespace", + "--namespace", vsoNamespace, + // picking a lower version to ensure we see some CRD changes. + "--version", "0.2.0", + releaseName, + "hashicorp/vault-secrets-operator", + )) + + var origCRDs apiextensionsv1.CustomResourceDefinitionList + require.NoError(t, client.List(ctx, &origCRDs)) + + origCRDsByName := map[string]apiextensionsv1.CustomResourceDefinition{} + for _, o := range origCRDs.Items { + origCRDsByName[o.Name] = o + } + + require.NoError(t, upgradeVSO(t, ctx, + "--wait", + "--namespace", vsoNamespace, + "--set", "controller.manager.image.tag=0.0.0-dev", + releaseName, + chartPath, + )) + + crdsDir := filepath.Join(chartPath, "crds") + wantCRDs, err := utils.LoadCRDsFromDir(crdsDir) + require.NoError(t, err, "failed to load CRDs from %q", crdsDir) + require.Greater(t, len(wantCRDs), 0, "no CRDs found in %q", crdsDir) + + var updatedCRDs apiextensionsv1.CustomResourceDefinitionList + require.NoError(t, client.List(ctx, &updatedCRDs)) + assert.NotEqual(t, origCRDs.Items, updatedCRDs.Items) + + for _, wantCRD := range wantCRDs { + var updatedCRD apiextensionsv1.CustomResourceDefinition + require.NoError(t, client.Get(ctx, ctrlclient.ObjectKeyFromObject(&wantCRD), &updatedCRD)) + if wantCRD.Spec.Conversion == nil { + updatedCRD.Spec.Conversion = nil + } + + if o, ok := origCRDsByName[wantCRD.Name]; ok { + assert.Greater(t, updatedCRD.Generation, o.Generation) + assert.Equal(t, o.UID, updatedCRD.UID) + } else { + assert.Equal(t, int64(1), updatedCRD.Generation) + } + + assert.Equal(t, wantCRD.Spec, updatedCRD.Spec, "CRD %q .spec mismatch", wantCRD.Name) + assert.Equal(t, wantCRD.Labels, updatedCRD.Labels, "CRD %q .metadata.labels mismatch", wantCRD.Name) + assert.Equal(t, wantCRD.Annotations, updatedCRD.Annotations, "CRD %q .metadata.annotations mismatch", wantCRD.Name) + } +} + +func installVSO(t *testing.T, ctx context.Context, extraArgs ...string) error { + t.Helper() + ctx_, cancel := context.WithTimeout(ctx, time.Minute*5) + defer cancel() + return runHelm(t, ctx_, append([]string{"install"}, extraArgs...)...) +} + +func upgradeVSO(t *testing.T, ctx context.Context, extraArgs ...string) error { + t.Helper() + ctx_, cancel := context.WithTimeout(ctx, time.Minute*5) + defer cancel() + return runHelm(t, ctx_, append([]string{"upgrade"}, extraArgs...)...) +} + +func uninstallVSO(t *testing.T, ctx context.Context, extraArgs ...string) error { + t.Helper() + ctx_, cancel := context.WithTimeout(ctx, time.Minute*3) + defer cancel() + return runHelm(t, ctx_, append([]string{"uninstall"}, extraArgs...)...) +} + +func runHelm(t *testing.T, ctx context.Context, args ...string) error { + t.Helper() + return runCommand(t, ctx, "helm", args...) +} + +func runKind(t *testing.T, ctx context.Context, args ...string) error { + ctx_, cancel := context.WithTimeout(ctx, time.Minute*5) + defer cancel() + t.Helper() + return runCommand(t, ctx_, "kind", args...) +} + +func runCommand(t *testing.T, ctx context.Context, name string, args ...string) error { + t.Helper() + cmd := exec.CommandContext(ctx, name, args...) + cmd.Stdout = os.Stdout + cmd.Stderr = os.Stderr + t.Logf("Running command %q", cmd) + return cmd.Run() +} + +// // setupSignalHandler registers for SIGTERM and SIGINT. A context is returned +// // which is canceled on one of these signals. If a second signal is caught, the program +// // is terminated with exit code 1. +func setupSignalHandler() (context.Context, context.CancelFunc) { + close(onlyOneSignalHandler) // panics when called twice + + ctx, cancel := context.WithCancel(context.Background()) + + c := make(chan os.Signal, 2) + signal.Notify(c, shutdownSignals...) + go func() { + <-c + cancel() + <-c + os.Exit(1) // second signal. Exit directly. + }() + + return ctx, cancel +} From 44d097ce6d411778cb9c90cf2ac9bf37696c2046 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Fri, 31 May 2024 18:04:55 +0000 Subject: [PATCH 10/13] Update CI --- .github/workflows/build.yaml | 41 ++++++++++++++++++++++++++++++++++++ Makefile | 4 +++- test/chart/chart_test.go | 15 +++++++++++-- 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index e51f3d931..87c773cdb 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -239,6 +239,47 @@ jobs: exit 1 fi + chart-upgrade-tests: + runs-on: ubuntu-latest + needs: + - get-product-version + - build-pre-checks + - build-docker + strategy: + fail-fast: false + matrix: + start-chart-version: ["0.2.0"] + steps: + - uses: actions/download-artifact@65a9edc5881444af0b9093a5e628f2fe47ea3b2e # v4.1.7 + with: + name: ${{ github.event.repository.name }}_release-default_linux_amd64_${{ needs.get-product-version.outputs.product-version }}_${{ github.sha }}.docker.tar + - name: Load docker image + shell: bash + run: | + docker load --input ${{ github.event.repository.name }}_release-default_linux_amd64_${{ needs.get-product-version.outputs.product-version }}_${{ github.sha }}.docker.tar + - name: Install kind + uses: helm/kind-action@0025e74a8c7512023d06dc019c617aa3cf561fde # v1.10.0 + with: + version: "v0.22.0" + install_only: true + - uses: azure/setup-helm@fe7b79cd5ee1e45176fcad797de68ecaf3ca4814 # v4.2.0 + id: setup-helm + with: + version: "v3.15.1" + - name: Add repo + shell: bash + run: | + helm repo add hashicorp https://helm.releases.hashicorp.com + - uses: actions/checkout@a5ac7e51b41094c92402da3b24376905380afc29 # v4.1.6 + - name: Setup go + uses: actions/setup-go@cdcb36043654635271a94b9a6d1392de5bb323a7 # v5.0.1 + with: + go-version-file: .go-version + - name: Run tests + shell: bash + run: | + export TEST_START_CHART_VERSION="${{ matrix.start-chart-version }}" + make integration-test-chart VERSION="${{ needs.get-product-version.outputs.product-version }}" versions: runs-on: ubuntu-latest steps: diff --git a/Makefile b/Makefile index 4fe305c89..125aad1a9 100644 --- a/Makefile +++ b/Makefile @@ -340,7 +340,9 @@ integration-test-both: ## Run integration tests against Vault Enterprise and Vau .PHONY: integration-test-chart integration-test-chart: - INTEGRATION_TESTS=true go test github.com/hashicorp/vault-secrets-operator/test/chart/... $(TESTARGS) -timeout=10m + IMG=$(IMG) \ + INTEGRATION_TESTS=true \ + go test github.com/hashicorp/vault-secrets-operator/test/chart/... $(TESTARGS) -timeout=10m .PHONY: setup-kind setup-kind: ## create a kind cluster for running the acceptance tests locally diff --git a/test/chart/chart_test.go b/test/chart/chart_test.go index b99c5f81c..3f3a37981 100644 --- a/test/chart/chart_test.go +++ b/test/chart/chart_test.go @@ -133,6 +133,17 @@ func TestMain(m *testing.M) { } func TestChart_upgradeCRDs(t *testing.T) { + startChartVersion := os.Getenv("TEST_START_CHART_VERSION") + if startChartVersion == "" { + startChartVersion = "0.2.0" + } + + // IMG is set in the Makefile + image := os.Getenv("IMG") + if image == "" { + image = "hashicorp/vault-secrets-operator:0.0.0-dev" + } + releaseName := strings.Replace(strings.ToLower(t.Name()), "_", "-", -1) ctx := context.Background() t.Cleanup(func() { @@ -144,7 +155,7 @@ func TestChart_upgradeCRDs(t *testing.T) { }) require.NoError(t, runKind(t, ctx, - "load", "docker-image", "hashicorp/vault-secrets-operator:0.0.0-dev", + "load", "docker-image", image, "--name", kindClusterName, )) @@ -153,7 +164,7 @@ func TestChart_upgradeCRDs(t *testing.T) { "--create-namespace", "--namespace", vsoNamespace, // picking a lower version to ensure we see some CRD changes. - "--version", "0.2.0", + "--version", startChartVersion, releaseName, "hashicorp/vault-secrets-operator", )) From 2858e87487855c669dcae0d835d8aee19e978c9f Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Fri, 31 May 2024 23:49:42 -0400 Subject: [PATCH 11/13] Use VERSION and IMAGE_TAG_BASE instead of IMG --- Makefile | 3 ++- test/chart/chart_test.go | 19 ++++++++++++------- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/Makefile b/Makefile index 125aad1a9..2d088d3a1 100644 --- a/Makefile +++ b/Makefile @@ -340,7 +340,8 @@ integration-test-both: ## Run integration tests against Vault Enterprise and Vau .PHONY: integration-test-chart integration-test-chart: - IMG=$(IMG) \ + IMAGE_TAG_BASE=$(IMAGE_TAG_BASE) \ + VERSION=$(VERSION) \ INTEGRATION_TESTS=true \ go test github.com/hashicorp/vault-secrets-operator/test/chart/... $(TESTARGS) -timeout=10m diff --git a/test/chart/chart_test.go b/test/chart/chart_test.go index 3f3a37981..c18c249cf 100644 --- a/test/chart/chart_test.go +++ b/test/chart/chart_test.go @@ -133,17 +133,21 @@ func TestMain(m *testing.M) { } func TestChart_upgradeCRDs(t *testing.T) { + operatorImageRepo := os.Getenv("IMAGE_TAG_BASE") + if operatorImageRepo == "" { + require.Fail(t, "IMAGE_TAG_BASE is not set") + } + operatorImageTag := os.Getenv("VERSION") + if operatorImageTag == "" { + require.Fail(t, "VERSION is not set") + } + startChartVersion := os.Getenv("TEST_START_CHART_VERSION") if startChartVersion == "" { startChartVersion = "0.2.0" } - // IMG is set in the Makefile - image := os.Getenv("IMG") - if image == "" { - image = "hashicorp/vault-secrets-operator:0.0.0-dev" - } - + image := fmt.Sprintf("%s:%s", operatorImageRepo, operatorImageTag) releaseName := strings.Replace(strings.ToLower(t.Name()), "_", "-", -1) ctx := context.Background() t.Cleanup(func() { @@ -180,7 +184,8 @@ func TestChart_upgradeCRDs(t *testing.T) { require.NoError(t, upgradeVSO(t, ctx, "--wait", "--namespace", vsoNamespace, - "--set", "controller.manager.image.tag=0.0.0-dev", + "--set", fmt.Sprintf("controller.manager.image.repository=%s", operatorImageRepo), + "--set", fmt.Sprintf("controller.manager.image.tag=%s", operatorImageTag), releaseName, chartPath, )) From d08823254a38d9dc7a4fe354f6a523690713e054 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Mon, 3 Jun 2024 23:06:43 +0000 Subject: [PATCH 12/13] Support multi-doc YAML manifests Tests: - make expected upgrades dynamic based in coming CRDs --- .github/workflows/build.yaml | 9 ++- go.mod | 2 +- internal/utils/utils.go | 39 ++++++++++++- internal/utils/utils_test.go | 2 +- test/chart/chart_test.go | 110 ++++++++++++++++++++++++----------- 5 files changed, 123 insertions(+), 39 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index 87c773cdb..f7dbbb4af 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -248,7 +248,14 @@ jobs: strategy: fail-fast: false matrix: - start-chart-version: ["0.2.0"] + start-chart-version: + - "0.1.0" + - "0.2.0" + - "0.3.0" + - "0.4.0" + - "0.5.0" + - "0.6.0" + - "0.7.0" steps: - uses: actions/download-artifact@65a9edc5881444af0b9093a5e628f2fe47ea3b2e # v4.1.7 with: diff --git a/go.mod b/go.mod index e4ecf6307..b2a961f31 100644 --- a/go.mod +++ b/go.mod @@ -29,6 +29,7 @@ require ( github.com/stretchr/testify v1.9.0 golang.org/x/crypto v0.23.0 google.golang.org/api v0.181.0 + gopkg.in/yaml.v3 v3.0.1 k8s.io/api v0.30.1 k8s.io/apiextensions-apiserver v0.30.1 k8s.io/apimachinery v0.30.1 @@ -168,7 +169,6 @@ require ( gopkg.in/inf.v0 v0.9.1 // indirect gopkg.in/tomb.v1 v1.0.0-20141024135613-dd632973f1e7 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect - gopkg.in/yaml.v3 v3.0.1 // indirect k8s.io/klog/v2 v2.120.1 // indirect k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect diff --git a/internal/utils/utils.go b/internal/utils/utils.go index 854561b79..26dc63227 100644 --- a/internal/utils/utils.go +++ b/internal/utils/utils.go @@ -13,6 +13,7 @@ import ( "path/filepath" "strings" + yamlv3 "gopkg.in/yaml.v3" apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -73,7 +74,7 @@ func LoadCRDsFromDir(dir string) ([]apiextensionsv1.CustomResourceDefinition, er return nil, err } - var crds []apiextensionsv1.CustomResourceDefinition + var ret []apiextensionsv1.CustomResourceDefinition for _, f := range d { if f.IsDir() { continue @@ -89,7 +90,35 @@ func LoadCRDsFromDir(dir string) ([]apiextensionsv1.CustomResourceDefinition, er return nil, err } - b, err := io.ReadAll(fh) + crds, err := DecodeCRDs(fh) + if err != nil { + return nil, err + } + + ret = append(ret, crds...) + } + + return ret, nil +} + +type empty struct{} + +// DecodeCRDs reads input to decode CustomResourceDefinition YAML manifest data. +// It supports multiple YAML documents. +func DecodeCRDs(input io.Reader) ([]apiextensionsv1.CustomResourceDefinition, error) { + var crds []apiextensionsv1.CustomResourceDefinition + dec := yamlv3.NewDecoder(input) + seen := map[string]empty{} + for { + var doc any + if err := dec.Decode(&doc); err != nil { + if errors.Is(err, io.EOF) { + break + } + return nil, err + } + + b, err := yaml.Marshal(doc) if err != nil { return nil, err } @@ -108,9 +137,13 @@ func LoadCRDsFromDir(dir string) ([]apiextensionsv1.CustomResourceDefinition, er continue } + if _, ok := seen[crd.Name]; ok { + return nil, fmt.Errorf("duplicate CRD %q found", crd.Name) + } + seen[crd.Name] = empty{} + crds = append(crds, crd) } - return crds, nil } diff --git a/internal/utils/utils_test.go b/internal/utils/utils_test.go index 2c5365f56..52f35a828 100644 --- a/internal/utils/utils_test.go +++ b/internal/utils/utils_test.go @@ -240,7 +240,7 @@ metadata: otherManifests: []string{"foo:\n-bar"}, createDir: true, wantErr: func(t assert.TestingT, err error, i ...interface{}) bool { - return assert.ErrorContains(t, err, `yaml: line 3: could not find expected ':'`) + return assert.ErrorContains(t, err, `yaml: line 2: could not find expected ':'`) }, }, } diff --git a/test/chart/chart_test.go b/test/chart/chart_test.go index c18c249cf..874770827 100644 --- a/test/chart/chart_test.go +++ b/test/chart/chart_test.go @@ -4,15 +4,19 @@ package chart import ( + "bytes" "context" "fmt" + "io" "log" "os" "os/exec" "os/signal" "path" "path/filepath" + "reflect" "runtime" + "slices" "strings" "sync" "syscall" @@ -144,9 +148,36 @@ func TestChart_upgradeCRDs(t *testing.T) { startChartVersion := os.Getenv("TEST_START_CHART_VERSION") if startChartVersion == "" { - startChartVersion = "0.2.0" + require.Fail(t, "TEST_START_CHART_VERSION is not set") } + // incoming CRDS + b := bytes.NewBuffer([]byte{}) + require.NoError(t, + runHelm(t, context.Background(), time.Second*30, b, nil, + "show", "crds", + "--version", startChartVersion, + "hashicorp/vault-secrets-operator", + ), + ) + + incomingCRDs, err := utils.DecodeCRDs(bytes.NewReader(b.Bytes())) + require.NoError(t, err) + slices.SortFunc(incomingCRDs, func(a, b apiextensionsv1.CustomResourceDefinition) int { + return strings.Compare(a.Name, b.Name) + }) + + crdsDir := filepath.Join(chartPath, "crds") + wantCRDs, err := utils.LoadCRDsFromDir(crdsDir) + require.NoError(t, err, "failed to load CRDs from %q", crdsDir) + require.Greater(t, len(wantCRDs), 0, "no CRDs found in %q", crdsDir) + slices.SortFunc(incomingCRDs, func(a, b apiextensionsv1.CustomResourceDefinition) int { + return strings.Compare(a.Name, b.Name) + }) + + expectCRDUpgrade := !reflect.DeepEqual(incomingCRDs, wantCRDs) + t.Logf("Expect upgrade from version %s: %t", startChartVersion, expectCRDUpgrade) + image := fmt.Sprintf("%s:%s", operatorImageRepo, operatorImageTag) releaseName := strings.Replace(strings.ToLower(t.Name()), "_", "-", -1) ctx := context.Background() @@ -167,18 +198,17 @@ func TestChart_upgradeCRDs(t *testing.T) { "--wait", "--create-namespace", "--namespace", vsoNamespace, - // picking a lower version to ensure we see some CRD changes. "--version", startChartVersion, releaseName, "hashicorp/vault-secrets-operator", )) - var origCRDs apiextensionsv1.CustomResourceDefinitionList - require.NoError(t, client.List(ctx, &origCRDs)) + var currentCRDs apiextensionsv1.CustomResourceDefinitionList + require.NoError(t, client.List(ctx, ¤tCRDs)) - origCRDsByName := map[string]apiextensionsv1.CustomResourceDefinition{} - for _, o := range origCRDs.Items { - origCRDsByName[o.Name] = o + curCRDsByName := map[string]apiextensionsv1.CustomResourceDefinition{} + for _, o := range currentCRDs.Items { + curCRDsByName[o.Name] = o } require.NoError(t, upgradeVSO(t, ctx, @@ -190,14 +220,11 @@ func TestChart_upgradeCRDs(t *testing.T) { chartPath, )) - crdsDir := filepath.Join(chartPath, "crds") - wantCRDs, err := utils.LoadCRDsFromDir(crdsDir) - require.NoError(t, err, "failed to load CRDs from %q", crdsDir) - require.Greater(t, len(wantCRDs), 0, "no CRDs found in %q", crdsDir) - var updatedCRDs apiextensionsv1.CustomResourceDefinitionList require.NoError(t, client.List(ctx, &updatedCRDs)) - assert.NotEqual(t, origCRDs.Items, updatedCRDs.Items) + if expectCRDUpgrade { + assert.NotEqual(t, currentCRDs.Items, updatedCRDs.Items) + } for _, wantCRD := range wantCRDs { var updatedCRD apiextensionsv1.CustomResourceDefinition @@ -206,8 +233,15 @@ func TestChart_upgradeCRDs(t *testing.T) { updatedCRD.Spec.Conversion = nil } - if o, ok := origCRDsByName[wantCRD.Name]; ok { - assert.Greater(t, updatedCRD.Generation, o.Generation) + if o, ok := curCRDsByName[wantCRD.Name]; ok { + if expectCRDUpgrade { + assert.Greater(t, updatedCRD.Generation, o.Generation, + "Upgrade expected, CRD %q .metadata.generation", wantCRD.Name, + ) + } else { + assert.Equal(t, updatedCRD.Generation, o.Generation, + "Upgrade unexpected, CRD %q .metadata.generation", wantCRD.Name) + } assert.Equal(t, o.UID, updatedCRD.UID) } else { assert.Equal(t, int64(1), updatedCRD.Generation) @@ -221,42 +255,52 @@ func TestChart_upgradeCRDs(t *testing.T) { func installVSO(t *testing.T, ctx context.Context, extraArgs ...string) error { t.Helper() - ctx_, cancel := context.WithTimeout(ctx, time.Minute*5) - defer cancel() - return runHelm(t, ctx_, append([]string{"install"}, extraArgs...)...) + return runHelm(t, ctx, time.Minute*5, nil, nil, append([]string{"install"}, extraArgs...)...) } func upgradeVSO(t *testing.T, ctx context.Context, extraArgs ...string) error { t.Helper() - ctx_, cancel := context.WithTimeout(ctx, time.Minute*5) - defer cancel() - return runHelm(t, ctx_, append([]string{"upgrade"}, extraArgs...)...) + return runHelm(t, ctx, time.Minute*5, nil, nil, append([]string{"upgrade"}, extraArgs...)...) } func uninstallVSO(t *testing.T, ctx context.Context, extraArgs ...string) error { t.Helper() - ctx_, cancel := context.WithTimeout(ctx, time.Minute*3) - defer cancel() - return runHelm(t, ctx_, append([]string{"uninstall"}, extraArgs...)...) + return runHelm(t, ctx, time.Minute*3, nil, nil, append([]string{"uninstall"}, extraArgs...)...) } -func runHelm(t *testing.T, ctx context.Context, args ...string) error { +func runHelm(t *testing.T, ctx context.Context, timeout time.Duration, stdout, stderr io.Writer, args ...string) error { t.Helper() - return runCommand(t, ctx, "helm", args...) + return runCommandWithTimeout(t, ctx, timeout, stdout, stderr, "helm", args...) } func runKind(t *testing.T, ctx context.Context, args ...string) error { - ctx_, cancel := context.WithTimeout(ctx, time.Minute*5) - defer cancel() t.Helper() - return runCommand(t, ctx_, "kind", args...) + return runCommandWithTimeout(t, ctx, time.Minute*5, nil, nil, "kind", args...) } -func runCommand(t *testing.T, ctx context.Context, name string, args ...string) error { +func runCommandWithTimeout(t *testing.T, ctx context.Context, timeout time.Duration, stdout, stderr io.Writer, name string, args ...string) error { t.Helper() - cmd := exec.CommandContext(ctx, name, args...) - cmd.Stdout = os.Stdout - cmd.Stderr = os.Stderr + var ctx_ context.Context + var cancel context.CancelFunc + if timeout > 0 { + ctx_, cancel = context.WithTimeout(ctx, timeout) + defer cancel() + } else { + ctx_ = ctx + } + + cmd := exec.CommandContext(ctx_, name, args...) + if stdout != nil { + cmd.Stdout = stdout + } else { + cmd.Stdout = os.Stdout + } + if stderr != nil { + cmd.Stderr = stderr + } else { + cmd.Stderr = os.Stderr + } + t.Logf("Running command %q", cmd) return cmd.Run() } From 9b5c3a12a05ea0cce3a8938cbc98bbc4c2662de4 Mon Sep 17 00:00:00 2001 From: Ben Ash Date: Tue, 4 Jun 2024 10:36:52 -0400 Subject: [PATCH 13/13] Post review updates --- .github/workflows/build.yaml | 8 +++--- internal/utils/utils_test.go | 2 +- test/chart/chart_test.go | 45 +++++++++++++++++++++----------- test/unit/hook-upgrade-crds.bats | 6 ----- 4 files changed, 36 insertions(+), 25 deletions(-) diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index f7dbbb4af..e0a4685a9 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -248,14 +248,16 @@ jobs: strategy: fail-fast: false matrix: + # Test upgrading from the previous version to the current build. + # This list should be updated with each new release. + # We probably only want to maintain the last 5-6 versions. start-chart-version: - - "0.1.0" - "0.2.0" - - "0.3.0" + - "0.3.1" - "0.4.0" - "0.5.0" - "0.6.0" - - "0.7.0" + - "0.7.1" steps: - uses: actions/download-artifact@65a9edc5881444af0b9093a5e628f2fe47ea3b2e # v4.1.7 with: diff --git a/internal/utils/utils_test.go b/internal/utils/utils_test.go index 52f35a828..196a3b07c 100644 --- a/internal/utils/utils_test.go +++ b/internal/utils/utils_test.go @@ -268,7 +268,7 @@ metadata: require.NoError(t, os.Mkdir(subDir, 0o700)) } - for i := 0; i < tt.subDirs; i++ { + for i := 0; i < tt.canaries; i++ { filename := filepath.Join(dir, fmt.Sprintf("canary-%d", i)) require.NoError(t, os.WriteFile(filename, []byte{}, 0o600)) } diff --git a/test/chart/chart_test.go b/test/chart/chart_test.go index 874770827..bdda96775 100644 --- a/test/chart/chart_test.go +++ b/test/chart/chart_test.go @@ -16,7 +16,6 @@ import ( "path/filepath" "reflect" "runtime" - "slices" "strings" "sync" "syscall" @@ -151,31 +150,43 @@ func TestChart_upgradeCRDs(t *testing.T) { require.Fail(t, "TEST_START_CHART_VERSION is not set") } - // incoming CRDS + // incoming CRDS are used to determine if an upgrade is expected. b := bytes.NewBuffer([]byte{}) + chart := "hashicorp/vault-secrets-operator" require.NoError(t, runHelm(t, context.Background(), time.Second*30, b, nil, "show", "crds", "--version", startChartVersion, - "hashicorp/vault-secrets-operator", + chart, ), ) incomingCRDs, err := utils.DecodeCRDs(bytes.NewReader(b.Bytes())) require.NoError(t, err) - slices.SortFunc(incomingCRDs, func(a, b apiextensionsv1.CustomResourceDefinition) int { - return strings.Compare(a.Name, b.Name) - }) + incomingCRDsMap := map[string]apiextensionsv1.CustomResourceDefinition{} + for _, crd := range incomingCRDs { + incomingCRDsMap[crd.Name] = crd + } crdsDir := filepath.Join(chartPath, "crds") wantCRDs, err := utils.LoadCRDsFromDir(crdsDir) require.NoError(t, err, "failed to load CRDs from %q", crdsDir) require.Greater(t, len(wantCRDs), 0, "no CRDs found in %q", crdsDir) - slices.SortFunc(incomingCRDs, func(a, b apiextensionsv1.CustomResourceDefinition) int { - return strings.Compare(a.Name, b.Name) - }) - expectCRDUpgrade := !reflect.DeepEqual(incomingCRDs, wantCRDs) + var expectCRDUpgrade bool + // expectUpgradeMap is used to determine if a CRD should have been upgraded. + expectUpgradeMap := map[string]bool{} + for _, crd := range wantCRDs { + if o, ok := incomingCRDsMap[crd.Name]; ok { + if reflect.DeepEqual(o.Spec, crd.Spec) && reflect.DeepEqual(o.ObjectMeta, crd.ObjectMeta) { + expectUpgradeMap[crd.Name] = false + } else { + expectCRDUpgrade = true + expectUpgradeMap[crd.Name] = true + } + } + } + t.Logf("Expect upgrade from version %s: %t", startChartVersion, expectCRDUpgrade) image := fmt.Sprintf("%s:%s", operatorImageRepo, operatorImageTag) @@ -200,15 +211,15 @@ func TestChart_upgradeCRDs(t *testing.T) { "--namespace", vsoNamespace, "--version", startChartVersion, releaseName, - "hashicorp/vault-secrets-operator", + chart, )) var currentCRDs apiextensionsv1.CustomResourceDefinitionList require.NoError(t, client.List(ctx, ¤tCRDs)) - curCRDsByName := map[string]apiextensionsv1.CustomResourceDefinition{} + installedCRDsMap := map[string]apiextensionsv1.CustomResourceDefinition{} for _, o := range currentCRDs.Items { - curCRDsByName[o.Name] = o + installedCRDsMap[o.Name] = o } require.NoError(t, upgradeVSO(t, ctx, @@ -225,6 +236,7 @@ func TestChart_upgradeCRDs(t *testing.T) { if expectCRDUpgrade { assert.NotEqual(t, currentCRDs.Items, updatedCRDs.Items) } + assert.Equal(t, len(wantCRDs), len(updatedCRDs.Items), "CRD count mismatch") for _, wantCRD := range wantCRDs { var updatedCRD apiextensionsv1.CustomResourceDefinition @@ -233,8 +245,8 @@ func TestChart_upgradeCRDs(t *testing.T) { updatedCRD.Spec.Conversion = nil } - if o, ok := curCRDsByName[wantCRD.Name]; ok { - if expectCRDUpgrade { + if o, ok := installedCRDsMap[wantCRD.Name]; ok { + if expect, _ := expectUpgradeMap[o.Name]; expect { assert.Greater(t, updatedCRD.Generation, o.Generation, "Upgrade expected, CRD %q .metadata.generation", wantCRD.Name, ) @@ -250,6 +262,9 @@ func TestChart_upgradeCRDs(t *testing.T) { assert.Equal(t, wantCRD.Spec, updatedCRD.Spec, "CRD %q .spec mismatch", wantCRD.Name) assert.Equal(t, wantCRD.Labels, updatedCRD.Labels, "CRD %q .metadata.labels mismatch", wantCRD.Name) assert.Equal(t, wantCRD.Annotations, updatedCRD.Annotations, "CRD %q .metadata.annotations mismatch", wantCRD.Name) + assert.Len(t, updatedCRD.Status.Conditions, 2, "CRD %q .status.conditions mismatch", wantCRD.Name) + assert.Equal(t, wantCRD.Spec.Names, updatedCRD.Status.AcceptedNames, "CRD %q .status.acceptedNames mismatch", wantCRD.Name) + assert.Equal(t, len(updatedCRD.Status.StoredVersions), len(wantCRD.Spec.Versions), "CRD %q .status.storedVersions", wantCRD.Name) } } diff --git a/test/unit/hook-upgrade-crds.bats b/test/unit/hook-upgrade-crds.bats index 01a2583e8..6c49a029d 100755 --- a/test/unit/hook-upgrade-crds.bats +++ b/test/unit/hook-upgrade-crds.bats @@ -124,8 +124,6 @@ load _helpers yq '.spec.backoffLimit == 5')" = "true" ] [ "$(echo "${job}" | \ yq '(.spec.template.spec.containers | length) == "1"')" = "true" ] - [ "$(echo "${job}" | \ - yq '(.spec.template.spec.containers | length) == "1"')" = "true" ] [ "$(echo "${job}" | \ yq '.spec.template.spec.containers[0].command[0] == "/scripts/upgrade-crds"')" = "true" ] [ "$(echo "${job}" | \ @@ -151,8 +149,6 @@ load _helpers yq '.spec.backoffLimit == 5')" = "true" ] [ "$(echo "${job}" | \ yq '(.spec.template.spec.containers | length) == "1"')" = "true" ] - [ "$(echo "${job}" | \ - yq '(.spec.template.spec.containers | length) == "1"')" = "true" ] [ "$(echo "${job}" | \ yq '.spec.template.spec.containers[0].command[0] == "/scripts/upgrade-crds"')" = "true" ] [ "$(echo "${job}" | \ @@ -182,8 +178,6 @@ load _helpers [ "$(echo "${job}" | \ yq '.spec.backoffLimit == 10')" = "true" ] [ "$(echo "${job}" | yq '.kind == "Job"')" = "true" ] - [ "$(echo "${job}" | \ - yq '(.spec.template.spec.containers | length) == "1"')" = "true" ] [ "$(echo "${job}" | \ yq '(.spec.template.spec.containers | length) == "1"')" = "true" ] [ "$(echo "${job}" | \