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

[feature] Rewrite KFP profile controller to use production ready HTTP server #10904

Open
AndersBennedsgaard opened this issue Jun 14, 2024 · 8 comments

Comments

@AndersBennedsgaard
Copy link

Feature Area

/area backend

What feature would you like to see?

As the default http.server implementation in Python is not recommended for production due to its limited security features, we should rewrite the KFP profile controller to using a production-ready HTTP server.
We have for example successfully implemented the Pipelines profile controller using FastAPI.

What is the use case or pain point?

As the server doesn't implement many security checks, unauthorized users on the cluster might be able to access it, and mess around with other tenant's data.
The server is currently extremely simple and copy-pastes the same MinIO credentials to all namespaces, so the security issue might not be very critical at the moment. However, if we start doing more complicated stuff like accessing the storage layer to set up separate credentials to each namespace (see for example #7725) this becomes much more important.

Is there a workaround currently?

Continue as usual.
To increase security, we can cover the controller in NetworkPolicies, Istio AuthorizationPolicies, and other security measures, to ensure only authorized users (metacontroller) can access the server.


Love this idea? Give it a 👍.

@juliusvonkohout
Copy link
Member

It should be protected by the networkpolicies https://github.com/kubeflow/manifests/tree/master/common/networkpolicies. If not please file an issue in Kubeflow/manifests. I really like the python approach, because it is modifiable by end users.

@AndersBennedsgaard
Copy link
Author

Using FastAPI or some other Python-based code base would still make it modifiable by users, so I don't really understand that response.
However, even if it was written in Go, Rust, C, or some other compiled language, there are a programming architectures that would allow for user modifications. For example a plugin architecture where you deploy a MinIO auth plugin if you use MinIO, a Apache Ozone plugin for Ozone, and so on.

@juliusvonkohout
Copy link
Member

"Using FastAPI or some other Python-based code base would still make it modifiable by users, so I don't really understand that response." FasrAPI is what I would go for then.

@AndersBennedsgaard
Copy link
Author

Then this would require building a new image. I guess that means updating https://github.com/kubeflow/pipelines/blob/master/.cloudbuild.yaml and https://github.com/kubeflow/pipelines/blob/master/.release.cloudbuild.yaml, like mentioned here: #7725 (comment)

@juliusvonkohout
Copy link
Member

I just hope that we can keep the code in the configmap as it is now. This allows for many changes without rebuilding the image.

Copy link

This issue 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.

@github-actions github-actions bot added the lifecycle/stale The issue / pull request is stale, any activities remove this label. label Aug 18, 2024
@AndersBennedsgaard
Copy link
Author

I just hope that we can keep the code in the configmap as it is now. This allows for many changes without rebuilding the image.

Since it would be written in Python, there is no real reason for loading the main code in with ConfigMaps. If we create a custom container image for the profile controller, anyone that want to change something in the code can just write over the specific Python file in the container (even though it would be pretty bad practice)

@stale stale bot removed the lifecycle/stale The issue / pull request is stale, any activities remove this label. label Aug 18, 2024
@juliusvonkohout
Copy link
Member

I just hope that we can keep the code in the configmap as it is now. This allows for many changes without rebuilding the image.

Since it would be written in Python, there is no real reason for loading the main code in with ConfigMaps. If we create a custom container image for the profile controller, anyone that want to change something in the code can just write over the specific Python file in the container (even though it would be pretty bad practice)

You will have a higher chance of getting this merged if we keep the code in the configuration map. "If we create a custom container image for the profile controller" i think this is what we want to avoid.

/lifecycle frozen

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

No branches or pull requests

2 participants