Skip to content
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

🐛 Change data conversion to allow for nil resources. Fix azure monitor as an example #2321

Closed
wants to merge 2 commits into from

Conversation

preslavgerchev
Copy link
Contributor

@preslavgerchev preslavgerchev commented Oct 21, 2023

Some of the resources we try to fetch may not be present, consider this as an example:


// Azure Monitor Diagnostic Setting
private azure.subscription.monitorService.diagnosticsetting @defaults("id name") {
  .....
  // Diagnostic setting storage account
  storageAccount() azure.subscription.storageService.account
}

When computing the storage account it is totally possible that this resource is not present. Until now, trying to return a nil resource resulted in a panic which was fixed by returning an error, something like

return nil, errors.New("storage account not present for the diagnostic setting resource")

which led to

cnquery> azure.subscription.monitor.diagnosticSettings{storageAccount}
1 error occurred:
        * diagnostic settings has no storage account
azure.subscription.monitor.diagnosticSettings: [
  0: {
    storageAccount: diagnostic settings has no storage account
  }
]

The problem with this is that there's not really an error, it's just that the storage account isn't present.

To fix this, I saw that we have a plugin.NotReady error which seemed like a good indicator of this use case. This now results in

cnquery> azure.subscription.monitor.diagnosticSettings{storageAccount}
azure.subscription.monitor.diagnosticSettings: [
  0: {
    storageAccount: null
  }
]

which will allow for better assertions

Chaining on null resources remains functional:

cnquery> azure.subscription.monitor.diagnosticSettings{storageAccount.id}
azure.subscription.monitor.diagnosticSettings: [
  0: {
    storageAccount.id: null
  }
]

@chris-rock
Copy link
Member

chris-rock commented Oct 21, 2023

I am not sure I understand why we are not just returning nil if the resource does not exist. We use the return nil behavior in other places.

@preslavgerchev
Copy link
Contributor Author

preslavgerchev commented Oct 21, 2023

I am not sure I understand why we are not just returning nil if the resource does not exist. We use the return nil behavior in other places.

return nil, nil does not work, it panics. It's a long stack but summarized: the code in GetOrCompute (plugin/runtime.go) will assume there's no error and return a TValue object where both Data and Error are nil. The conversion to DataRes then goes wrong as it's thinking there's something inside the struct

@arlimus
Copy link
Member

arlimus commented Oct 23, 2023

I am not sure I understand why we are not just returning nil if the resource does not exist. We use the return nil behavior in other places.

return nil, nil does not work, it panics. It's a long stack but summarized: the code in GetOrCompute (plugin/runtime.go) will assume there's no error and return a TValue object where both Data and Error are nil. The conversion to DataRes then goes wrong as it's thinking there's something inside the struct

I have an example in a few other places: you set the field locally and then you can return nil.

Here is an example from GCP storage buckets. It both sets the value and returns nil, nil:

	g.StorageBucket.State = plugin.StateIsSet | plugin.StateIsNull
	return nil, nil

See https://github.com/mondoohq/cnquery/blob/main/providers/gcp/resources/logging.go#L324-L325

I'm closing this PR for now just to make sure we don't accidentally merge it, I think it has some far-reaching consequences.

@arlimus arlimus closed this Oct 23, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Oct 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants