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

Checking IAM for permissions #5387

Merged
merged 6 commits into from
May 9, 2020
Merged

Checking IAM for permissions #5387

merged 6 commits into from
May 9, 2020

Conversation

gja
Copy link
Contributor

@gja gja commented May 7, 2020

This PR allows you to create backups with credentials provided from IAM roles


This change is Reviewable

@gja gja requested a review from martinmr May 7, 2020 10:26
@gja gja requested review from manishrjain and a team as code owners May 7, 2020 10:26
Copy link
Contributor Author

@gja gja left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @manishrjain and @martinmr)


worker/s3_handler.go, line 64 at r1 (raw file):

		return err
	}

I moved this check up because there is no need to make API calls to get IAM credentials if the request has AccessKey and SecretKey


worker/s3_handler.go, line 114 at r1 (raw file):

	var creds credentials.Value
	switch {
	case h.creds.isAnonymous():

Technically, all of these cases could have also been handled with credentials.Chain, but I'm doing minimal changes here.

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

One comment.

Also, we don't have automatic tests for s3 so you'd need to write one for minio if possible. Otherwise manual tests are fine.

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gja and @manishrjain)


worker/s3_handler.go, line 84 at r1 (raw file):

Quoted 8 lines of code…

	req.AccessKey = defaultCreds.AccessKeyID


	req.SecretKey = defaultCreds.SecretAccessKey


	req.SessionToken = defaultCreds.SessionToken

This logic is different now. I think the original only filled the the empty values.
The if above covers the access and secret key but not the session token.


worker/s3_handler.go, line 114 at r1 (raw file):

Previously, gja (Tejas Dinkar) wrote…

Technically, all of these cases could have also been handled with credentials.Chain, but I'm doing minimal changes here.

Ok. I was not very familiar with the minio library but I can look into this later.

Copy link
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

Defer to @martinmr .

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gja and @manishrjain)

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

If we are going to make the credentials all be the default or all be in the request, then this is good with me.

:lgtm:

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @gja)

Copy link
Contributor

@martinmr martinmr left a comment

Choose a reason for hiding this comment

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

Just a couple of comments but otherwise it :lgtm:

Reviewed 7 of 7 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @gja)


worker/s3_handler.go, line 59 at r1 (raw file):

// FillRestoreCredentials fills the empty values with the default credentials so that
// a restore request is sent to all the groups with the same credentials.
func FillRestoreCredentials(location string, req *pb.RestoreRequest) error {

You should cherry pick these changes into the 1.2 and 20.03 branches.

However, this function is part of the online restore, which is not in any of those branches. So if there's a conflict, you can just drop this function.


worker/s3_handler.go, line 100 at r2 (raw file):

	if h.creds.isAnonymous() {
		return minio.New(uri.Host, h.creds.AccessKey, h.creds.SecretKey, secure)
	}

Is this the right thing to do if the bucket is anonymous? Wouldn't we want to pass an empty access and secret key?

@gja
Copy link
Contributor Author

gja commented May 8, 2020

@martinmr I just kept the existing behavior for anonymous. I think this is a new feature, so I don't think it's worth it to backport it (maybe to 20.03 at the most)

@gja
Copy link
Contributor Author

gja commented May 9, 2020

@martinmr I took your suggestion, and anonymous now takes empty string as credentials.

@gja gja merged commit 1b311d5 into master May 9, 2020
@gja gja deleted the use-iam branch May 9, 2020 10:38
dna2github pushed a commit to dna2fork/dgraph that referenced this pull request Jul 18, 2020
* Checking IAM for permissions

* FillRestoreCredentials uses IAM

* Upgrading minio
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants