-
Notifications
You must be signed in to change notification settings - Fork 25
Deprecate service/discovery API and implement the new one #706
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
33c8701 to
787f14e
Compare
… update readme and envrc.template
| switch svc.ServiceName { | ||
| case IdentityServiceName: | ||
| for _, ep := range svc.Endpoints { | ||
| if ep.Type == "main" && ep.IsActive && ep.API != "" { |
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 was not sure if I should check the Type and IsActive but I've added them. I don't understand the difference between Type main and crdr.
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 either. Let's merge as-is and adapt it if we learn more from the API team.
| return nil, fmt.Errorf("didn't find %s in service discovery response, "+ | ||
| "which may indicate a suspended tenant; unable to detect CyberArk Identity API URL", IdentityServiceName) | ||
| } | ||
| //TODO: Should add a check for discoveryContextAPI too? |
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.
We check for identityAPI but not for discoveryContextAPI. Was this intentional?
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 thought about adding it, but didn't know what actionable message to return if it's not found.
Thanks for adding the TODO, lets address that in a future PR.
wallrj-cyberark
left a comment
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.
Thanks @mladen-rusev-cyberark
Looks great.
| return nil, fmt.Errorf("didn't find %s in service discovery response, "+ | ||
| "which may indicate a suspended tenant; unable to detect CyberArk Identity API URL", IdentityServiceName) | ||
| } | ||
| //TODO: Should add a check for discoveryContextAPI too? |
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 thought about adding it, but didn't know what actionable message to return if it's not found.
Thanks for adding the TODO, lets address that in a future PR.
| switch svc.ServiceName { | ||
| case IdentityServiceName: | ||
| for _, ep := range svc.Endpoints { | ||
| if ep.Type == "main" && ep.IsActive && ep.API != "" { |
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 either. Let's merge as-is and adapt it if we learn more from the API team.
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 tested this and it worked well:
$ go run pkg/internal/cyberark/identity/cmd/testidentity/main.go -subdomain $ARK_SUBDOMAIN -username $ARK_USERNAME
I0903 16:45:13.361918 1414535 round_trippers.go:632] "Response" verb="GET" url="https://platform-discovery.integration-cyberark.cloud/api/public/tenant-discovery?bySubdomain=tlskp-test" status="200 OK" milliseconds=354
I0903 16:45:13.905341 1414535 round_trippers.go:632] "Response" verb="POST" url="https://anb5751.id.integration-cyberark.cloud/Security/StartAuthentication" status="200 OK" milliseconds=536
I0903 16:45:13.906041 1414535 identity.go:303] "made successful request to StartAuthentication" source="Identity.doStartAuthentication" summary="NewPackage"
I0903 16:45:14.772488 1414535 round_trippers.go:632] "Response" verb="POST" url="https://anb5751.id.integration-cyberark.cloud/Security/AdvanceAuthentication" status="200 OK" milliseconds=866
I0903 16:45:14.773137 1414535 identity.go:419] "successfully completed AdvanceAuthentication request to CyberArk Identity; login complete" username="<REDACTED>"
|
@mladen-rusev-cyberark I'll merge this because I want to use it in my helm branch. Hope you don't mind. |
We were made aware in an email thread that the
v2/service/discoveryAPI will be deprecated in the end of 2025. A new API will be provided by Q3 2025. It is detailed in this document under/api/tenant-discovery/public. It will take a subdomain name as a query parameter&bySubdomain=and return a response in the following format:In this MR:
identity_administrationanddiscoverycontext) and return their URLsMockDiscoveryServerto accommodate the changes@wallrj-cyberark invited me to the test tenant https://tlskp-test.integration-cyberark.cloud/ which allowed me to run the
TestCyberArkClient_PutSnapshot_RealAPItest