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.

Sign up for GitHub

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

internal/generate: align CRD generator with controller-gen #2201

Merged
merged 36 commits into from
Jan 6, 2020
Merged

internal/generate: align CRD generator with controller-gen #2201

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
ee02715
This commit aligns the CRD generator with controller-gen's.
estroz Nov 13, 2019
510d5ba
doc: discuss storage version setting in CRDs
estroz Nov 15, 2019
0cf9bd9
update code comment
estroz Nov 15, 2019
8827ac5
update comments and license year to 2019
estroz Nov 15, 2019
adb541b
internal/generate,cmd/operator-sdk: use genutil.Config for common fields
estroz Nov 18, 2019
10cc359
update gencrd.New* -> NewCRD*
estroz Nov 18, 2019
7dcdff3
use resource.Validate()
estroz Nov 18, 2019
6cf5f5f
Merge branch 'master' into align-crd-generator-controller-gen
estroz Nov 19, 2019
2875c25
Merge branch 'master' into align-crd-generator-controller-gen
estroz Nov 19, 2019
658249a
Merge branch 'master' into align-crd-generator-controller-gen
estroz Nov 21, 2019
0e10559
updates based on PR comments
estroz Dec 2, 2019
6a311cc
Merge branch 'master' into align-crd-generator-controller-gen
estroz Dec 2, 2019
af7b9b1
remove CRD generator from add api, relying on OpenAPIGen's instead
estroz Dec 2, 2019
8618098
return err in test
estroz Dec 3, 2019
53b0ee8
Merge branch 'master' into align-crd-generator-controller-gen
estroz Dec 5, 2019
4048006
add back newline
estroz Dec 5, 2019
730e77b
Merge branch 'master' into align-crd-generator-controller-gen
estroz Dec 6, 2019
8b6c940
fix linter errors
estroz Dec 6, 2019
2bf52f0
Apply suggestions from code review
Dec 6, 2019
9e0a18c
updates based on PR comments
estroz Dec 6, 2019
6820652
return struct values instead of pointers and align error messages wit…
estroz Dec 6, 2019
d7db46e
Merge branch 'master' into align-crd-generator-controller-gen
estroz Dec 10, 2019
188e591
fix bug caught by unit test
estroz Dec 10, 2019
e2a903d
internal/generate/gen/config.go: Config provides configuration for ge…
estroz Dec 19, 2019
034f7f2
Merge branch 'master' into align-crd-generator-controller-gen
estroz Dec 19, 2019
d74399d
rename paths after merge
estroz Dec 19, 2019
d3ed3c8
Merge branch 'feature/gen-config' into align-crd-generator-controller…
estroz Dec 19, 2019
b1e73a4
remove global key for Config.Inputs
estroz Dec 27, 2019
83e7099
generalize IncludeFuncs to Filters, can be used for +/- filtering
estroz Dec 28, 2019
8e6532b
add test cases for Config types
estroz Dec 28, 2019
9d5b9bd
Merge branch 'master' into feature/gen-config
estroz Dec 30, 2019
200b572
align package names
estroz Dec 30, 2019
d4fed40
Merge branch 'feature/gen-config' into align-crd-generator-controller…
estroz Dec 30, 2019
04204d1
cmd: do not print usage on 'generate crds' error
estroz Jan 6, 2020
f4a61a5
pass testing struct to helper and improve coverage with Generate() test
estroz Jan 6, 2020
f227322
remove unused Config test
estroz Jan 6, 2020
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
9 changes: 8 additions & 1 deletion cmd/operator-sdk/add/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"path/filepath"

"github.com/operator-framework/operator-sdk/cmd/operator-sdk/internal/genutil"
gencrd "github.com/operator-framework/operator-sdk/internal/generate/crd"
"github.com/operator-framework/operator-sdk/internal/scaffold"
"github.com/operator-framework/operator-sdk/internal/scaffold/input"
"github.com/operator-framework/operator-sdk/internal/util/projutil"
Expand Down Expand Up @@ -125,12 +126,18 @@ func apiRun(cmd *cobra.Command, args []string) error {
&scaffold.Register{Resource: r},
&scaffold.Doc{Resource: r},
&scaffold.CR{Resource: r},
&scaffold.CRD{Resource: r, IsOperatorGo: projutil.IsOperatorGo()},
)
if err != nil {
return fmt.Errorf("api scaffold failed: (%v)", err)
}

crd := gencrd.NewGo(scaffold.ApisDir, scaffold.CRDsDir)
if err := crd.Generate(); err != nil {
return errors.Wrapf(err, "error generating CRDs from APIs in %s", scaffold.ApisDir)
}

log.Info("Generated CustomResourceDefinition manifests.")

// update deploy/role.yaml for the given resource r.
if err := scaffold.UpdateRoleForResource(r, absProjectPath); err != nil {
return fmt.Errorf("failed to update the RBAC manifest for the resource (%v, %v): (%v)", r.APIVersion, r.Kind, err)
Expand Down
16 changes: 10 additions & 6 deletions cmd/operator-sdk/add/crd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,12 @@ import (
"path/filepath"
"strings"

gencrd "github.com/operator-framework/operator-sdk/internal/generate/crd"
"github.com/operator-framework/operator-sdk/internal/scaffold"
"github.com/operator-framework/operator-sdk/internal/scaffold/input"
"github.com/operator-framework/operator-sdk/internal/util/projutil"

"github.com/pkg/errors"
log "github.com/sirupsen/logrus"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -74,7 +76,7 @@ func crdFunc(cmd *cobra.Command, args []string) error {
return err
}

log.Infof("Generating Custom Resource Definition (CRD) version %s for kind %s.", apiVersion, kind)
log.Infof("Generating CustomResourceDefinition (CRD) version %s for kind %s.", apiVersion, kind)

// generate CR/CRD file
resource, err := scaffold.NewResource(apiVersion, kind)
Expand All @@ -84,11 +86,6 @@ func crdFunc(cmd *cobra.Command, args []string) error {

s := scaffold.Scaffold{}
err = s.Execute(cfg,
&scaffold.CRD{
Input: input.Input{IfExistsAction: input.Skip},
Resource: resource,
IsOperatorGo: projutil.IsOperatorGo(),
},
&scaffold.CR{
Input: input.Input{IfExistsAction: input.Skip},
Resource: resource,
Expand All @@ -98,6 +95,13 @@ func crdFunc(cmd *cobra.Command, args []string) error {
return fmt.Errorf("crd scaffold failed: (%v)", err)
}

// This command does not consider an APIs dir. Instead it adds a plain CRD
// for the provided resource. We can use NewNonGo to get this behavior.
crd := gencrd.NewNonGo(scaffold.CRDsDir, scaffold.CRDsDir, *resource)
if err := crd.Generate(); err != nil {
return errors.Wrapf(err, "error generating CRD for %s", resource)
}

// update deploy/role.yaml for the given resource r.
if err := scaffold.UpdateRoleForResource(resource, cfg.AbsProjectPath); err != nil {
return fmt.Errorf("failed to update the RBAC manifest for the resource (%v, %v): (%v)", resource.APIVersion, resource.Kind, err)
Expand Down
37 changes: 5 additions & 32 deletions cmd/operator-sdk/internal/genutil/openapi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ import (
"path/filepath"
"strings"

gencrd "github.com/operator-framework/operator-sdk/internal/generate/crd"
"github.com/operator-framework/operator-sdk/internal/scaffold"
"github.com/operator-framework/operator-sdk/internal/scaffold/input"
"github.com/operator-framework/operator-sdk/internal/util/k8sutil"
"github.com/operator-framework/operator-sdk/internal/util/projutil"

Expand All @@ -36,9 +36,7 @@ import (
func OpenAPIGen() error {
projutil.MustInProjectRoot()

absProjectPath := projutil.MustGetwd()
repoPkg := projutil.GetGoPkg()

gvMap, err := k8sutil.ParseGroupSubpackages(scaffold.ApisDir)
if err != nil {
return fmt.Errorf("failed to parse group versions: (%v)", err)
Expand All @@ -57,36 +55,11 @@ func OpenAPIGen() error {
return err
}

s := &scaffold.Scaffold{}
cfg := &input.Config{
Repo: repoPkg,
AbsProjectPath: absProjectPath,
ProjectName: filepath.Base(absProjectPath),
}
crds, err := k8sutil.GetCRDs(scaffold.CRDsDir)
if err != nil {
return err
}
for _, crd := range crds {
g, v, k := crd.Spec.Group, crd.Spec.Version, crd.Spec.Names.Kind
if v == "" {
if len(crd.Spec.Versions) != 0 {
v = crd.Spec.Versions[0].Name
} else {
return fmt.Errorf("crd of group %s kind %s has no version", g, k)
}
}
r, err := scaffold.NewResource(g+"/"+v, k)
if err != nil {
return err
}
err = s.Execute(cfg,
&scaffold.CRD{Resource: r, IsOperatorGo: projutil.IsOperatorGo()},
)
if err != nil {
return err
}
crd := gencrd.NewGo(scaffold.ApisDir, scaffold.CRDsDir)
if err := crd.Generate(); err != nil {
return errors.Wrapf(err, "error generating CRDs from APIs in %s", scaffold.ApisDir)
}
log.Info("Generated CustomResourceDefinition manifests.")

log.Info("Code-generation complete.")
return nil
Expand Down
17 changes: 15 additions & 2 deletions cmd/operator-sdk/new/cmd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"path/filepath"
"strings"

gencrd "github.com/operator-framework/operator-sdk/internal/generate/crd"
"github.com/operator-framework/operator-sdk/internal/scaffold"
"github.com/operator-framework/operator-sdk/internal/scaffold/ansible"
"github.com/operator-framework/operator-sdk/internal/scaffold/helm"
Expand Down Expand Up @@ -224,7 +225,6 @@ func doAnsibleScaffold() error {
&scaffold.ServiceAccount{},
&scaffold.Role{},
&scaffold.RoleBinding{},
&scaffold.CRD{Resource: resource},
&scaffold.CR{Resource: resource},
&ansible.BuildDockerfile{GeneratePlaybook: generatePlaybook},
&ansible.RolesReadme{Resource: *resource},
Expand Down Expand Up @@ -260,6 +260,13 @@ func doAnsibleScaffold() error {
return fmt.Errorf("new ansible scaffold failed: (%v)", err)
}

crdsDir := filepath.Join(projectName, scaffold.CRDsDir)
crd := gencrd.NewNonGo(crdsDir, crdsDir, *resource)
if err := crd.Generate(); err != nil {
return errors.Wrapf(err, "error generating CRD for %s", resource)
}
log.Info("Generated CustomResourceDefinition manifests.")

// Remove placeholders from empty directories
err = os.Remove(filepath.Join(s.AbsProjectPath, roleFiles.Path))
if err != nil {
Expand Down Expand Up @@ -331,7 +338,6 @@ func doHelmScaffold() error {
&roleScaffold,
&scaffold.RoleBinding{IsClusterScoped: roleScaffold.IsClusterScoped},
&helm.Operator{},
&scaffold.CRD{Resource: resource},
&scaffold.CR{
Resource: resource,
Spec: crSpec,
Expand All @@ -341,6 +347,13 @@ func doHelmScaffold() error {
return fmt.Errorf("new helm scaffold failed: (%v)", err)
}

crdsDir := filepath.Join(projectName, scaffold.CRDsDir)
crd := gencrd.NewNonGo(crdsDir, crdsDir, *resource)
if err := crd.Generate(); err != nil {
return errors.Wrapf(err, "error generating CRD for %s", resource)
}
log.Info("Generated CustomResourceDefinition manifests.")

if err := scaffold.UpdateRoleForResource(resource, cfg.AbsProjectPath); err != nil {
return fmt.Errorf("failed to update the RBAC manifest for resource (%v, %v): (%v)", resource.APIVersion, resource.Kind, err)
}
Expand Down
31 changes: 19 additions & 12 deletions doc/user-guide.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ type MemcachedSpec struct {
Size int32 `json:"size"`
}
type MemcachedStatus struct {
// Nodes are the names of the memcached pods
// Nodes are the names of the memcached pods
// +listType=set
Nodes []string `json:"nodes"`
}
Expand All @@ -99,7 +99,23 @@ After modifying the `*_types.go` file always run the following command to update
$ operator-sdk generate k8s
```

### OpenAPI validation
### Updating CRD manifests

Now that `MemcachedSpec` and `MemcachedStatus` have fields and possibly annotations, the CRD corresponding to the API's group and kind must be updated. To do so, run the following command:

```console
$ operator-sdk generate openapi
```

**Notes:**
- - Your CRD *must* specify exactly one [storage version][crd-storage-version]. Use the `+kubebuilder:storageversion` [marker][crd-markers] to indicate the GVK that should be used to store data by the API server. This marker should be in a comment above your `Memcached` type.
Copy link
Member

Choose a reason for hiding this comment

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

I noticed that without manually adding +kubebuilder:storageversion in advance, operator-sdk add api errors out when another version is added to an existing CRD with the same group & kind.

This problem may be out-of-scope for this PR, but something we may want to think about whether there's a more graceful way to handle from a user experience perspective.

Here's the error I get:

$ operator-sdk generate crds
INFO[0000] Running CRD generator.
example.com/go-operator/pkg/apis/example/v1:-: CRD for GoOperator.example.com has no storage version
Error: error generating CRDs from APIs in pkg/apis: error generating CRD manifests: error running CRD generator: not all generators ran successfully
Usage:
  operator-sdk generate crds [flags]

Flags:
  -h, --help   help for crds

Global Flags:
      --verbose   Enable verbose logging

It's a good hint, but could be improved by telling the user how to fix the problem. Can we put this note into the error message, nearly verbatim and skip outputting the usage?

Copy link
Member

Choose a reason for hiding this comment

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

Also, I just noticed that it's possible to define +kubebuilder:storageversion in both versions of a CRD's types.go files and controller-gen happily creates a CRD where storage: true exists in both versions. That should also be an error right?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 on not printing usage. By note do you mean:

example.com/go-operator/pkg/apis/example/v1:-: CRD for GoOperator.example.com has no storage version

Two CRD versions with storage: true is an error, but controller-tools does not consider this an error during generation. I will submit an issue if one does not exist already upstream.

Copy link
Member

Choose a reason for hiding this comment

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

Oh I meant based on the note that you have in the user guide:

Something like:

Your CRD must specify exactly one storage version. Use the +kubebuilder:storageversion marker to indicate the GVK that should be used to store data by the API server. This marker should be in a comment above your CRDs root storage type in _types.go.

See https://book.kubebuilder.io/multiversion-tutorial/api-changes.html#storage-versions

Copy link
Member Author

Choose a reason for hiding this comment

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

Generator errors are not returned by Runtime.Run() so we can't check if the error returned pertains to no +kubebuilder:storageversion annotation. How about linking https://book.kubebuilder.io/reference/generating-crd.html in an error message for Runtime.Run() errors? This should cover this and other errors, but isn't as specific.

- You may see errors like "API rule violation" when running the above command. For information on these errors, read the below section and see the [API rules][api-rules] documentation.

[crd-storage-version]:https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#writing-reading-and-updating-versioned-customresourcedefinition-objects
[crd-markers]:https://book.kubebuilder.io/reference/markers/crd.html
[api-rules]: https://github.com/kubernetes/kubernetes/tree/36981002246682ed7dc4de54ccc2a96c1a0cbbdb/api/api-rules

#### OpenAPI validation

OpenAPIv3 schemas are added to CRD manifests in the `spec.validation` block when the manifests are generated. This validation block allows Kubernetes to validate the properties in a Memcached Custom Resource when it is created or updated. Additionally a `pkg/apis/<group>/<version>/zz_generated.openapi.go` file is generated containing the Go representation of this validation block if the `+k8s:openapi-gen=true` annotation is present above the kind type declaration (present by default). This auto-generated code is your Go kind type's OpenAPI model, from which you can create a full OpenAPI spec and generate a client. Check out [this issue comment][openapi-details] for steps on how to do so.

Expand All @@ -112,15 +128,7 @@ type Alias string

Usage of markers in API code is discussed in the kubebuilder [CRD generation][generating-crd] and [marker][markers] documentation. A full list of OpenAPIv3 validation markers can be found [here][crd-markers].

To update the OpenAPI validation section in the CRD `deploy/crds/cache.example.com_memcacheds_crd.yaml`, run the following command:

```console
$ operator-sdk generate openapi
```

**Note:** You may see errors like "API rule violation" when running the above command. For information on these errors see the [API rules][api-rules] documentation

An example of the generated YAML is as follows:
An example of generated validation YAML is as follows:

```YAML
spec:
Expand All @@ -141,7 +149,6 @@ To learn more about OpenAPI v3.0 validation schemas in Custom Resource Definitio
[generating-crd]: https://book.kubebuilder.io/reference/generating-crd.html
[markers]: https://book.kubebuilder.io/reference/markers.html
[crd-markers]: https://book.kubebuilder.io/reference/markers/crd-validation.html
[api-rules]: https://github.com/kubernetes/kubernetes/tree/36981002246682ed7dc4de54ccc2a96c1a0cbbdb/api/api-rules

## Add a new Controller

Expand Down
13 changes: 7 additions & 6 deletions doc/user/migrating-existing-apis.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,13 +210,12 @@ This command will automatically update all CRD manifests.

#### CRD Versioning

<!-- TODO: change SDK version to the last release before controller-tools v0.2.0 refactor -->
Kubernetes 1.11+ supports CRD [`spec.versions`][crd-versions] and `spec.version` is [deprecated][crd-version-deprecated] as of Kubernetes 1.12. SDK versions `v0.10.x` and below leverage [`controller-tools@v0.1.x`][controller-tools]' CRD generator which generates a now-deprecated `spec.version` value based on the version contained in an API's import path. Names of CRD manifest files generated by those SDK versions contain the `spec.version`, i.e. one CRD manifest is created *per version in a group* with the file name format `<group>_<version>_<kind>_crd.yaml`. SDK versions `v0.11+` use `controller-tools@v0.2.x`, which generates `spec.versions` but not `spec.version` by default, and use the file name format `<full_group>_<resource>_crd.yaml`.

Kubernetes 1.11+ supports CRD [`spec.versions`][crd-versions] and `spec.version` is [deprecated][crd-version-deprecated] as of Kubernetes 1.12. SDK versions `v0.10.x` and below leverage [`controller-tools`][controller-tools]' CRD generator `v0.1.x` which generates a now-deprecated `spec.version` value based on the version contained in an APIs import path. Names of CRD manifest files generated by those SDK versions contain the `spec.version`, i.e. one CRD manifest is created *per version in a group* with the form `<group>_<version>_<kind>_crd.yaml`. The SDK is in the process of upgrading to `controller-tools` `v0.2.x`, which generates `spec.versions` but not `spec.version` by default. Once the upgrade is complete, future SDK versions will place all versions in a group in `spec.versions`. File names will then have the format `<full_group>_<resource>_crd.yaml`.

**Note**: `<full group>` is the full group name of your CRD while `<group>` is the last subdomain of `<full group>`, ex. `foo.bar.com` vs `foo`. `<resource>` is the plural lower-case of CRD `Kind` specified at `spec.names.plural`.

**Note:** If your operator does not have custom data manually added to its CRD's, you can skip to the [following section](#golang-api-migrations-types-and-commonalities); `operator-sdk generate openapi` will handle CRD updates in that case.
**Notes:**
- `<full group>` is the full group name of your CRD while `<group>` is the last subdomain of `<full group>`, ex. `foo.bar.com` vs `foo`. `<resource>` is the plural lower-case of CRD `Kind` specified at `spec.names.plural`.
- Your CRD *must* specify exactly one [storage version][crd-storage-version]. Use the `+kubebuilder:storageversion` [marker][crd-markers] to indicate the GVK that should be used to store data by the API server. This marker should be in a comment above your `CatalogSourceConfig` type.
- If your operator does not have custom data manually added to its CRD's, you can skip to the [following section](#golang-api-migrations-types-and-commonalities); `operator-sdk generate openapi` will handle CRD updates in that case.

Upgrading from `spec.version` to `spec.versions` will be demonstrated using the following CRD manifest example:

Expand Down Expand Up @@ -404,6 +403,8 @@ TODO
[deepcopy-gen]:https://godoc.org/k8s.io/gengo/examples/deepcopy-gen
[client-gen]:https://github.com/kubernetes/community/blob/master/contributors/devel/sig-api-machinery/generating-clientset.md
[openapi-gen]:https://github.com/kubernetes/kube-openapi
[crd-storage-version]:https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#writing-reading-and-updating-versioned-customresourcedefinition-objects
[crd-markers]:https://book.kubebuilder.io/reference/markers/crd.html
[controller-tools]:https://github.com/kubernetes-sigs/controller-tools
[crd-versions]:https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/
[crd-conv]:https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definition-versioning/#webhook-conversion
Expand Down
Loading