Surface inaccessible GDrive items in SourceCrawler status - #294
Surface inaccessible GDrive items in SourceCrawler status#294gshikhar2021 wants to merge 3 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the observability of the SourceCrawler service by ensuring that files and folders inaccessible to the service account during a Google Drive crawl are explicitly tracked and reported. By updating the API schema and the reconciliation logic, users can now identify specific inaccessible items directly within the SourceCrawler custom resource status, facilitating easier troubleshooting of permission-related issues. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
When crawling Google Drive folders, files and folders the service account cannot access are now reported in the SourceCrawler CR status with full detail (folder IDs, file IDs, shortcut targets) instead of being silently discarded
e925de7 to
d87684b
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces tracking of inaccessible items (folders, files, and shortcuts) during the source crawling process, updating the API types, CRD manifests, and controller logic to propagate this information to the SourceCrawler status. The feedback recommends improving error resilience by updating the status with partial results and adding nil checks when syncing fails, ensuring consistency in S3BucketSource by returning partial results on error, adding kubebuilder list markers to the new API fields for correct CRD merging, and pre-allocating slices in the conversion helper to optimize performance.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| syncResult, err := source.SyncFilesToFilestore(ctx, r.fileStore) | ||
| if err != nil { | ||
| return ctrl.Result{}, r.handleError(ctx, sourceCrawlerCR, fmt.Errorf("failed to store files to filestore: %w", err)) | ||
| } |
There was a problem hiding this comment.
If SyncFilesToFilestore returns an error (e.g., due to a transient download failure of a single file), any partial results (such as successfully stored files and newly discovered inaccessible items) are currently discarded because the controller returns early.
To make the status updates more resilient and ensure inaccessible items are surfaced even when some errors occur, we should update the status with the partial results before returning the error. Additionally, we should add a defensive nil check on syncResult to prevent potential nil pointer dereferences.
| syncResult, err := source.SyncFilesToFilestore(ctx, r.fileStore) | |
| if err != nil { | |
| return ctrl.Result{}, r.handleError(ctx, sourceCrawlerCR, fmt.Errorf("failed to store files to filestore: %w", err)) | |
| } | |
| syncResult, err := source.SyncFilesToFilestore(ctx, r.fileStore) | |
| if err != nil { | |
| if syncResult != nil { | |
| inaccessible := toAPIInaccessibleItems(syncResult.InaccessibleItems) | |
| _ = controllerutils.StatusPatch(ctx, r.Client, sourceCrawlerCR, func() { | |
| sourceCrawlerCR.Status.FilesProcessed += int64(len(syncResult.StoredFiles)) | |
| sourceCrawlerCR.Status.InaccessibleItems = inaccessible | |
| }) | |
| } | |
| return ctrl.Result{}, r.handleError(ctx, sourceCrawlerCR, fmt.Errorf("failed to store files to filestore: %w", err)) | |
| } | |
| if syncResult == nil { | |
| return ctrl.Result{}, r.handleError(ctx, sourceCrawlerCR, fmt.Errorf("sync result is nil")) | |
| } |
| } | ||
|
|
||
| return storedFiles, nil | ||
| return &SyncResult{StoredFiles: storedFiles}, nil |
There was a problem hiding this comment.
In S3BucketSource.SyncFilesToFilestore, when len(errorMessage) > 0 (at line 120), the method currently returns nil, errors.New(errorMessage).
To ensure consistency with GDriveSource and to allow the controller to safely update the status with successfully processed files even when some errors occur, please update line 120 to return the partial SyncResult instead of nil:
return &SyncResult{StoredFiles: storedFiles}, errors.New(errorMessage)| type InaccessibleItems struct { | ||
| Folders []InaccessibleFolder `json:"folders,omitempty"` | ||
| Files []InaccessibleFile `json:"files,omitempty"` | ||
| ShortcutTargetFolders []InaccessibleShortcutFolder `json:"shortcutTargetFolders,omitempty"` | ||
| ShortcutTargetFiles []InaccessibleShortcutFile `json:"shortcutTargetFiles,omitempty"` | ||
| } |
There was a problem hiding this comment.
For list fields in Kubernetes CRDs, it is highly recommended to explicitly define +listType and +listMapKey markers. This ensures correct merging behavior during Server-Side Apply (SSA) and prevents duplicate keys.
Please update the struct as follows:
type InaccessibleItems struct {
// +listType=map
// +listMapKey=folderID
Folders []InaccessibleFolder `json:"folders,omitempty"`
// +listType=map
// +listMapKey=fileID
Files []InaccessibleFile `json:"files,omitempty"`
// +listType=map
// +listMapKey=shortcutFileID
ShortcutTargetFolders []InaccessibleShortcutFolder `json:"shortcutTargetFolders,omitempty"`
// +listType=map
// +listMapKey=shortcutFileID
ShortcutTargetFiles []InaccessibleShortcutFile `json:"shortcutTargetFiles,omitempty"`
}References
- Avoid implementing redundant manual validation for duplicate keys in list fields within controller logic if duplicate prevention is already enforced at the CRD level using kubebuilder markers like +listType=map and +listMapKey.
| result := &operatorv1alpha1.InaccessibleItems{} | ||
| for _, f := range items.Folders { | ||
| result.Folders = append(result.Folders, operatorv1alpha1.InaccessibleFolder{ | ||
| FolderID: f.FolderID, FolderName: f.FolderName, RootFolderID: f.RootFolderID, | ||
| }) | ||
| } | ||
| for _, f := range items.Files { | ||
| result.Files = append(result.Files, operatorv1alpha1.InaccessibleFile{ | ||
| FileID: f.FileID, ParentFolderID: f.ParentFolderID, RootFolderID: f.RootFolderID, | ||
| }) | ||
| } | ||
| for _, f := range items.ShortcutTargetFolders { | ||
| result.ShortcutTargetFolders = append(result.ShortcutTargetFolders, operatorv1alpha1.InaccessibleShortcutFolder{ | ||
| ShortcutFileID: f.ShortcutFileID, TargetFolderID: f.TargetFolderID, RootFolderID: f.RootFolderID, | ||
| }) | ||
| } | ||
| for _, f := range items.ShortcutTargetFiles { | ||
| result.ShortcutTargetFiles = append(result.ShortcutTargetFiles, operatorv1alpha1.InaccessibleShortcutFile{ | ||
| ShortcutFileID: f.ShortcutFileID, TargetFileID: f.TargetFileID, RootFolderID: f.RootFolderID, | ||
| }) | ||
| } | ||
| return result |
There was a problem hiding this comment.
Pre-allocating the slices in operatorv1alpha1.InaccessibleItems using make with the appropriate capacity avoids multiple allocations and slice copies during the append loops.
result := &operatorv1alpha1.InaccessibleItems{
Folders: make([]operatorv1alpha1.InaccessibleFolder, 0, len(items.Folders)),
Files: make([]operatorv1alpha1.InaccessibleFile, 0, len(items.Files)),
ShortcutTargetFolders: make([]operatorv1alpha1.InaccessibleShortcutFolder, 0, len(items.ShortcutTargetFolders)),
ShortcutTargetFiles: make([]operatorv1alpha1.InaccessibleShortcutFile, 0, len(items.ShortcutTargetFiles)),
}
for _, f := range items.Folders {
result.Folders = append(result.Folders, operatorv1alpha1.InaccessibleFolder{
FolderID: f.FolderID, FolderName: f.FolderName, RootFolderID: f.RootFolderID,
})
}
for _, f := range items.Files {
result.Files = append(result.Files, operatorv1alpha1.InaccessibleFile{
FileID: f.FileID, ParentFolderID: f.ParentFolderID, RootFolderID: f.RootFolderID,
})
}
for _, f := range items.ShortcutTargetFolders {
result.ShortcutTargetFolders = append(result.ShortcutTargetFolders, operatorv1alpha1.InaccessibleShortcutFolder{
ShortcutFileID: f.ShortcutFileID, TargetFolderID: f.TargetFolderID, RootFolderID: f.RootFolderID,
})
}
for _, f := range items.ShortcutTargetFiles {
result.ShortcutTargetFiles = append(result.ShortcutTargetFiles, operatorv1alpha1.InaccessibleShortcutFile{
ShortcutFileID: f.ShortcutFileID, TargetFileID: f.TargetFileID, RootFolderID: f.RootFolderID,
})
}
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces tracking and reporting of inaccessible items (folders, files, and shortcuts) during source crawling. It updates the DataSource interface and its implementations to return a SyncResult containing both stored files and inaccessible items, and updates the SourceCrawler CRD and controller to store this information in the resource status. The review feedback suggests two improvements: first, updating the status with partial results (successfully processed files and inaccessible items) even if SyncFilesToFilestore returns an error, and second, refactoring toAPIInaccessibleItems to return the total count of inaccessible items to avoid redundant calculations in the controller.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| syncResult, err := source.SyncFilesToFilestore(ctx, r.fileStore) | ||
| if err != nil { | ||
| return ctrl.Result{}, r.handleError(ctx, sourceCrawlerCR, fmt.Errorf("failed to store files to filestore: %w", err)) | ||
| } |
There was a problem hiding this comment.
If SyncFilesToFilestore returns an error (e.g., due to transient download failures of some files in Phase 2), the controller immediately returns and calls r.handleError. This completely discards any successfully processed files and, more importantly, the list of inaccessible items discovered during the crawl phase (Phase 1). We should update the status with the partial results even if there's an error, as long as syncResult is not nil.
| syncResult, err := source.SyncFilesToFilestore(ctx, r.fileStore) | |
| if err != nil { | |
| return ctrl.Result{}, r.handleError(ctx, sourceCrawlerCR, fmt.Errorf("failed to store files to filestore: %w", err)) | |
| } | |
| syncResult, err := source.SyncFilesToFilestore(ctx, r.fileStore) | |
| if err != nil { | |
| if syncResult != nil { | |
| inaccessible, _ := toAPIInaccessibleItems(syncResult.InaccessibleItems) | |
| _ = controllerutils.StatusPatch(ctx, r.Client, sourceCrawlerCR, func() { | |
| sourceCrawlerCR.Status.FilesProcessed += int64(len(syncResult.StoredFiles)) | |
| sourceCrawlerCR.Status.InaccessibleItems = inaccessible | |
| sourceCrawlerCR.UpdateStatus("", err) | |
| }) | |
| } else { | |
| _ = r.handleError(ctx, sourceCrawlerCR, err) | |
| } | |
| return ctrl.Result{}, fmt.Errorf("failed to store files to filestore: %w", err) | |
| } |
| inaccessible := toAPIInaccessibleItems(syncResult.InaccessibleItems) | ||
| if inaccessible != nil { | ||
| total := len(syncResult.InaccessibleItems.Folders) + len(syncResult.InaccessibleItems.Files) + | ||
| len(syncResult.InaccessibleItems.ShortcutTargetFolders) + len(syncResult.InaccessibleItems.ShortcutTargetFiles) | ||
| successMessage += fmt.Sprintf(", %d inaccessible items (service account may lack access)", total) | ||
| } |
There was a problem hiding this comment.
The total number of inaccessible items is calculated redundantly here. Since toAPIInaccessibleItems already calculates total to check if it's 0, we can return this count from toAPIInaccessibleItems to avoid recalculating it in Reconcile.
inaccessible, total := toAPIInaccessibleItems(syncResult.InaccessibleItems)
if inaccessible != nil {
successMessage += fmt.Sprintf(", %d inaccessible items (service account may lack access)", total)
}| func toAPIInaccessibleItems(items gdrive.InaccessibleItems) *operatorv1alpha1.InaccessibleItems { | ||
| total := len(items.Folders) + len(items.Files) + len(items.ShortcutTargetFolders) + len(items.ShortcutTargetFiles) | ||
| if total == 0 { | ||
| return nil | ||
| } | ||
| result := &operatorv1alpha1.InaccessibleItems{} | ||
| for _, f := range items.Folders { | ||
| result.Folders = append(result.Folders, operatorv1alpha1.InaccessibleFolder{ | ||
| FolderID: f.FolderID, FolderName: f.FolderName, RootFolderID: f.RootFolderID, | ||
| }) | ||
| } | ||
| for _, f := range items.Files { | ||
| result.Files = append(result.Files, operatorv1alpha1.InaccessibleFile{ | ||
| FileID: f.FileID, ParentFolderID: f.ParentFolderID, RootFolderID: f.RootFolderID, | ||
| }) | ||
| } | ||
| for _, f := range items.ShortcutTargetFolders { | ||
| result.ShortcutTargetFolders = append(result.ShortcutTargetFolders, operatorv1alpha1.InaccessibleShortcutFolder{ | ||
| ShortcutFileID: f.ShortcutFileID, TargetFolderID: f.TargetFolderID, RootFolderID: f.RootFolderID, | ||
| }) | ||
| } | ||
| for _, f := range items.ShortcutTargetFiles { | ||
| result.ShortcutTargetFiles = append(result.ShortcutTargetFiles, operatorv1alpha1.InaccessibleShortcutFile{ | ||
| ShortcutFileID: f.ShortcutFileID, TargetFileID: f.TargetFileID, RootFolderID: f.RootFolderID, | ||
| }) | ||
| } | ||
| return result | ||
| } |
There was a problem hiding this comment.
Modify toAPIInaccessibleItems to return both the mapped inaccessible items and the total count. This avoids redundant calculations in the reconciler.
func toAPIInaccessibleItems(items gdrive.InaccessibleItems) (*operatorv1alpha1.InaccessibleItems, int) {
total := len(items.Folders) + len(items.Files) + len(items.ShortcutTargetFolders) + len(items.ShortcutTargetFiles)
if total == 0 {
return nil, 0
}
result := &operatorv1alpha1.InaccessibleItems{}
for _, f := range items.Folders {
result.Folders = append(result.Folders, operatorv1alpha1.InaccessibleFolder{
FolderID: f.FolderID, FolderName: f.FolderName, RootFolderID: f.RootFolderID,
})
}
for _, f := range items.Files {
result.Files = append(result.Files, operatorv1alpha1.InaccessibleFile{
FileID: f.FileID, ParentFolderID: f.ParentFolderID, RootFolderID: f.RootFolderID,
})
}
for _, f := range items.ShortcutTargetFolders {
result.ShortcutTargetFolders = append(result.ShortcutTargetFolders, operatorv1alpha1.InaccessibleShortcutFolder{
ShortcutFileID: f.ShortcutFileID, TargetFolderID: f.TargetFolderID, RootFolderID: f.RootFolderID,
})
}
for _, f := range items.ShortcutTargetFiles {
result.ShortcutTargetFiles = append(result.ShortcutTargetFiles, operatorv1alpha1.InaccessibleShortcutFile{
ShortcutFileID: f.ShortcutFileID, TargetFileID: f.TargetFileID, RootFolderID: f.RootFolderID,
})
}
return result, total
}
|
how does the final YAML look like? |
25acfe6 to
012b891
Compare
012b891 to
b8c24e4
Compare
| return reconcileErr | ||
| } | ||
|
|
||
| func toAPIInaccessibleItems(items gdrive.InaccessibleItems) (*operatorv1alpha1.InaccessibleItems, int) { |
There was a problem hiding this comment.
Can we have a better function name ?
|
|
||
| // Merge and filter crawl records to only successful non-folder files | ||
| var fileRecords []gdrive.CrawlRecord | ||
| var mergedInaccessible gdrive.InaccessibleItems |
There was a problem hiding this comment.
Will this work if the source is s3 ?
| if err := controllerutils.StatusPatch(ctx, r.Client, sourceCrawlerCR, func() { | ||
| sourceCrawlerCR.Status.FilesProcessed += int64(len(syncResult.StoredFiles)) | ||
| sourceCrawlerCR.Status.InaccessibleItems = inaccessible | ||
| sourceCrawlerCR.UpdateStatus("", syncErr) |
There was a problem hiding this comment.
Are we updating the status twice here ?
There was a problem hiding this comment.
agreed, we are writing error twice, @gshikhar2021, can you please fix this?
| LastAppliedGeneration int64 `json:"lastAppliedGeneration,omitempty"` | ||
| Conditions []metav1.Condition `json:"conditions,omitempty"` | ||
| FilesProcessed int64 `json:"filesProcessed,omitempty"` | ||
| InaccessibleItems *InaccessibleItems `json:"inaccessibleItems,omitempty"` |
There was a problem hiding this comment.
We need to have some limit here, because if there are let's say for example 1000 folders then the CR size would be huge and it would be restricted by k8s
| type InaccessibleFolder struct { | ||
| FolderID string `json:"folderID"` | ||
| FolderName string `json:"folderName"` | ||
| RootFolderID string `json:"rootFolderID"` | ||
| } | ||
|
|
||
| type InaccessibleFile struct { | ||
| FileID string `json:"fileID"` | ||
| ParentFolderID string `json:"parentFolderID"` | ||
| RootFolderID string `json:"rootFolderID"` | ||
| } | ||
|
|
||
| type InaccessibleShortcutFolder struct { | ||
| ShortcutFileID string `json:"shortcutFileID"` | ||
| TargetFolderID string `json:"targetFolderID"` | ||
| RootFolderID string `json:"rootFolderID"` | ||
| } | ||
|
|
||
| type InaccessibleShortcutFile struct { | ||
| ShortcutFileID string `json:"shortcutFileID"` | ||
| TargetFileID string `json:"targetFileID"` | ||
| RootFolderID string `json:"rootFolderID"` | ||
| } |
There was a problem hiding this comment.
instead of defining 4 different types, can we do in one?
type InaccessibleItem struct {
Kind string `json:"kind"` // folder, file, shortcutFolder, shortcutFile
ID string `json:"itemID"`
Name string `json:"name,omitempty"`
TargetID string `json:"targetID,omitempty"` // for shortcuts
RootFolderID string `json:"rootFolderID"`
Reason string `json:"reason"`
}also included a reason field which I think will be helpful. In case of gdrive, we can get the information from GetFileMetadata/ListFolderContents
| Folders []InaccessibleFolder `json:"folders,omitempty"` | ||
| Files []InaccessibleFile `json:"files,omitempty"` | ||
| ShortcutTargetFolders []InaccessibleShortcutFolder `json:"shortcutTargetFolders,omitempty"` | ||
| ShortcutTargetFiles []InaccessibleShortcutFile `json:"shortcutTargetFiles,omitempty"` |
There was a problem hiding this comment.
this can then be:
| Folders []InaccessibleFolder `json:"folders,omitempty"` | |
| Files []InaccessibleFile `json:"files,omitempty"` | |
| ShortcutTargetFolders []InaccessibleShortcutFolder `json:"shortcutTargetFolders,omitempty"` | |
| ShortcutTargetFiles []InaccessibleShortcutFile `json:"shortcutTargetFiles,omitempty"` | |
| Items []InaccessibleItem `json:"items,omitempty"` | |
| Count int `json:"count"` |
| inaccessible, _ := toAPIInaccessibleItems(syncResult.InaccessibleItems) | ||
| if err := controllerutils.StatusPatch(ctx, r.Client, sourceCrawlerCR, func() { | ||
| sourceCrawlerCR.Status.FilesProcessed += int64(len(syncResult.StoredFiles)) | ||
| sourceCrawlerCR.Status.InaccessibleItems = inaccessible |
There was a problem hiding this comment.
we might have to see what's the upper cap that CR can hold else we might end up blowing 1.5MB limit
ffcc51e to
0bcfc78
Compare
3 participants
When crawling Google Drive folders, files and folders the service account cannot access are now reported in the SourceCrawler CR status with full detail (folder IDs, file IDs, shortcut targets) instead of being silently discarded.
Example status
New structure