feat: enable --header flag to specify headers with requests #794

wangxiaoxuan273 · 2023-02-09T16:14:35Z

Resolves #521

Designed in accordance to the behavior(as exact spec can't be found) of curl-H flag. Can be used multiple times.

Terminal output:

Some minor issues/questions:

when parsing input like "c: d, e, f" into map[string][]string of golang, the spaces before and after the commas are not stripped. i.e. the values are "d", " e", " f".
(update: splitting is removed, the argument is kept as what is after the colon.)
curl parses input -H "a:b" -H "a:b" to two repeated headers "a:b", and -H "a:b" -H "a:c" to headers "a:b", "a:c". In this pr these behaviors are maintained.

TerryHowe

generally looks good to me, couple comments

cmd/oras/internal/option/remote.go

TerryHowe · 2023-02-09T19:01:18Z

cmd/oras/internal/option/remote.go

+			before, after, found := strings.Cut(h, ":")
+			if found && after != "" {
+				if headers[before] == nil {
+					headers[before] = strings.Split(after, ",")


I hadn't noticed this syntax before in other commands. It looks convenient, but not sure people will think to use it.

Did you mean the argument of the -H command? I designed it according to the behavior of curl -H flag. @TerryHowe

No, I was talking about the way you can provide multiple header values with the comma separation.

@TerryHowe Are you suggesting we shouldn't unbox the input value and just forward what is provided?

I don't have a strong opinion about it, but it seemed a bit odd. I don't think curl uses that comma separated format. I think you just have to specify the same header with multiple -H flags

curl don't use comma to separate header values because:

value parsing can be done on server side. This also implies that oras don't need to split any header value.

the header key can be duplicated for curl. This is different here, because in golang, the underlying data struct http.Header is a map, and thus header key cannot be duplicated. So we might need to concatenate duplicated header with comma explicitly.

Update: removed comma unboxing.

qweeah

Made some comments on implementation and UX. Thanks again for contributing!

cmd/oras/internal/option/remote.go

qweeah · 2023-02-10T01:13:52Z

cmd/oras/internal/option/remote.go

+	if len(opts.headers) != 0 {
+		headers := map[string][]string{}
+		for _, h := range opts.headers {
+			before, after, found := strings.Cut(h, ":")


UX: should return an error if a flag is not formatted as <name>:<comma-separated-values>, so users can be aware of the potential errors in their input.
cc @FeynmanZhou

Returned error currently, need further discussion and confirmation.

cmd/oras/internal/option/remote.go

qweeah · 2023-02-10T01:23:38Z

when parsing input like "c: d, e, f" into map[string][]string of golang, the spaces before and after the commas are not stripped. i.e. the values are "d", " e", " f".

We can just forward the value with trailing space. By design it should be ignored. I didn't find the RFC for this but there are some MDN docs mentioning it.

curl parses input -H "a:b" -H "a:b" to two repeated headers "a:b", and -H "a:b" -H "a:c" to headers "a:b", "a:c". In this pr these behaviors are maintained.

Shouldn't -H "a:b" -H "a:c" be parsed to a:b,c(in your implementation)?

codecov-commenter · 2023-02-10T06:42:27Z

Codecov Report

Merging #794 (a254d7a) into main (d6240d6) will increase coverage by 0.52%.
The diff coverage is 84.21%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #794      +/-   ##
==========================================
+ Coverage   55.50%   56.03%   +0.52%     
==========================================
  Files          23       23              
  Lines         935      953      +18     
==========================================
+ Hits          519      534      +15     
- Misses        374      376       +2     
- Partials       42       43       +1

Impacted Files	Coverage Δ
cmd/oras/internal/option/remote.go	`69.34% <84.21%> (+1.39%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

qweeah · 2023-02-10T07:10:33Z

cmd/oras/internal/option/remote.go

+			name, csv, found := strings.Cut(h, ":")
+			if !found || strings.TrimSpace(csv) == "" {
+				return fmt.Errorf("cannot parse headers: %q", h)
+			}
+			headers[name] = append(headers[name], strings.Split(csv, ",")...)


No need to unbox the value, just add it to the http headers.

Suggested change

name, csv, found := strings.Cut(h, ":")

if !found || strings.TrimSpace(csv) == "" {

return fmt.Errorf("cannot parse headers: %q", h)

}

headers[name] = append(headers[name], strings.Split(csv, ",")...)

name, value, found := strings.Cut(h, ":")

if !found || strings.TrimSpace(name) == "" || strings.TrimSpace(value) == "" {

return fmt.Errorf("invalid header: %q", h)

}

headers[name] = append(headers[name], value)

removed the unboxing. Behavior changed according to the RFC 2616 spec.

shizhMSFT · 2023-02-10T06:58:20Z

cmd/oras/internal/option/remote.go

@@ -51,6 +51,9 @@ type Remote struct {
 	resolveDialContext    func(dialer *net.Dialer) func(context.Context, string, string) (net.Conn, error)
 	applyDistributionSpec bool
 	distributionSpec      distributionSpec
+


nit: remove empty line

shizhMSFT · 2023-02-10T07:10:49Z

cmd/oras/internal/option/remote.go

+			if !found || strings.TrimSpace(csv) == "" {
+				return fmt.Errorf("cannot parse headers: %q", h)
+			}
+			headers[name] = append(headers[name], strings.Split(csv, ",")...)


We should consume the value as it is without splitting.

You can try -H "foo: a" -H "foo: b,c" with curl. It will generate

> foo: a > foo: b,c

Changed according to Billy's comment above. Splitting is removed.

shizhMSFT · 2023-02-10T08:03:43Z

cmd/oras/internal/option/remote.go

+		for _, h := range opts.headerFlags {
+			name, value, found := strings.Cut(h, ":")
+			if !found || strings.TrimSpace(value) == "" {
+				return fmt.Errorf("cannot parse headers: %q", h)


nit:

Suggested change

return fmt.Errorf("cannot parse headers: %q", h)

return fmt.Errorf("invalid header: %q", h)

changed accordingly.

shizhMSFT · 2023-02-10T08:04:34Z

cmd/oras/internal/option/remote.go

+		headers := map[string][]string{}
+		for _, h := range opts.headerFlags {
+			name, value, found := strings.Cut(h, ":")
+			if !found || strings.TrimSpace(value) == "" {


According to RFC 2616 Section 4.2, the name must not be empty and the value can be empty.

Note:

package main import ( "net/http" "os" ) func main() { headers := make(http.Header) headers[""] = []string{"abc"} headers[" "] = []string{"efg"} headers["empty"] = []string{""} headers["space"] = []string{" "} headers["foo"] = []string{"bar"} headers["foo2"] = []string{" bar2"} headers.Write(os.Stdout) }

outputs

empty: foo: bar foo2: bar2 space:

According to RFC 2616 Section 4.2, the name must not be empty and the value can be empty.

curl's behavior is the other way around. Which should I follow, curl or the RFC spec?

Always follow the spec since the spec is the standard.

Changed the behavior of parsing to be conformant to the spec.

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

cmd/oras/internal/option/remote.go

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

cmd/oras/internal/option/remote_test.go

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

qweeah

LGTM

shizhMSFT

LGTM with suggestions

shizhMSFT · 2023-02-13T14:45:34Z

cmd/oras/internal/option/remote.go

@@ -62,6 +64,7 @@ func (opts *Remote) EnableDistributionSpecFlag() {
 func (opts *Remote) ApplyFlags(fs *pflag.FlagSet) {
 	opts.ApplyFlagsWithPrefix(fs, "", "")
 	fs.BoolVarP(&opts.PasswordFromStdin, "password-stdin", "", false, "read password or identity token from stdin")
+	fs.StringArrayVarP(&opts.headerFlags, "header", "H", []string{}, "add custom headers to requests")


nit: in golang, nil slice is still a valid slice.

Suggested change

fs.StringArrayVarP(&opts.headerFlags, "header", "H", []string{}, "add custom headers to requests")

fs.StringArrayVarP(&opts.headerFlags, "header", "H", nil, "add custom headers to requests")

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

shizhMSFT

LGTM

wangxiaoxuan273 changed the title ~~feat: enable custom headers with requests~~ feat: enable --header flag to specify headers with requests Feb 9, 2023

TerryHowe reviewed Feb 9, 2023

View reviewed changes

qweeah requested changes Feb 10, 2023

View reviewed changes

qweeah reviewed Feb 10, 2023

View reviewed changes

shizhMSFT reviewed Feb 10, 2023

View reviewed changes

wangxiaoxuan273 requested a review from shizhMSFT February 10, 2023 07:49

shizhMSFT reviewed Feb 10, 2023

View reviewed changes

wangxiaoxuan273 added 5 commits February 13, 2023 07:00

feat: enable custom headers with requests

fe4ebdd

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

updated corner case tests and handling

32be258

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

resolved comments and added unit test cases

39f1ccc

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

resolved comments

b157b4b

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

resolved the comments

b5a6d7f

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

qweeah reviewed Feb 13, 2023

View reviewed changes

cmd/oras/internal/option/remote.go Show resolved Hide resolved

added a doc comment

9e20d2f

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

wangxiaoxuan273 requested review from shizhMSFT and qweeah and removed request for shizhMSFT February 13, 2023 07:14

qweeah requested changes Feb 13, 2023

View reviewed changes

cmd/oras/internal/option/remote_test.go Outdated Show resolved Hide resolved

cmd/oras/internal/option/remote_test.go Outdated Show resolved Hide resolved

improved test cases

11576e1

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

qweeah approved these changes Feb 13, 2023

View reviewed changes

shizhMSFT approved these changes Feb 13, 2023

View reviewed changes

applied suggestion

a254d7a

Signed-off-by: wangxiaoxuan273 <wangxiaoxuan119@gmail.com>

shizhMSFT approved these changes Feb 14, 2023

View reviewed changes

shizhMSFT merged commit 1543ce2 into oras-project:main Feb 14, 2023

wangxiaoxuan273 deleted the headers branch February 14, 2023 03:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable --header flag to specify headers with requests #794

feat: enable --header flag to specify headers with requests #794

wangxiaoxuan273 commented Feb 9, 2023 •

edited

Loading

TerryHowe left a comment

TerryHowe Feb 9, 2023

wangxiaoxuan273 Feb 10, 2023 •

edited

Loading

TerryHowe Feb 10, 2023

qweeah Feb 10, 2023

TerryHowe Feb 10, 2023

qweeah Feb 10, 2023 •

edited

Loading

wangxiaoxuan273 Feb 13, 2023

qweeah left a comment

qweeah Feb 10, 2023

wangxiaoxuan273 Feb 10, 2023

qweeah commented Feb 10, 2023 •

edited

Loading

codecov-commenter commented Feb 10, 2023 •

edited

Loading

qweeah Feb 10, 2023 •

edited

Loading

wangxiaoxuan273 Feb 13, 2023 •

edited

Loading

shizhMSFT Feb 10, 2023

wangxiaoxuan273 Feb 10, 2023

shizhMSFT Feb 10, 2023

wangxiaoxuan273 Feb 10, 2023 •

edited

Loading

shizhMSFT Feb 10, 2023

wangxiaoxuan273 Feb 13, 2023

shizhMSFT Feb 10, 2023

shizhMSFT Feb 10, 2023

wangxiaoxuan273 Feb 10, 2023

shizhMSFT Feb 10, 2023

wangxiaoxuan273 Feb 13, 2023

qweeah left a comment

shizhMSFT left a comment

shizhMSFT Feb 13, 2023

shizhMSFT left a comment

	return fmt.Errorf("cannot parse headers: %q", h)
	return fmt.Errorf("invalid header: %q", h)

	fs.StringArrayVarP(&opts.headerFlags, "header", "H", []string{}, "add custom headers to requests")
	fs.StringArrayVarP(&opts.headerFlags, "header", "H", nil, "add custom headers to requests")

feat: enable --header flag to specify headers with requests #794

feat: enable --header flag to specify headers with requests #794

Conversation

wangxiaoxuan273 commented Feb 9, 2023 • edited Loading

TerryHowe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wangxiaoxuan273 Feb 10, 2023 • 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

qweeah Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qweeah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qweeah commented Feb 10, 2023 • edited Loading

codecov-commenter commented Feb 10, 2023 • edited Loading

Codecov Report

qweeah Feb 10, 2023 • edited Loading

Choose a reason for hiding this comment

wangxiaoxuan273 Feb 13, 2023 • 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

wangxiaoxuan273 Feb 10, 2023 • 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

qweeah left a comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shizhMSFT left a comment

Choose a reason for hiding this comment

wangxiaoxuan273 commented Feb 9, 2023 •

edited

Loading

wangxiaoxuan273 Feb 10, 2023 •

edited

Loading

qweeah Feb 10, 2023 •

edited

Loading

qweeah commented Feb 10, 2023 •

edited

Loading

codecov-commenter commented Feb 10, 2023 •

edited

Loading

qweeah Feb 10, 2023 •

edited

Loading

wangxiaoxuan273 Feb 13, 2023 •

edited

Loading

wangxiaoxuan273 Feb 10, 2023 •

edited

Loading