Fix aws connector automatic endpoint discovery

doodlesbykumbi commented 3 years ago

What does this PR do?

What's changed? Why were these changes made?

There were 2 bugs.

We modified only req.URL{Scheme, Host} with the new endpoint, we missed req.Host.
We were modifying the endpoint after signing the request.

2 did not show up until 1 was resolved. This is likely because the request signing doesn't take req.URL{Scheme, Host} into account.

How should the reviewer approach this PR, especially if manual tests are required?

N/A

Are there relevant screenshots you can add to the PR description?

N/A.

What ticket does this PR close?

Resolves #1369

Checklists

Change log

[x] The CHANGELOG has been updated, or
[ ] This PR does not include user-facing changes and doesn't require a CHANGELOG update

Test coverage

[ ] This PR includes new unit and integration tests to go with the code changes, or
[ ] The changes in this PR do not require tests

Documentation

[ ] This PR does not require updating any documentation, or
[x] Docs (e.g. READMEs) were updated in this PR, and/or there is a follow-on issue to update docs

(For releases only) Manual tests

[ ] Manually tested Keychain provider
[ ] Manually run Kubernetes-Conjur demo with a local Secretless Broker image build of your branch

doodlesbykumbi commented 3 years ago

@diverdane Thanks, CHANGELOG entry added

codeclimate[bot] commented 3 years ago

Code Climate has analyzed commit b54d2d96 and detected 1 issue on this pull request.

Here's the issue category breakdown:

Category	Count
Style	1

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 53.0% (0.0% change).

View more on Code Climate.

izgeri commented 3 years ago

@doodlesbykumbi this may be out of scope for this PR, but I think I saw you mention a mock AWS server that we might be able to use for AWS integration tests. should we file an issue in this project to implement that? it would have been nice to have some integration tests for this connector.

doodlesbykumbi commented 3 years ago

@izgeri I have an issue for that https://github.com/cyberark/secretless-broker/issues/1371. I'll tackle it at some point this week

cyberark / secretless-broker