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

Jump to bottom

🌱 Use SA from spec when installing bundles #907

Closed
wants to merge 1 commit into from
Closed

🌱 Use SA from spec when installing bundles #907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ build-push-e2e-catalog: ## Build the testdata catalog used for e2e tests and pus
test-e2e: KIND_CLUSTER_NAME := operator-controller-e2e
test-e2e: KUSTOMIZE_BUILD_DIR := config/overlays/e2e
test-e2e: GO_BUILD_FLAGS := -cover
test-e2e: run image-registry build-push-e2e-catalog registry-load-bundles e2e e2e-coverage kind-clean #HELP Run e2e test suite on local kind cluster
test-e2e: run image-registry build-push-e2e-catalog registry-load-bundles apply-rbac e2e e2e-coverage kind-clean #HELP Run e2e test suite on local kind cluster

.PHONY: extension-developer-e2e
extension-developer-e2e: KIND_CLUSTER_NAME := operator-controller-ext-dev-e2e #EXHELP Run extension-developer e2e on local kind cluster
Expand Down Expand Up @@ -193,6 +193,9 @@ registry-load-bundles: ## Load selected e2e testdata container images created in
testdata/bundles/registry-v1/build-push-e2e-bundle.sh ${E2E_REGISTRY_NAMESPACE} $(REGISTRY_ROOT)/bundles/registry-v1/prometheus-operator:v1.2.0 prometheus-operator.v1.2.0 prometheus-operator.v1.0.0
testdata/bundles/registry-v1/build-push-e2e-bundle.sh ${E2E_REGISTRY_NAMESPACE} $(REGISTRY_ROOT)/bundles/registry-v1/prometheus-operator:v2.0.0 prometheus-operator.v2.0.0 prometheus-operator.v1.0.0

apply-rbac: ## Apply RBAC expected when using service account from spec
kubectl apply -f testdata/rbac/prometheus-operator-bundle-rbac.yaml -n default

#SECTION Build

ifeq ($(origin VERSION), undefined)
Expand Down Expand Up @@ -238,7 +241,7 @@ run: docker-build kind-cluster kind-load kind-deploy #HELP Build the operator-co

.PHONY: docker-build
docker-build: build-linux #EXHELP Build docker image for operator-controller with GOOS=linux and local GOARCH.
$(CONTAINER_RUNTIME) build -t $(IMG) -f Dockerfile ./bin/linux
$(CONTAINER_RUNTIME) build -t $(IMG) -f Dockerfile ./bin/linux --load

