Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -132,34 +132,6 @@ type Dependencies struct {
serviceName string,
) (name string, version string, err error)

// probeAgentPrincipal is a test seam for the
// `remote.agent-identity-roles` check (Phase 5 C12). It returns
// the agent's managed-identity principal ID by calling
// GetAgentVersion and reading `instance_identity.principal_id`.
// Production wiring leaves this nil; the check substitutes
// `makeRealProbeAgentPrincipal(deps.AgentAPIVersion)` when nil.
probeAgentPrincipal func(
ctx context.Context,
endpoint, agentName, agentVersion string,
) agentIdentityProbeResult

// queryAgentIdentityRoles is a test seam for the
// `remote.agent-identity-roles` check (Phase 5 C12). When
// non-nil it replaces the production
// `project.QueryAgentIdentityRoles` call inside the check,
// letting unit tests exercise per-agent classification
// (fine / underscoped / empty / unknown) and aggregate folding
// without instantiating real ARM clients. Signature mirrors
// `project.QueryAgentIdentityRoles` exactly so the wiring is a
// single `if query == nil { query = project.QueryAgentIdentityRoles }`
// substitution. Production wiring leaves this nil.
queryAgentIdentityRoles func(
ctx context.Context,
azdClient *azdext.AzdClient,
projectResourceID string,
principals []project.AgentPrincipal,
) (*project.AgentIdentityRolesResult, error)

