internal/pkg/scaffold/crd.go: overwrite CRD manifests for Go operators (operator-framework#1278)

* internal/pkg/scaffold: overwrite CRD's with newly generated ones unless operator is non-go

* internal/pkg/scaffold/crd.go: set names if not empty, and set Repo to
override PROJECT file check by the CRD generator

* Gopkg.toml: add controller-tools override (temporary)

* revendor

* CHANGELOG.md: added kubebuilder annotation bug and change entries

Author: estroz

CHANGELOG.md

@@ -14,6 +14,7 @@
 - Renamed `--docker-build-args` option to `--image-build-args` option for `build` subcommand, because this option can now be shared with other image build tools than docker when `--image-builder` option is specified. ([#1311](https://github.com/operator-framework/operator-sdk/pull/1311))
 - Reduces Helm release information in CR status to only the release name and manifest and moves it from `status.conditions` to a new top-level `deployedRelease` field. ([#1309](https://github.com/operator-framework/operator-sdk/pull/1309))
   - **WARNING**: Users with active CRs and releases who are upgrading their helm-based operator should upgrade to one based on v0.7.0 before upgrading further. Helm operators based on v0.8.0+ will not seamlessly transition release state to the persistent backend, and will instead uninstall and reinstall all managed releases.
+- Go operator CRDs are overwritten when being regenerated by [`operator-sdk generate openapi`](https://github.com/operator-framework/operator-sdk/blob/master/doc/sdk-cli-reference.md#openapi). Users can now rely on `+kubebuilder` annotations in their API code, which provide access to most OpenAPIv3 [validation properties](https://github.com/OAI/OpenAPI-Specification/blob/master/versions/3.0.0.md#schema-object) (the full set will be supported in the near future, see [this PR](https://github.com/kubernetes-sigs/controller-tools/pull/190)) and [other CRD fields](https://book.kubebuilder.io/beyond_basics/generating_crd.html). ([#1278](https://github.com/operator-framework/operator-sdk/pull/1278))
 
 ### Deprecated
 
@@ -24,6 +25,7 @@
 ### Bug Fixes
 
 - In Helm-based operators, when a custom resource with a failing release is reverted back to a working state, the `ReleaseFailed` condition is now correctly removed. ([#1321](https://github.com/operator-framework/operator-sdk/pull/1321))
+- [`operator-sdk generate openapi`](https://github.com/operator-framework/operator-sdk/blob/master/doc/sdk-cli-reference.md#openapi) no longer overwrites CRD values derived from `+kubebuilder` annotations in Go API code. See issues ([#1212](https://github.com/operator-framework/operator-sdk/issues/1212)) and ([#1323](https://github.com/operator-framework/operator-sdk/issues/1323)) for discussion. ([#1278](https://github.com/operator-framework/operator-sdk/pull/1278))
 
 ## v0.7.0
 

Gopkg.lock

@@ -30,6 +30,12 @@
   name = "sigs.k8s.io/controller-runtime"
   version = "=v0.1.10"
 
+# This override revision has a fix that allows CRD unit tests to run correctly.
+# Remove once v0.1.11 is released.
+[[override]]
+  name = "sigs.k8s.io/controller-tools"
+  revision = "9d55346c2bde73fb3326ac22eac2e5210a730207"
+
 [[constraint]]
   name = "github.com/sergi/go-diff"
   version = "1.0.0"

Gopkg.toml

@@ -16,8 +16,6 @@ package scaffold
 
 import (
 	"fmt"
-	"io/ioutil"
-	"os"
 	"path/filepath"
 	"strings"
 	"sync"
@@ -41,6 +39,20 @@ type CRD struct {
 
 	// IsOperatorGo is true when the operator is written in Go.
 	IsOperatorGo bool
+
+	once sync.Once
+	fs   afero.Fs // For testing, ex. afero.NewMemMapFs()
+}
+
+func (s *CRD) initFS(fs afero.Fs) {
+	s.once.Do(func() {
+		s.fs = fs
+	})
+}
+
+func (s *CRD) getFS() afero.Fs {
+	s.initFS(afero.NewOsFs())
+	return s.fs
 }
 
 func (s *CRD) GetInput() (input.Input, error) {
@@ -76,77 +88,78 @@ func initCache() {
 	})
 }
 
-func (s *CRD) SetFS(_ afero.Fs) {}
+var _ CustomRenderer = &CRD{}
+
+func (s *CRD) SetFS(fs afero.Fs) { s.initFS(fs) }
 
 func (s *CRD) CustomRender() ([]byte, error) {
-	i, _ := s.GetInput()
-	// controller-tools generates crd file names with no _crd.yaml suffix:
-	// <group>_<version>_<kind>.yaml.
-	path := strings.Replace(filepath.Base(i.Path), "_crd.yaml", ".yaml", 1)
-
-	// controller-tools' generators read and make crds for all apis in pkg/apis,
-	// so generate crds in a cached, in-memory fs to extract the data we need.
-	if s.IsOperatorGo && !cache.fileExists(path) {
-		g := &crdgenerator.Generator{
-			RootPath:          s.AbsProjectPath,
-			Domain:            strings.SplitN(s.Resource.FullGroup, ".", 2)[1],
-			OutputDir:         ".",
-			SkipMapValidation: false,
-			OutFs:             cache,
-		}
-		if err := g.ValidateAndInitFields(); err != nil {
-			return nil, err
-		}
-		if err := g.Do(); err != nil {
-			return nil, err
-		}
+	i, err := s.GetInput()
+	if err != nil {
+		return nil, err
 	}
 
-	dstCRD := newCRDForResource(s.Resource)
-	// Get our generated crd's from the in-memory fs. If it doesn't exist in the
-	// fs, the corresponding API does not exist yet, so scaffold a fresh crd
-	// without a validation spec.
-	// If the crd exists in the fs, and a local crd exists, append the validation
-	// spec. If a local crd does not exist, use the generated crd.
-	if _, err := cache.Stat(path); err != nil && !os.IsNotExist(err) {
-		return nil, err
-	} else if err == nil {
+	crd := &apiextv1beta1.CustomResourceDefinition{}
+	if s.IsOperatorGo {
+		// controller-tools generates crd file names with no _crd.yaml suffix:
+		// <group>_<version>_<kind>.yaml.
+		path := strings.Replace(filepath.Base(i.Path), "_crd.yaml", ".yaml", 1)
+
+		// controller-tools' generators read and make crds for all apis in pkg/apis,
+		// so generate crds in a cached, in-memory fs to extract the data we need.
+		if !cache.fileExists(path) {
+			g := &crdgenerator.Generator{
+				RootPath:          s.AbsProjectPath,
+				Domain:            strings.SplitN(s.Resource.FullGroup, ".", 2)[1],
+				Repo:              s.Repo,
+				OutputDir:         ".",
+				SkipMapValidation: false,
+				OutFs:             cache,
+			}
+			if err := g.ValidateAndInitFields(); err != nil {
+				return nil, err
+			}
+			if err := g.Do(); err != nil {
+				return nil, err
+			}
+		}
+
 		b, err := afero.ReadFile(cache, path)
 		if err != nil {
 			return nil, err
 		}
-		dstCRD = &apiextv1beta1.CustomResourceDefinition{}
-		if err = yaml.Unmarshal(b, dstCRD); err != nil {
+		if err = yaml.Unmarshal(b, crd); err != nil {
 			return nil, err
 		}
-		val := dstCRD.Spec.Validation.DeepCopy()
-
-		// If the crd exists at i.Path, append the validation spec to its crd spec.
-		if _, err := os.Stat(i.Path); err == nil {
-			cb, err := ioutil.ReadFile(i.Path)
+		// controller-tools does not set ListKind or Singular names.
+		setCRDNamesForResource(crd, s.Resource)
+		// Remove controller-tools default label.
+		delete(crd.Labels, "controller-tools.k8s.io")
+	} else {
+		// There are currently no commands to update CRD manifests for non-Go
+		// operators, so if a CRD manifests already exists for this gvk, this
+		// scaffold is a no-op.
+		path := filepath.Join(s.AbsProjectPath, i.Path)
+		if _, err = s.getFS().Stat(path); err == nil {
+			b, err := afero.ReadFile(s.getFS(), path)
 			if err != nil {
 				return nil, err
 			}
-			if len(cb) > 0 {
-				dstCRD = &apiextv1beta1.CustomResourceDefinition{}
-				if err = yaml.Unmarshal(cb, dstCRD); err != nil {
+			if len(b) == 0 {
+				crd = newCRDForResource(s.Resource)
+			} else {
+				if err = yaml.Unmarshal(b, crd); err != nil {
 					return nil, err
 				}
-				dstCRD.Spec.Validation = val
 			}
 		}
-		// controller-tools does not set ListKind or Singular names.
-		dstCRD.Spec.Names = getCRDNamesForResource(s.Resource)
-		// Remove controller-tools default label.
-		delete(dstCRD.Labels, "controller-tools.k8s.io")
 	}
-	addCRDSubresource(dstCRD)
-	addCRDVersions(dstCRD)
-	return k8sutil.GetObjectBytes(dstCRD)
+
+	setCRDVersions(crd)
+	return k8sutil.GetObjectBytes(crd)
 }
 
 func newCRDForResource(r *Resource) *apiextv1beta1.CustomResourceDefinition {
-	return &apiextv1beta1.CustomResourceDefinition{
+	crd := &apiextv1beta1.CustomResourceDefinition{
 		TypeMeta: metav1.TypeMeta{
 			APIVersion: "apiextensions.k8s.io/v1beta1",
 			Kind:       "CustomResourceDefinition",
@@ -156,35 +169,33 @@ func newCRDForResource(r *Resource) *apiextv1beta1.CustomResourceDefinition {
 		},
 		Spec: apiextv1beta1.CustomResourceDefinitionSpec{
 			Group:   r.FullGroup,
-			Names:   getCRDNamesForResource(r),
 			Scope:   apiextv1beta1.NamespaceScoped,
 			Version: r.Version,
 			Subresources: &apiextv1beta1.CustomResourceSubresources{
 				Status: &apiextv1beta1.CustomResourceSubresourceStatus{},
 			},
 		},
 	}
+	setCRDNamesForResource(crd, r)
+	return crd
 }
 
-func getCRDNamesForResource(r *Resource) apiextv1beta1.CustomResourceDefinitionNames {
-	return apiextv1beta1.CustomResourceDefinitionNames{
-		Kind:     r.Kind,
-		ListKind: r.Kind + "List",
-		Plural:   r.Resource,
-		Singular: r.LowerKind,
+func setCRDNamesForResource(crd *apiextv1beta1.CustomResourceDefinition, r *Resource) {
+	if crd.Spec.Names.Kind == "" {
+		crd.Spec.Names.Kind = r.Kind
 	}
-}
-
-func addCRDSubresource(crd *apiextv1beta1.CustomResourceDefinition) {
-	if crd.Spec.Subresources == nil {
-		crd.Spec.Subresources = &apiextv1beta1.CustomResourceSubresources{}
+	if crd.Spec.Names.ListKind == "" {
+		crd.Spec.Names.ListKind = r.Kind + "List"
+	}
+	if crd.Spec.Names.Plural == "" {
+		crd.Spec.Names.Plural = r.Resource
 	}
-	if crd.Spec.Subresources.Status == nil {
-		crd.Spec.Subresources.Status = &apiextv1beta1.CustomResourceSubresourceStatus{}
+	if crd.Spec.Names.Singular == "" {
+		crd.Spec.Names.Singular = r.LowerKind
 	}
 }
 
-func addCRDVersions(crd *apiextv1beta1.CustomResourceDefinition) {
+func setCRDVersions(crd *apiextv1beta1.CustomResourceDefinition) {
 	// crd.Version is deprecated, use crd.Versions instead.
 	var crdVersions []apiextv1beta1.CustomResourceDefinitionVersion
 	if crd.Spec.Version != "" {

internal/pkg/scaffold/crd.go

@@ -15,13 +15,14 @@
 package scaffold
 
 import (
-	"os"
 	"path/filepath"
-	"strings"
 	"testing"
 
-	"github.com/operator-framework/operator-sdk/internal/pkg/scaffold/input"
+	testutil "github.com/operator-framework/operator-sdk/internal/pkg/scaffold/internal/testutil"
 	"github.com/operator-framework/operator-sdk/internal/util/diffutil"
+	"github.com/operator-framework/operator-sdk/internal/util/fileutil"
+
+	"github.com/spf13/afero"
 )
 
 func TestCRDGoProject(t *testing.T) {
@@ -30,28 +31,18 @@ func TestCRDGoProject(t *testing.T) {
 		t.Fatal(err)
 	}
 	s, buf := setupScaffoldAndWriter()
-	absPath, err := os.Getwd()
+	s.Fs = afero.NewMemMapFs()
+	cfg, err := setupTestFrameworkConfig()
 	if err != nil {
 		t.Fatal(err)
 	}
-	// Set the project and repo paths to {abs}/test/test-framework, which
-	// contains pkg/apis for the memcached-operator.
-	tfDir := filepath.Join("test", "test-framework")
-	pkgIdx := strings.Index(absPath, "internal/pkg")
-	cfg := &input.Config{
-		Repo:           filepath.Join(absPath[strings.Index(absPath, "github.com"):pkgIdx], tfDir),
-		AbsProjectPath: filepath.Join(absPath[:pkgIdx], tfDir),
-		ProjectName:    tfDir,
-	}
-	if err := os.Chdir(cfg.AbsProjectPath); err != nil {
+
+	err = testutil.WriteOSPathToFS(afero.NewOsFs(), s.Fs, cfg.AbsProjectPath)
+	if err != nil {
 		t.Fatal(err)
 	}
-	defer func() { os.Chdir(absPath) }()
-	err = s.Execute(cfg, &CRD{
-		Input:        input.Input{Path: filepath.Join(tfDir, "cache_v1alpha1_memcached.yaml")},
-		Resource:     r,
-		IsOperatorGo: true,
-	})
+
+	err = s.Execute(cfg, &CRD{Resource: r, IsOperatorGo: true})
 	if err != nil {
 		t.Fatalf("Failed to execute the scaffold: (%v)", err)
 	}
@@ -74,8 +65,6 @@ spec:
     plural: memcacheds
     singular: memcached
   scope: Namespaced
-  subresources:
-    status: {}
   validation:
     openAPIV3Schema:
       properties:
@@ -118,13 +107,31 @@ spec:
 `
 
 func TestCRDNonGoProject(t *testing.T) {
+	s, buf := setupScaffoldAndWriter()
+	s.Fs = afero.NewMemMapFs()
+
 	r, err := NewResource(appApiVersion, appKind)
 	if err != nil {
 		t.Fatal(err)
 	}
-	s, buf := setupScaffoldAndWriter()
-	err = s.Execute(appConfig, &CRD{Resource: r})
+
+	crd := &CRD{Resource: r}
+	i, err := crd.GetInput()
 	if err != nil {
+		t.Fatal(err)
+	}
+	cfg, err := setupTestFrameworkConfig()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	path := filepath.Join(cfg.AbsProjectPath, i.Path)
+	err = afero.WriteFile(s.Fs, path, []byte(crdNonGoExp), fileutil.DefaultFileMode)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err = s.Execute(cfg, crd); err != nil {
 		t.Fatalf("Failed to execute the scaffold: (%v)", err)
 	}
 
@@ -134,6 +141,9 @@ func TestCRDNonGoProject(t *testing.T) {
 	}
 }
 
+// crdNonGoExp contains a simple validation block to make sure manually-added
+// validation is not overwritten. Non-go projects don't have the luxury of
+// kubebuilder annotations.
 const crdNonGoExp = `apiVersion: apiextensions.k8s.io/v1beta1
 kind: CustomResourceDefinition
 metadata:
@@ -148,6 +158,26 @@ spec:
   scope: Namespaced
   subresources:
     status: {}
+  validation:
+    openAPIV3Schema:
+      properties:
+        spec:
+          properties:
+            size:
+              format: int32
+              type: integer
+          required:
+          - size
+          type: object
+        status:
+          properties:
+            nodes:
+              items:
+                type: string
+              type: array
+          required:
+          - nodes
+          type: object
   version: v1alpha1
   versions:
   - name: v1alpha1

internal/pkg/scaffold/crd_test.go

@@ -62,7 +62,7 @@ required = [
 
 [[override]]
   name = "sigs.k8s.io/controller-tools"
-  version = "=v0.1.8"
+  revision = "9d55346c2bde73fb3326ac22eac2e5210a730207"
 
 [[override]]
   name = "k8s.io/api"