fix: overlapping resource names and improved rbac rules (#290)

* chore: ignore .log files

Signed-off-by: Dustin Scott <sdustin@vmware.com>

* chore: fix typo in ErrProcessManifest error message

Signed-off-by: Dustin Scott <sdustin@vmware.com>

* fix: Fixes #284 error when resources do not have unique names

Rather than de-duplicate the names into something that may not be obvious to
an end user, we error out when we do not have unique resource names and print
a friendly error message.  This will force the user to ensure that the manifests
that they input have a uniqueness in their resources.

Signed-off-by: Dustin Scott <sdustin@vmware.com>

* fix: Fixes #281 added rbac rules to each resource

Signed-off-by: Dustin Scott <sdustin@vmware.com>

* fix: added space between constant and kubebuilder rbac markers

This is needed so that controller-gen can properly consume the rbac markers
in order to generate the proper rbac code for the controller.

Signed-off-by: Dustin Scott <sdustin@vmware.com>

.gitignore

@@ -9,3 +9,4 @@ test/cases/collection/*
 !test/cases/edge-collection/.workloadConfig/
 !test/cases/edge-standalone/.workloadConfig/
 !test/cases/collection/.workloadConfig/
+**/*.log

internal/plugins/workload/v1/scaffolds/templates/api/resources/definition.go

@@ -57,6 +57,11 @@ import (
 )
 
 {{ range .Manifest.ChildResources }}
+{{ range .RBAC }}
+{{- .ToMarker }}
+{{ end }}
+{{ if ne .NameConstant "" }}const {{ .UniqueName }} = "{{ .NameConstant }}"{{ end }}
+
 // {{ .CreateFuncName }} creates the {{ .Name }} {{ .Kind }} resource.
 func {{ .CreateFuncName }} (
 	parent *{{ $.Resource.ImportAlias }}.{{ $.Resource.Kind }},

internal/workload/v1/kinds/workload.go

@@ -72,7 +72,8 @@ type WorkloadBuilder interface {
 
 var (
 	ErrLoadManifests   = errors.New("error loading manifests")
-	ErrProcessManifest = errors.New("error proessing manifest file")
+	ErrProcessManifest = errors.New("error processing manifest file")
+	ErrUniqueName      = errors.New("child resource unique name error")
 )
 
 // WorkloadAPISpec contains fields shared by all workload specs.
@@ -217,6 +218,9 @@ func processManifestError(err error, manifest *manifests.Manifest) error {
 func (ws *WorkloadSpec) processManifests(markerTypes ...markers.MarkerType) error {
 	ws.init()
 
+	// track the unique names so that we can handle when we have an overlap
+	uniqueNames := map[string]bool{}
+
 	for _, manifestFile := range *ws.Manifests {
 		err := ws.processMarkers(manifestFile, markerTypes...)
 		if err != nil {
@@ -240,21 +244,23 @@ func (ws *WorkloadSpec) processManifests(markerTypes ...markers.MarkerType) erro
 				)
 			}
 
-			// add the rules for this manifest
-			rules, err := rbac.ForManifest(&manifestObject)
+			// create the new child resource and validate its unique name
+			childResource, err := manifests.NewChildResource(manifestObject)
 			if err != nil {
+				return processManifestError(err, manifestFile)
+			}
+
+			if uniqueNames[childResource.UniqueName] {
 				return processManifestError(
 					fmt.Errorf(
-						"%w; error generating rbac for resource kind [%s] with name [%s]",
-						err, manifestObject.GetKind(), manifestObject.GetName(),
+						"%w; error generating resource definition for resource kind [%s] with name [%s]",
+						ErrUniqueName, manifestObject.GetKind(), manifestObject.GetName(),
 					),
 					manifestFile,
 				)
 			}
 
-			ws.RBACRules.Add(rules)
-
-			childResource := manifests.NewChildResource(manifestObject)
+			uniqueNames[childResource.UniqueName] = true
 
 			// generate the object source code
 			resourceDefinition, err := generate.Generate([]byte(manifest), "resourceObj")

internal/workload/v1/manifests/child_resource.go

@@ -11,11 +11,13 @@ import (
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
 	"github.com/vmware-tanzu-labs/operator-builder/internal/workload/v1/markers"
+	"github.com/vmware-tanzu-labs/operator-builder/internal/workload/v1/rbac"
 )
 
 var (
 	ErrChildResourceResourceMarkerInspect = errors.New("error inspecting resource markers for child resource")
 	ErrChildResourceResourceMarkerProcess = errors.New("error processing resource markers for child resource")
+	ErrChildResourceRBACGenerate          = errors.New("error generating RBAC for child resource")
 )
 
 // ChildResource contains attributes for resources created by the custom resource.
@@ -32,18 +34,28 @@ type ChildResource struct {
 	StaticContent string
 	SourceCode    string
 	IncludeCode   string
+	RBAC          *rbac.Rules
 }
 
 // NewChildResource returns a representation of a ChildResource object given an unstructured
 // Kubernetes object.
-func NewChildResource(object unstructured.Unstructured) *ChildResource {
+func NewChildResource(object unstructured.Unstructured) (*ChildResource, error) {
+	rbacRules, err := rbac.ForResource(&object)
+	if err != nil {
+		return nil, fmt.Errorf(
+			"%w with kind [%s] and name [%s]",
+			ErrChildResourceRBACGenerate, object.GetKind(), object.GetName(),
+		)
+	}
+
 	return &ChildResource{
 		Name:       object.GetName(),
 		UniqueName: uniqueName(object),
 		Group:      object.GetObjectKind().GroupVersionKind().Group,
 		Version:    object.GetObjectKind().GroupVersionKind().Version,
 		Kind:       object.GetKind(),
-	}
+		RBAC:       rbacRules,
+	}, nil
 }
 
 //nolint:gocritic // needed to satisfy the stringer interface
@@ -107,6 +119,17 @@ func (resource *ChildResource) InitFuncName() string {
 	return ""
 }
 
+// NameConstant returns the constant which is generated in the code for re-use.  It
+// is needed for instances that the metadata.name field is a field marker, in which
+// case we have no way to return the name constant.
+func (resource *ChildResource) NameConstant() string {
+	if strings.HasPrefix(strings.ToLower(resource.Name), "!!start") {
+		return ""
+	}
+
+	return resource.Name
+}
+
 // uniqueName returns the unique name of a resource.  This combines the name, namespace, and kind
 // into a name that is unique.
 //

internal/workload/v1/rbac/rbac.go

@@ -59,14 +59,14 @@ func knownIrregulars() map[string]string {
 	}
 }
 
-// ForManifest will return a set of rules for a particular manifest.  This includes
-// a rule for the manifest itself, in addition to adding particular rules for whatever
+// ForResource will return a set of rules for a particular kubernetes resource.  This includes
+// a rule for the resource itself, in addition to adding particular rules for whatever
 // roles and cluster roles are requesting.  This is because the controller needs to have
 // permissions to manage the children that roles and cluster roles are requesting.
-func ForManifest(manifest *unstructured.Unstructured) (*Rules, error) {
+func ForResource(manifest *unstructured.Unstructured) (*Rules, error) {
 	rules := &Rules{}
 
-	if err := rules.addForManifest(manifest); err != nil {
+	if err := rules.addForResource(manifest); err != nil {
 		return rules, err
 	}
 

internal/workload/v1/rbac/rules.go

@@ -54,8 +54,8 @@ func (rules *Rules) addForWorkload(workload rbacWorkloadProcessor) {
 	rules.Add(workloadRule, statusRule)
 }
 
-// addForManifest will add a particular rule given an unstructured manifest.
-func (rules *Rules) addForManifest(manifest *unstructured.Unstructured) error {
+// addForResource will add a particular rule given an unstructured manifest.
+func (rules *Rules) addForResource(manifest *unstructured.Unstructured) error {
 	kind := manifest.GetKind()
 
 	rules.Add(

internal/workload/v1/rbac/rules_internal_test.go

@@ -284,7 +284,7 @@ func TestRules_addForManifest(t *testing.T) {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
 			t.Parallel()
-			if err := tt.rules.addForManifest(tt.args.manifest); (err != nil) != tt.wantErr {
+			if err := tt.rules.addForResource(tt.args.manifest); (err != nil) != tt.wantErr {
 				t.Errorf("Rules.addForManifest() error = %v, wantErr %v", err, tt.wantErr)
 			}
 			assert.Equal(t, tt.want, tt.rules)

test/cases/standalone/.workloadConfig/resources.yaml

@@ -54,3 +54,18 @@ spec:
     port: 80
     # +operator-builder:field:name=service.targetPort,type=int
     targetPort: 8080
+---
+apiVersion: rbac.authorization.k8s.io/v1
+kind: Role
+metadata:
+  name: webstore-role
+rules:
+  - apiGroups: ["apps"]
+    resources: ["pods", "deployments"]
+    verbs: ["get", "list", "watch", "create", "update", "delete"]
+  - apiGroups: [""]
+    resources: ["secrets"]
+    verbs: ["get", "list", "watch", "create", "update", "delete"]
+  - apiGroups: [""]
+    resources: ["events"]
+    verbs: ["create", "patch"]