-
Notifications
You must be signed in to change notification settings - Fork 25
feat: add client for cyberark service discovery #646
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
Conversation
819cd69
to
75be7df
Compare
75be7df
to
06d51be
Compare
3f958af
to
2224945
Compare
Signed-off-by: Ashley Davis <ashley.davis@cyberark.com>
2224945
to
9219164
Compare
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.
Pull Request Overview
This PR introduces a CyberArk service discovery client designed specifically to fetch the Identity API endpoint while also laying the groundwork for extending discovery to other services. Key changes include:
- A new client implementation in discovery.go with configurable endpoints.
- Unit tests in discovery_test.go covering various scenarios including successful responses, missing services, and malformed JSON.
- Test data documentation in README.md detailing the discovery responses.
Reviewed Changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
File | Description |
---|---|
pkg/internal/cyberark/servicediscovery/testdata/README.md | Added documentation for test data. |
pkg/internal/cyberark/servicediscovery/discovery_test.go | Introduced thorough tests for different scenarios. |
pkg/internal/cyberark/servicediscovery/discovery.go | Implemented the discovery client with error handling. |
Files not reviewed (1)
- pkg/internal/cyberark/servicediscovery/testdata/discovery_success.json: Language not supported
Comments suppressed due to low confidence (2)
pkg/internal/cyberark/servicediscovery/discovery_test.go:141
- Consider comparing errors using errors.Is or errors.As rather than converting them to strings, to improve the robustness and clarity of the tests.
if err.Error() != testSpec.expectedError.Error() {
pkg/internal/cyberark/servicediscovery/discovery.go:82
- [nitpick] Consider using the %w verb for error wrapping (e.g. fmt.Errorf(... %w)) so that the original error context is preserved.
return "", fmt.Errorf("failed to build a valid URL for subdomain %s; possibly an invalid endpoint: %s", subdomain, err)
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 haven't been involved in the discussions around this new feature so I'm a bit out of the loop and don't know the plan.
But it looks like a nice self-contained start.
Some of the canned responses in the test server didn't match what I got from the live server, but that might be because I'm not making the right requests.
@@ -0,0 +1,3 @@ | |||
# Test data for CyberArk Discovery | |||
|
|||
All data in this folder is derived from an unauthenticated endpoint accessible from the public internet. |
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'd like to know how to recreate or update this data.
I tried using curl , but got an empty response.
$ curl https://platform-discovery.integration-cyberark.cloud/api/v2/services/subdomain/venafi-test; echo
{}
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.
That tenant was decommissioned and replaced with a new one I'm using to test the Identity client. There's a different example tenant which can be used for getting this kind of data: curl -v https://platform-discovery.integration-cyberark.cloud/api/v2/services/subdomain/beyonceknowles
I didn't want to add the old one or the one above to that README because I don't know if they're intended to be long lived, so my instinct is to avoid referring to either in code right now!
w.WriteHeader(http.StatusForbidden) | ||
_, _ = w.Write([]byte(`{"message":"Missing Authentication Token"}`)) | ||
return | ||
} |
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 get a different response when I use curl to GET that endpoint:
$ curl -v https://platform-discovery.integration-cyberark.cloud/api/v2/services/asd | jq
...
> GET /api/v2/services/asd HTTP/2
...
< HTTP/2 400
...
{
"error": "input validation error"
}
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.
Ah that's my fault for just making up a random identifier; I'll update this in the Identity client which is to follow!
Thanks for the review Richard, really appreciate it! |
Currently, the only aim of this is to fetch the URL of the Identity API. A future PR will use this client to fetch that API URL and log in via Identity.
I've written this to be specific to Identity, but to be easily extensible later if we want to discover other services.