From b1cf34f59d9d9c8ec683d6c5caedc88596e349d2 Mon Sep 17 00:00:00 2001 From: Copilot Date: Thu, 2 Jul 2026 17:27:42 +0800 Subject: [PATCH 1/2] Stop assigning Azure AI User role to per-agent identity The Foundry service now grants each hosted agent's per-agent managed identity its required permissions internally, so the client-side Azure AI User role assignment in the post-deploy handler was redundant. It also failed noisily when the deploying user lacked roleAssignments/write, blocking deploys for users holding only data-plane roles. Remove the post-deploy role-assignment path and the now-false remote.agent-identity-roles doctor check, which enumerated ARM role assignments the service no longer creates and therefore reported false failures. Fixes #8940 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../cmd/doctor/checks_agent_identity_roles.go | 655 ------------------ .../checks_agent_identity_roles_test.go | 562 --------------- .../internal/cmd/doctor/checks_local.go | 28 - .../internal/cmd/doctor/checks_remote.go | 10 +- .../internal/cmd/doctor/checks_remote_test.go | 53 +- .../internal/cmd/doctor/shared_test.go | 7 +- .../internal/cmd/doctor/types.go | 6 +- .../internal/cmd/doctor_format.go | 3 +- .../azure.ai.agents/internal/cmd/listen.go | 50 +- .../internal/project/agent_identity_query.go | 335 --------- .../internal/project/agent_identity_rbac.go | 263 +------ 11 files changed, 40 insertions(+), 1932 deletions(-) delete mode 100644 cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_agent_identity_roles.go delete mode 100644 cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_agent_identity_roles_test.go delete mode 100644 cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_query.go diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_agent_identity_roles.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_agent_identity_roles.go deleted file mode 100644 index a223d1b01bc..00000000000 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_agent_identity_roles.go +++ /dev/null @@ -1,655 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -package doctor - -import ( - "context" - "errors" - "fmt" - "sort" - "strings" - "sync" - "time" - - "azureaiagent/internal/pkg/agents/agent_api" - "azureaiagent/internal/project" - - "github.com/Azure/azure-sdk-for-go/sdk/azcore" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" -) - -// agentIdentityProbeTimeout caps each per-agent principal-ID fetch. -// Matches the agent-status probe's timeout (6 s) because the same -// `GetAgentVersion` endpoint serves both — a project that's -// returning principal IDs within budget for check 11 will do the -// same here. Held shorter than the overall check budget so a single -// stalled agent doesn't drag the doctor past the design's per-check -// 6 s ceiling. -const agentIdentityProbeTimeout = 6 * time.Second - -// agentIdentityConcurrency bounds the per-agent fan-out used to -// fetch principal IDs in parallel. Matches probeConcurrency from -// the agent-status check; same rate-limit reasoning applies (Foundry -// per-token rate cap on the agents endpoint). -const agentIdentityConcurrency = 4 - -// agentIdentityClass values bucket a single agent's per-scope role -// inventory into a coarse class the aggregate folder consumes. -// Ordering used by `agentIdentityClassRank` is encoded by the int -// values: higher = worse for the aggregate. -const ( - agentIdentityClassFine = "fine" // project + (account|RG) → pass condition met - agentIdentityClassUnderscoped = "underscoped" // assignments somewhere but pass condition unmet - agentIdentityClassEmpty = "empty" // zero assignments anywhere reachable - agentIdentityClassUnknown = "unknown" // probe error — principal fetch failed -) - -// agentIdentityClassRank gives a strict ordering so the aggregate -// classifier can pick the dominant class without ambiguity. Higher -// values win. -var agentIdentityClassRank = map[string]int{ - agentIdentityClassFine: 0, - agentIdentityClassUnknown: 1, - agentIdentityClassUnderscoped: 2, - agentIdentityClassEmpty: 3, -} - -// agentIdentityRoleEntry is the unit the per-agent classifier -// produces. Surfaced verbatim under `Details["agents"]` for JSON -// consumers; aggregate Message picks the worst-class agent's name -// for headline rendering. -type agentIdentityRoleEntry struct { - AgentName string `json:"agentName"` - AgentVersion string `json:"agentVersion,omitempty"` - PrincipalID string `json:"principalId,omitempty"` - ProjectRoles []string `json:"projectRoles"` - AccountRoles []string `json:"accountRoles"` - RGRoles []string `json:"resourceGroupRoles"` - - // Errors per scope. nil = success (including legitimate empty - // list); non-nil = probe failure (the listing is unknown). - ProjectErr string `json:"projectErr,omitempty"` - AccountErr string `json:"accountErr,omitempty"` - RGErr string `json:"resourceGroupErr,omitempty"` - - Class string `json:"class"` - Detail string `json:"detail"` -} - -// agentIdentityProbeResult is the outcome of a single -// GetAgentVersion call used purely to extract -// `instance_identity.principal_id`. Distinct from -// agentStatusProbeResult because callers care about a different -// field (PrincipalID vs Status); keeping the type narrow protects -// against future drift. -type agentIdentityProbeResult struct { - PrincipalID string - StatusCode int - Err error -} - -// newCheckAgentIdentityRoles produces Check -// `remote.agent-identity-roles`. For each agent in the prior -// `remote.agent-status` Details that was classified Active, the -// check fetches the agent's `instance_identity.principal_id` and -// lists role assignments at three scopes (project, account, RG). -// Per-agent classification: -// -// - fine — project ≥ 1 and (account ≥ 1 OR RG ≥ 1) → -// contributes to an aggregate INFO. Pass condition per design. -// - underscoped — assignments somewhere but pass condition unmet -// (e.g., project only). Aggregate folds onto WARN. -// - empty — zero assignments anywhere reachable. Aggregate -// folds onto FAIL. -// - unknown — probe error during principal fetch or listing. -// Single-occurrence on its own is rendered as WARN; aggregated -// with empty entries it does not lower the FAIL severity. -// -// Aggregate rules: -// -// - All "fine" → INFO (informational role listing). -// - Any "empty" → FAIL (zero-roles agent is the -// smoking-gun for "every tool call 403s"). -// - Worst is "underscoped" → WARN. -// - Worst is "unknown" → WARN (probe couldn't classify). -// -// Skip cascade: -// - `remote.agent-status` must Pass (per the design, "for each -// active agent found in check 11"). -// - `local.environment-selected`, `local.agent-service-detected`, -// `remote.auth`, `remote.foundry-endpoint` — same precondition -// chain as `remote.agent-status`; surface a single Skip rather -// than re-validating each. -func newCheckAgentIdentityRoles(deps Dependencies) Check { - apiVersion := deps.AgentAPIVersion - return Check{ - ID: "remote.agent-identity-roles", - Name: "Agent identity role assignments", - Remote: true, - Fn: func(ctx context.Context, opts Options, prior []Result) Result { - if deps.AzdClient == nil { - return Result{ - Status: StatusSkip, - Message: "skipped: azd extension not reachable.", - } - } - if priorBlocked(prior, "local.environment-selected") { - return Result{ - Status: StatusSkip, - Message: "skipped: no azd environment is selected " + - "(see check `local.environment-selected`).", - } - } - if priorBlocked(prior, "local.agent-service-detected") { - return Result{ - Status: StatusSkip, - Message: "skipped: no `azure.ai.agent` service in " + - "azure.yaml (see check " + - "`local.agent-service-detected`).", - } - } - if priorBlocked(prior, "remote.auth") { - return Result{ - Status: StatusSkip, - Message: "skipped: auth probe did not succeed " + - "(see check `remote.auth`).", - } - } - if priorBlocked(prior, "remote.foundry-endpoint") { - return Result{ - Status: StatusSkip, - Message: "skipped: Foundry endpoint did not respond " + - "(see check `remote.foundry-endpoint`).", - } - } - if !priorPassed(prior, "remote.agent-status") { - return Result{ - Status: StatusSkip, - Message: "skipped: agent status check did not pass " + - "(see check `remote.agent-status`).", - } - } - endpoint := readProjectEndpoint(prior) - if endpoint == "" { - return Result{ - Status: StatusSkip, - Message: "skipped: upstream check did not surface " + - "FOUNDRY_PROJECT_ENDPOINT in its Details.", - } - } - if apiVersion == "" { - return Result{ - Status: StatusSkip, - Message: "skipped: doctor wiring did not provide an " + - "agent API version for the probe.", - } - } - - actives := readActiveAgents(prior) - if len(actives) == 0 { - return Result{ - Status: StatusSkip, - Message: "skipped: no active agents reported by " + - "`remote.agent-status`.", - } - } - - projectResourceID := "" - var prErr error - if deps.readProjectResourceIDFn != nil { - projectResourceID, prErr = deps.readProjectResourceIDFn(ctx, deps.AzdClient) - } else { - projectResourceID, prErr = readProjectResourceID(ctx, deps.AzdClient) - } - if prErr != nil || projectResourceID == "" { - return Result{ - Status: StatusSkip, - Message: "skipped: AZURE_AI_PROJECT_ID is unset " + - "(needed to scope role-assignment listing).", - } - } - - principalProbe := deps.probeAgentPrincipal - if principalProbe == nil { - principalProbe = makeRealProbeAgentPrincipal(apiVersion) - } - - principals := fetchAllAgentPrincipals( - ctx, actives, endpoint, principalProbe) - - query := deps.queryAgentIdentityRoles - if query == nil { - query = project.QueryAgentIdentityRoles - } - - result, err := query(ctx, deps.AzdClient, projectResourceID, principals) - if err != nil { - if errors.Is(err, project.ErrInvalidProjectResourceID) { - return Result{ - Status: StatusSkip, - Message: "skipped: AZURE_AI_PROJECT_ID is " + - "malformed (cannot derive role-assignment scopes).", - } - } - return Result{ - Status: StatusWarn, - Message: "could not list agent identity roles: " + - firstLine(sanitizeScopeARNs(err.Error())), - Suggestion: "Re-run `azd ai agent doctor` once the " + - "transient failure clears.", - } - } - - entries := buildAgentIdentityRoleEntries(actives, result, opts.Unredacted) - return classifyAgentIdentityRolesAggregate(entries, result.Scopes, opts.Unredacted) - }, - } -} - -// activeAgentMeta is the per-agent triple the check fans out across. -// AgentVersion is preserved verbatim from the prior -// `remote.agent-status` entry so the Detail rendering shows the -// same version string the user already saw on the previous check. -type activeAgentMeta struct { - Service string - AgentName string - AgentVersion string -} - -// readActiveAgents pulls the agent name/version triples for active -// agents out of the upstream `remote.agent-status` Details. Returns -// nil if the Details are missing or the wrong shape; the caller -// folds that into a Skip rather than guessing. -// -// Only entries with Classification `active` are returned — the -// design explicitly scopes this check to active agents, and feeding -// a Creating/Failed agent into a role-listing probe would fail with -// confusing "agent has no identity yet" errors. -func readActiveAgents(prior []Result) []activeAgentMeta { - for _, p := range prior { - if p.ID != "remote.agent-status" { - continue - } - raw, ok := p.Details["services"] - if !ok { - return nil - } - entries, ok := raw.([]agentStatusEntry) - if !ok { - return nil - } - out := make([]activeAgentMeta, 0, len(entries)) - for _, e := range entries { - if e.Classification != agentClassActive { - continue - } - if e.AgentName == "" { - continue - } - out = append(out, activeAgentMeta{ - Service: e.Service, - AgentName: e.AgentName, - AgentVersion: e.AgentVersion, - }) - } - return out - } - return nil -} - -// fetchAllAgentPrincipals fans out principal-ID fetches with bounded -// concurrency. Order in the returned slice mirrors `actives` so the -// downstream Details rendering is deterministic. -func fetchAllAgentPrincipals( - ctx context.Context, - actives []activeAgentMeta, - endpoint string, - probe func(context.Context, string, string, string) agentIdentityProbeResult, -) []project.AgentPrincipal { - out := make([]project.AgentPrincipal, len(actives)) - sem := make(chan struct{}, agentIdentityConcurrency) - var wg sync.WaitGroup - for i, a := range actives { - sem <- struct{}{} - wg.Go(func() { - defer func() { <-sem }() - probeCtx, cancel := context.WithTimeout(ctx, agentIdentityProbeTimeout) - defer cancel() - res := probe(probeCtx, endpoint, a.AgentName, a.AgentVersion) - out[i] = project.AgentPrincipal{ - AgentName: a.AgentName, - AgentVersion: a.AgentVersion, - PrincipalID: res.PrincipalID, - } - }) - } - wg.Wait() - return out -} - -// buildAgentIdentityRoleEntries folds the project.QueryAgentIdentityRoles -// output into the per-agent classification structs the aggregate -// classifier consumes. The function is total — every input agent -// produces an output entry — so the aggregate can rely on -// `len(entries) == len(actives)`. -func buildAgentIdentityRoleEntries( - actives []activeAgentMeta, - res *project.AgentIdentityRolesResult, - unredacted bool, -) []agentIdentityRoleEntry { - byName := make(map[string]project.AgentIdentityRolesEntry, len(res.Entries)) - for _, e := range res.Entries { - byName[e.AgentName] = e - } - - out := make([]agentIdentityRoleEntry, 0, len(actives)) - for _, a := range actives { - entry := agentIdentityRoleEntry{ - AgentName: a.AgentName, - AgentVersion: a.AgentVersion, - } - qe, ok := byName[a.AgentName] - if !ok { - entry.Class = agentIdentityClassUnknown - entry.Detail = fmt.Sprintf( - "agent %q: role-assignment listing did not return.", - a.AgentName) - out = append(out, entry) - continue - } - entry.PrincipalID = redactID(qe.PrincipalID, unredacted) - if qe.PrincipalID == "" { - entry.Class = agentIdentityClassUnknown - entry.Detail = fmt.Sprintf( - "agent %q: could not resolve managed-identity "+ - "principal ID from Foundry.", - a.AgentName) - out = append(out, entry) - continue - } - - entry.ProjectRoles = qe.ProjectScope.Roles - entry.AccountRoles = qe.AccountScope.Roles - entry.RGRoles = qe.RGScope.Roles - if qe.ProjectScope.Err != nil { - entry.ProjectErr = redactErrorText(qe.ProjectScope.Err.Error(), unredacted) - } - if qe.AccountScope.Err != nil { - entry.AccountErr = redactErrorText(qe.AccountScope.Err.Error(), unredacted) - } - if qe.RGScope.Err != nil { - entry.RGErr = redactErrorText(qe.RGScope.Err.Error(), unredacted) - } - - entry.Class = classifyOneAgent(qe) - entry.Detail = describeOneAgent(qe) - out = append(out, entry) - } - sort.SliceStable(out, func(i, j int) bool { - return out[i].AgentName < out[j].AgentName - }) - return out -} - -// redactErrorText scrubs ARM scope ARNs and bare GUIDs out of an -// error string and returns its first line. When unredacted is true, -// the error's first line is returned verbatim so operators running -// `--unredacted` see the raw backend response. Centralized here so -// every per-scope error path applies the same masking sequence. -func redactErrorText(s string, unredacted bool) string { - if unredacted { - return firstLine(s) - } - return firstLine(sanitizeScopeARNs(s)) -} - -// classifyOneAgent buckets a single agent's per-scope listing into -// one of fine / underscoped / empty / unknown. A scope counts as -// "covered" when its Err is nil and Roles is non-empty. The -// pass-condition (per design): project covered AND (account -// covered OR RG covered). -func classifyOneAgent(qe project.AgentIdentityRolesEntry) string { - projectCovered := qe.ProjectScope.Err == nil && len(qe.ProjectScope.Roles) > 0 - accountCovered := qe.AccountScope.Err == nil && len(qe.AccountScope.Roles) > 0 - rgCovered := qe.RGScope.Err == nil && len(qe.RGScope.Roles) > 0 - - // All three probes errored — we can't classify. - if qe.ProjectScope.Err != nil && qe.AccountScope.Err != nil && qe.RGScope.Err != nil { - return agentIdentityClassUnknown - } - - anyCovered := projectCovered || accountCovered || rgCovered - if !anyCovered { - return agentIdentityClassEmpty - } - if projectCovered && (accountCovered || rgCovered) { - return agentIdentityClassFine - } - return agentIdentityClassUnderscoped -} - -// describeOneAgent renders the one-line per-agent Detail. Format: -// -// : project=N, account=M, resource-group=K -// -// with `?` for probe-error scopes. Designed to fit on one line of an -// 80-col terminal even with 4-char wide role names plus the lead -// `: ` prefix. -func describeOneAgent(qe project.AgentIdentityRolesEntry) string { - return fmt.Sprintf( - "%s: project=%s, account=%s, resource-group=%s", - qe.AgentName, - formatScopeCount(qe.ProjectScope), - formatScopeCount(qe.AccountScope), - formatScopeCount(qe.RGScope), - ) -} - -// formatScopeCount renders one scope's count for describeOneAgent. -// Returns the literal "?" when the per-scope probe errored — the -// row tells the user the listing is incomplete without needing to -// open Details. -func formatScopeCount(sr project.AgentScopeRoles) string { - if sr.Err != nil { - return "?" - } - return fmt.Sprintf("%d", len(sr.Roles)) -} - -// classifyAgentIdentityRolesAggregate folds the per-agent entries -// into a single doctor Result. The aggregate Status is the worst -// per-agent Class's bucket; the Message picks the worst entry for -// the headline and the per-agent breakdown lands in Details. A -// remediation suggestion is attached only to FAIL and WARN -// classes — the INFO state has nothing actionable to offer. -// -// Empty entries (no active agents to enumerate) are folded into -// Skip upstream; if the function is somehow reached with an empty -// slice it produces a Skip rather than a Pass to avoid emitting an -// empty INFO line. -func classifyAgentIdentityRolesAggregate( - entries []agentIdentityRoleEntry, - scopes project.AgentIdentityScopes, - unredacted bool, -) Result { - if len(entries) == 0 { - return Result{ - Status: StatusSkip, - Message: "no active agents to enumerate.", - } - } - - worst := agentIdentityClassFine - for _, e := range entries { - if rankAgentIdentity(e.Class) > rankAgentIdentity(worst) { - worst = e.Class - } - } - - byClass := map[string]int{} - for _, e := range entries { - byClass[e.Class]++ - } - - details := map[string]any{ - "agents": entries, - "byClassification": byClass, - "scopes": map[string]string{ - "project": redactScope(scopes.Project, unredacted), - "account": redactScope(scopes.Account, unredacted), - "resource-group": redactScope(scopes.ResourceGroup, unredacted), - }, - } - - detailLines := func() []string { - out := make([]string, 0, len(entries)) - for _, e := range entries { - out = append(out, e.Detail) - } - return out - } - - switch worst { - case agentIdentityClassFine: - return Result{ - Status: StatusInfo, - Message: fmt.Sprintf( - "%d of %d agents have role assignments at the "+ - "project scope plus at least one of "+ - "account/resource-group scope.", - byClass[agentIdentityClassFine], len(entries)), - Details: details, - Suggestion: "Role assignments listed; no action needed. " + - "Use `azd ai agent doctor --output json` for the " + - "machine-readable per-agent breakdown.\n " + - strings.Join(detailLines(), "\n "), - } - case agentIdentityClassUnknown: - // Pure-unknown aggregate: every agent had a probe failure. - // Surface as WARN with a re-run hint. - return Result{ - Status: StatusWarn, - Message: fmt.Sprintf( - "%d of %d agents: could not list role assignments.", - byClass[agentIdentityClassUnknown], len(entries)), - Details: details, - Suggestion: "Re-run `azd ai agent doctor` once the " + - "transient failure clears. Per-agent detail:\n " + - strings.Join(detailLines(), "\n "), - } - case agentIdentityClassUnderscoped: - // At least one agent is underscoped (assignments exist - // somewhere but pass condition unmet). - return Result{ - Status: StatusWarn, - Message: fmt.Sprintf( - "%d of %d agents have under-privileged role assignments.", - byClass[agentIdentityClassUnderscoped], len(entries)), - Details: details, - Suggestion: "Agents may not have permission to access " + - "project / account resources at runtime. Grant " + - "a role on the missing scope:\n " + - "az role assignment create --assignee " + - "--role --scope \n" + - "Per-agent detail:\n " + - strings.Join(detailLines(), "\n "), - } - case agentIdentityClassEmpty: - return Result{ - Status: StatusFail, - Message: fmt.Sprintf( - "%d of %d agents have zero role assignments at any "+ - "reachable scope.", - byClass[agentIdentityClassEmpty], len(entries)), - Details: details, - Suggestion: "Agents will likely 403 on every tool call. " + - "Grant Cognitive Services User (or stronger) on " + - "the project scope:\n " + - "az role assignment create --assignee " + - "--role \"Cognitive Services User\" --scope " + - "\nPer-agent detail:\n " + - strings.Join(detailLines(), "\n "), - } - default: - // Defensive: an unrecognized class collapses to WARN with - // the raw class string surfaced for diagnostic purposes. - return Result{ - Status: StatusWarn, - Message: fmt.Sprintf( - "unrecognized aggregate class %q.", worst), - Details: details, - } - } -} - -func rankAgentIdentity(class string) int { - if r, ok := agentIdentityClassRank[class]; ok { - return r - } - return -1 -} - -// makeRealProbeAgentPrincipal returns the production closure used -// to fetch one agent's `instance_identity.principal_id`. Mirrors -// makeRealProbeAgentStatus from checks_agent_status.go (same -// credential and SDK client; different response field consumed) -// so a Pass here matches what the runtime invoke flow would see. -func makeRealProbeAgentPrincipal( - apiVersion string, -) func(context.Context, string, string, string) agentIdentityProbeResult { - return func( - ctx context.Context, - endpoint, agentName, agentVersion string, - ) agentIdentityProbeResult { - cred, err := azidentity.NewAzureDeveloperCLICredential( - &azidentity.AzureDeveloperCLICredentialOptions{}, - ) - if err != nil { - return agentIdentityProbeResult{ - Err: fmt.Errorf("create credential: %w", err), - } - } - client := agent_api.NewAgentClient(endpoint, cred) - v, err := client.GetAgentVersion( - ctx, agentName, agentVersion, apiVersion) - if err != nil { - if respErr, ok := errors.AsType[*azcore.ResponseError](err); ok { - return agentIdentityProbeResult{ - StatusCode: respErr.StatusCode, - Err: err, - } - } - return agentIdentityProbeResult{Err: err} - } - if v == nil { - return agentIdentityProbeResult{ - Err: errors.New("GetAgentVersion returned nil"), - } - } - if v.InstanceIdentity == nil { - return agentIdentityProbeResult{ - StatusCode: 200, - Err: fmt.Errorf("agent %q has no instance_identity", - agentName), - } - } - return agentIdentityProbeResult{ - StatusCode: 200, - PrincipalID: v.InstanceIdentity.PrincipalID, - } - } -} - -// priorPassed reports whether a prior check with the given ID -// produced StatusPass. False both for "check not in slice" and -// "check present but didn't pass" — callers handle the two -// outcomes the same way (Skip). -func priorPassed(prior []Result, id string) bool { - for _, p := range prior { - if p.ID == id { - return p.Status == StatusPass - } - } - return false -} diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_agent_identity_roles_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_agent_identity_roles_test.go deleted file mode 100644 index 76ee2379ffa..00000000000 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_agent_identity_roles_test.go +++ /dev/null @@ -1,562 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -package doctor - -import ( - "context" - "errors" - "strings" - "testing" - - "azureaiagent/internal/project" - - "github.com/azure/azure-dev/cli/azd/pkg/azdext" - "github.com/stretchr/testify/require" -) - -// agentIdentityPriorResults produces a complete prior-result slice -// satisfying every skip-cascade gate `remote.agent-identity-roles` -// declares. The `agentStatusEntries` slice is the production-shape -// listing the upstream `remote.agent-status` check surfaces under -// `Details["services"]`; only entries with Classification == "active" -// will be consumed by C12. -func agentIdentityPriorResults( - agentStatusEntries []agentStatusEntry, - endpoint string, -) []Result { - return []Result{ - {ID: "local.environment-selected", Status: StatusPass}, - {ID: "local.agent-service-detected", Status: StatusPass}, - {ID: "local.project-endpoint-set", Status: StatusPass, Details: map[string]any{ - "projectEndpoint": endpoint, - }}, - {ID: "remote.auth", Status: StatusPass}, - {ID: "remote.foundry-endpoint", Status: StatusPass}, - {ID: "remote.agent-status", Status: StatusPass, Details: map[string]any{ - "services": agentStatusEntries, - }}, - } -} - -// runIdentityCheck wires deps with default test-friendly values and -// invokes the check. Tests that need to override a seam pass it in -// `deps`; defaults preserve the no-network contract. -func runIdentityCheck(t *testing.T, deps Dependencies, prior []Result) Result { - t.Helper() - if deps.AzdClient == nil { - deps.AzdClient = &azdext.AzdClient{} - } - if deps.AgentAPIVersion == "" { - deps.AgentAPIVersion = "v1" - } - if deps.readProjectResourceIDFn == nil { - deps.readProjectResourceIDFn = func(_ context.Context, _ *azdext.AzdClient) (string, error) { - return "/subscriptions/sub-1/resourceGroups/rg-1/providers/" + - "Microsoft.CognitiveServices/accounts/acc-1/projects/proj-1", nil - } - } - if deps.probeAgentPrincipal == nil { - // Default: every probe returns a deterministic principal ID - // derived from the agent name. Tests overriding need do so - // explicitly via deps.probeAgentPrincipal. - deps.probeAgentPrincipal = func(_ context.Context, _, name, _ string) agentIdentityProbeResult { - return agentIdentityProbeResult{ - StatusCode: 200, - PrincipalID: "principal-" + name, - } - } - } - c := newCheckAgentIdentityRoles(deps) - require.NotNil(t, c.Fn, "newCheckAgentIdentityRoles must return a non-nil Fn") - return c.Fn(t.Context(), Options{}, prior) -} - -// ---- Skip-cascade gates ---- - -func TestCheckAgentIdentityRoles_SkipsWhenAzdClientNil(t *testing.T) { - t.Parallel() - c := newCheckAgentIdentityRoles(Dependencies{}) - res := c.Fn(t.Context(), Options{}, nil) - require.Equal(t, StatusSkip, res.Status) - require.Contains(t, res.Message, "azd extension not reachable") -} - -func TestCheckAgentIdentityRoles_SkipsWhenAgentStatusNotPassed(t *testing.T) { - t.Parallel() - prior := []Result{ - {ID: "local.environment-selected", Status: StatusPass}, - {ID: "local.agent-service-detected", Status: StatusPass}, - {ID: "remote.auth", Status: StatusPass}, - {ID: "remote.foundry-endpoint", Status: StatusPass}, - {ID: "remote.agent-status", Status: StatusFail}, - } - res := runIdentityCheck(t, Dependencies{}, prior) - require.Equal(t, StatusSkip, res.Status) - require.Contains(t, res.Message, "remote.agent-status") -} - -func TestCheckAgentIdentityRoles_SkipsWhenProjectEndpointMissing(t *testing.T) { - t.Parallel() - // Drop the project-endpoint Result's Details - prior := agentIdentityPriorResults( - []agentStatusEntry{{Service: "svc", AgentName: "a", AgentVersion: "1", Classification: agentClassActive}}, - "") - res := runIdentityCheck(t, Dependencies{}, prior) - require.Equal(t, StatusSkip, res.Status) - require.Contains(t, res.Message, "FOUNDRY_PROJECT_ENDPOINT") -} - -func TestCheckAgentIdentityRoles_SkipsWhenNoActiveAgents(t *testing.T) { - t.Parallel() - // Only Creating / Failed entries — no active ones. - prior := agentIdentityPriorResults( - []agentStatusEntry{ - {Service: "a", AgentName: "an", AgentVersion: "1", Classification: agentClassDeploying}, - {Service: "b", AgentName: "bn", AgentVersion: "1", Classification: agentClassFailed}, - }, - "https://example.local") - res := runIdentityCheck(t, Dependencies{}, prior) - require.Equal(t, StatusSkip, res.Status) - require.Contains(t, res.Message, "active agents") -} - -func TestCheckAgentIdentityRoles_SkipsWhenAPIVersionEmpty(t *testing.T) { - t.Parallel() - // Bypass runIdentityCheck so AgentAPIVersion stays empty — - // runIdentityCheck would auto-populate it. Build deps with the - // minimum needed to clear the AzdClient nil guard. - deps := Dependencies{ - AzdClient: &azdext.AzdClient{}, - // AgentAPIVersion deliberately empty - readProjectResourceIDFn: func(_ context.Context, _ *azdext.AzdClient) (string, error) { - return "/subscriptions/sub-1/resourceGroups/rg-1/providers/" + - "Microsoft.CognitiveServices/accounts/acc-1/projects/proj-1", nil - }, - } - prior := agentIdentityPriorResults( - []agentStatusEntry{{Service: "svc", AgentName: "a", AgentVersion: "1", Classification: agentClassActive}}, - "https://example.local") - res := newCheckAgentIdentityRoles(deps).Fn(t.Context(), Options{}, prior) - require.Equal(t, StatusSkip, res.Status) - require.Contains(t, res.Message, "agent API version") -} - -func TestCheckAgentIdentityRoles_SkipsWhenProjectResourceIDUnset(t *testing.T) { - t.Parallel() - deps := Dependencies{ - readProjectResourceIDFn: func(_ context.Context, _ *azdext.AzdClient) (string, error) { - return "", nil - }, - } - prior := agentIdentityPriorResults( - []agentStatusEntry{{Service: "svc", AgentName: "a", AgentVersion: "1", Classification: agentClassActive}}, - "https://example.local") - res := runIdentityCheck(t, deps, prior) - require.Equal(t, StatusSkip, res.Status) - require.Contains(t, res.Message, "AZURE_AI_PROJECT_ID") -} - -func TestCheckAgentIdentityRoles_SkipsWhenProjectResourceIDMalformed(t *testing.T) { - t.Parallel() - deps := Dependencies{ - queryAgentIdentityRoles: func(_ context.Context, _ *azdext.AzdClient, _ string, _ []project.AgentPrincipal) (*project.AgentIdentityRolesResult, error) { - return nil, project.ErrInvalidProjectResourceID - }, - } - prior := agentIdentityPriorResults( - []agentStatusEntry{{Service: "svc", AgentName: "a", AgentVersion: "1", Classification: agentClassActive}}, - "https://example.local") - res := runIdentityCheck(t, deps, prior) - require.Equal(t, StatusSkip, res.Status) - require.Contains(t, res.Message, "malformed") -} - -// ---- Aggregate classification ---- - -func makeQueryReturning(result *project.AgentIdentityRolesResult) func( - context.Context, *azdext.AzdClient, string, []project.AgentPrincipal, -) (*project.AgentIdentityRolesResult, error) { - return func(_ context.Context, _ *azdext.AzdClient, _ string, principals []project.AgentPrincipal) (*project.AgentIdentityRolesResult, error) { - // Echo the input principals' agent names through to entries - // when the test supplied a single-entry result keyed by name; - // otherwise return the canned result verbatim. - _ = principals - return result, nil - } -} - -func TestCheckAgentIdentityRoles_AggregateInfoWhenAllAgentsFine(t *testing.T) { - t.Parallel() - deps := Dependencies{ - queryAgentIdentityRoles: makeQueryReturning(&project.AgentIdentityRolesResult{ - Entries: []project.AgentIdentityRolesEntry{ - { - AgentName: "a", - PrincipalID: "principal-a", - ProjectScope: project.AgentScopeRoles{Scope: "project", Roles: []string{"Azure AI User"}}, - AccountScope: project.AgentScopeRoles{Scope: "account", Roles: []string{"Cognitive Services User"}}, - RGScope: project.AgentScopeRoles{Scope: "resource-group", Roles: []string{}}, - }, - }, - Scopes: project.AgentIdentityScopes{Project: "scope-p", Account: "scope-a", ResourceGroup: "scope-rg"}, - }), - } - prior := agentIdentityPriorResults( - []agentStatusEntry{{Service: "svc-a", AgentName: "a", AgentVersion: "1", Classification: agentClassActive}}, - "https://example.local") - res := runIdentityCheck(t, deps, prior) - require.Equal(t, StatusInfo, res.Status) - require.Contains(t, res.Message, "1 of 1 agents") - require.Contains(t, res.Suggestion, "no action needed") -} - -func TestCheckAgentIdentityRoles_AggregateFailWhenAnyAgentEmpty(t *testing.T) { - t.Parallel() - deps := Dependencies{ - queryAgentIdentityRoles: makeQueryReturning(&project.AgentIdentityRolesResult{ - Entries: []project.AgentIdentityRolesEntry{ - { - AgentName: "a", - PrincipalID: "principal-a", - ProjectScope: project.AgentScopeRoles{Scope: "project", Roles: []string{"Azure AI User"}}, - AccountScope: project.AgentScopeRoles{Scope: "account", Roles: []string{}}, - RGScope: project.AgentScopeRoles{Scope: "resource-group", Roles: []string{}}, - }, - { - AgentName: "b", - PrincipalID: "principal-b", - ProjectScope: project.AgentScopeRoles{Scope: "project", Roles: []string{}}, - AccountScope: project.AgentScopeRoles{Scope: "account", Roles: []string{}}, - RGScope: project.AgentScopeRoles{Scope: "resource-group", Roles: []string{}}, - }, - }, - }), - } - prior := agentIdentityPriorResults( - []agentStatusEntry{ - {Service: "svc-a", AgentName: "a", AgentVersion: "1", Classification: agentClassActive}, - {Service: "svc-b", AgentName: "b", AgentVersion: "1", Classification: agentClassActive}, - }, - "https://example.local") - res := runIdentityCheck(t, deps, prior) - require.Equal(t, StatusFail, res.Status) - require.Contains(t, res.Message, "zero role assignments") - require.Contains(t, res.Suggestion, "az role assignment create") -} - -func TestCheckAgentIdentityRoles_AggregateWarnWhenAgentUnderscoped(t *testing.T) { - t.Parallel() - deps := Dependencies{ - queryAgentIdentityRoles: makeQueryReturning(&project.AgentIdentityRolesResult{ - Entries: []project.AgentIdentityRolesEntry{ - { - AgentName: "a", - PrincipalID: "principal-a", - // project covered but neither account nor RG — underscoped - ProjectScope: project.AgentScopeRoles{Scope: "project", Roles: []string{"Azure AI User"}}, - AccountScope: project.AgentScopeRoles{Scope: "account", Roles: []string{}}, - RGScope: project.AgentScopeRoles{Scope: "resource-group", Roles: []string{}}, - }, - }, - }), - } - prior := agentIdentityPriorResults( - []agentStatusEntry{{Service: "svc-a", AgentName: "a", AgentVersion: "1", Classification: agentClassActive}}, - "https://example.local") - res := runIdentityCheck(t, deps, prior) - require.Equal(t, StatusWarn, res.Status) - require.Contains(t, res.Message, "under-privileged") -} - -func TestCheckAgentIdentityRoles_AggregateWarnOnQueryError(t *testing.T) { - t.Parallel() - deps := Dependencies{ - queryAgentIdentityRoles: func(_ context.Context, _ *azdext.AzdClient, _ string, _ []project.AgentPrincipal) (*project.AgentIdentityRolesResult, error) { - return nil, errors.New("ARM transient") - }, - } - prior := agentIdentityPriorResults( - []agentStatusEntry{{Service: "svc-a", AgentName: "a", AgentVersion: "1", Classification: agentClassActive}}, - "https://example.local") - res := runIdentityCheck(t, deps, prior) - require.Equal(t, StatusWarn, res.Status) - require.Contains(t, res.Message, "ARM transient") -} - -// ---- Per-agent classifier ---- - -func TestClassifyOneAgent_FineWhenProjectPlusAccountOrRG(t *testing.T) { - t.Parallel() - cases := []struct { - name string - qe project.AgentIdentityRolesEntry - want string - }{ - { - name: "project+account → fine", - qe: project.AgentIdentityRolesEntry{ - ProjectScope: project.AgentScopeRoles{Roles: []string{"r"}}, - AccountScope: project.AgentScopeRoles{Roles: []string{"r"}}, - RGScope: project.AgentScopeRoles{Roles: []string{}}, - }, - want: agentIdentityClassFine, - }, - { - name: "project+RG → fine", - qe: project.AgentIdentityRolesEntry{ - ProjectScope: project.AgentScopeRoles{Roles: []string{"r"}}, - AccountScope: project.AgentScopeRoles{Roles: []string{}}, - RGScope: project.AgentScopeRoles{Roles: []string{"r"}}, - }, - want: agentIdentityClassFine, - }, - { - name: "project only → underscoped", - qe: project.AgentIdentityRolesEntry{ - ProjectScope: project.AgentScopeRoles{Roles: []string{"r"}}, - AccountScope: project.AgentScopeRoles{Roles: []string{}}, - RGScope: project.AgentScopeRoles{Roles: []string{}}, - }, - want: agentIdentityClassUnderscoped, - }, - { - name: "account only → underscoped (no project coverage)", - qe: project.AgentIdentityRolesEntry{ - ProjectScope: project.AgentScopeRoles{Roles: []string{}}, - AccountScope: project.AgentScopeRoles{Roles: []string{"r"}}, - RGScope: project.AgentScopeRoles{Roles: []string{}}, - }, - want: agentIdentityClassUnderscoped, - }, - { - name: "all empty → empty", - qe: project.AgentIdentityRolesEntry{ - ProjectScope: project.AgentScopeRoles{Roles: []string{}}, - AccountScope: project.AgentScopeRoles{Roles: []string{}}, - RGScope: project.AgentScopeRoles{Roles: []string{}}, - }, - want: agentIdentityClassEmpty, - }, - { - name: "all errored → unknown", - qe: project.AgentIdentityRolesEntry{ - ProjectScope: project.AgentScopeRoles{Err: errors.New("e")}, - AccountScope: project.AgentScopeRoles{Err: errors.New("e")}, - RGScope: project.AgentScopeRoles{Err: errors.New("e")}, - }, - want: agentIdentityClassUnknown, - }, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - got := classifyOneAgent(tc.qe) - require.Equal(t, tc.want, got) - }) - } -} - -// ---- Detail formatting ---- - -func TestDescribeOneAgent_RendersScopeCounts(t *testing.T) { - t.Parallel() - qe := project.AgentIdentityRolesEntry{ - AgentName: "agent-x", - ProjectScope: project.AgentScopeRoles{Roles: []string{"a", "b"}}, - AccountScope: project.AgentScopeRoles{Roles: []string{}}, - RGScope: project.AgentScopeRoles{Err: errors.New("listing failed")}, - } - got := describeOneAgent(qe) - require.Equal(t, "agent-x: project=2, account=0, resource-group=?", got) -} - -// ---- Redaction ---- - -// TestCheckAgentIdentityRoles_RedactedDetailsDoNotLeakIdentifiers -// asserts the doctor's redaction contract: when Options.Unredacted -// is false (the default), Details must not surface raw principal IDs, -// raw ARM scope ARNs, or scope-bearing error strings. With -// Unredacted=true, the same identifiers must pass through verbatim -// so operators running `--unredacted` see what the backend returned. -func TestCheckAgentIdentityRoles_RedactedDetailsDoNotLeakIdentifiers(t *testing.T) { - t.Parallel() - rawPrincipal := "11111111-2222-3333-4444-555555555555" - rawProjectScope := "/subscriptions/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/" + - "resourceGroups/rg-secret/providers/Microsoft.CognitiveServices/" + - "accounts/acc-secret/projects/proj-secret" - rawAccountScope := "/subscriptions/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/" + - "resourceGroups/rg-secret/providers/Microsoft.CognitiveServices/" + - "accounts/acc-secret" - rawRGScope := "/subscriptions/aaaaaaaa-bbbb-cccc-dddd-eeeeeeeeeeee/" + - "resourceGroups/rg-secret" - rawScopeBearingErr := errors.New("failed to list role assignments at " + - "scope " + rawProjectScope + ": forbidden") - canned := &project.AgentIdentityRolesResult{ - Entries: []project.AgentIdentityRolesEntry{ - { - AgentName: "a", - PrincipalID: rawPrincipal, - ProjectScope: project.AgentScopeRoles{Scope: "project", Err: rawScopeBearingErr}, - AccountScope: project.AgentScopeRoles{Scope: "account", Roles: []string{"r"}}, - RGScope: project.AgentScopeRoles{Scope: "resource-group", Roles: []string{"r"}}, - }, - }, - Scopes: project.AgentIdentityScopes{ - Project: rawProjectScope, - Account: rawAccountScope, - ResourceGroup: rawRGScope, - }, - } - deps := Dependencies{ - queryAgentIdentityRoles: makeQueryReturning(canned), - } - prior := agentIdentityPriorResults( - []agentStatusEntry{{Service: "svc-a", AgentName: "a", AgentVersion: "1", Classification: agentClassActive}}, - "https://example.local") - - // Redacted (default). - res := runIdentityCheck(t, deps, prior) - // Serialize Details to a single string so the assertion catches - // leaks regardless of struct key path. - detailsRedacted := flattenDetails(res.Details) - require.NotContains(t, detailsRedacted, rawPrincipal, - "redacted Details must not contain raw principal ID") - require.NotContains(t, detailsRedacted, rawProjectScope, - "redacted Details must not contain raw project scope ARN") - require.NotContains(t, detailsRedacted, rawAccountScope, - "redacted Details must not contain raw account scope ARN") - require.NotContains(t, detailsRedacted, rawRGScope, - "redacted Details must not contain raw RG scope ARN") - require.Contains(t, detailsRedacted, "", - "redacted Details must contain the redacted placeholder") - - // Unredacted: same fixture, opts.Unredacted=true. - check := newCheckAgentIdentityRoles(Dependencies{ - AzdClient: &azdext.AzdClient{}, - readProjectResourceIDFn: func(_ context.Context, _ *azdext.AzdClient) (string, error) { - return "/subscriptions/sub-1/resourceGroups/rg-1/providers/" + - "Microsoft.CognitiveServices/accounts/acc-1/projects/proj-1", nil - }, - probeAgentPrincipal: func(_ context.Context, _, _, _ string) agentIdentityProbeResult { - return agentIdentityProbeResult{PrincipalID: rawPrincipal, StatusCode: 200} - }, - queryAgentIdentityRoles: makeQueryReturning(canned), - AgentAPIVersion: "v1", - }) - resU := check.Fn(t.Context(), Options{Unredacted: true}, prior) - detailsUnredacted := flattenDetails(resU.Details) - require.Contains(t, detailsUnredacted, rawPrincipal, - "unredacted Details must contain raw principal ID") - require.Contains(t, detailsUnredacted, rawProjectScope, - "unredacted Details must contain raw project scope ARN") -} - -// flattenDetails walks the Details map and returns a single string -// suitable for substring assertions. Used by redaction tests so -// callers don't need to know the exact key path the check emits. -func flattenDetails(d map[string]any) string { - if d == nil { - return "" - } - var sb strings.Builder - for k, v := range d { - sb.WriteString(k) - sb.WriteString("=") - sb.WriteString(stringify(v)) - sb.WriteString("\n") - } - return sb.String() -} - -func stringify(v any) string { - switch t := v.(type) { - case string: - return t - case map[string]string: - var sb strings.Builder - for k, val := range t { - sb.WriteString(k) - sb.WriteString("=") - sb.WriteString(val) - sb.WriteString(";") - } - return sb.String() - case []agentIdentityRoleEntry: - var sb strings.Builder - for _, e := range t { - sb.WriteString(e.AgentName) - sb.WriteString(":") - sb.WriteString(e.PrincipalID) - sb.WriteString(";") - sb.WriteString(e.ProjectErr) - sb.WriteString(";") - sb.WriteString(e.AccountErr) - sb.WriteString(";") - sb.WriteString(e.RGErr) - sb.WriteString("|") - } - return sb.String() - default: - return "" - } -} - -// ---- Missing-principal degradation ---- - -func TestCheckAgentIdentityRoles_DegradesWhenPrincipalMissing(t *testing.T) { - t.Parallel() - // Build a Result where principal probe is "missing"; the query - // fake will surface that as an unknown-class entry. - deps := Dependencies{ - probeAgentPrincipal: func(_ context.Context, _, _, _ string) agentIdentityProbeResult { - return agentIdentityProbeResult{Err: errors.New("no identity")} - }, - queryAgentIdentityRoles: func(_ context.Context, _ *azdext.AzdClient, _ string, principals []project.AgentPrincipal) (*project.AgentIdentityRolesResult, error) { - // Echo missing-principal entries with all-error scopes - // to mirror what production QueryAgentIdentityRoles - // produces when PrincipalID == "". - entries := make([]project.AgentIdentityRolesEntry, 0, len(principals)) - for _, p := range principals { - entries = append(entries, project.AgentIdentityRolesEntry{ - AgentName: p.AgentName, - PrincipalID: "", - ProjectScope: project.AgentScopeRoles{Err: errors.New("principal ID unavailable")}, - AccountScope: project.AgentScopeRoles{Err: errors.New("principal ID unavailable")}, - RGScope: project.AgentScopeRoles{Err: errors.New("principal ID unavailable")}, - }) - } - return &project.AgentIdentityRolesResult{Entries: entries}, nil - }, - } - prior := agentIdentityPriorResults( - []agentStatusEntry{{Service: "svc-a", AgentName: "a", AgentVersion: "1", Classification: agentClassActive}}, - "https://example.local") - res := runIdentityCheck(t, deps, prior) - require.Equal(t, StatusWarn, res.Status) - require.True(t, strings.Contains(res.Message, "could not list") || strings.Contains(res.Message, "transient")) -} - -// ---- readActiveAgents filtering ---- - -func TestReadActiveAgents_FiltersToActiveOnly(t *testing.T) { - t.Parallel() - prior := []Result{ - {ID: "remote.agent-status", Status: StatusPass, Details: map[string]any{ - "services": []agentStatusEntry{ - {Service: "a", AgentName: "an", AgentVersion: "1", Classification: agentClassActive}, - {Service: "b", AgentName: "bn", AgentVersion: "1", Classification: agentClassFailed}, - {Service: "c", AgentName: "cn", AgentVersion: "1", Classification: agentClassDeploying}, - {Service: "d", AgentName: "", AgentVersion: "1", Classification: agentClassActive}, // missing name dropped - }, - }}, - } - got := readActiveAgents(prior) - require.Len(t, got, 1) - require.Equal(t, "an", got[0].AgentName) -} - -func TestReadActiveAgents_ReturnsNilWhenAgentStatusMissing(t *testing.T) { - t.Parallel() - got := readActiveAgents(nil) - require.Nil(t, got) -} diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local.go index f38e01f3bac..27e89140d7a 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_local.go @@ -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 diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_remote.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_remote.go index fecfb08afad..b1818fe9bcc 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_remote.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_remote.go @@ -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`) // @@ -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 @@ -85,7 +88,6 @@ func NewRemoteChecks(deps Dependencies) []Check { newCheckFoundryEndpoint(deps), newCheckRBAC(deps), newCheckAgentStatus(deps), - newCheckAgentIdentityRoles(deps), newCheckConnections(deps), } } diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_remote_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_remote_test.go index a3297bf8bb1..2f877a81cd4 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_remote_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/checks_remote_test.go @@ -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") @@ -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 diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/shared_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/shared_test.go index 57ea73ce788..2b32a7243fe 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/shared_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/shared_test.go @@ -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) { diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/types.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/types.go index e0491d35bb3..1f630e5d454 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/types.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor/types.go @@ -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" ) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor_format.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor_format.go index 109b581064b..13ee14e2cb3 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor_format.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/doctor_format.go @@ -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", diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/listen.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/listen.go index 29f70766347..9ac9bc0bbbc 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/listen.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/listen.go @@ -15,7 +15,6 @@ import ( "sync" "azureaiagent/internal/exterrors" - "azureaiagent/internal/pkg/agents/agent_api" "azureaiagent/internal/pkg/agents/optimize_api" "azureaiagent/internal/project" @@ -295,13 +294,10 @@ 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. 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) + return fmt.Errorf("failed to get current environment: %w", err) } envName := envResp.Environment.Name @@ -337,47 +333,7 @@ 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 == "" { - 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) + return fmt.Errorf("failed to create credential: %w", err) } // Report optimization candidate deployment (best-effort: panics are logged, not propagated). diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_query.go b/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_query.go deleted file mode 100644 index aef493f8c21..00000000000 --- a/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_query.go +++ /dev/null @@ -1,335 +0,0 @@ -// Copyright (c) Microsoft Corporation. All rights reserved. -// Licensed under the MIT License. - -package project - -import ( - "context" - "errors" - "fmt" - "strings" - "sync" - - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" - "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v3" - "github.com/azure/azure-dev/cli/azd/pkg/azdext" -) - -// AgentPrincipal identifies one deployed agent's managed-identity -// principal — the entity whose role assignments the doctor's -// `remote.agent-identity-roles` check enumerates. -// -// AgentName / AgentVersion are the deployment-side coordinates surfaced -// to the user in Details and Messages. PrincipalID is the AAD object -// ID returned by Foundry's `GetAgentVersion` under -// `instance_identity.principal_id`; QueryAgentIdentityRoles is purely -// read-side and does not create or look up principals itself. -type AgentPrincipal struct { - AgentName string - AgentVersion string - PrincipalID string -} - -// AgentScopeRoles is the per-scope, per-agent listing the -// `remote.agent-identity-roles` check renders. Empty Roles is a -// meaningful state ("no role assignment on this scope") — callers must -// distinguish nil Roles (probe failed for this scope) from an empty -// non-nil slice (the probe succeeded and the principal has no roles -// there). -type AgentScopeRoles struct { - // Scope is the friendly label ("project", "account", - // "resource-group") used in user-facing output. The raw ARM scope - // ARN is omitted — the caller already knows it from - // `AgentIdentityRolesResult.Scopes` and surfacing it again per-row - // hurts redaction more than it helps the user. - Scope string - // Roles is the list of human-readable role names (e.g., - // "Cognitive Services User"). When the listing succeeded but the - // principal had no assignments at this scope, Roles is non-nil - // and empty. nil indicates the per-scope probe failed and the - // caller should treat the scope as "unknown". - Roles []string - // Err captures the per-scope probe error when the listing failed. - // nil for successful empty-list responses. - Err error -} - -// AgentIdentityRolesEntry is the per-agent listing folded across the -// three probed scopes. AgentName / AgentVersion / PrincipalID echo the -// input AgentPrincipal so consumers do not need to thread the input -// alongside the output. ProjectScope / AccountScope / RGScope each -// carry the per-scope outcome. -type AgentIdentityRolesEntry struct { - AgentName string - AgentVersion string - PrincipalID string - ProjectScope AgentScopeRoles - AccountScope AgentScopeRoles - RGScope AgentScopeRoles -} - -// AgentIdentityRolesResult is the side-effect-free outcome of -// QueryAgentIdentityRoles. Entries preserves the input order (sorted -// by AgentName upstream so output is deterministic). Scopes captures -// the raw ARN of each scope the listings ran against — diagnostics -// surface the friendly label (`ProjectScope.Scope`) but JSON -// consumers may need the raw ARN; redacting consumers can replace -// these with `` after assembly. -type AgentIdentityRolesResult struct { - Entries []AgentIdentityRolesEntry - Scopes AgentIdentityScopes -} - -// AgentIdentityScopes is the resolved scope ARN trio for an agent's -// identity-role listing. Account is the parent AI account ARN -// (`/subscriptions/.../accounts/`); Project is the agent's -// hosting project ARN (`/subscriptions/.../accounts/.../projects/

