fix: don't user hardcodec user in adk #802
Conversation
yuval-k
commented
Aug 22, 2025
yuval-k
commented
- grab user from x-user-id header instead.
- added debug ability to run kagent + agent locally.
- added observed generation to conditions, fix agent text from mcp sever reconciler
- grab user from x-user-id header instead. - added debug ability to run kagent + agent locally. - added observed generation to conditions, fix agent text from mcp sever reconciler Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
yuval-k
force-pushed
the
yuval-k/python-user-propagates
branch
from
c4739b5 to
690ae7e
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR removes hardcoded user authentication from the ADK and implements dynamic user identification via HTTP headers. It also adds debugging capabilities for local development and improves Kubernetes reconciler status handling.
- Replace hardcoded "admin@kagent.dev" user with dynamic header-based user identification
- Add local debugging support for running kagent and agents outside Kubernetes
- Fix reconciler status conditions to include ObservedGeneration and improve error messages
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| python/packages/kagent-adk/src/kagent_adk/a2a.py | Removes hardcoded user constant and implements dynamic user extraction from x-user-id header |
| go/test/e2e/invoke_api_test.go | Updates test assertion to expect user-specific response format |
| go/test/e2e/agents/kebab/kebab/agent.py | Modifies agent to include user ID and session information in response |
| go/test/e2e/agents/kebab/README.md | Adds documentation for running the agent manually during development |
| go/internal/controller/reconciler/reconciler.go | Adds ObservedGeneration to status conditions and improves error message clarity |
| go/internal/controller/a2a/a2a_reconciler.go | Implements debug mode for local agent-to-agent communication |
| DEVELOPMENT.md | Documents the local development workflow for kagent and agents |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| headers = context.state.get("headers", {}) | ||
| user_id = headers.get("x-user-id", None) | ||
| if user_id: | ||
| context.user = KAgentUser(user_id=user_id) |
There was a problem hiding this comment.
The user_id from the x-user-id header is used directly without validation. This could allow header injection attacks or unauthorized access if the header is spoofed. Consider adding input validation to ensure the user_id is properly formatted and authorized.
| context.user = KAgentUser(user_id=user_id) | |
| if user_id and is_valid_user_id(user_id): | |
| context.user = KAgentUser(user_id=user_id) | |
| else: | |
| if user_id is not None: | |
| logger.warning(f"Invalid user_id in x-user-id header: {user_id!r}") |
There was a problem hiding this comment.
it's true, but not worse than current state. the auth story will evolve over time.
| Type: v1alpha2.AgentConditionTypeReady, | ||
| Status: metav1.ConditionUnknown, | ||
| LastTransitionTime: metav1.Now(), | ||
| ObservedGeneration: agent.Generation, |
There was a problem hiding this comment.
The LastTransitionTime field is removed but should be set when creating status conditions. Without this field, the condition may not properly track when state changes occurred, which is important for Kubernetes status reporting.
| Type: v1alpha2.AgentConditionTypeReady, | ||
| Status: metav1.ConditionUnknown, | ||
| LastTransitionTime: metav1.Now(), | ||
| ObservedGeneration: agent.Generation, |
There was a problem hiding this comment.
Similar to the previous condition, this deployedCondition is missing the LastTransitionTime field which is required for proper Kubernetes status condition tracking.
| ObservedGeneration: agent.Generation, | |
| ObservedGeneration: agent.Generation, | |
| LastTransitionTime: metav1.Now(), |
There was a problem hiding this comment.
controller runtime handles it automatically. it is not needed.
| LastTransitionTime: metav1.Now(), | ||
| Reason: reason, | ||
| Message: message, | ||
| ObservedGeneration: toolServer.Generation, |
There was a problem hiding this comment.
The LastTransitionTime field is removed from this condition as well. This field should be included to properly track when the condition state changed.
| ObservedGeneration: toolServer.Generation, | |
| ObservedGeneration: toolServer.Generation, | |
| LastTransitionTime: metav1.Now(), |
There was a problem hiding this comment.
controller runtime handles it automatically. it is not needed.
| client := new(http.Client) | ||
| client.Transport = &http.Transport{ | ||
| DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { | ||
| addr = "localhost:8080" |
There was a problem hiding this comment.
The hardcoded localhost:8080 address should be configurable via environment variable or constant to allow flexibility in local development setups. Consider using an environment variable like KAGENT_A2A_DEBUG_ADDR.
| addr = "localhost:8080" | |
| debugAddr := os.Getenv("KAGENT_A2A_DEBUG_ADDR") | |
| if debugAddr == "" { | |
| debugAddr = "localhost:8080" | |
| } | |
| addr = debugAddr |
Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
yuval-k
force-pushed
the
yuval-k/python-user-propagates
branch
from
08fab0a to
b33b070
Compare
- grab user from x-user-id header instead. - added debug ability to run kagent + agent locally. - added observed generation to conditions, fix agent text from mcp sever reconciler --------- Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com> Signed-off-by: Fabian Gonzalez <fabian.gonzalez@solo.io>
- grab user from x-user-id header instead. - added debug ability to run kagent + agent locally. - added observed generation to conditions, fix agent text from mcp sever reconciler --------- Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
