Lift service discovery client out of CyberArkClient#765
Conversation
Service discovery will also be required for getting encryption keys for envelope encryption. This change makes the service discovery client available so we can fetch keys before we sanitise / encrypt secret data Signed-off-by: Ashley Davis <ashley.davis@cyberark.com>
SgtCoDFish
force-pushed
the
lift-sd
branch
2 times, most recently
from
12fb61c to
b08d214
Compare
| identityClient := identity.New(httpClient, identityAPI, cfg.Subdomain) | ||
|
|
||
| err := identityClient.LoginUsernamePassword(ctx, cfg.Username, []byte(cfg.Secret)) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
note: Moving the login down isn't strictly required in this PR, but I did it so that we check both the identity and discovery API availability first, before we try the login. This means if we didn't get a disco API endpoint from service discovery, we won't do a pointless login attempt.
Signed-off-by: Ashley Davis <ashley.davis@cyberark.com>
wallrj-cyberark
left a comment
wallrj-cyberark
left a comment
There was a problem hiding this comment.
A couple of suggestions, for this PR or a followup.
Looks fine to merge as-is if you want to avoid another round of CI and review.
| func NewDatauploadClient(ctx context.Context, httpClient *http.Client, serviceMap *servicediscovery.Services, cfg ClientConfig) (*dataupload.CyberArkClient, error) { | ||
| identityAPI := serviceMap.Identity.API | ||
| if identityAPI == "" { | ||
| return nil, errors.New("service discovery returned an empty identity API") |
There was a problem hiding this comment.
Optional: This error still talks about "service discovery returned..."
| return nil, errors.New("service discovery returned an empty identity API") | |
| return nil, errors.New("missing or empty serviceMap.Identity.API") |
|
|
||
| discoveryAPI := serviceMap.DiscoveryContext.API | ||
| if discoveryAPI == "" { | ||
| return nil, errors.New("service discovery returned an empty discovery API") |
There was a problem hiding this comment.
Optional: This error still talks about "service discovery returned..."
| return nil, errors.New("service discovery returned an empty discovery API") | |
| return nil, errors.New("missing or empty serviceMap.DiscoveryContext.API") |
| if err != nil { | ||
| return nil, err | ||
| } | ||
| func NewDatauploadClient(ctx context.Context, httpClient *http.Client, serviceMap *servicediscovery.Services, cfg ClientConfig) (*dataupload.CyberArkClient, error) { |
There was a problem hiding this comment.
Maybe check for nil serviceMap here, and return an error, to avoid a panic, or make the argument a value rather than a pointer.
|
Thanks for the review; I'll think about the nil check in a future PR. I think the errors are fine because ultimately the URLs come from service discovery, right? |
2 participants
First commit; Service discovery will also be required for getting encryption keys for envelope encryption. This change makes the service discovery client available earlier, so we can fetch keys before we sanitise / encrypt secret data.
Second commit: Bit more visibility into potentially flaky tests.