`); -// ResourceGroup is `/subscriptions/.../resourceGroups/`. -type AgentIdentityScopes struct { - Account string - Project string - ResourceGroup string -} - -// QueryAgentIdentityRoles enumerates each principal's role assignments -// at the agent's three reachable scopes (project, account, resource -// group) and returns a structured listing for the doctor's -// `remote.agent-identity-roles` check. -// -// The function follows the same credential-acquisition pattern as -// EnsureAgentIdentityRBAC: parse the project ARM ID, resolve the -// user-access tenant via the azd extension, and create an -// AzureDeveloperCLICredential pinned to that tenant. Per-principal -// probing fans out across scopes; a failure on one scope does not -// short-circuit the others so the user always sees a complete picture -// of where the listing succeeded. -// -// Callers MUST validate the projectResourceID first (e.g., via -// ValidateProjectResourceID) — this function returns a hard error if -// parsing fails so the doctor can surface "AZURE_AI_PROJECT_ID is -// malformed" without rendering an empty listing. -// -// An empty `principals` slice returns a result with empty Entries and -// no error (the caller's check fires a Skip in that case — there is -// nothing to enumerate but the listing path itself is healthy). -func QueryAgentIdentityRoles( - ctx context.Context, - azdClient *azdext.AzdClient, - projectResourceID string, - principals []AgentPrincipal, -) (*AgentIdentityRolesResult, error) { - info, err := parseAgentIdentityInfo(projectResourceID) - if err != nil { - return nil, fmt.Errorf("%w: %w", ErrInvalidProjectResourceID, err) - } - - scopes := AgentIdentityScopes{ - Account: info.AccountScope, - Project: info.ProjectScope, - ResourceGroup: fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", info.SubscriptionID, info.ResourceGroup), - } - - if len(principals) == 0 { - return &AgentIdentityRolesResult{Scopes: scopes}, nil - } - - tenantResponse, err := azdClient.Account().LookupTenant(ctx, &azdext.LookupTenantRequest{ - SubscriptionId: info.SubscriptionID, - }) - if err != nil { - return nil, fmt.Errorf("failed to look up tenant for subscription %s: %w", info.SubscriptionID, err) - } - - cred, err := azidentity.NewAzureDeveloperCLICredential(&azidentity.AzureDeveloperCLICredentialOptions{ - TenantID: tenantResponse.TenantId, - AdditionallyAllowedTenants: []string{"*"}, - }) - if err != nil { - return nil, fmt.Errorf("failed to create Azure credential: %w", err) - } - - return queryAgentIdentityRolesWithLister(ctx, scopes, principals, func(ctx context.Context, scope, principalID string) ([]string, error) { - return listRoleNamesAtScope(ctx, cred, info.SubscriptionID, scope, principalID) - }) -} - -// queryAgentIdentityRolesWithLister is the test-seam-friendly core of -// QueryAgentIdentityRoles. It accepts an injected `lister` so unit -// tests can drive every per-scope branch (success / empty / -// transport-error) without standing up an ARM fake. -// -// Production callers use QueryAgentIdentityRoles, which builds the -// real lister from an AzureDeveloperCLICredential. -func queryAgentIdentityRolesWithLister( - ctx context.Context, - scopes AgentIdentityScopes, - principals []AgentPrincipal, - lister func(ctx context.Context, scope, principalID string) ([]string, error), -) (*AgentIdentityRolesResult, error) { - out := &AgentIdentityRolesResult{ - Entries: make([]AgentIdentityRolesEntry, len(principals)), - Scopes: scopes, - } - - var wg sync.WaitGroup - for i, p := range principals { - wg.Go(func() { - entry := AgentIdentityRolesEntry{ - AgentName: p.AgentName, - AgentVersion: p.AgentVersion, - PrincipalID: p.PrincipalID, - } - if p.PrincipalID == "" { - err := errors.New("principal ID unavailable") - entry.ProjectScope = AgentScopeRoles{Scope: "project", Err: err} - entry.AccountScope = AgentScopeRoles{Scope: "account", Err: err} - entry.RGScope = AgentScopeRoles{Scope: "resource-group", Err: err} - out.Entries[i] = entry - return - } - entry.ProjectScope = probeOneScope(ctx, "project", scopes.Project, p.PrincipalID, lister) - entry.AccountScope = probeOneScope(ctx, "account", scopes.Account, p.PrincipalID, lister) - entry.RGScope = probeOneScope(ctx, "resource-group", scopes.ResourceGroup, p.PrincipalID, lister) - out.Entries[i] = entry - }) - } - wg.Wait() - return out, nil -} - -// probeOneScope wraps a per-scope listing call so the caller's -// AgentIdentityRolesEntry assembly stays a flat three-line composition. -// Returns a non-nil Roles (possibly empty) on success and nil Roles -// with a populated Err on failure. -func probeOneScope( - ctx context.Context, - label, scope, principalID string, - lister func(ctx context.Context, scope, principalID string) ([]string, error), -) AgentScopeRoles { - roles, err := lister(ctx, scope, principalID) - if err != nil { - return AgentScopeRoles{Scope: label, Err: err} - } - if roles == nil { - roles = []string{} - } - return AgentScopeRoles{Scope: label, Roles: roles} -} - -// listRoleNamesAtScope returns the role names assigned to principalID -// at the supplied ARM scope. The function uses ARM's server-side -// `assignedTo()` filter to avoid pulling every assignment in the -// scope, then resolves each role-definition ID into a human-readable -// name (with caching across calls within a single QueryAgentIdentityRoles -// invocation via the wg.Go workers' captured closure — caching is not -// strictly necessary at 3 calls × N agents but trims a few ARM round -// trips when a role is reused). -// -// The function is intentionally tolerant of partial failures: any -// per-assignment resolution error becomes an empty name; the listing -// still returns the rest of the assignments. The doctor surfaces the -// listing as INFO so missing role names are a soft degradation, not -// a hard failure. -func listRoleNamesAtScope( - ctx context.Context, - cred *azidentity.AzureDeveloperCLICredential, - subscriptionID, scope, principalID string, -) ([]string, error) { - if scope == "" { - return nil, fmt.Errorf("empty scope") - } - if principalID == "" { - return nil, fmt.Errorf("empty principal ID") - } - if subscriptionID == "" { - return nil, fmt.Errorf("empty subscription ID") - } - - client, err := armauthorization.NewRoleAssignmentsClient(subscriptionID, cred, nil) - if err != nil { - return nil, fmt.Errorf("failed to create role-assignments client: %w", err) - } - defClient, err := armauthorization.NewRoleDefinitionsClient(cred, nil) - if err != nil { - return nil, fmt.Errorf("failed to create role-definitions client: %w", err) - } - - filter := fmt.Sprintf("assignedTo('%s')", principalID) - pager := client.NewListForScopePager(scope, &armauthorization.RoleAssignmentsClientListForScopeOptions{ - Filter: &filter, - }) - - roleDefIDs := make([]string, 0, 4) - for pager.More() { - page, err := pager.NextPage(ctx) - if err != nil { - return nil, fmt.Errorf("failed to list role assignments at scope %s: %w", scope, err) - } - for _, ra := range page.Value { - if ra.Properties == nil || ra.Properties.RoleDefinitionID == nil { - continue - } - roleDefIDs = append(roleDefIDs, *ra.Properties.RoleDefinitionID) - } - } - - cache := make(map[string]string, len(roleDefIDs)) - names := make([]string, 0, len(roleDefIDs)) - for _, defID := range roleDefIDs { - if cached, ok := cache[defID]; ok { - if cached != "" { - names = append(names, cached) - } - continue - } - name := resolveRoleName(ctx, defClient, defID) - cache[defID] = name - if name != "" { - names = append(names, name) - } - } - return names, nil -} - -// resolveRoleName fetches the human-readable role-definition name for -// a `/.../roleDefinitions/` ARM ID. The scope passed to -// `RoleDefinitions.Get` is the resource scope of the assignment, but -// since the role definition is global to its assignable scope chain -// (typically subscription-level for built-in roles), we use the -// subscription extracted from the role-definition ARM ID itself as -// the listing scope. -// -// Returns "" on any failure — callers omit empty names from the -// rendered listing. This matches the design's principle that the -// check's INFO classification is a soft surface; a missing role name -// should not turn the whole check red. -func resolveRoleName( - ctx context.Context, - defClient *armauthorization.RoleDefinitionsClient, - roleDefID string, -) string { - // Role-definition ARM IDs are of the form: - // /subscriptions//providers/Microsoft.Authorization/roleDefinitions/ - // For built-in roles (which is the common case for agent MIs) - // the listing scope is the subscription. Strip the trailing - // `/providers/...` to derive the scope; on a parse miss, treat - // the ARM ID itself as both scope and name input. - idx := strings.Index(roleDefID, "/providers/") - scope := roleDefID - if idx > 0 { - scope = roleDefID[:idx] - } - name := roleDefID[strings.LastIndex(roleDefID, "/")+1:] - if name == "" { - return "" - } - - resp, err := defClient.Get(ctx, scope, name, nil) - if err != nil { - return "" - } - if resp.Properties == nil || resp.Properties.RoleName == nil { - return "" - } - return *resp.Properties.RoleName -} diff --git a/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_rbac.go b/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_rbac.go index c08e642f996..36b6aea4d29 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_rbac.go +++ b/cli/azd/extensions/azure.ai.agents/internal/project/agent_identity_rbac.go @@ -5,32 +5,15 @@ package project import ( "context" - "errors" "fmt" - "maps" - "net/http" - "os" - "slices" - "strconv" "strings" - "sync" - "time" - "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v3" - "github.com/azure/azure-dev/cli/azd/pkg/azdext" - "github.com/azure/azure-dev/cli/azd/pkg/output" "github.com/google/uuid" ) -const ( - roleAzureAIUser = "53ca6127-db72-4b80-b1b0-d745d6d5456d" - agentIdentitySuffix = "AgentIdentity" - - rbacVerifyMaxAttempts = 12 - rbacVerifyPollInterval = 5 * time.Second -) +const roleAzureAIUser = "53ca6127-db72-4b80-b1b0-d745d6d5456d" // agentIdentityInfo holds the parsed project information needed for agent identity RBAC. type agentIdentityInfo struct { @@ -87,193 +70,6 @@ func parseAgentIdentityInfo(projectResourceID string) (*agentIdentityInfo, error return info, nil } -// EnsureAgentIdentityRBAC assigns the required RBAC roles to per-agent identity service principals. -// This is designed to be called from the postdeploy handler after agent deployment. -// -// EnsureAgentIdentityRBAC assigns Cognitive Services OpenAI Contributor roles to -// each deployed agent's instance identity. The principal ID is expected to be -// provided in the agentIdentities map from the deploy response. If a principal ID -// is empty, the agent is skipped with a warning. -func EnsureAgentIdentityRBAC( - ctx context.Context, - azdClient *azdext.AzdClient, - agentIdentities map[string]string, -) error { - if len(agentIdentities) == 0 { - return nil - } - - envClient := azdClient.Environment() - envResp, err := envClient.GetCurrent(ctx, &azdext.EmptyRequest{}) - if err != nil { - return fmt.Errorf("failed to get current environment: %w", err) - } - envName := envResp.Environment.Name - - skipValue := "" - skipResp, err := envClient.GetValue(ctx, &azdext.GetEnvRequest{ - EnvName: envName, - Key: "AZD_AGENT_SKIP_ROLE_ASSIGNMENTS", - }) - if err == nil { - skipValue = skipResp.Value - } - if skipValue == "" { - skipValue = os.Getenv("AZD_AGENT_SKIP_ROLE_ASSIGNMENTS") - } - if skip, parseErr := strconv.ParseBool(skipValue); parseErr == nil && skip { - fmt.Println(" (-) Skipping agent identity RBAC (AZD_AGENT_SKIP_ROLE_ASSIGNMENTS is set)") - return nil - } - - projectResp, err := envClient.GetValue(ctx, &azdext.GetEnvRequest{ - EnvName: envName, - Key: "AZURE_AI_PROJECT_ID", - }) - if err != nil || projectResp.Value == "" { - return fmt.Errorf("AZURE_AI_PROJECT_ID not set, unable to ensure agent identity RBAC") - } - - info, err := parseAgentIdentityInfo(projectResp.Value) - if err != nil { - return fmt.Errorf("failed to parse project resource ID: %w", err) - } - - // Get tenant ID and create credential - tenantResponse, err := azdClient.Account().LookupTenant(ctx, &azdext.LookupTenantRequest{ - SubscriptionId: info.SubscriptionID, - }) - if err != nil { - return fmt.Errorf("failed to get tenant ID: %w", err) - } - - cred, err := azidentity.NewAzureDeveloperCLICredential(&azidentity.AzureDeveloperCLICredentialOptions{ - TenantID: tenantResponse.TenantId, - AdditionallyAllowedTenants: []string{"*"}, - }) - if err != nil { - return fmt.Errorf("failed to create Azure credential: %w", err) - } - - return ensureAgentIdentityRBACWithCred(ctx, cred, info, agentIdentities) -} - -// ensureAgentIdentityRBACWithCred performs the core per-agent identity RBAC logic using -// the provided credential. For each agent, it assigns Azure AI User scoped to the Foundry Project. -// The principal ID must be provided from the deploy response. Agents are processed in parallel. -func ensureAgentIdentityRBACWithCred( - ctx context.Context, - cred *azidentity.AzureDeveloperCLICredential, - info *agentIdentityInfo, - agentIdentities map[string]string, -) error { - fmt.Println() - fmt.Println("Agent Identity RBAC") - fmt.Printf(" AI Account: %s\n", info.AccountName) - fmt.Printf(" Project: %s\n", info.ProjectName) - fmt.Printf(" Agents: %d\n", len(agentIdentities)) - - // Process agents in parallel — each has an independent identity. - type agentResult struct { - name string - err error - } - - sortedNames := slices.Sorted(maps.Keys(agentIdentities)) - results := make([]agentResult, len(sortedNames)) - var wg sync.WaitGroup - - for i, agentName := range sortedNames { - principalID := agentIdentities[agentName] - wg.Add(1) - go func(i int, name, pid string) { - defer wg.Done() - results[i] = agentResult{ - name: name, - err: ensureSingleAgentRBAC(ctx, cred, info, name, pid), - } - }(i, agentName, principalID) - } - - wg.Wait() - - // Report results in order and return first error. - for _, r := range results { - if r.err != nil { - return fmt.Errorf("agent identity RBAC failed for %q: %w", r.name, r.err) - } - } - - fmt.Println() - fmt.Println("✓ Agent identity RBAC complete") - return nil -} - -// ensureSingleAgentRBAC handles role assignment for a single agent. -// The principalID must be provided from the deploy response's instance identity. -func ensureSingleAgentRBAC( - ctx context.Context, - cred *azidentity.AzureDeveloperCLICredential, - info *agentIdentityInfo, - agentName string, - principalID string, -) error { - if principalID == "" { - fmt.Printf(" %s\n", output.WithErrorFormat( - "ERROR: agent %q has no instance identity principal ID — skipping RBAC assignment. ", agentName, - )) - return nil - } - - fmt.Printf(" Agent identity: %s (principal: %s)\n", agentName, principalID) - - created, err := assignRoleToIdentity( - ctx, cred, principalID, roleAzureAIUser, "Azure AI User → Foundry Project", info.ProjectScope, - armauthorization.PrincipalTypeServicePrincipal, - ) - if err != nil { - if respErr, ok := errors.AsType[*azcore.ResponseError](err); ok && - respErr.StatusCode == http.StatusForbidden { - manualRemediationCommand := fmt.Sprintf( - "az role assignment create --assignee-object-id %s --assignee-principal-type ServicePrincipal --role %s --scope %s", - strconv.Quote(principalID), - strconv.Quote("Azure AI User"), - strconv.Quote(info.ProjectScope), - ) - - // Write with warning color so it appears as a yellow warning, not a red error. - fmt.Printf("%s\n", output.WithWarningFormat( - "Could not assign 'Azure AI User' to agent identity '%s' (403 Forbidden).\n"+ - " The agent may not have access to the Foundry Project until this role is assigned.\n"+ - " Principal ID: %s\n"+ - " Foundry Project scope: %s\n"+ - " To remediate manually, run:\n"+ - " %s\n"+ - " Re-run after granting role assignment write, or set AZD_AGENT_SKIP_ROLE_ASSIGNMENTS=true.", - agentName, - principalID, - info.ProjectScope, - manualRemediationCommand, - )) - return nil - } - return fmt.Errorf("failed to assign Azure AI User role: %w", err) - } - - if created { - fmt.Println(" ✓ Azure AI User → Foundry Project (created)") - fmt.Println(" ⏳ Verifying Azure AI User...") - if err := verifyRoleAssignment(ctx, cred, principalID, roleAzureAIUser, info.ProjectScope); err != nil { - return fmt.Errorf("failed to verify Azure AI User role assignment: %w", err) - } - fmt.Println(" ✓ Azure AI User → Foundry Project (verified)") - } else { - fmt.Println(" ✓ Azure AI User → Foundry Project (already assigned)") - } - - return nil -} - // assignRoleToIdentity assigns a single RBAC role to a principal at the given scope. // It is idempotent: existing assignments are detected and skipped. // Returns true if a new assignment was created, false if it already existed. @@ -352,63 +148,6 @@ func assignRoleToIdentity( return true, nil } -// verifyRoleAssignment polls until the given role assignment is visible in the scope's -// role assignment list, confirming that RBAC propagation has completed. -func verifyRoleAssignment( - ctx context.Context, - cred *azidentity.AzureDeveloperCLICredential, - principalID string, - roleID string, - scope string, -) error { - subscriptionID := extractSubscriptionID(scope) - if subscriptionID == "" { - return fmt.Errorf("could not extract subscription ID from scope: %s", scope) - } - - client, err := armauthorization.NewRoleAssignmentsClient(subscriptionID, cred, nil) - if err != nil { - return fmt.Errorf("failed to create role assignments client: %w", err) - } - - roleDefinitionSuffix := fmt.Sprintf("/roleDefinitions/%s", roleID) - - for attempt := range rbacVerifyMaxAttempts { - pager := client.NewListForScopePager(scope, &armauthorization.RoleAssignmentsClientListForScopeOptions{ - Filter: new("atScope()"), - }) - - for pager.More() { - page, err := pager.NextPage(ctx) - if err != nil { - return fmt.Errorf("failed to list role assignments: %w", err) - } - for _, assignment := range page.Value { - if assignment.Properties != nil && - assignment.Properties.PrincipalID != nil && - assignment.Properties.RoleDefinitionID != nil && - *assignment.Properties.PrincipalID == principalID && - strings.HasSuffix(*assignment.Properties.RoleDefinitionID, roleDefinitionSuffix) { - return nil - } - } - } - - if attempt < rbacVerifyMaxAttempts-1 { - select { - case <-ctx.Done(): - return ctx.Err() - case <-time.After(rbacVerifyPollInterval): - // wait for the next polling attempt - } - } - } - - return fmt.Errorf( - "role assignment not visible after %d attempts (principal: %s, role: %s, scope: %s)", - rbacVerifyMaxAttempts, principalID, roleID, scope) -} - // extractSubscriptionID pulls the subscription ID from an Azure resource ID. func extractSubscriptionID(resourceID string) string { parts := strings.Split(resourceID, "/") From 58cc338172700ebf3de8765f5364423802e7b99d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wei=20Meng=E2=9A=94=EF=B8=8F?= Date: Fri, 3 Jul 2026 12:23:44 +0800 Subject: [PATCH 2/2] fix(agents): don't fail post-deploy hook on missing optimization telemetry env Addresses PR #8941 review follow-up: with the client-side agent-identity RBAC assignment removed, the endpoint/tenant/credential resolution in postdeployHandler now feeds only best-effort optimization reporting (wrapped in recover, never propagated). Downgrade the FOUNDRY_PROJECT_ENDPOINT / AZURE_TENANT_ID / credential setup guards from hard errors to logged warnings + return nil, so missing optimization telemetry inputs can no longer fail an otherwise-successful deploy. Adds TestPostdeployHandler_MissingTelemetryEnv_ReturnsNil covering the missing/ empty endpoint and missing tenant cases. --- .../azure.ai.agents/internal/cmd/listen.go | 27 ++++++-- .../internal/cmd/listen_test.go | 65 +++++++++++++++++++ 2 files changed, 86 insertions(+), 6 deletions(-) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/listen.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/listen.go index 9ac9bc0bbbc..3dc4c61368f 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/listen.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/listen.go @@ -295,9 +295,14 @@ func postdeployHandler(ctx context.Context, azdClient *azdext.AzdClient, args *a } // Set up the project endpoint and credential used by optimization reporting. + // 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: %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 @@ -308,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. @@ -320,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( @@ -333,7 +346,9 @@ func postdeployHandler(ctx context.Context, azdClient *azdext.AzdClient, args *a }, ) if err != nil { - return fmt.Errorf("failed to create credential: %w", err) + log.Printf("postdeploy: skipping optimization reporting for %s: "+ + "failed to create credential: %v", svc.Name, err) + return nil } // Report optimization candidate deployment (best-effort: panics are logged, not propagated). diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/listen_test.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/listen_test.go index d8f3e2f41bf..81d63020a88 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/listen_test.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/listen_test.go @@ -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()