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.

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

Run workflows in provider namespaces #406

Open
grahamia opened this issue Dec 5, 2024 · 4 comments · May be fixed by #428
Open

Run workflows in provider namespaces #406

grahamia opened this issue Dec 5, 2024 · 4 comments · May be fixed by #428
Assignees
Labels
enhancement New feature or request

Comments

@grahamia
Copy link
Contributor

grahamia commented Dec 5, 2024

Is your feature request related to a problem? Please describe.
Currently, any calls to Reconcile which trigger workflows to run, i.e. new pipeline resource created the generated workflow runs within the kfp-operator-system namespace. To allow for segregation between providers it should be possible to run the workflows within a provider specific namespace, therefore allowing for separate management of the provider namespaces along with the service account setup therefore making it possible to apply correct security to data access etc between providers.

Describe the solution you'd like
On a KFP Operator user resource being created/updated/deleted and triggering a reconcile it should be possible to set the generated workflow to run within the provider namespace rather than al providers workflows running within the same namespace (kfp-operator-system by default)

Currently all workflows are generated in the configured namespace:

Namespace:    workflows.Config.WorkflowNamespace

in the KfpControllerConfigSpec

This configuration value could be made optional and if not set then the namespace of the provider resource can be used. Keeping this configuration value allows users to configure Argo Workflows to only run in a specific namespace, e.g. for security reasons.

KFP Operator service account will need the necessary access permissions to create the resource in the namespace.

@grahamia
Copy link
Contributor Author

grahamia commented Dec 5, 2024

Solution should still allow for simple deployments with all resources etc in the single namespace.

@grahamia grahamia added the enhancement New feature or request label Dec 5, 2024
@aidandunlop aidandunlop linked a pull request Jan 8, 2025 that will close this issue
2 tasks
@aidandunlop
Copy link
Contributor

aidandunlop commented Jan 8, 2025

We need to work out how resource controllers will know which namespace to load the requested Provider resource from. At the moment it's assumed that the Provider is deployed under the same namespace as the workflows:

provider, err := r.loadProvider(ctx, r.Config.WorkflowNamespace, pipeline.Spec.Provider)

After this card, the workflows are to be created under the Provider namespace if Config.WorkflowNamespace is not set, but the Providers themselves are loaded from Config.WorkflowNamespace?

We might need to update the resources to specify the namespace of the Provider as well as the provider name

Update: We will update spec.Provider in the appropriate resource to be of type NamespacedName such that we can determine the correct namespace for the provider. This can be parsed as a string so won't be a breaking change

@TobyPinfold
Copy link
Contributor

TobyPinfold commented Jan 10, 2025

Running common workflow templates from a different namespace requires the workflows to be ClusterWorkflowTemplates. This makes the scope visible across the cluster but also brings its own challenges with service account usage.

In order for worklfows to be run within a providers given namespace it requires a few pre-requisites:

  1. There exists a Role for argo to use to create necessary pods:
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: workflow-executor
namespace: provider-namespace
rules:
- apiGroups:
- ""
resources:
- pods
verbs:
- get
- patch
  1. There exists a Service Account for the provider.
apiVersion: v1
kind: ServiceAccount
metadata:
 name: my-provider-service-account
 namespace: provider-namespace
  1. There exists a RoleBinding of the above Role to the Provider Service account.
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
  name: kfp-operator-providers-workflow-executor-rolebinding
  namespace: provider-namespace
roleRef:
  apiGroup: rbac.authorization.k8s.io
  kind: Role
  name: workflow-executor
subjects:
- kind: ServiceAccount
  name: my-provider-service-account
  namespace: provider-namespace
  1. Upon creation of a workflow from the KFP operator controller, the input parameters should now include the service account of the provider (located within the provider CRD) that should be used to create the argo workflow pods.

Example Provider Helm:

provider:
  name: my-provider
  namespace: provider-namespace
  type: vai
  executionMode: v2
  serviceAccount:
    name: my-provider-service-account

Example Workflow Input:

apiVersion: argoproj.io/v1alpha1
kind: ClusterWorkflowTemplate
metadata:
  name: parameterized-service-account-cluster-workflow-template
spec:
  entrypoint: main
  arguments:
    parameters:
    - name: serviceAccountName
      value: default-service-account  # Default value for the service account will be overridden on creation
  1. The Workflow templates should take this new parameter and use it within each step of the template like so:
  templates:
  - name: main
    steps:
    - - name: step1
         template: run-some-task
         serviceAccountName: "{{workflow.parameters.serviceAccountName}}"

Further:

  1. With changes made within Provider Controller (Step 5) #332 there is less work done by the workflows, so it may open a discussion on the importance of this work.
  2. Alternative solution is to have templates deployed to each provider namespace though the duplication is wasteful, it might avoid issues with the cluster wide visibility.

@aidandunlop
Copy link
Contributor

Pausing this issue until #332 and #376 are complete, as those changes touch similar files and the approach will change slightly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants