microsoftgraph/msgraph-sdk-go

Panic while parsing Administrative Unit data

djuneja-uptycs opened this issue · 5 comments

djuneja-uptycs commented

Description:

  • Our use case requires us to fetch scoped role memberships information for all the Active Directory (now Entra ID) Administrative Units, but we are getting a panic during the serialisation of the response. Below is the error we are seeing:
panic: The writer has already been closed. Call Reset instead of Close to reuse it or instantiate a new one.

goroutine 8245 [running]:
github.com/microsoft/kiota-serialization-json-go.(*JsonSerializationWriter).getWriter(...)
  /home/ubuntu/cloudquery/go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.4/json_serialization_writer.go:42
github.com/microsoft/kiota-serialization-json-go.(*JsonSerializationWriter).writeRawValue(0x103e827e?, {0xc0184bfc80?, 0xc0184bfcd0?, 0x2?})
  /home/ubuntu/cloudquery/go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.4/json_serialization_writer.go:48 +0x7e
github.com/microsoft/kiota-serialization-json-go.(*JsonSerializationWriter).writePropertyName(...)
  /home/ubuntu/cloudquery/go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.4/json_serialization_writer.go:64
github.com/microsoft/kiota-serialization-json-go.(*JsonSerializationWriter).WriteStringValue(0xc013762890?, {0x103e827e?, 0x2}, 0xc01509d070)
  /home/ubuntu/cloudquery/go/pkg/mod/github.com/microsoft/kiota-serialization-json-go@v1.0.4/json_serialization_writer.go:86 +0x8c
github.com/microsoftgraph/msgraph-sdk-go/models.(*Entity).Serialize(0x47cc4a?, {0x118efe78, 0xc01d17c880})
  /home/ubuntu/cloudquery/go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go@v1.14.0/models/entity.go:1507 +0x48
github.com/microsoftgraph/msgraph-sdk-go/models.(*ScopedRoleMembership).Serialize(0xc01509d500, {0x118efe78, 0xc01d17c880})
  /home/ubuntu/cloudquery/go/pkg/mod/github.com/microsoftgraph/msgraph-sdk-go@v1.14.0/models/scoped_role_membership.go:92 +0x28
github.com/Uptycs/uptycs-cloudquery/extension/azure/active_directory.GetScopedRoleMembersInfo({0xc01ab86a80, 0x2, 0xc019f15610?})
  /home/ubuntu/workspace/release-build/extension/azure/active_directory/azure_active_directory_administrative_unit.go:469 +0x12c
github.com/Uptycs/uptycs-cloudquery/extension/azure/active_directory.(*UptAdministrativeUnitTable).getInventory(0xc0011ee6c0, {0x11863890, 0xc00744d3e0}, {0xc015501950?}, 0xc004f140c0)
  /home/ubuntu/workspace/release-build/extension/azure/active_directory/azure_active_directory_administrative_unit.go:321 +0x13f2
github.com/Uptycs/uptycs-cloudquery/extension/azure/active_directory.(*UptAdministrativeUnitTable).GetFullInventory(0xc00319f160?, {0x11863890?, 0xc00744d3e0?}, {0xc00022f1d0?}, 0xa9?)
  /home/ubuntu/workspace/release-build/extension/azure/active_directory/azure_active_directory_administrative_unit.go:123 +0x25
github.com/Uptycs/uptycs-cloudquery/extension/pubsub.(*InventoryTableDriver).processAzureTenant(0xc0016ad180, {0x11863890, 0xc00744d3e0}, {0x3a?}, 0xc004f140c0, 0xc015045e60)
  /home/ubuntu/workspace/release-build/extension/pubsub/inventory_table_driver.go:1399 +0x11d
github.com/Uptycs/uptycs-cloudquery/extension/pubsub.(*InventoryTableDriver).collectAzureData(0xc0016ad180, {0x11863858, 0x19174ce0}, {0xc015501950?})
  /home/ubuntu/workspace/release-build/extension/pubsub/inventory_table_driver.go:1280 +0xe85
created by github.com/Uptycs/uptycs-cloudquery/extension/pubsub.(*InventoryTableDriver).GetData in goroutine 8243
  /home/ubuntu/workspace/release-build/extension/pubsub/inventory_table_driver.go:432 +0x39b
  • Below is the code snippet:
func main() {
    scopedMembers, err := msgraphcore.GetBatchResponseById[models.ScopedRoleMembershipCollectionResponseable](
    scopedMembersResponse, scopedMembersMapObjIdToRefId[objectId],
    models.CreateScopedRoleMembershipCollectionResponseFromDiscriminatorValue)
    if err != nil {
	  utilities.GetLogger().WithFields(log.Fields{
  			"tableName": inventoryTable.GetName(),
  			"tenantId":  account.TenantID,
  			"objectId":  objectId,
  			"error":     err.Error(),
    }).Error("failed to get Administrative Unit ScopedRoleMembers")
    return resultMap, err
   }
   if scopedMembers != nil {
    administrativeUnitInfo.ScopedRoleMemberships = GetScopedRoleMembersInfo(scopedMembers.GetValue())
   }
}

func GetScopedRoleMembersInfo(scopedMembers []models.ScopedRoleMembershipable) []map[string]interface{} {
  resultList := make([]map[string]interface{}, 0)
  serializer := jsonserialization.NewJsonSerializationWriter()

  for _, val := range scopedMembers {
  	val.Serialize(serializer)
  	content, err := serializer.GetSerializedContent()
  	if err != nil {
  		continue
  	}

  	contentStr := "{" + string(content) + "}"
  	ownerContent := make(map[string]interface{}, 0)
  	err = json.Unmarshal([]byte(contentStr), &ownerContent)
  	if err != nil {
  		utilities.GetLogger().WithFields(log.Fields{
  			"error": err.Error(),
  		}).Error("failed to unmarshal Scoped Role Members")
  		continue
  	}
  	resultList = append(resultList, ownerContent)
  	serializer.Close()
  }

  return resultList
}
  • Library versions used:
    github.com/microsoftgraph/msgraph-sdk-go -> v1.14.0
    github.com/microsoftgraph/msgraph-sdk-go-core -> v1.0.0
    github.com/microsoft/kiota-serialization-json-go -> v1.0.4
    github.com/microsoftgraph/msgraph-sdk-go-core -> v1.0.0
baywet commented

Hi @djuneja-uptycs
Thanks for using the SDK and for reaching out.
Have you tried moving this line inside of the for loop?

serializer := jsonserialization.NewJsonSerializationWriter()
djuneja-uptycs commented

Thanks for responding back @baywet,
So you mean to say I need to create a new serializer for each scoped member? From design perspective is that a correct approach? I am just trying to understand the difference between what I posted vs the suggestion here, it will help me dev test this issue on my end too.

baywet commented

the rule of thumb is that you need to create a new serialization writer every time you're going to call get serialized content.
Serialization writers can't be reused for multiple payloads

djuneja-uptycs commented

Thanks @baywet for the update. It seemed to have been working before, but unfortunately it suddenly started throwing panics. Let me make the changes in my codebase and come back with an update on this. I will update here in a couple of days. Thanks!

djuneja-uptycs commented

@baywet I tried the fix suggested by you and it works like a charm. Now the code looks like this:

func GetScopedRoleMembersInfo(scopedMembers []models.ScopedRoleMembershipable) []map[string]interface{} {
  resultList := make([]map[string]interface{}, 0)

  for _, val := range scopedMembers {
  	val.Serialize(jsonserialization.NewJsonSerializationWriter())
  	content, err := serializer.GetSerializedContent()
  	if err != nil {
  		continue
  	}

  	contentStr := "{" + string(content) + "}"
  	ownerContent := make(map[string]interface{}, 0)
  	err = json.Unmarshal([]byte(contentStr), &ownerContent)
  	if err != nil {
  		utilities.GetLogger().WithFields(log.Fields{
  			"error": err.Error(),
  		}).Error("failed to unmarshal Scoped Role Members")
  		continue
  	}
  	resultList = append(resultList, ownerContent)
  	serializer.Close()
  }

  return resultList
}

I also did an RCA on why it started crashing all of a sudden. It seems we never had this field populated in our Azure Tenant. Hence the loop was never executed because there were no members at all and the serialization never happened. I'm glad we faced the issue to find such a dangerous bug sooner that later.

Appreciate your input on this feedback, I will close the ticket.