Bazel CRD and RBAC role generation #543

randomvariable · 2019-02-02T09:41:16Z

What this PR does / why we need it:
Moves CRD and RBAC yaml generation directly into Bazel.
Our current set of RBAC were not automatically generated as intended from Kubebuilder.
The annotations have been put into the actuators to make this happen.

Eventually, this should move to Cluster API and/or Kubebuilder.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Extracted from #371

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

k8s-ci-robot · 2019-02-02T09:41:24Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: randomvariable

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [randomvariable]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

chuckha

working through this one, off to a good start!

chuckha · 2019-02-04T18:50:28Z

build/install.bzl

+
+def _install_impl(ctx):
+
+    cmd = ""


An alternative implementation to consider:

def _install_impl(ctx): copy_format = "cp -f {} {}" touch_format = "touch {}" cmds = [] for s in ctx.attr.srcs: files = s.files.to_list() file = files[0].path root_file = paths.relativize(file, "bazel-out/k8-fastbuild/genfiles/") cmds.append(copy_format.format(file, root_file)) cmds.append(touch_format.append(ctx.outputs.record.path)) cmd = " && ".join(cmd) ...

If you want to be really safe, the last line should be:

# Remove empty items from cmdList to avoid an erroneous " && && " " && ".join(filter(None, cmdList))

I'm also looking into how to make this work on a Darwin system. It's failing for me with this error:

salazar:cluster-api-provider-aws cha$ make crds bazel build //config:config_install WARNING: failed to raise resource limit 8 to 524288: Invalid argument Starting local Bazel server and connecting to it... INFO: Invocation ID: 456b2a2b-7895-4f8f-865b-45226a773acc ERROR: /Users/cha/go/src/sigs.k8s.io/cluster-api-provider-aws/config/BUILD.bazel:17:1: in install rule //config:config_install: Traceback (most recent call last): File "/Users/cha/go/src/sigs.k8s.io/cluster-api-provider-aws/config/BUILD.bazel", line 17 install(name = 'config_install') File "/Users/cha/go/src/sigs.k8s.io/cluster-api-provider-aws/build/install.bzl", line 28, in _install_impl paths.relativize(file, "bazel-out/k8-fastbuild/genf...") File "/private/var/tmp/_bazel_cha/b7ce3b297716e241b109da3b2c60846f/external/bazel_skylib/lib/paths.bzl", line 186, in paths.relativize fail(("Path '%s' is not beneath '%s'"...))) Path 'bazel-out/darwin-fastbuild/genfiles/config/rbac/rbac_role.yaml' is not beneath 'bazel-out/k8-fastbuild/genfiles/' ERROR: Analysis of target '//config:config_install' failed; build aborted: Analysis of target '//config:config_install' failed; build aborted INFO: Elapsed time: 13.016s INFO: 0 processes. FAILED: Build did NOT complete successfully (358 packages loaded, 9342 targets configured) make: *** [crds] Error 1

Thanks for checking this out, was interested how it'd behave on os x. Will dig deeper, or more likely, split it out into a separate PR

chuckha · 2019-02-04T19:11:43Z

build/install.bzl

+    for s in ctx.attr.srcs:
+      files = s.files.to_list()
+      file = files[0].path
+      root_file = paths.relativize(file,"bazel-out/k8-fastbuild/genfiles/")


define a build_dir outside the for loop:

build_dir = "bazel-out/{}-{}/genfiles".format(ctx.var["TARGET_CPU"], ctx.var["COMPILATION_MODE"])

and then this line can be

root_file = paths.relativize(file, build_dir)

lol or just ctx.var["GENDIR"] thx Bazel

chuckha · 2019-02-04T21:05:32Z

pkg/cloud/aws/actuators/machine/actuator.go

@@ -41,6 +41,11 @@ import (
 	controllerError "sigs.k8s.io/cluster-api/pkg/controller/error"
 )

+//+kubebuilder:rbac:groups=awsprovider.k8s.io,resources=awsmachineproviderconfigs;awsmachineproviderstatuses,verbs=get;list;watch;create;update;patch;delete


Are these being added so that the rbac rules are a bit more restrictive?

chuckha · 2019-02-06T14:42:05Z

Should this PR also clean up the Makefile and remove the crd generation from the make manifests make target?

vincepri

Some minor comments, otherwise this looks great!

vincepri · 2019-02-06T16:27:22Z

Makefile

@@ -163,6 +163,10 @@ manifests: cmd/clusterctl/examples/aws/out/credentials ## Generate manifests for
 manifests-dev: dep-ensure dep-install binaries-dev ## Builds development manifests
 	MANAGER_IMAGE=$(DEV_MANAGER_IMAGE) MANAGER_IMAGE_PULL_POLICY="Always" $(MAKE) manifests

+.PHONY: crds
+crds:
+	bazel build //config:config_install


vincepri · 2019-02-06T16:28:01Z

build/controller_gen.bzl

+def _qualified_genfile(label):
+  return "$$GO_GENRULE_EXECROOT/$(location %s)" % label
+
+def controller_gen(name, importpath, api, visibility, deps = []):


I would love to bring these upstream if that's even possible. We should also add some more comments around these functions and their objective.

100% agree. Have added comments for now, then I'll PR to the controller-runtime repo after we know we're happy with it.

vincepri · 2019-02-06T16:28:27Z

build/install.bzl

+    for s in ctx.attr.srcs:
+      files = s.files.to_list()
+      file = files[0].path
+      root_file = paths.relativize(file,"bazel-out/k8-fastbuild/genfiles/")


Space between arguments?

* Add annotations to controllers as required * New controller_gen build rule for generating CRD and RBAC Signed-off-by: Naadir Jeewa <[email protected]>

randomvariable · 2019-02-06T23:42:20Z

Alright, taken out the install.bzl magic until I understand the sandboxing across Linux and OS X better.

chuckha · 2019-02-07T18:51:10Z

@randomvariable Huh, which part do you want to understand more? It looked pretty good to me with that one fix for darwin architecture.

Also should this PR also clean up the Makefile and remove the crd generation from the make manifests make target? Or am I misunderstanding something?

Signed-off-by: Naadir Jeewa <[email protected]>

randomvariable · 2019-02-08T09:43:48Z

@chuckha Was short on time this week, so just want to do it as a separate PR now.

Have cleaned up the Makefile - deleted the CRD gen from make manifests as it's still user facing, added as a dep to make manifests-dev

randomvariable · 2019-02-08T10:20:49Z

whoops. was cleaning up dead branches in my fork...

chuckha · 2019-02-08T16:11:20Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 2, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2019

k8s-ci-robot requested review from detiber and roberthbailey February 2, 2019 09:41

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 2, 2019

randomvariable force-pushed the bazel-crd-generation branch 3 times, most recently from f149b7b to 9abb7ea Compare February 2, 2019 10:14

randomvariable mentioned this pull request Feb 2, 2019

[WIP]: Provider components as artifacts #371

Closed

8 tasks

randomvariable changed the title ~~Bazel CRD generation~~ Bazel CRD and RBAC role generation Feb 2, 2019

chuckha reviewed Feb 4, 2019

View reviewed changes

vincepri reviewed Feb 6, 2019

View reviewed changes

randomvariable force-pushed the bazel-crd-generation branch from aebe53d to c3b856f Compare February 6, 2019 23:28

bazel: Move CRD and RBAC generation into bazel

5e2dbe5

* Add annotations to controllers as required * New controller_gen build rule for generating CRD and RBAC Signed-off-by: Naadir Jeewa <[email protected]>

randomvariable force-pushed the bazel-crd-generation branch from c3b856f to 5e2dbe5 Compare February 6, 2019 23:40

Makefile: Remove CRD generation from make manifests

b16870c

Signed-off-by: Naadir Jeewa <[email protected]>

randomvariable closed this Feb 8, 2019

randomvariable deleted the bazel-crd-generation branch February 8, 2019 10:19

randomvariable restored the bazel-crd-generation branch February 8, 2019 10:19

randomvariable reopened this Feb 8, 2019

k8s-ci-robot assigned chuckha Feb 8, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2019

k8s-ci-robot merged commit b97293e into kubernetes-sigs:master Feb 8, 2019

justaugustus mentioned this pull request Feb 13, 2019

Utilize bazel for CRD and RBAC generation kubernetes-sigs/cluster-api-provider-azure#89

Closed

randomvariable deleted the bazel-crd-generation branch February 21, 2019 01:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bazel CRD and RBAC role generation #543

Bazel CRD and RBAC role generation #543

randomvariable commented Feb 2, 2019

k8s-ci-robot commented Feb 2, 2019

chuckha left a comment

chuckha Feb 4, 2019 •

edited

Loading

randomvariable Feb 4, 2019

chuckha Feb 4, 2019

chuckha Feb 4, 2019

randomvariable Feb 4, 2019

chuckha Feb 4, 2019 •

edited

Loading

chuckha commented Feb 6, 2019

vincepri left a comment

vincepri Feb 6, 2019

vincepri Feb 6, 2019

randomvariable Feb 6, 2019

vincepri Feb 6, 2019

randomvariable commented Feb 6, 2019

chuckha commented Feb 7, 2019

randomvariable commented Feb 8, 2019

randomvariable commented Feb 8, 2019

chuckha commented Feb 8, 2019

Bazel CRD and RBAC role generation #543

Bazel CRD and RBAC role generation #543

Conversation

randomvariable commented Feb 2, 2019

k8s-ci-robot commented Feb 2, 2019

chuckha left a comment

Choose a reason for hiding this comment

chuckha Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuckha Feb 4, 2019 • edited Loading

Choose a reason for hiding this comment

chuckha commented Feb 6, 2019

vincepri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randomvariable commented Feb 6, 2019

chuckha commented Feb 7, 2019

randomvariable commented Feb 8, 2019

randomvariable commented Feb 8, 2019

chuckha commented Feb 8, 2019

chuckha Feb 4, 2019 •

edited

Loading

chuckha Feb 4, 2019 •

edited

Loading