add AK/SK Certification for uploading snapshot to cloud #972

liuqinguestc · 2019-07-23T02:13:16Z

What this PR does / why we need it:
add AK/SK Certification for uploading snapshot to cloud
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

codecov · 2019-07-23T03:58:31Z

Codecov Report

Merging #972 into master will increase coverage by 0.29%.
The diff coverage is 52.52%.

@@            Coverage Diff             @@
##           master     #972      +/-   ##
==========================================
+ Coverage   34.32%   34.61%   +0.29%     
==========================================
  Files          89       92       +3     
  Lines       16608    16858     +250     
==========================================
+ Hits         5700     5835     +135     
- Misses      10096    10197     +101     
- Partials      812      826      +14

Impacted Files	Coverage Δ
contrib/backup/multicloud/client.go	`0% <0%> (ø)`	⬆️
contrib/drivers/lvm/lvm.go	`23.66% <0%> (-0.34%)`	⬇️
...icloud/credentials/keystonecredentials/provider.go	`65.9% <65.9%> (ø)`
...ontrib/backup/multicloud/credentials/credential.go	`69.23% <69.23%> (ø)`
contrib/backup/multicloud/signer/signature.go	`71.85% <71.85%> (ø)`

skdwriting · 2019-07-23T04:49:53Z

Can you please attach the test report for this PR?

wisererik · 2019-07-23T05:50:19Z

@liuqinguestc I don't think your solution is correct when considering the identification when hotpot uploads snapshot to cloud through multi-cloud. IMO, signature should be generated in hotpot which will be passed to multi-cloud and identified in multi-cloud.

liuqinguestc · 2019-07-23T07:06:30Z

@wisererik I just do as you said , I will delete some codes that do not need.

xing-yang · 2019-07-27T21:08:08Z

Please add a release note.

xxwjj · 2019-07-29T02:45:07Z

contrib/backup/multicloud/auth/keystone.go

@@ -0,0 +1,189 @@
+// Copyright 2019 The OpenSDS Authors.


client.go has the same feature code, please use that one.

sorry，I can not use that, because the code is not same, and function is not the same.

xxwjj · 2019-07-29T02:46:16Z

contrib/backup/multicloud/auth/auth.go

@@ -0,0 +1,37 @@
+// Copyright 2019 The OpenSDS Authors.


fileter is used for api-server client, please remove this code.

sorry，this is used for keystone.go, I can not remove this code.

xxwjj · 2019-07-29T02:49:56Z

Gopkg.toml

@@ -101,3 +101,7 @@
  go-tests = true
  non-go = true
  unused-packages = true
+
+[[constraint]]
+  name = "github.com/emicklei/go-restful"


why do we need go-restfull lib?

just for getting signature. the code uses go-restful lib.

xxwjj · 2019-07-29T02:51:01Z

contrib/backup/multicloud/context/context.go

@@ -0,0 +1,61 @@
+// Copyright 2019 The OpenSDS Authors.


please remove this file.

this file is used by auth.go

xxwjj · 2019-07-29T02:52:35Z

contrib/backup/multicloud/credentials/keystonecredentials/provider.go

+	"strings"
+
+	"github.com/opensds/opensds/contrib/backup/multicloud/auth"
+


remove blank line

xxwjj · 2019-07-29T02:55:25Z

pkg/utils/constants/constants.go

@@ -56,4 +56,10 @@ const (
 	Read    = "Read"
 	Write   = "Write"
 	Execute = "Execute"
+
+	//Signature parameter name


these constants is only used for upload to snapshot to cloud, please move to contrib/backup/multi-cloud directory.

but as erik said. we should use public lib

rhsakarpos · 2019-07-31T03:58:13Z

Gopkg.toml

@@ -66,8 +66,8 @@
  version = "2.0.0"

 [[constraint]]
+  branch = "master"


this PR is too huge for a single commit. 16K+ lines of code, please split it up.

rhsakarpos · 2019-07-31T03:58:34Z

contrib/backup/multicloud/auth/auth.go

@@ -0,0 +1,37 @@
+// Copyright 2019 The OpenSDS Authors.
+//


OpenSDS copyright missing

Is these lines written newly by us?

OpenSDS copyright missing

Its there in the line 1

rhsakarpos · 2019-07-31T03:58:50Z

contrib/backup/multicloud/auth/keystone.go

@@ -0,0 +1,189 @@
+// Copyright 2019 The OpenSDS Authors.
+//


rhsakarpos · 2019-07-31T03:59:17Z

contrib/backup/multicloud/auth/keystone.go

+
+const (
+	ConfFile             = "/etc/opensds/driver/multi-cloud.yaml"
+	DefaultUploadTimeout = 30 // in Seconds


please add to conf

rhsakarpos · 2019-07-31T03:59:42Z

contrib/backup/multicloud/auth/keystone.go

+
+func (k *Keystone) loadConf(p string) (*MultiCloudConf, error) {
+	conf := &MultiCloudConf{
+		Endpoint:      "http://127.0.0.1:8088",


rhsakarpos · 2019-07-31T04:01:32Z

contrib/backup/multicloud/client.go

+			return nil, nil, err
+		}
+
+		requestDate := time.Now().UTC().Format("20060102")


why are the dates and times hard-coded ?

rhsakarpos · 2019-07-31T04:04:13Z

contrib/backup/multicloud/signer/signature.go

+		// and a termination string ("sign_request") in lowercase characters.
+		sign.credentialString,
+		//Step 4: Append the hash of the canonical request created in Task 1
+		hex.EncodeToString(makeSha256([]byte(sign.canonicalString))),


is it secure to store in String ? in JAVA it is recommended to store in char[], since due to GC not being done, the value of the String variable can be seen by a memory profiler

rhsakarpos · 2019-07-31T04:04:39Z

contrib/backup/multicloud/signer/signature.go

+func (sign *Signature) buildSignature() {
+	// Step 1:  Create the signing key, use the secret access key to create a series of
+	// hash-based message authentication codes (HMACs).
+	kSecret := sign.credValues.SecretAccessKey


error handling ? in case any makeHMAC fails ?

rhsakarpos · 2019-07-31T04:05:15Z

contrib/backup/multicloud/signer/signature.go

+	// hash-based message authentication codes (HMACs).
+	kSecret := sign.credValues.SecretAccessKey
+	kDate := makeHmac([]byte("OPENSDS"+kSecret), []byte(sign.requestDate))
+	kRegion := makeHmac(kDate, []byte(sign.Region))


key and value not matching here, please check all

rhsakarpos · 2019-07-31T04:06:05Z

contrib/backup/multicloud/signer/signature.go

+}
+
+// stripExcessSpaces will trim multiple side-by-side spaces.
+func stripExcessSpaces(vals []string) {


can we use strings.TrimSpace(s) ?

rhsakarpos · 2019-07-31T04:06:40Z

contrib/backup/multicloud/signer/signature.go

+	var canonicalURI string
+
+	if len(url.Opaque) > 0 {
+		canonicalURI = "/" + strings.Join(strings.Split(url.Opaque, "/")[3:], "/")


significance of [3:] ?

skdwriting

Why 142 file changes for this?! Kindly update the points in the PR description as well.

skdwriting · 2019-08-01T11:57:04Z

Gopkg.toml

@@ -66,8 +66,8 @@
  version = "2.0.0"

 [[constraint]]
+  branch = "master"


skdwriting · 2019-08-01T11:57:55Z

contrib/backup/multicloud/auth/auth.go

@@ -0,0 +1,37 @@
+// Copyright 2019 The OpenSDS Authors.
+//


Is these lines written newly by us?

skdwriting · 2019-08-01T11:58:32Z

contrib/backup/multicloud/auth/auth.go

@@ -0,0 +1,37 @@
+// Copyright 2019 The OpenSDS Authors.
+//


OpenSDS copyright missing

Its there in the line 1

skdwriting · 2019-08-01T12:04:26Z

contrib/backup/multicloud/auth/keystone.go

+
+func (k *Keystone) loadConf(p string) (*MultiCloudConf, error) {
+	conf := &MultiCloudConf{
+		Endpoint:      "http://127.0.0.1:8088",


Why hard-coded?

skdwriting · 2019-08-01T12:09:37Z

contrib/backup/multicloud/auth/keystone.go

+	var r tokens.GetResult
+	// The service token may be expired or revoked, so retry to get new token.
+	err := utils.Retry(2, "verify token", false, func(retryIdx int, lastErr error) error {
+		if retryIdx > 0 {


What are you trying to do here. I see some confusion here!

liuqinguestc added 11 commits July 17, 2019 07:46

add vendor dependences

963e7f4

add multi-cloud dependence

4b83c4b

add vendor dependences

6c00b6a

add snapshot upload function

c5f9eb6

add snapshot upload file

11bcc97

modify multi-cloud.yaml

d256ce8

optimizing the code.

e780435

Merge branch 'master' of https://github.com/liuqinguestc/opensds

530ce6a

modify driver_test error

d69e517

modify the provider_test error

86fbd78

modify signature_test error

7932ac4

liuqinguestc added 2 commits July 23, 2019 16:51

Optimize the codes for uploading snapshot to cloud

3b3207f

Optimize the codes for uploading snapshot to cloud

816dc7f

xing-yang added the feature there is a huge framework change or feature addition label Jul 27, 2019

xxwjj reviewed Jul 29, 2019

View reviewed changes

rhsakarpos reviewed Jul 31, 2019

View reviewed changes

contrib/backup/multicloud/auth/keystone.go

@@ -0,0 +1,189 @@

// Copyright 2019 The OpenSDS Authors.

//

Copy link

Contributor

rhsakarpos Jul 31, 2019

Choose a reason for hiding this comment

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

copyright

rhsakarpos reviewed Jul 31, 2019

View reviewed changes

skdwriting reviewed Aug 1, 2019

View reviewed changes

skdwriting requested changes Aug 1, 2019

View reviewed changes

		"strings"

		"github.com/opensds/opensds/contrib/backup/multicloud/auth"

add AK/SK Certification for uploading snapshot to cloud #972

Are you sure you want to change the base?

add AK/SK Certification for uploading snapshot to cloud #972

Conversation

liuqinguestc commented Jul 23, 2019

codecov bot commented Jul 23, 2019 • edited Loading

Codecov Report

skdwriting commented Jul 23, 2019

wisererik commented Jul 23, 2019

liuqinguestc commented Jul 23, 2019

xing-yang commented Jul 27, 2019

xxwjj Jul 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skdwriting left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jul 23, 2019 •

edited

Loading

xxwjj Jul 29, 2019 •

edited

Loading