feat: Add support for Entra Application by theunrepentantgeek · Pull Request #5232 · Azure/azure-service-operator

theunrepentantgeek · 2026-03-12T23:05:08Z

What this PR does

Adds support for Entra Application as a new resource type.

How does this PR make you feel?

theunrepentantgeek

Deep Adversarial Code Review

Three independent review perspectives (Advocate, Skeptic, Architect) analyzed this PR. Findings synthesized below, posted as inline comments at relevant locations.

Priority	Count	Summary
🔴 Critical	1	Nil pointer panic on external deletion
🟠 High	4	OData injection, can't clear fields, FEATURE.md, dead field
🟡 Medium	3	Status from pre-PATCH, missing configmap save, DisplayName validation
🔵 Low	3	Extract shared reconciler, decouple SDK, cosmetic consistency

Verified robust: Annotation persistence on partial create failure ✅, Delete path ✅, Webhook defaulting ✅

The FEATURE.md file is an implementation plan that belongs in the PR description or issue, not in the repository root alongside README.md and CONTRIBUTING.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The CredentialProvider field is declared on both EntraApplicationReconciler and EntraSecurityGroupReconciler but never populated by their constructors and never referenced anywhere. The credential flow goes through EntraClientFactory → EntraClientCache.GetConnection() → the provider, making this field unnecessary. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Replace reconcile.Result{} with ctrl.Result{} in both the Application and SecurityGroup reconcilers for consistency. They are the same underlying type, but all other return sites use ctrl.Result{}. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Extract the duplicated isNotFound method from both EntraApplicationReconciler and EntraSecurityGroupReconciler into a shared package-level function in errors.go. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

loadApplicationByID returns (nil, nil) for 404 errors, not an error. The isNotFound(err) check was dead code since err is always nil for a 404. The nil Applicationable would reach AssignToApplication and panic. Fix by checking for nil result after the error check instead of checking isNotFound on the error. Apply same fix to SecurityGroup reconciler which has the same pattern. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Display names containing single quotes (e.g., "O'Brien's App") produce malformed OData filters, causing 400 errors from the Graph API. Escape single quotes by doubling them per OData conventions. Fix applied to both Application and SecurityGroup reconcilers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The PATCH return value was discarded and status was populated from the locally-mutated model. If the Graph API normalizes, rejects, or augments any field, status would diverge from reality. Now use the server response from PATCH to populate status, ensuring it reflects the authoritative server state. Applied to both Application and SecurityGroup reconcilers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

The create() path saves ConfigMaps but update() did not, meaning ConfigMaps would not be refreshed after an update until the next UpdateStatus() cycle. Add the call for symmetry with create(). Applied to both Application and SecurityGroup reconcilers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Previously, nil/empty guards meant that removing a field from the spec (e.g., clearing identifierUris, tags, web, spa, or publicClient) would never send the empty value to the API - the field would persist unchanged in Entra. Now all fields are always set so that clearing a value in the K8s spec correctly propagates to the Graph API. Same fix applied to sub-type assignment methods (Web, Spa, PublicClient). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Co-authored-by: Copilot <copilot@github.com>

Copilot

Pull request overview

Adds a new hand-written Entra (Azure AD) Application resource to ASO v2 (non-ARM), including controller reconciliation via Microsoft Graph, webhooks, samples, docs, and record/replay tests to validate CRUD behavior.

Changes:

Introduces entra.azure.com/v1 Application API type, webhook defaulting/validation, and controller registration.
Implements a Microsoft Graph-based reconciler for Applications (adopt/create/update/delete) and updates SecurityGroup reconciler behavior.
Adds samples, docs reference entries, and test recordings + a new CRUD test for Entra Applications.

Reviewed changes

Copilot reviewed 15 out of 17 changed files in this pull request and generated 9 comments.

Show a summary per file