// lookupToolboxEnv is a test seam for the `local.toolboxes`
// check (Phase 5 C14). When non-nil it replaces the production
// `makeRealToolboxEnvLookup` closure inside the check, letting
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,9 +60,6 @@ func NewRemoteChecks(deps Dependencies) []Check {
// (`remote.rbac`)
// - C17 (landed): per-service agent version status
// (`remote.agent-status`)
// - C12 (landed): per-agent managed-identity role
// listing across project/account/RG scopes
// (`remote.agent-identity-roles`)
// - C15 (landed): manifest connections exist on the
// Foundry project (`remote.connections`)
//
Expand All @@ -75,6 +72,12 @@ func NewRemoteChecks(deps Dependencies) []Check {
// (or the azd env) instead. See manifest.go's walker for the
// populated `state.ModelRefs` slice that the new check can reuse.
//
// Note: a `remote.agent-identity-roles` check (C12) was removed
// because the Foundry service now grants the per-agent identity
// its permissions internally, with no explicit ARM role
// assignment. Enumerating ARM role assignments therefore folded
// every agent into a false aggregate FAIL.
//
// Ordering matters for skip-cascade: each entry reads `prior
// []Result` produced by every check earlier in the combined
// local-then-remote sequence. Append checks in the order their
Expand All @@ -85,7 +88,6 @@ func NewRemoteChecks(deps Dependencies) []Check {
newCheckFoundryEndpoint(deps),
newCheckRBAC(deps),
newCheckAgentStatus(deps),
newCheckAgentIdentityRoles(deps),
newCheckConnections(deps),
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,38 +12,35 @@ import (

// ---- NewRemoteChecks contract ----

// TestNewRemoteChecks_HasAuthFoundryEndpointRBACAgentStatusIdentityRolesConnections
// pins the current shape of the remote chain: exactly six checks,
// TestNewRemoteChecks_HasAuthFoundryEndpointRBACAgentStatusConnections
// pins the current shape of the remote chain: exactly five checks,
// in the order `remote.auth` → `remote.foundry-endpoint` →
// `remote.rbac` → `remote.agent-status` → `remote.agent-identity-roles`
// → `remote.connections`, all with Remote=true. The ordering matters
// because `remote.foundry-endpoint` skip-cascades against
// `remote.auth`'s prior Result, `remote.rbac` skip-cascades against
// `remote.auth` (but NOT `remote.foundry-endpoint`, per the design's
// dependency matrix — RBAC reads ARM, not the data plane),
// `remote.rbac` → `remote.agent-status` → `remote.connections`, all
// with Remote=true. The ordering matters because
// `remote.foundry-endpoint` skip-cascades against `remote.auth`'s
// prior Result, `remote.rbac` skip-cascades against `remote.auth`
// (but NOT `remote.foundry-endpoint`, per the design's dependency
// matrix — RBAC reads ARM, not the data plane),
// `remote.agent-status` skip-cascades against `remote.auth` +
// `remote.foundry-endpoint` (Reader-level Foundry call, deliberately
// bypasses RBAC), `remote.agent-identity-roles` cascades against
// `remote.agent-status` Pass so the per-agent role enumeration only
// runs against agents the previous check confirmed active, and
// `remote.connections` cascades against `remote.auth` +
// `remote.foundry-endpoint` plus `state.HasConnections` because it
// lists Foundry connections via the data plane. Any future
// bypasses RBAC), and `remote.connections` cascades against
// `remote.auth` + `remote.foundry-endpoint` plus `state.HasConnections`
// because it lists Foundry connections via the data plane. Any future
// re-ordering or insertion has to come through this assertion.
//
// Note: a `remote.model-deployments` check used to sit between
// agent-identity-roles and connections; it was removed pending a
// redesign because its comparison was incorrect — the manifest's
// `resources[].name` is a logical alias, not a Foundry deployment
// name.
func TestNewRemoteChecks_HasAuthFoundryEndpointRBACAgentStatusIdentityRolesConnections(t *testing.T) {
// Note: a `remote.agent-identity-roles` check used to sit between
// agent-status and connections; it was removed because the Foundry
// service now grants the per-agent identity its permissions internally
// (no explicit ARM role assignment), so enumerating ARM role
// assignments produced false failures.
func TestNewRemoteChecks_HasAuthFoundryEndpointRBACAgentStatusConnections(t *testing.T) {
t.Parallel()

got := NewRemoteChecks(Dependencies{})

require.Len(t, got, 6,
require.Len(t, got, 5,
"NewRemoteChecks should contain auth, foundry-endpoint, rbac, agent-status, "+
"agent-identity-roles, and connections today")
"and connections today")
require.Equal(t, "remote.auth", got[0].ID)
require.Equal(t, "authentication", got[0].Name)
require.True(t, got[0].Remote, "remote.auth must declare Remote=true")
Expand All @@ -60,14 +57,10 @@ func TestNewRemoteChecks_HasAuthFoundryEndpointRBACAgentStatusIdentityRolesConne
require.Equal(t, "Hosted agents are active", got[3].Name)
require.True(t, got[3].Remote, "remote.agent-status must declare Remote=true")
require.NotNil(t, got[3].Fn, "remote.agent-status must have a non-nil Fn")
require.Equal(t, "remote.agent-identity-roles", got[4].ID)
require.Equal(t, "Agent identity role assignments", got[4].Name)
require.True(t, got[4].Remote, "remote.agent-identity-roles must declare Remote=true")
require.NotNil(t, got[4].Fn, "remote.agent-identity-roles must have a non-nil Fn")
require.Equal(t, "remote.connections", got[5].ID)
require.Equal(t, "Manifest connections exist on Foundry project", got[5].Name)
require.True(t, got[5].Remote, "remote.connections must declare Remote=true")
require.NotNil(t, got[5].Fn, "remote.connections must have a non-nil Fn")
require.Equal(t, "remote.connections", got[4].ID)
require.Equal(t, "Manifest connections exist on Foundry project", got[4].Name)
require.True(t, got[4].Remote, "remote.connections must declare Remote=true")
require.NotNil(t, got[4].Fn, "remote.connections must have a non-nil Fn")
}

// TestNewLocalAndRemoteChecks_ProductionCompositionLocalsFirst pins the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,9 @@ func fixedAssembler(
}

// fixedProjectIDReader returns a readProjectResourceIDFn that yields
// the supplied id (or error) on every call. Mirrors the rbac /
// agent-identity-roles test pattern so checks that derive an ARM
// scope from AZURE_AI_PROJECT_ID can be exercised without a real
// azd env.
// the supplied id (or error) on every call. Mirrors the rbac test
// pattern so checks that derive an ARM scope from AZURE_AI_PROJECT_ID
// can be exercised without a real azd env.
func fixedProjectIDReader(
id string, err error,
) func(context.Context, *azdext.AzdClient) (string, error) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ const (
// StatusInfo — the check completed cleanly but the result is
// primarily informational (no action required, no problem detected).
// Used by checks whose value is the listing they produce rather than
// a pass/fail verdict — e.g., `remote.agent-identity-roles` renders
// the agent's role assignments so the user can confirm they match
// their mental model. Does NOT contribute to a non-zero exit code.
// a pass/fail verdict — e.g., a check that renders discovered
// resources so the user can confirm they match their mental model.
// Does NOT contribute to a non-zero exit code.
StatusInfo Status = "info"
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,7 @@ func remediationForCheckID(id string) (remediation, bool) {
case "remote.auth":
return remediation{command: "azd auth login", desc: "sign in to Azure", order: 10}, true
case "remote.foundry-endpoint",
"remote.connections",
"remote.agent-identity-roles":
"remote.connections":
return remediation{
command: "azd provision",
desc: "create the missing Foundry resources",
Expand Down
71 changes: 21 additions & 50 deletions cli/azd/extensions/azure.ai.agents/internal/cmd/listen.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"sync"

"azureaiagent/internal/exterrors"
"azureaiagent/internal/pkg/agents/agent_api"
"azureaiagent/internal/pkg/agents/optimize_api"
"azureaiagent/internal/project"

Expand Down Expand Up @@ -295,13 +294,15 @@ func postdeployHandler(ctx context.Context, azdClient *azdext.AzdClient, args *a
return nil
}

// Collect agent identity from the hosted agent service that was deployed.
// After deploy, each hosted agent's name/version is stored as AGENT_{SERVICE_KEY}_NAME/VERSION.
// We fetch the full agent version object from the API to get the instance identity principal ID,
// which allows us to skip the slow Graph API discovery during RBAC assignment.
// Set up the project endpoint and credential used by optimization reporting.
Comment thread
m5i-work marked this conversation as resolved.
// This path now feeds only best-effort optimization telemetry (the client-side
// agent-identity RBAC assignment was removed), so any setup failure is logged as
// a warning and skipped rather than failing an otherwise-successful deploy.
envResp, err := azdClient.Environment().GetCurrent(ctx, &azdext.EmptyRequest{})
if err != nil {
return fmt.Errorf("failed to get current environment for agent identity RBAC: %w", err)
log.Printf("postdeploy: skipping optimization reporting for %s: "+
"failed to get current environment: %v", svc.Name, err)
return nil
}

envName := envResp.Environment.Name
Expand All @@ -312,10 +313,14 @@ func postdeployHandler(ctx context.Context, azdClient *azdext.AzdClient, args *a
Key: "FOUNDRY_PROJECT_ENDPOINT",
})
if err != nil {
return fmt.Errorf("failed to read FOUNDRY_PROJECT_ENDPOINT: %w", err)
log.Printf("postdeploy: skipping optimization reporting for %s: "+
"failed to read FOUNDRY_PROJECT_ENDPOINT: %v", svc.Name, err)
return nil
}
if endpointResp.Value == "" {
return fmt.Errorf("FOUNDRY_PROJECT_ENDPOINT is not set in the environment")
log.Printf("postdeploy: skipping optimization reporting for %s: "+
"FOUNDRY_PROJECT_ENDPOINT is not set in the environment", svc.Name)
return nil
}

// Create a credential for API calls.
Expand All @@ -324,10 +329,14 @@ func postdeployHandler(ctx context.Context, azdClient *azdext.AzdClient, args *a
Key: "AZURE_TENANT_ID",
})
if err != nil {
return fmt.Errorf("failed to read AZURE_TENANT_ID: %w", err)
log.Printf("postdeploy: skipping optimization reporting for %s: "+
"failed to read AZURE_TENANT_ID: %v", svc.Name, err)
return nil
}
if tenantResp.Value == "" {
return fmt.Errorf("AZURE_TENANT_ID is not set in the environment")
log.Printf("postdeploy: skipping optimization reporting for %s: "+
"AZURE_TENANT_ID is not set in the environment", svc.Name)
return nil
}

cred, err := azidentity.NewAzureDeveloperCLICredential(
Expand All @@ -337,49 +346,11 @@ func postdeployHandler(ctx context.Context, azdClient *azdext.AzdClient, args *a
},
)
if err != nil {
return fmt.Errorf("failed to create credential for agent identity RBAC: %w", err)
}

agentClient := agent_api.NewAgentClient(endpointResp.Value, cred)

// Fetch the agent version to get the instance identity principal ID.
serviceKey := toServiceKey(svc.Name)

versionResp, err := azdClient.Environment().GetValue(ctx, &azdext.GetEnvRequest{
EnvName: envName,
Key: fmt.Sprintf("AGENT_%s_VERSION", serviceKey),
})
if err != nil {
return fmt.Errorf(
"failed to read AGENT_%s_VERSION from environment: %w",
serviceKey, err,
)
}
if versionResp.Value == "" {
log.Printf("postdeploy: skipping optimization reporting for %s: "+
"failed to create credential: %v", svc.Name, err)
return nil
}

versionObj, err := agentClient.GetAgentVersion(
ctx, svc.Name, versionResp.Value, DefaultAgentAPIVersion,
)
if err != nil {
return fmt.Errorf(
"failed to fetch agent version for %s/%s: %w",
svc.Name, versionResp.Value, err,
)
}

principalID := ""
if versionObj.InstanceIdentity != nil {
principalID = versionObj.InstanceIdentity.PrincipalID
}

agentIdentities := map[string]string{svc.Name: principalID}

if err := project.EnsureAgentIdentityRBAC(ctx, azdClient, agentIdentities); err != nil {
return fmt.Errorf("agent identity RBAC setup failed: %w", err)
}

// Report optimization candidate deployment (best-effort: panics are logged, not propagated).
func() {
defer func() {
Expand Down
65 changes: 65 additions & 0 deletions cli/azd/extensions/azure.ai.agents/internal/cmd/listen_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,71 @@ func TestPostdeployHandler_NonHostedAgent_NoOp(t *testing.T) {
}
}

// TestPostdeployHandler_MissingTelemetryEnv_ReturnsNil verifies that a hosted
// agent whose environment is missing the optional telemetry inputs
// (FOUNDRY_PROJECT_ENDPOINT / AZURE_TENANT_ID) does NOT fail the post-deploy
// hook. Since the client-side agent-identity RBAC assignment was removed, this
// endpoint/tenant/credential setup now feeds only best-effort optimization
// reporting, so a missing value is logged and skipped rather than propagated as
// an error that would fail an otherwise-successful deploy (PR #8941 follow-up).
func TestPostdeployHandler_MissingTelemetryEnv_ReturnsNil(t *testing.T) {
t.Parallel()

// Lay down a minimal hosted agent.yaml so isHostedAgentService returns true
// and the handler proceeds past the non-hosted early return.
projectRoot := t.TempDir()
serviceDir := filepath.Join(projectRoot, "echo")
if err := os.MkdirAll(serviceDir, 0o750); err != nil {
t.Fatalf("failed to create service dir: %v", err)
}
if err := os.WriteFile(
filepath.Join(serviceDir, "agent.yaml"),
[]byte("kind: hostedAgent\nname: echo\n"), 0o600); err != nil {
t.Fatalf("failed to write agent.yaml: %v", err)
}

tests := []struct {
name string
values map[string]map[string]string
}{
{
name: "FOUNDRY_PROJECT_ENDPOINT not set",
values: nil, // GetValue returns NotFound for every key
},
{
name: "FOUNDRY_PROJECT_ENDPOINT empty",
values: map[string]map[string]string{"dev": {"FOUNDRY_PROJECT_ENDPOINT": ""}},
},
{
name: "AZURE_TENANT_ID not set",
values: map[string]map[string]string{
"dev": {"FOUNDRY_PROJECT_ENDPOINT": "https://example.services.ai.azure.com/api/projects/p"},
},
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
t.Parallel()

envServer := &testEnvironmentServiceServer{
current: &azdext.Environment{Name: "dev"},
values: tt.values,
}
azdClient := newTestAzdClient(t, envServer, &testWorkflowServiceServer{})

args := &azdext.ServiceEventArgs{
Project: &azdext.ProjectConfig{Path: projectRoot},
Service: &azdext.ServiceConfig{Name: "echo", Host: AiAgentHost, RelativePath: "echo"},
}

if err := postdeployHandler(t.Context(), azdClient, args); err != nil {
t.Fatalf("expected nil (best-effort telemetry setup must not fail deploy), got: %v", err)
}
})
}
}

func TestIsHostedAgentServiceRejectsTraversal(t *testing.T) {
t.Parallel()

Expand Down
Loading
Loading