elevenlabs analyzer #3850
elevenlabs analyzer #3850
Conversation
| If write permission API calls was not as expected than only we make read permission API calls | ||
| This we only do for those API calls which does not add any resources to secretInfo | ||
| */ | ||
| func getResources(client *http.Client, key string, secretInfo *SecretInfo) error { |
There was a problem hiding this comment.
Points to Consider for Implementation while getting resource:
-
Aggregate Errors:
Instead of failing on the first error, collect all errors encountered during the process and return them as a single aggregated error at the end. This ensures that users get a complete picture of what went wrong, rather than having to address issues one at a time. -
Graceful Error Handling in CLI:
For the CLI, log errors when checking a specific scope or fetching resource fails, but continue processing other tasks. Improving the user experience and allowing for partial success. -
Concurrent API Calls:
Use Go routines to call APIs concurrently. This will significantly improve performance by reducing the total time spent waiting for API responses.
Let me know your thoughts on these points.
There was a problem hiding this comment.
My vote will be for concurrent API calls with error handling for each permission.
| If write permission API calls was not as expected than only we make read permission API calls | ||
| This we only do for those API calls which does not add any resources to secretInfo | ||
| */ | ||
| func getResources(client *http.Client, key string, secretInfo *SecretInfo) error { |
There was a problem hiding this comment.
My vote will be for concurrent API calls with error handling for each permission.
kashifkhan0771
force-pushed
the
feat/oss-97-elevenlabs-analyzer
branch
from
3ba85a9 to
4975781
Compare
Co-authored-by: Eng Zer Jun <[email protected]>
| secretInfo.ElevenLabsResources = append(secretInfo.ElevenLabsResources, ElevenLabsResource{ | ||
| ID: user.ID, | ||
| Name: user.Name, | ||
| Type: "User", | ||
| Permission: PermissionStrings[UserRead], |
There was a problem hiding this comment.
As per my understanding, resource identification is related where Resoruce/Permission mapping is decided. We can move it to Analyze(...) func
There was a problem hiding this comment.
This is called from AnalyzePermissions(). The permission mapping here is not for the analyzer resource binding but for the ElevenLabsResource Permission, which we add to secretInfo for later use. The actual permission binding occurs in secretInfoToAnalyzerResult, within Analyze().
|
|
||
| Note: The permissions in eleven labs is either Read or Read and Write. There is not separate permission for Write. | ||
| */ | ||
| func getElevenLabsResources(client *http.Client, key string, secretInfo *SecretInfo) error { |
There was a problem hiding this comment.
chore -- @kashifkhan0771 can this be simplified using json structure ? e.g. Opsgenie Analyzer
There was a problem hiding this comment.
I considered that too, but for this analyzer, we need to treat multiple API responses as resources and store them in secretInfo. Opsgenie only checks whether an API permission exists from status code, which we can handle through JSON.
However, mapping all APIs to JSON would add complexity in defining which structure handles which API response and how different responses are mapped to secretInfo.
If you look at APIMAP in requests.go, you'll notice that some APIs only require a status code check without processing the response. Mixing JSON-based handling with direct handling could cause confusion, so I decided to use a consistent approach for all APIs.
| // SecretInfo hold information about key | ||
| type SecretInfo struct { | ||
| User User // the owner of key | ||
| Valid bool |
There was a problem hiding this comment.
What is the purpose of this boolean? at some places, SecretInfo was being checked as nil. Does that conditions represent the validity of API keys ? If yes, Valid boolean seems redundant to me.
There was a problem hiding this comment.
Honestly speaking, I keep it just to be consistent with other analyzers. This bool is marked as true once we pass the fetchUser step.
| Reference string | ||
| Permissions []string // list of Permissions assigned to the key | ||
| ElevenLabsResources []ElevenLabsResource // list of resources the key has access to | ||
| Misc map[string]string |
There was a problem hiding this comment.
I do not see any use of this Property.
There was a problem hiding this comment.
Same here. This was my first analyzer, so I aimed for consistency with the others. We can remove it if it's not necessary, but at a higher level, I believe we should maintain some consistency in SecretInfo across analyzers.
| if strings.Contains(apiUrl, "%s") { | ||
| return fmt.Sprintf(apiUrl, fakeID) | ||
| } |
There was a problem hiding this comment.
why not add the fakeID's value in the url directly?
There was a problem hiding this comment.
I guess I just came up with a fancy way to handle API endpoints from permissionToAPIMap 😄.
Now that I see your comment, it makes sense to add fakeID directly to the API endpoint.
| case http.StatusInternalServerError: | ||
| // for some reason if we send fake id and token has the permission, the history api return 500 error instead of 404 | ||
| // issue opened in elevenlabs-docs: https://github.com/elevenlabs/elevenlabs-docs/issues/649 | ||
| return handleErrorStatus(response, PermissionStrings[SpeechHistoryWrite], secretInfo, InternalServerError) |
There was a problem hiding this comment.
does 500 response shows that credentials has the permission to perform the action?
There was a problem hiding this comment.
Yes, ideally, it should return 400 or 404 since the ID shouldn't exist. We rely on an error response to confirm that the permission exists while the ID does not. However, this API was returning a 500 error instead. I opened an issue in their repository, and they acknowledged it, but I'm not sure when they'll fix it.
| switch statusCode { | ||
| case http.StatusInternalServerError: | ||
| // for some reason if we send fake id and token has the permission, the history api return 500 error instead of 404 | ||
| // issue opened in elevenlabs-docs: https://github.com/elevenlabs/elevenlabs-docs/issues/649 | ||
| return handleErrorStatus(response, PermissionStrings[SpeechHistoryWrite], secretInfo, InternalServerError) | ||
| case http.StatusUnauthorized: | ||
| return handleErrorStatus(response, "", secretInfo, MissingPermissions) | ||
| default: | ||
| return fmt.Errorf("unexpected status code: %d while checking history write scope", statusCode) | ||
| } |
There was a problem hiding this comment.
I might be overseeing but all the conditions are representing the error cases. What is the success condition ?
There was a problem hiding this comment.
Yes, we expect an error response for APIs where we attempt a write operation using a fakeID. This helps determine whether we have the required permission. If the permission is missing, we receive a 401 error; otherwise, we may get 400, 401, or in some cases, a 500 error with a specific message.
| case http.StatusBadRequest: | ||
| // if permission was assigned to scope we should get 400 error with voice not found status | ||
| return handleErrorStatus(response, PermissionStrings[VoicesWrite], secretInfo, VoiceDoesNotExist) | ||
| case http.StatusUnauthorized: | ||
| return handleErrorStatus(response, "", secretInfo, MissingPermissions) | ||
| default: | ||
| return fmt.Errorf("unexpected status code: %d while checking voice write scope", statusCode) | ||
| } |
There was a problem hiding this comment.
Same, no condition to mark successful permission
There was a problem hiding this comment.
Same as above.
| case http.StatusBadRequest: | ||
| // if permission was assigned to token we should get 400 error with project not found status | ||
| return handleErrorStatus(response, PermissionStrings[ProjectsWrite], secretInfo, ProjectNotFound) | ||
| case http.StatusForbidden: | ||
| // if token has the permission but trail is free, projects are not accessible | ||
| return handleErrorStatus(response, PermissionStrings[ProjectsWrite], secretInfo, InvalidSubscription) | ||
| case http.StatusUnauthorized: | ||
| return handleErrorStatus(response, "", secretInfo, MissingPermissions) | ||
| default: | ||
| return fmt.Errorf("unexpected status code: %d while checking project write scope", statusCode) | ||
| } |
There was a problem hiding this comment.
same, no condition of success.
There was a problem hiding this comment.
I added comments to explain which error condition is considered as a success.
| Permissions []string // list of Permissions assigned to the key | ||
| ElevenLabsResources []ElevenLabsResource // list of resources the key has access to |
There was a problem hiding this comment.
in each go routine, Append operation is performed on same array, which is not thread safe. Might see varying results or race conditions if ran with multiple CPU core.
There was a problem hiding this comment.
This is handled in getElevenLabsResources function using Mutexes
There was a problem hiding this comment.
This approach works, but I'd argue it's generally best to avoid holding mutexes during API calls, or any I/O operations for that matter. Doing so blocks other goroutines from making progress. We should minimize the critical section that requires synchronization—specifically, the concurrent updates to the shared underlying slice.
Another risk of holding a mutex during an API call is that if the call hangs or doesn’t respond (and there’s no timeout on the context), the system could end up deadlocked.
There was a problem hiding this comment.
in each go routine, Append operation is performed on same array, which is not thread safe. Might see varying results or race conditions if ran with multiple CPU core.
This approach works, but I'd argue it's generally best to avoid holding mutexes during API calls, or any I/O operations for that matter. Doing so blocks other goroutines from making progress. We should minimize the critical section that requires synchronization—specifically, the concurrent updates to the shared underlying slice.
Added thread-safe methods in SecretInfo to append and read permissions, as well as to append new resources.
Tested locally, and it works fine without any race conditions. The test case is also passing successfully.
|
Sorry for the delay on this PR! This was the first one opened for the new analyzers from me, but I was also working on other analyzers that got merged earlier. This analyzer have some complexity and had some outstanding questions, so I hadn’t been focusing on it as much as the others. I’ll address all comments as soon as possible. Thanks for your patience! |
ahrav
dismissed
abmussani’s stale review
Dismissing as the changes have been addressed.
Description:
This Pull requests add a new analyzer for ElevenLabs
Test Cases:
Checklist:
make test-community)?make lintthis requires golangci-lint)?