File	Description
v2/tools/generator/internal/astbuilder/comments.go	Minor linter directive formatting adjustment.
v2/samples/entra/v1/v1_application.yaml	New sample manifest for Entra Application.
v2/internal/testsamples/recordings/Test_Samples_CreationAndDeletion/Test_Entra_v1_CreationAndDeletion.yaml	Updated sample-test VCR recording to include Application interactions.
v2/internal/reconcilers/entra/odata.go	Adds OData string escaping helper for Graph filters.
v2/internal/reconcilers/entra/errors.go	Adds shared Graph “not found” detection helper.
v2/internal/reconcilers/entra/entra_securitygroup_reconciler.go	Refactors not-found handling + adds status/resource export behavior changes.
v2/internal/reconcilers/entra/entra_application_reconciler.go	New reconciler implementing CRUD/adoption for Entra Applications + configmap export.
v2/internal/controllers/recordings/Test_Entra_SecurityGroup_v1_CRUD.yaml	Updated SecurityGroup controller VCR recording.
v2/internal/controllers/recordings/Test_Entra_Application_v1_CRUD.yaml	New Application controller VCR recording.
v2/internal/controllers/entra_application_v1_test.go	New controller CRUD test for Entra Application + configmap assertions.
v2/internal/controllers/controller_resources.go	Registers Application storage type and webhook.
v2/azure-arm/entra.yaml	Marks Application as supported from v2.20.0 in Entra group config.
v2/api/entra/v1/zz_generated.deepcopy.go	Generated deepcopy updates for new Application types.
v2/api/entra/v1/webhook/application_webhook_types.go	Adds defaulting + validator webhook for Application.
v2/api/entra/v1/application_types.go	Defines Application spec/status and Graph model assignment helpers.
docs/hugo/content/reference/entra/_index.md	Documents Application availability and links to sample.
docs/hugo/content/reference/_index.md	Adds Application to the global reference index.

Files not reviewed (1)

v2/api/entra/v1/zz_generated.deepcopy.go: Generated file

+            - chunked
+        content_length: -1
+        uncompressed: true
+        body: '{"@odata.context":"https://graph.microsoft.com/v1.0/$metadata#applications/$entity","id":"12bffd98-e5b5-4a9e-a8c1-25ac9b532f27","deletedDateTime":null,"appId":"e9cd4cdd-db0c-4d5e-a4f1-3b189514f799","applicationTemplateId":null,"disabledByMicrosoftStatus":null,"createdByAppId":"04b07795-8ddb-461a-bbee-02f9e1bf7b46","createdDateTime":"2001-02-03T04:05:06Z","displayName":"ASO Test Application","description":null,"groupMembershipClaims":null,"identifierUris":[],"isDeviceOnlyAuthSupported":null,"isDisabled":null,"isFallbackPublicClient":null,"nativeAuthenticationApisEnabled":null,"notes":null,"publisherDomain":"bevanarpsoutlook.onmicrosoft.com","serviceManagementReference":null,"signInAudience":"AzureADMyOrg","tags":[],"tokenEncryptionKeyId":null,"uniqueName":null,"samlMetadataUrl":null,"defaultRedirectUri":null,"certification":null,"optionalClaims":null,"servicePrincipalLockConfiguration":null,"requestSignatureVerification":null,"addIns":[],"api":{"acceptMappedClaims":null,"knownClientApplications":[],"requestedAccessTokenVersion":null,"oauth2PermissionScopes":[],"preAuthorizedApplications":[]},"appRoles":[],"info":{"logoUrl":null,"marketingUrl":null,"privacyStatementUrl":null,"supportUrl":null,"termsOfServiceUrl":null},"keyCredentials":[],"parentalControlSettings":{"countriesBlockedForMinors":[],"legalAgeGroupRule":"Allow"},"passwordCredentials":[],"publicClient":{"redirectUris":[]},"requiredResourceAccess":[],"verifiedPublisher":{"displayName":null,"verifiedPublisherId":null,"addedDateTime":null},"web":{"homePageUrl":null,"logoutUrl":null,"redirectUris":[],"implicitGrantSettings":{"enableAccessTokenIssuance":false,"enableIdTokenIssuance":false},"redirectUriSettings":[]},"spa":{"redirectUris":[]}}'


+            Content-Type:
+                - application/json;odata.metadata=minimal;odata.streaming=true;IEEE754Compatible=false;charset=utf-8
+            Location:
+                - https://graph.microsoft.com/v1.0/9b0c84c7-21c4-4c92-ae62-d9c1eb53333f/directoryObjects/12bffd98-e5b5-4a9e-a8c1-25ac9b532f27/Microsoft.DirectoryServices.Application


matthchr · 2026-06-18T17:30:00Z

+	app, err := r.asApplication(obj)
+	if err != nil {
+		return ctrl.Result{}, eris.Wrapf(err, "creating or updating application %s", app.Name)
+	}


This should probably be fixed

Applies throughout

+	app, err := r.asApplication(obj)
+	if err != nil {
+		return ctrl.Result{}, eris.Wrapf(err, "deleting application %s", app.Name)
+	}


+	app, err := r.asApplication(obj)
+	if err != nil {
+		return eris.Wrapf(err, "updating status of application %s", app.Name)
+	}


+	if result == nil {
+		// Didn't get a result back from the patch, load the group again to get the latest state
+		log.V(Status).Info("No result returned from update, reloading group to get latest state")
+
+		result, err = r.loadGroupByID(ctx, id, client.Client())
+		if err != nil {
+			return ctrl.Result{}, eris.Wrapf(err, "getting group by ID %s after update returned no result", id)
+		}
+	}


 	groupable, err := r.loadGroupByID(ctx, id, client.Client())
 	if err != nil {
 		// If the group doesn't exist, nothing to do as we're probably in the midst of deleting it
-		if r.isNotFound(err) {
+		if isNotFound(err) {
 			return nil


matthchr · 2026-06-18T17:05:53Z

+	model.SetGroupMembershipClaims(spec.GroupMembershipClaims)
+}
+
+type ApplicationStatus struct {


How did you decide what to put in status, versus what not to?

matthchr · 2026-06-18T17:06:26Z

+}
+
+func (status *ApplicationStatus) AssignFromApplication(model models.Applicationable) {
+	if model != nil {


minor, style:

you could avoid nesting by bailing out early if model != nil

if model != nil { return }

matthchr · 2026-06-18T17:30:00Z

+	app, err := r.asApplication(obj)
+	if err != nil {
+		return ctrl.Result{}, eris.Wrapf(err, "creating or updating application %s", app.Name)
+	}


This should probably be fixed

matthchr · 2026-06-18T17:32:03Z

 )

 // EntraSecurityGroupReconciler reconciles an Entra security group.
 // TODO: Factor out common code shared with other Entra resources into entraGenericReconciler


Is this something that makes sense to do as part of this PR?

Related to https://github.com/Azure/azure-service-operator/pull/5232/changes#r3439149410

matthchr · 2026-06-18T17:32:22Z

+	app, err := r.asApplication(obj)
+	if err != nil {
+		return ctrl.Result{}, eris.Wrapf(err, "creating or updating application %s", app.Name)
+	}


Applies throughout

matthchr · 2026-06-18T17:45:16Z

+		}
+
+		var displayName string
+		if app.Spec.DisplayName != nil {


displayName = to.FromPtr() would work here, though see comment below I dont even think you need/want this naem at all?

matthchr · 2026-06-18T21:46:15Z

+	}
+
+	if result == nil {
+		// Didn't get a result back from the patch, load the application again to get the latest state


Can you expand... is this expected? I don't fully understand what "no response from patch" means in terms of if it was expected or what ends up happening (which is that we load it and then return?)

matthchr · 2026-06-18T22:02:57Z

+	// Try to find the application by DisplayName
+	displayName := app.Spec.DisplayName
+	if to.Value(displayName) == "" {
+		// Can't adopt without a display name


Is this an error condition? Or success?

Seems unexpected, should this be an error?

matthchr · 2026-06-18T22:07:48Z

+
+// saveAssociatedKubernetesResources retrieves Kubernetes resources to create and saves them to Kubernetes.
+// If there are no resources to save this method is a no-op.
+func (r *EntraApplicationReconciler) saveAssociatedKubernetesResources(


Duplicate from EntraSecurityGroupReconciler? Wondering if we can combine some of this w/ like an interface that exposes operatorSpec.ConfigMaps? Similar to what we do with the generic reconciler?

matthchr · 2026-06-18T22:09:22Z

 		return ctrl.Result{}, eris.Wrapf(err, "getting group by ID %s", id)
 	}

+	if g == nil {


minor: why move this?

theunrepentantgeek added 3 commits March 13, 2026 11:53

Initial plan

dbc97c3

Initial commit

105c94f

Update test recordings

178491a

github-project-automation Bot added this to Azure Service Operator Roadmap Mar 12, 2026

github-project-automation Bot moved this to In Progress in Azure Service Operator Roadmap Mar 12, 2026

theunrepentantgeek self-assigned this Mar 15, 2026

theunrepentantgeek commented Apr 8, 2026

View reviewed changes

theunrepentantgeek and others added 19 commits June 11, 2026 13:11

Merge main

571218e

Update docs

6a704cf

Update deepcopy files

b9d489e

Reformat code

6eac993

Remove implementation plan document from repo root

87b6c33

The FEATURE.md file is an implementation plan that belongs in the PR description or issue, not in the repository root alongside README.md and CONTRIBUTING.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Extract isNotFound as a package-level helper function

676b816

Extract the duplicated isNotFound method from both EntraApplicationReconciler and EntraSecurityGroupReconciler into a shared package-level function in errors.go. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Add EoLn at EoF

9ca989f

Improve logging

d5f07a7

Bugfix pass

31df27d

Co-authored-by: Copilot <copilot@github.com>

Format code

822d325

Update sample recording

e58c9e0

Fix Security Group bug

656775f

Co-authored-by: Copilot <copilot@github.com>

theunrepentantgeek commented Jun 16, 2026

View reviewed changes

Comment thread v2/azure-arm/entra.yaml Outdated

theunrepentantgeek marked this pull request as ready for review June 16, 2026 03:16

theunrepentantgeek requested review from matthchr and tallaxes as code owners June 16, 2026 03:16

Copilot AI review requested due to automatic review settings June 16, 2026 03:16

theunrepentantgeek assigned matthchr and unassigned theunrepentantgeek Jun 16, 2026

Copilot started reviewing on behalf of theunrepentantgeek June 16, 2026 03:17 View session

Copilot AI reviewed Jun 16, 2026

View reviewed changes

theunrepentantgeek added this to the v2.20.0 milestone Jun 18, 2026

matthchr reviewed Jun 18, 2026

View reviewed changes

theunrepentantgeek assigned theunrepentantgeek and unassigned matthchr Jun 19, 2026

theunrepentantgeek changed the title ~~Add support for Entra Application~~ feat: Add support for Entra Application Jun 24, 2026

theunrepentantgeek modified the milestones: v2.20.0, v2.21.0 Jun 25, 2026

This was referenced Jun 26, 2026

Feature: Add support for Entra Applications #5217

Open

feat: Entra specific redaction #5499

Merged

Uh oh!

Conversation

theunrepentantgeek commented Mar 12, 2026 • edited by matthchr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What this PR does

How does this PR make you feel?

Uh oh!

theunrepentantgeek left a comment

Choose a reason for hiding this comment

Deep Adversarial Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

theunrepentantgeek commented Mar 12, 2026 •

edited by matthchr

Loading