Skip to content

Add automatic generation of CRDs for CNP and CCNP#11607

Merged
aanm merged 20 commits intocilium:masterfrom
christarazi:pr/christarazi/auto-generate-cnp-ccnp
Aug 6, 2020
Merged

Add automatic generation of CRDs for CNP and CCNP#11607
aanm merged 20 commits intocilium:masterfrom
christarazi:pr/christarazi/auto-generate-cnp-ccnp

Conversation

@christarazi
Copy link
Copy Markdown
Member

@christarazi christarazi commented May 20, 2020

This paves the path for generating CRDs using controller-tools's controller-gen tool. This will help in preparation for the Kubernetes 1.19 release. A significant benefit is the ability to generate the validation schemas, instead of writing them by hand, which can be error-prone.

See commit msgs.

Updates #11476
Related #11142

@maintainer-s-little-helper
Copy link
Copy Markdown

Please set the appropriate release note label.

@christarazi christarazi added area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. kind/enhancement This would improve or streamline existing functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. upgrade-impact This PR has potential upgrade or downgrade impact. and removed dont-merge/needs-release-note labels May 20, 2020
@christarazi
Copy link
Copy Markdown
Member Author

test-me-please

@stale
Copy link
Copy Markdown

stale Bot commented Jun 20, 2020

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale Bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jun 20, 2020
@christarazi christarazi added pinned These issues are not marked stale by our issue bot. and removed stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. labels Jun 20, 2020
@christarazi christarazi force-pushed the pr/christarazi/auto-generate-cnp-ccnp branch 9 times, most recently from 868111a to d0f027a Compare July 17, 2020 20:36
@christarazi
Copy link
Copy Markdown
Member Author

test-me-please

@christarazi christarazi force-pushed the pr/christarazi/auto-generate-cnp-ccnp branch 2 times, most recently from 09791d1 to b498944 Compare July 18, 2020 02:47
@christarazi
Copy link
Copy Markdown
Member Author

test-me-please

@christarazi christarazi force-pushed the pr/christarazi/auto-generate-cnp-ccnp branch from b498944 to 03d0d6d Compare July 20, 2020 22:53
@christarazi
Copy link
Copy Markdown
Member Author

test-me-please

@coveralls
Copy link
Copy Markdown

coveralls commented Jul 20, 2020

Coverage Status

Coverage increased (+0.06%) to 37.023% when pulling 2bee3db on christarazi:pr/christarazi/auto-generate-cnp-ccnp into 2c600b1 on cilium:master.

@christarazi
Copy link
Copy Markdown
Member Author

Rebased again

christarazi and others added 20 commits August 5, 2020 23:14
To generate CRD validation schemas (also known as structual validation
schemas[1]), we bring in controller-tools. This houses a tool called
controller-gen which performs various K8s related code generation.

Note that we are using a fork of controller-tools due to the upstream
(as of v0.3.0) lacking support in two main areas.

1) A type which implements custom JSON marshalling has its validation
   schema generation skipped. In our case, api.Rule (which is set as the
   Spec field in CNP and CCNP) implements custom JSON marshalling. With
   the upstream tool, this skips the validation generation for it. In
   our fork, we revert this feature. See PR:
   kubernetes-sigs/controller-tools#427
2) For CNP and CCNP, we require `oneOf` validation for the
   endpointSelector and nodeSelector fields. This is because we want to
   validate that either endpointSelector OR nodeSelector is present, but
   not both, and at least one. In the upstream, there's currently an
   open PR that implements support for this. We have manually merged it
   in our fork. See PR:
   kubernetes-sigs/controller-tools#298

This commit also brings updates to our dependencies:

- gomega v1.7.0 -> v1.8.1
- golang.org/x/internal/{tools,event...}

[1]:
https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#specifying-a-structural-schema

Co-authored-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: André Martins <andre@cilium.io>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
This is used for embedding the generated CRD YAMLs into binary form.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
Now that the "Check k8s generated files" step includes generating CRDs
and running go-bindata to serialize the CRDs into binary form, we need
to install go-bindata.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
JSON tags are required for controller-gen, a tool for generating CRDs.
This commit adds JSON tags to all struct fields that are part of the
API, i.e. CRD.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
The reason for this commit is because the controller-gen tool used for
CRD generation requires fields in the a CRD struct to be exported. In
addition, the tool requires that all struct fields have JSON tags.

This commit temporarily exports these fields and leaves indicators for
future refactor.

Once https://go-review.googlesource.com/c/tools/+/245857 is merged, this
commit can be reverted.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
This commit separates out CiliumNetworkPolicy from v2/types.go to
prepare for use with the controller-gen tool for CRD generation.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
This commit separates out CiliumClusterwideNetworkPolicy (CCNP) from
v2/types.go to prepare for use with the controller-gen tool for
automatic CRD generation.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
This commit explicitly embeds metav1.TypeMeta and metav1.ObjectMeta, as these
are needed for the following commits which introduce automated CRD generation
using the controller-gen tool. Due to the (redundant) embedding, changes were
required in pkg/k8s. We need to embed these fields (for now) because
controller-gen will not detect a CRD type unless it has those fields.

This commit also modifies the deepequal code for CNP and CCNP to
deduplicate the functions.

The deepcopy generated code has also been updated.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
This allows us to avoid any JSON marshaling issues with controller-gen
tool, and gets rid of our handmade Timestamp type.

Note that this commit adds the extra JSON tag, as that will be important
when we start using the controller-gen tool to automate the generation
of CRDs. Without the tag, the tool will complain about struct fields
which don't have JSON tags.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
This commit helps keep the description field in the CRD to have
consistent formatting across all the types. It also moves all multi-line
marker comments together akin to the formatting from the core K8s CRD
files (pkg/k8s/**/k8s/apis/core/v1/types.go).

Signed-off-by: Chris Tarazi <chris@isovalent.com>
This commit provides a replacement for handwritten CRD schema validation
via annotating struct fields with markers for use with the
controller-gen tool.

Running the tool and generating the binary representation have been
automated in these Makefile targets:

  1) `make manifests` which will run the automatic CRD generator tool
     and outputs CRD YAMLs into `examples/crds/`.

  2) `make go-bindata` which will embed the binary representation of the
     CRD YAMLs so that it can be imported directly into Cilium.

Note that this change bring back go-bindata, a tool that we previously
removed in cilium#10177. This can be swapped in the future if we decide
there's a better alternative.

Commits following this will add the bindata.go file which contains the
binary representation of the generated CRD.

Co-authored-by: Chris Tarazi <chris@isovalent.com>
Signed-off-by: André Martins <andre@cilium.io>
Signed-off-by: Chris Tarazi <chris@isovalent.com>
The reason for this commit is to generate validation for MatchLabels. In
upstream controller-tools, in order to generate validation for maps, a
new type (either for key or value) or a type alias must be defined so
that validation markers (`+kubebuilder...`) can be placed on the type.
There is not upstream support which would allow not having to define new
types. If this changes in the future, we should revert this commit, and
apply the new markers appropriately.

This commit relies on support in our fork of controller-tools:
christarazi/controller-tools@1b05110

Signed-off-by: Chris Tarazi <chris@isovalent.com>
This commit allows us to reject policies with invalid entities at the
apiserver level as soon as a user has applied the YAML.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
For unknown reasons, we need to manually add the
metadata.creationTimestamp column (as "Age" in kubectl output) even
though this is not a Cilium-specific field (it is part of
metav1.ObjectMeta type). Thus, we are omitting adding the description to
this column because we want to avoid hardcoding the documentation of the
field.

This CRD is generated using
https://book.kubebuilder.io/reference/generating-crd.html.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
Fakes and deepcopy generated code have also been updated.

This CRD is generated using
https://book.kubebuilder.io/reference/generating-crd.html.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
This commit introduces a function called GetPregeneratedCRD.

Now that we generate CRDs with the controller-gen tool, it allows us to
also generate validation schema as well. This commit refactors how we
retrieve the pregenerated CRDs. This refactor allows us to pass the
pregenerated CRD to the validator package.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
Previously, this validation was done at the apiserver level with
handwritten schema validation. Now that we have automated CRD schema
validation, we must implement this check at the agent level as a
stopgap.

The reason for this is that given the relationship of CCNP and CNP (CCNP
embeds CNP), it was impossible to represent the following in
controller-gen marker comments to generate validation schema:

- CNP only supports EndpointSelector.
- CCNP supports both EndpointSelector and NodeSelector, but only one at
  a time in a policy.

This commit is meant to be reverted once an approach for seperating the
CNP and CCNP CRD types has been implemented.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
Bumping the version following updates allowing automated schema
generation.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
Copy link
Copy Markdown
Member Author

As discussed offline, the following additions were made:

  • Removed GO111MODULE from make manifests target
  • Updated dev environment requirements to include go-bindata
  • Consolidated make manifest and make go-bindata to run inside ./contrib/scripts/check-k8s-code-gen.sh so that it is part of the GH Action, preventing CRD drift
    • Updated generate-k8s-api GH Action to install go-bindata as a prerequisite

@christarazi
Copy link
Copy Markdown
Member Author

test-me-please

Copy link
Copy Markdown
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

💯

@aanm
Copy link
Copy Markdown
Member

aanm commented Aug 6, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. kind/enhancement This would improve or streamline existing functionality. pinned These issues are not marked stale by our issue bot. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. upgrade-impact This PR has potential upgrade or downgrade impact.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants