-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Added azure COSMOSDB detector #3951
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
Added azure COSMOSDB detector #3951
Conversation
|
||
dbKeyPattern = regexp.MustCompile(`([A-Za-z0-9+/=]{88})`) | ||
// account name can contain only lowercase letters, numbers and the `-` character, must be between 3 and 44 characters long. | ||
accountUrlPattern = regexp.MustCompile(`(https://[a-z0-9-]{3,44}.documents\.azure\.com:[0-9]{3})`) |
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.
@kashifkhan0771 Does Azure Cosmos db only supports 3 digits port number ? As per my knowledge, A port number can be between 0 to 65535, includes reserved and general use port numbers.
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.
Apologies for that. The port is fixed at 443, and there doesn't seem to be an option to specify a custom port when creating a Cosmos NoSQL database. I will change this.
var _ detectors.Detector = (*Scanner)(nil) | ||
|
||
func (s Scanner) Type() detectorspb.DetectorType { | ||
return detectorspb.DetectorType_AzureCosmosDB |
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.
Can you check how this detector is different than already CosmosDBKey one
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.
This one does not exist. The entry only exist in Proto file somehow.
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.
Deprecated the old one.
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.
Does it make sense to create a new entry when there's an existing (unused) one?
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.
I'm not sure why this old entry was added. Was there a detector for it, or was it added by mistake? We can reuse the old entry for the new detector, but I want to deprecate it to keep a record that this key existed.
Anyway I'll update it 😃
|
||
s1 := detectors.Result{ | ||
DetectorType: detectorspb.DetectorType_AzureCosmosDB, | ||
Raw: []byte(key), |
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.
The URL should also be added. #3938 (comment)
pkg/detectors/azure_cosmosdb/azure_cosmosdb_integration_test.go
Outdated
Show resolved
Hide resolved
}{ | ||
{ | ||
name: "valid document db pattern", | ||
input: validDocumentDBPattern, |
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.
For better code readability, can we keep the inputs inline instead of assigning them to separate variables?
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.
Added inline comment about caching.
Finally 🥳 - Thanks @abmussani and @rgmz for detailed review. |
Description:
This PR adds a new detector for azure cosmosdb.

Note: This for now only work for
documents.azure.com
&table.cosmos.azure.com
account urls.Checklist:
make test-community
)?make lint
this requires golangci-lint)?