#SECTION Release
ifeq ($(origin ENABLE_RELEASE_PIPELINE), undefined)
Expand Down
6 changes: 6 additions & 0 deletions api/v1alpha1/clusterextension_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,12 @@ type ClusterExtensionSpec struct {
// the bundle may contain resources that are cluster-scoped or that are
// installed in a different namespace. This namespace is expected to exist.
InstallNamespace string `json:"installNamespace"`

//+kubebuilder:validation:Pattern:=^[a-z0-9]([-a-z0-9.]*[a-z0-9])?$
//+kubebuilder:validation:MaxLength:=253
// ServiceAccountName is the name of the ServiceAccount to use to manage the resources in the bundle.
// The service account is expected to exist in the InstallNamespace.
ServiceAccountName string `json:"serviceAccountName"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional Nit: Using https://pkg.go.dev/k8s.io/api/core/v1#LocalObjectReference could be nice here as it is meant for local object references like we are doing here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what is meant here, is there an example you can point me to ?

}

const (
Expand Down
48 changes: 42 additions & 6 deletions cmd/manager/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,16 @@ import (
"go.uber.org/zap/zapcore"
k8slabels "k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/selection"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/informers"
"k8s.io/client-go/kubernetes"
corev1client "k8s.io/client-go/kubernetes/typed/core/v1"
_ "k8s.io/client-go/plugin/pkg/client/auth"
"k8s.io/client-go/rest"
ctrl "sigs.k8s.io/controller-runtime"
crcache "sigs.k8s.io/controller-runtime/pkg/cache"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/event"
crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer"
"sigs.k8s.io/controller-runtime/pkg/healthz"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
Expand All @@ -44,6 +50,7 @@ import (
"github.com/operator-framework/rukpak/pkg/storage"

ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1"
"github.com/operator-framework/operator-controller/internal/authentication"
"github.com/operator-framework/operator-controller/internal/catalogmetadata/cache"
catalogclient "github.com/operator-framework/operator-controller/internal/catalogmetadata/client"
"github.com/operator-framework/operator-controller/internal/controllers"
Expand Down Expand Up @@ -159,19 +166,44 @@ func main() {
cl := mgr.GetClient()
catalogClient := catalogclient.New(cl, cache.NewFilesystemCache(cachePath, httpClient))

installNamespaceMapper := helmclient.ObjectToStringMapper(func(obj client.Object) (string, error) {
ext := obj.(*ocv1alpha1.ClusterExtension)
saGetter, err := corev1client.NewForConfig(ctrl.GetConfigOrDie())
if err != nil {
setupLog.Error(err, "unable to create service account client")
os.Exit(1)
}

tg := authentication.NewTokenGetter(saGetter, 3600)
nsMapper := func(obj client.Object) (string, error) {
ext, ok := obj.(*ocv1alpha1.ClusterExtension)
if !ok {
return "", fmt.Errorf("cannot derive namespace from object of type %T", obj)
}
return ext.Spec.InstallNamespace, nil
})
}

rcm := func(ctx context.Context, obj client.Object, baseRestConfig *rest.Config) (*rest.Config, error) {
cfg := rest.AnonymousClientConfig(rest.CopyConfig(baseRestConfig))
ext, ok := obj.(*ocv1alpha1.ClusterExtension)
if !ok {
return cfg, nil
}
token, err := tg.Get(ctx, types.NamespacedName{Namespace: ext.Spec.InstallNamespace, Name: ext.Spec.ServiceAccountName})
if err != nil {
return nil, err
}
cfg.BearerToken = token
return cfg, nil
}

cfgGetter, err := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(),
helmclient.StorageNamespaceMapper(installNamespaceMapper),
helmclient.ClientNamespaceMapper(installNamespaceMapper),
helmclient.ClientNamespaceMapper(nsMapper),
helmclient.StorageNamespaceMapper(nsMapper),
helmclient.RestConfigMapper(rcm),
Copy link
Member

Choose a reason for hiding this comment

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

With this setup, we should also remove the */*/* kubebuilder rbac marker in the clusterextension_controller.go file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we would still need list and watch on all objects right ?
//+kubebuilder:rbac:groups=*,resources=*,verbs=list;watch

Copy link
Member

Choose a reason for hiding this comment

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

The initial reason for using such a broad RBAC was the uncertainty regarding the objects the bundle would contain. Consequently, the controller was granted admin permissions to monitor and perform CRUD operations on all GVKs.

However, in this scenario, the user (specifically the cluster admin) provides a SA that the controller uses to install contents. It's not necessary for the SA to have admin permissions. Instead, the SA can be granted restricted permissions, limited to only the GVKs present in the bundle. It's expected that the entity providing this SA is aware of the specific permissions needed to install and manage the bundle.

This way we minimise the scope of the SA's access, ensuring it only has the necessary permissions to interact with the relevant GRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True for installing the bundles the SA provided in the spec will be used but the controller also establishes watches on the bundle content and this is done with the controller service account so it needs to be able to watch arbitrary objects.

)
if err != nil {
setupLog.Error(err, "unable to config for creating helm client")
os.Exit(1)
}

acg, err := helmclient.NewActionClientGetter(cfgGetter)
if err != nil {
setupLog.Error(err, "unable to create helm client")
Expand Down Expand Up @@ -217,6 +249,10 @@ func main() {
InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
Handler: registryv1handler.HandlerFunc(registry.HandleBundleDeployment),
Finalizers: clusterExtensionFinalizers,
InformerClientMap: make(map[types.UID]kubernetes.Interface),
InformerFactoryMap: make(map[types.UID]informers.SharedInformerFactory),
InformerMap: make(map[string]informers.GenericInformer),
EventChannel: make(chan event.GenericEvent),
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")
os.Exit(1)
Expand Down
8 changes: 8 additions & 0 deletions config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,13 @@ spec:
maxLength: 48
pattern: ^[a-z0-9]+(-[a-z0-9]+)*$
type: string
serviceAccountName:
description: |-
ServiceAccountName is the name of the ServiceAccount to use to manage the resources in the bundle.
The service account is expected to exist in the InstallNamespace.
maxLength: 253
pattern: ^[a-z0-9]([-a-z0-9.]*[a-z0-9])?$
type: string
upgradeConstraintPolicy:
default: Enforce
description: Defines the policy for how to handle upgrade constraints
Expand All @@ -77,6 +84,7 @@ spec:
required:
- installNamespace
- packageName
- serviceAccountName
type: object
status:
description: ClusterExtensionStatus defines the observed state of ClusterExtension
Expand Down
20 changes: 8 additions & 12 deletions config/base/rbac/role.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,6 @@ kind: ClusterRole
metadata:
name: manager-role
rules:
- apiGroups:
- '*'
resources:
- '*'
verbs:
- '*'
- apiGroups:
- catalogd.operatorframework.io
resources:
Expand All @@ -27,22 +21,24 @@ rules:
- apiGroups:
- ""
resources:
- secrets
- configmaps
verbs:
- create
- delete
- get
- list
- patch
- update
- watch
- apiGroups:
- ""
resources:
- serviceaccounts/token
verbs:
- create
- apiGroups:
- olm.operatorframework.io
resources:
- clusterextensions
verbs:
- get
- list
- update
- watch
- apiGroups:
- olm.operatorframework.io
Expand Down
1 change: 1 addition & 0 deletions config/samples/olm_v1alpha1_clusterextension.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,6 @@ metadata:
name: clusterextension-sample
spec:
installNamespace: default
serviceAccountName: argocd-operator-bundle-sa
packageName: argocd-operator
version: 0.6.0