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

Add metadata to ConsoleMe cred request body #48

Merged
merged 8 commits into from
Mar 2, 2021

Conversation

patricksanders
Copy link
Contributor

This PR adds some system metadata in ConsoleMe credential requests to be used as session tags when generating credentials. There's also a bit of refactoring to pay down some tech debt.

@patricksanders patricksanders marked this pull request as ready for review March 1, 2021 22:42
"github.com/netflix/weep/util"
)

func IamInfoHandler(w http.ResponseWriter, r *http.Request) {

awsArn, _ := util.ArnParse(metadata.Role)
// TODO: this was crashing because of a nil pointer dereference. Fix it!
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if you meant to commit with this TODO.
Is it nil because a role may not be specified for the new combined metadata and ECS mode? would a simple check to check metadata.Role first suffice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a TODO for later, along with the others. The old metadata package had almost everything refactored out of it other than a few variables that were directly accessed, which was an anti-pattern. I'll be able to fix it with the CLI UX refactor since we'll be able to count on a "default" role being set.

Also, the endpoint that this handler is for doesn't get called frequently (or maybe at all) -- if it was, it would panic with a nil pointer dereference. There's a decent chance we could just remove it entirely.

iamInfo := MetaDataIamInfoResponse{
Code: "Success",
//LastUpdated: metadata.LastRenewal.UTC().Format("2006-01-02T15:04:05Z"),
LastUpdated: "", // TODO: fix this
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto for this TODO. If we don't have metadata.LastRenewal, make it blank?


awsArn, err := util.ArnParse(metadata.Role)
// TODO: this was crashing because of a nil pointer dereference. Fix it!
awsArn, err := util.ArnParse("")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should bail on serving metadata if metadata.Role is not defined, and just serve a generic error?

@patricksanders patricksanders merged commit 77f6d7c into master Mar 2, 2021
@patricksanders patricksanders deleted the feat/cm-metadata branch March 2, 2021 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants