feat(budpipeline): add deployment actions with internal auth support #1194
Conversation
Summary of ChangesHello @dittops, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the BudPipeline service by introducing a new action for precise deployment scaling and extensively refactoring existing deployment-related actions. It integrates internal authentication for secure service-to-service communication, particularly for autoscale endpoints, and improves the model deployment process with a "Smart Mode" that leverages simulation for optimal configuration. Additionally, the changes address a critical UI bug in the BudAdmin pipeline editor and streamline the action set by removing deprecated functionalities, all while providing detailed documentation for future action development. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to the budpipeline service, adding a new scale action and refactoring existing deployment actions for better service invocation and asynchronous handling. The introduction of internal authentication for budapp endpoints is a great security improvement, and the fix for the budadmin UI dropdown enhances usability.
My review has identified a critical security vulnerability due to hardcoded credentials in a new documentation file, which must be addressed immediately. I've also pointed out a high-risk issue with how database migrations are being run at application startup, and I've provided a recommendation for a safer approach using an init container. Additionally, there's a minor suggestion to improve code consistency in the budapp service.
Overall, the refactoring of the pipeline actions is well-executed, with robust event handling and validation. The new documentation for creating actions is also a valuable addition to the repository.
.claude/skills/bud-testing/SKILL.md
Outdated
| Admin Email: admin@bud.studio | ||
| Admin Password: Budadmin@stud!0 |
There was a problem hiding this comment.
Hardcoding credentials, especially passwords, in source code is a major security risk. The admin password Budadmin@stud!0 is exposed in this file. These credentials should be removed from the repository and managed securely using a secret management tool (e.g., Kubernetes Secrets, HashiCorp Vault, AWS Secrets Manager). The application should then be configured to read these secrets from the environment at runtime.
| Admin Email: admin@bud.studio | |
| Admin Password: Budadmin@stud!0 | |
| Admin Email: admin@bud.studio | |
| Admin Password: [REDACTED - Load from a secure secret store] |
| image: {{ .Values.microservices.budpipeline.image }} | ||
| imagePullPolicy: {{ .Values.imagePullPolicy }} | ||
| command: ["sh", "-c", "uvicorn budpipeline.main:app --host 0.0.0.0 --port 8010"] | ||
| command: ["sh", "-c", "alembic upgrade head && uvicorn budpipeline.main:app --host 0.0.0.0 --port 8010"] |
There was a problem hiding this comment.
Running database migrations (alembic upgrade head) as part of the application's startup command can be problematic in a distributed environment like Kubernetes. If multiple pods start simultaneously during a deployment or scaling event, they could all attempt to run migrations concurrently, leading to race conditions, database locks, and potentially a corrupted schema. A safer pattern is to run migrations as a separate, one-off task before the application starts.
A recommended approach is to use a Kubernetes initContainer to run the migration. This ensures the migration completes successfully once before any application containers start. Here is an example of how you could structure it:
spec:
template:
spec:
initContainers:
- name: budpipeline-migrations
image: {{ .Values.microservices.budpipeline.image }}
command: ["alembic", "upgrade", "head"]
envFrom:
- secretRef:
name: {{ .Values.secrets.dbSecretName }}
- configMapRef:
name: {{ .Values.configmaps.budConfig }}
containers:
- name: budpipeline
...
command: ["sh", "-c", "uvicorn budpipeline.main:app --host 0.0.0.0 --port 8010"] command: ["sh", "-c", "uvicorn budpipeline.main:app --host 0.0.0.0 --port 8010"]| ) | ||
|
|
||
| # Build source_topic - include callback_topic for direct budpipeline notification | ||
| source_topics = [app_settings.source_topic, callback_topic] if callback_topic else app_settings.source_topic |
There was a problem hiding this comment.
The source_topics variable is assigned either a list or a string depending on whether callback_topic is present. This can lead to inconsistent types being sent in the payload to the budcluster service, which might not be robust enough to handle both. For better type safety and maintainability, it's recommended to always use a list.
source_topics = [app_settings.source_topic]
if callback_topic:
source_topics.append(callback_topic)| app_id=settings.budapp_app_id, | ||
| method_path=f"endpoints/{endpoint_id}/delete-workflow", | ||
| http_method="POST", | ||
| data={"callback_topic": CALLBACK_TOPIC}, |
There was a problem hiding this comment.
It's good practice to include the force parameter in the payload to the downstream service if it's intended to be used. Although the parameter is not yet implemented as per the description, forwarding it now will make future implementation easier without requiring another change in this action.
| data={"callback_topic": CALLBACK_TOPIC}, | |
| data={"callback_topic": CALLBACK_TOPIC, "force": force}, |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d7dbb13de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| budaiscaler_spec: dict[str, Any] = { | ||
| "enabled": True, # Must be enabled for scaling to work | ||
| "minReplicas": target_replicas, | ||
| "maxReplicas": target_replicas, | ||
| } |
There was a problem hiding this comment.
When target_replicas is 0 (explicitly allowed/mentioned as valid), the action sends minReplicas = maxReplicas = 0. Budapp’s BudAIScalerSpecification enforces maxReplicas >= 1, so /endpoints/{id}/autoscale will reject this with a 422 and the scale action will fail on any “scale to zero” request. Either disallow 0 in validation or translate it into a supported “disable autoscaling” shape so the API accepts it.
Useful? React with 👍 / 👎.
| @require_permissions(permissions=[PermissionEnum.ENDPOINT_MANAGE]) | ||
| async def delete_endpoint( | ||
| current_user: Annotated[User, Depends(get_current_active_user)], | ||
| current_user: Annotated[User, Depends(get_current_active_user_or_internal)], | ||
| session: Annotated[Session, Depends(get_session)], |
There was a problem hiding this comment.
This route is still wrapped by require_permissions, whose dependency is get_current_active_user, so internal Dapr calls (like the new budpipeline delete action that uses service invocation without a JWT) will be rejected before your get_current_active_user_or_internal parameter ever runs. In practice, pipeline-driven deletes will 401/403 unless you switch the decorator to require_permissions_or_internal (or otherwise allow internal auth for this endpoint).
Useful? React with 👍 / 👎.
dittops
force-pushed
the
feat/pipeline-deployment-actions
branch
2 times, most recently
from
ea7b6b9 to
d30cca7
Compare
## Changes ### BudPipeline Actions - Add Scale Deployment action to set fixed replica count - Refactor Deploy Model action with simulation support and smart mode - Refactor Delete Deployment action with proper cleanup - Refactor Rate Limit action with multiple algorithm support - Remove deprecated autoscale action (replaced by scale) - Remove cluster create/delete actions (not yet implemented) - Add CREATING_ACTIONS.md documentation guide ### BudApp Internal Authentication - Add internal auth support to autoscale endpoints for pipeline calls - Add internal auth support to delete-workflow endpoint - Use require_permissions_or_internal decorator pattern - Use get_current_active_user_or_internal dependency ### BudAdmin UI - Fix empty endpoint dropdown in pipeline editor - Add endpoints to dataSources for ActionConfigPanel - Fetch endpoints when entering edit mode ### ActionContext Enhancement - Add invoke_service method for Dapr service invocation - Support dapr-api-token header for internal auth ### Helm Chart - Add alembic migration to budpipeline startup ### Fixes from Review - Fix source_topics type consistency (always use list) - Add force param to delete action - Update cluster tests for removed actions - Fix scale to 0: ensure maxReplicas >= 1 per schema constraint Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
dittops
force-pushed
the
feat/pipeline-deployment-actions
branch
from
d30cca7 to
9940b41
Compare
…elated configurations
…nt request assertions
…ormat
The health.py action expects response in format {"data": {...}} but the
tests were mocking {"health": {...}}. This caused the test to fail because
the code would not find any health data and mark all checks as unavailable.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Update deployment action tests to use endpoint_id instead of deployment_id - Update test mocks to properly simulate successful execution - Update action count expectation from 20 to 17 (actions were consolidated) - Add credential_ref to expected ParamType values Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
Changes
BudPipeline Actions
minReplicas = maxReplicas = target_replicasCREATING_ACTIONS.mddocumentation guideBudApp Internal Authentication
/endpoints/{id}/autoscale)require_permissions_or_internaldecorator patternget_current_active_user_or_internaldependencyBudAdmin UI
endpointstodataSourcesforActionConfigPanelActionContext Enhancement
invoke_service()method for Dapr service invocationdapr-api-tokenheader for internal authenticationTest plan
🤖 Generated with Claude Code