-
Notifications
You must be signed in to change notification settings - Fork 35
Stop requesting Observability API permissions for blueprint agents by default #501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
bacea97
2d6f389
2b5766a
9e72b1b
cd937db
3d54061
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,8 +20,8 @@ namespace Microsoft.Agents.A365.DevTools.Cli.Commands.SetupSubcommands; | |
| /// 1. Requirements validation | ||
| /// 2. Blueprint creation (shared with DW) | ||
| /// 3. Batch permissions on the blueprint (shared with DW pipeline; non-DW spec set: | ||
| /// Observability API, Power Platform API, custom). MAC reads from the blueprint, | ||
| /// so stamping here gives the same set visibility there. | ||
| /// Power Platform API and custom; Observability API is not requested). MAC reads | ||
| /// from the blueprint, so stamping here gives the same set visibility there. | ||
| /// 4. Agent Identity creation via POST /beta/servicePrincipals/Microsoft.Graph.AgentIdentity | ||
| /// 5. Agent Identity permission grants (same spec set as step 3) — OBO or S2S | ||
| /// 6. Agent registration via Graph API (copilot/agentRegistrations) | ||
|
|
@@ -33,7 +33,7 @@ internal static class NonDwBlueprintSetupOrchestrator | |
| /// Prints a dry-run plan showing all resources that would be created or configured, | ||
| /// using actual names and values from the loaded config. Makes no API calls. | ||
| /// </summary> | ||
| public static void PrintDryRunPlan(Agent365Config config, ILogger logger, bool isBootstrap = false, string[]? rawArgs = null, bool skipRequirements = false, bool isM365 = false, bool agentRegistrationOnly = false, string? authMode = null, string? messagingEndpointOverride = null) | ||
| public static void PrintDryRunPlan(Agent365Config config, ILogger logger, bool isBootstrap = false, string[]? rawArgs = null, bool skipRequirements = false, bool isM365 = false, bool agentRegistrationOnly = false, string? authMode = null, string? messagingEndpointOverride = null, bool skipObservabilityPermissions = false) | ||
| { | ||
| var sub = new string(' ', SetupHelpers.DryRunValCol); | ||
| // --messaging-endpoint flag (if supplied) wins over the init-only config value for the plan. | ||
|
|
@@ -117,14 +117,17 @@ public static void PrintDryRunPlan(Agent365Config config, ILogger logger, bool i | |
| logger.LogInformation(sub + "create managed identity"); | ||
| } | ||
|
|
||
| // 3. Inheritable Permissions — non-DW spec set (Observability API, Power Platform API, custom) | ||
| // stamped on the blueprint via SetInheritablePermissionsAsync so MAC and other dependent | ||
| // systems can see them. The same set is applied to the agent identity SP in step 5. | ||
| // 3. Inheritable Permissions — non-DW spec set (Power Platform API and custom; Observability API is | ||
| // not requested) stamped on the blueprint via SetInheritablePermissionsAsync so MAC and other | ||
| // dependent systems can see them. The same set is applied to the agent identity SP in step 5. | ||
| var selectedAuthMode = authMode ?? config.AuthMode; | ||
| var effectiveMode = string.IsNullOrWhiteSpace(selectedAuthMode) | ||
| ? "obo" | ||
| : selectedAuthMode.Trim().ToLowerInvariant(); | ||
| logger.LogInformation(SetupHelpers.DryRunRow(3, "Inheritable Permissions") + "configure for Observability API, Power Platform API, and custom permissions (Global Administrator required; consent URL printed if absent)"); | ||
| logger.LogInformation(SetupHelpers.DryRunRow(3, "Inheritable Permissions") + "configure for {Resources} (Global Administrator required; consent URL printed if absent)", | ||
| skipObservabilityPermissions ? "Power Platform API and custom permissions" : "Observability API, Power Platform API, and custom permissions"); | ||
| if (skipObservabilityPermissions) | ||
| logger.LogInformation(sub + "Observability API not requested (registered agents export telemetry with an app-only token)"); | ||
|
|
||
| // 4. Blueprint Permission Grants — per authMode. The consent URL targets the blueprint | ||
| // app, and S2S app-role assignments are persisted as grants flowing from the blueprint; | ||
|
|
@@ -266,6 +269,7 @@ await ctx.ClientAppValidator.GrantConsentForPermissionsAsync( | |
| public static async Task<int> ExecuteAsync(SetupContext ctx) | ||
| { | ||
| ctx.Results.IsNonDwBlueprintFlow = true; | ||
| ctx.Results.ObservabilityPermissionsSkipped = ctx.SkipObservabilityPermissions; | ||
| ctx.Results.TenantId = ctx.Config.TenantId; | ||
| // Bootstrap already printed the "Running..." banner before auth steps; skip here to avoid duplication. | ||
| if (!ctx.IsBootstrap) | ||
|
|
@@ -362,8 +366,10 @@ public static async Task<int> ExecuteAsync(SetupContext ctx) | |
| // Step 3: Blueprint creation (shared with DW) | ||
| await AllSubcommand.ExecuteBlueprintStepAsync(ctx); | ||
|
|
||
| // Step 4: Build permission specs — stamps Graph, manifest MCP audiences, Observability, | ||
| // Power Platform, custom permissions, and Messaging Bot (only when isM365). Mirrors DW. | ||
| // Step 4: Build permission specs — stamps Graph, manifest MCP audiences, Power Platform, | ||
| // custom permissions, Messaging Bot (only when isM365), and Observability unless skipped. | ||
| if (ctx.SkipObservabilityPermissions) | ||
| ctx.Logger.LogInformation("Observability API permissions not requested: registered agents export telemetry with an app-only token."); | ||
| var buildResult = await AllSubcommand.BuildPermissionSpecsAsync(ctx); | ||
| specs = buildResult.specs; | ||
|
|
||
|
|
@@ -435,7 +441,7 @@ await AllSubcommand.ExecuteBatchPermissionsStepAsync( | |
| /// When <paramref name="skipIdentityAndPermissions"/> is true (--agent-registration-only), | ||
| /// identity creation and permission grants are skipped — only registration and project settings run. | ||
| /// </summary> | ||
| private static async Task ExecuteAgentIdentityAndRegistrationAsync( | ||
| internal static async Task ExecuteAgentIdentityAndRegistrationAsync( | ||
| SetupContext ctx, | ||
| List<ResourcePermissionSpec> specs, | ||
| bool skipIdentityAndPermissions = false) | ||
|
|
@@ -549,22 +555,31 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync( | |
| ctx.Logger.LogInformation(""); | ||
| ctx.Logger.LogInformation("Registering agent..."); | ||
|
|
||
| // Registration is the sole purpose of --agent-registration-only and, with OtelWrite skipped, | ||
| // the agent's only Observability authorization, so its failure must fail setup. | ||
| var registrationRequired = skipIdentityAndPermissions || ctx.SkipObservabilityPermissions; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: in real runs, |
||
| void RecordRegistrationFailure(string message) | ||
| { | ||
| ctx.Results.AgentRegistrationFailed = true; | ||
| (registrationRequired ? ctx.Results.Errors : ctx.Results.Warnings).Add(message); | ||
| ctx.Logger.Log(registrationRequired ? LogLevel.Error : LogLevel.Warning, message); | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(ctx.Config.AgenticAppId)) | ||
| { | ||
| var registrationSkippedMessage = | ||
| "Agent registration failed: agent identity ID is not available. " + | ||
| "Ensure the agent identity was created successfully, then retry with: a365 setup all --agent-registration-only"; | ||
| ctx.Results.Warnings.Add(registrationSkippedMessage); | ||
| using (ctx.Logger.Indent()) | ||
| ctx.Logger.LogWarning(registrationSkippedMessage); | ||
| ctx.Results.AgentRegistrationFailed = true; | ||
| RecordRegistrationFailure(registrationSkippedMessage); | ||
| } | ||
| else | ||
| { | ||
|
|
||
| // If a registration ID is already stored, verify it still exists before skipping creation. | ||
| string? registrationId = null; | ||
| bool registrationAlreadyExisted = false; | ||
| bool verificationFailed = false; | ||
|
|
||
| if (!string.IsNullOrWhiteSpace(ctx.Config.AgentRegistrationId)) | ||
| { | ||
|
|
@@ -591,6 +606,16 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync( | |
| // stale value on disk that would cause the same stale-ID check to repeat. | ||
| await ctx.ConfigService.SaveStateAsync(ctx.Config); | ||
| } | ||
| else if (registrationRequired) | ||
| { | ||
| // An unverifiable registration cannot be the agent's only authorization: keep the stored | ||
| // ID (no duplicate registration) but fail so the operator retries. | ||
| using (ctx.Logger.Indent()) | ||
| RecordRegistrationFailure( | ||
| $"Could not verify agent registration {ctx.Config.AgentRegistrationId} (auth or transient error). " + | ||
| "Retry with: a365 setup all --agent-registration-only"); | ||
| verificationFailed = true; | ||
| } | ||
| else | ||
| { | ||
| // Verification inconclusive (auth or transient error) — preserve the stored ID | ||
|
|
@@ -602,7 +627,7 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync( | |
| } | ||
| } | ||
|
|
||
| if (string.IsNullOrWhiteSpace(registrationId)) | ||
| if (!verificationFailed && string.IsNullOrWhiteSpace(registrationId)) | ||
| { | ||
| var (newId, fromConflict) = await ctx.GraphApiService.RegisterAgentInstanceAsyncV2( | ||
| ctx.Config.TenantId!, | ||
|
|
@@ -631,11 +656,9 @@ private static async Task ExecuteAgentIdentityAndRegistrationAsync( | |
| ctx.Logger.LogInformation(""); | ||
| } | ||
| } | ||
| else | ||
| else if (!verificationFailed) | ||
| { | ||
| ctx.Results.AgentRegistrationFailed = true; | ||
| ctx.Results.Warnings.Add("Agent registration failed via Graph copilot/agentRegistrations API."); | ||
| ctx.Logger.LogWarning("Agent registration failed via Graph copilot/agentRegistrations API."); | ||
| RecordRegistrationFailure("Agent registration failed via Graph copilot/agentRegistrations API."); | ||
| } | ||
|
|
||
| } // end else (AgenticAppId present) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
registrationRequiredcovers the create-failure path, but the inconclusive-verification branch below (AgentRegistrationExistsAsyncreturns null, "retaining stored value") still setsregistrationAlreadyExisted = trueand setup exits 0. With--skip-observability-permissionsregistration is the agent's only authorization, so an auth or transient failure there should be an error rather than a pass. Please treat the null case as an error whenregistrationRequiredis true, and add a test where the check returns null.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9e72b1b. When registration is required (
--agent-registration-only, or Observability permissions not requested, which is now every blueprint agent), a null fromAgentRegistrationExistsAsyncis recorded as an error and setup exits 1. The stored ID is kept and no duplicate registration is created. The optional path still retains the ID as before. Regression tests:Step6_RegistrationRequired_FailsWithoutReRegistering_WhenVerificationIsInconclusivecovers both required modes, and the existing retain test now runs on the optional path with unchanged assertions.