724 bi collection improvments #770
Conversation
| InstallationID: s.getInstallationID(), | ||
| Entries: entries, | ||
| }) | ||
| err := s.sender.UpdateMetadata(ctx, *accountMetadata) |
There was a problem hiding this comment.
couldn't this lead to a null dereference if the account metadata is null? this can happen if reading the account id fails no?
There was a problem hiding this comment.
no, if reading the account id fails, the cloudMetadataProvider.GetMetadata() returns nil, and then it will be ignored when creating the unified metadata object (serach for cloudMetadataProvider.GetMetadata())
cloud/gcp/metadata.go
Outdated
| func (m *MetadataProvider) GetMetadata() map[string]string { | ||
| projectID, err := metadata.NumericProjectID() | ||
| if err != nil { | ||
| m.logger.Errorf("%v: failed to get Google numeric project ID from instance metadata", err) |
There was a problem hiding this comment.
I think this should be a warning - it doesn't affect the health or correctness of lakeFS in any way (nor do we ever request explicit permissions to do this operation (also true for the AWS metadata provider))
block/s3/inventory.go
Outdated
| @@ -12,7 +12,7 @@ import ( | |||
| "github.com/aws/aws-sdk-go/service/s3" | |||
| "github.com/aws/aws-sdk-go/service/s3/s3iface" | |||
| "github.com/treeverse/lakefs/block" | |||
| inventorys3 "github.com/treeverse/lakefs/inventory/s3" | |||
| inventorys3 "github.com/treeverse/lakefs/cloud/aws/s3inventory" | |||
There was a problem hiding this comment.
why is the import named in reverse? what's wrong with s3inventory?
There was a problem hiding this comment.
it was like this before and I didn't want to bloat the diff. But now that you've noticed it I must
cloud/gcp/metadata.go
Outdated
| m.logger.Warnf("%v: failed to get Google numeric project ID from instance metadata", err) | ||
| return nil | ||
| } | ||
| return map[string]string{"google_numeric_project_id": projectID} |
There was a problem hiding this comment.
another thing that's missing (sorry for not catching this earlier) is one way hashing: we never want to send the actual account id, just to know that different installations come from the same one. I suggest hashing the received id here and in the aws provider as well.
in terms of metadata fields, i would seperate it into 2 fields: account_type ("aws", "gcp", etc) and "account_id" which is the hash. would make it easier downstream to consume.
No description provided.