Skip to content
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

136 iam credentials not used when calling neptune #140

Merged
merged 15 commits into from Aug 1, 2023

Conversation

charlesivie
Copy link
Collaborator

Issue #136

Description of changes:
Refactored the Proxy server to generate the IAM auth header correctly, and only when IAM is active.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@charlesivie charlesivie linked an issue Jul 26, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented Jul 26, 2023

Codecov Report

Merging #140 (3bc339f) into main (894977c) will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main    #140   +/-   ##
=====================================
  Coverage   9.09%   9.09%           
=====================================
  Files        444     444           
  Lines      29697   29697           
  Branches     208     208           
=====================================
  Hits        2701    2701           
  Misses     26990   26990           
  Partials       6       6           

Copy link

@gopuneet gopuneet Jul 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove all .DS_Store files from the PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, this has now been done.

fetch(url, { headers: req.headers, })
.then(async res => {
console.log("url: ", urlString);
console.log;
Copy link

@gopuneet gopuneet Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this line 74?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed this

service: "neptune-db",
region: headers["aws-neptune-region"],
}).then((data) => {
headers = data;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, we are replacing the headers passed to the function with the IAM headers here.
Q. Is there a risk that if any headers were passed in the request, they would be removed and if so, should we append the IAM headers to headers instead of replacing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

…rough on every request, even when IAM is turned on.
…rough on every request, even when IAM is turned on.
gopuneet
gopuneet previously approved these changes Jul 28, 2023
Copy link

@gopuneet gopuneet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Added some minor comments.

Feel free to override approval requirement and merge if addressing them, thanks.

Comment on lines 165 to 139
next(error);
console.log(error);
// send the json response in the next call
res.send(error);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have different code in the catch block of RDF summary call compared to PG summary call:
PG one has next(error); whereas this RDF one has res.send(error);

Not sure if they function similarly thus calling out to double-check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

refactored to have the same behaviour

console.error("No IAM credentials found in provider chain", e);
}
};
const e = require("express");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this might be unused, recommend removing it if not required.

Copy link

@gopuneet gopuneet Jul 28, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider deleting the 2 .DS_Store files explicitely since it looks like the updated .gitignore is not applying on already committed code.

@charlesivie charlesivie merged commit dad4c7f into main Aug 1, 2023
3 checks passed
@michaelnchin michaelnchin deleted the 136-iam-credentials-not-used-when-calling-neptune branch August 7, 2023 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

IAM credentials not used when calling Neptune
3 participants