Fix cloudfront middleware #2837

vishesh92 · 2019-01-30T13:12:01Z

@manishtomar According to documentation, ipfilteredby is not required. And if you don't set ipfilteredby for cloudfront middleware, registry crashes and you get the error panic: interface conversion: interface {} is nil, not string.

This PR fixes this issue and some minor formatting issues with cloudfront configuration docs.

GordonTheTurtle · 2019-01-30T13:12:04Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "fix-cloudfront-middleware" git@github.com:vishesh92/distribution.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354339184
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

manishtomar · 2019-01-31T00:00:27Z

@vishesh92 What is the fix here?

vishesh92 · 2019-01-31T11:18:03Z

@manishtomar According to documentation, ipfilteredby is not required. And if you don't set ipfilteredby for cloudfront middleware, registry crashes and you get the error panic: interface conversion: interface {} is nil, not string.

This PR fixes this issue and some minor formatting issues with cloudfront configuration docs.

manishtomar · 2019-01-31T23:41:11Z

@vishesh92 Code looks good. However the build is failing due to linter check. Not sure why. Can you please check and see if something stands out w.r.t this PR?

vishesh92 · 2019-02-01T06:09:36Z

@manishtomar make check is failing because of staticcheck warnings(https://travis-ci.org/docker/distribution/builds/486428072#L657). But there are no warnings in the file I made changes to. I am not sure why the build for master is passing.

vishesh92 · 2019-02-01T08:38:29Z

I triggered a build on master branch and Its failing there as well. https://travis-ci.org/vishesh92/distribution/builds/487321788

Its happening because version of gometalinter is not fixed and some changes were recently merged there: alecthomas/gometalinter#579

manishtomar · 2019-02-04T21:29:58Z

@vishesh92 Thanks for the info. Let me make a PR with fixes.

manishtomar · 2019-02-05T01:04:11Z

Closed and reopened to trigger build

codecov · 2019-02-05T01:11:27Z

Codecov Report

Merging #2837 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2837   +/-   ##
=======================================
  Coverage   60.45%   60.45%           
=======================================
  Files         102      102           
  Lines        8001     8001           
=======================================
  Hits         4837     4837           
  Misses       2517     2517           
  Partials      647      647

Flag	Coverage Δ
#linux	`60.45% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3ddc35...e1e72e9. Read the comment docs.

manishtomar

Could you please address the comment? Thanks!

manishtomar · 2019-02-05T01:10:02Z

registry/storage/driver/middleware/cloudfront/middleware.go

+			case "", "none":
+				awsIPs = nil
+			case "aws":
+				newAWSIPs(ipRangesURL, updateFrequency, nil)


Just noticed another bug (not w.r.t this PR): please assign this to awsIPs:

awsIPs = newAWSIPs(ipRangesURL, updateFrequency, nil)

vishesh92 · 2019-02-07T10:22:25Z

@manishtomar I have made those changes.

manishtomar

Thanks for the changes @vishesh92. LGTM. @dmcgowan @caervs Could any of you merge this?

vishesh92 · 2019-03-01T06:29:51Z

@dmcgowan @caervs @manishtomar any update on this?

caervs

@vishesh92 sorry for the delay. One last change from me and then I think we're good to merge.

registry/storage/driver/middleware/cloudfront/middleware.go

Signed-off-by: Vishesh Jindal <vishesh92@gmail.com>

vishesh92 · 2019-03-02T03:30:56Z

@caervs resolved your comments.

caervs · 2019-03-05T00:42:05Z

@caervs resolved your comments.

@vishesh92 thank you!

GordonTheTurtle added the dco/no label Jan 30, 2019

vishesh92 force-pushed the fix-cloudfront-middleware branch 3 times, most recently from 809fad8 to 6f111d0 Compare January 30, 2019 13:16

GordonTheTurtle removed the dco/no label Jan 30, 2019

vishesh92 force-pushed the fix-cloudfront-middleware branch 2 times, most recently from fa1c1ee to 49d16e9 Compare January 30, 2019 13:50

manishtomar mentioned this pull request Feb 5, 2019

Cloudfront storage documentation issue #2838

Closed

manishtomar closed this Feb 5, 2019

manishtomar reopened this Feb 5, 2019

manishtomar reviewed Feb 5, 2019

View reviewed changes

vishesh92 force-pushed the fix-cloudfront-middleware branch from 49d16e9 to f7de676 Compare February 7, 2019 06:50

manishtomar approved these changes Feb 7, 2019

View reviewed changes

caervs approved these changes Mar 1, 2019

View reviewed changes

registry/storage/driver/middleware/cloudfront/middleware.go Outdated Show resolved Hide resolved

caervs mentioned this pull request Mar 1, 2019

ipfilteredby is not required option in cloudfront, but do not validation check #2822

Closed

vishesh92 added 2 commits March 2, 2019 08:58

Bugfix: Make ipfilteredby not required

f9a0506

Signed-off-by: Vishesh Jindal <vishesh92@gmail.com>

Fix cloudfront documentation formatting

e1e72e9

Signed-off-by: Vishesh Jindal <vishesh92@gmail.com>

vishesh92 force-pushed the fix-cloudfront-middleware branch from f7de676 to e1e72e9 Compare March 2, 2019 03:29

caervs merged commit 6d62eb1 into distribution:master Mar 5, 2019

thaJeztah mentioned this pull request Dec 16, 2019

[release/2.7 backport] fix markdown issues on configuration page #3073

Merged

thaJeztah mentioned this pull request Jan 28, 2020

[release/2.7 backport] Bugfix: Make ipfilteredby not required #3088

Merged

Fix cloudfront middleware #2837

Fix cloudfront middleware #2837

Uh oh!

Conversation

vishesh92 commented Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GordonTheTurtle commented Jan 30, 2019

Uh oh!

manishtomar commented Jan 31, 2019

Uh oh!

vishesh92 commented Jan 31, 2019

Uh oh!

manishtomar commented Jan 31, 2019

Uh oh!

vishesh92 commented Feb 1, 2019

Uh oh!

vishesh92 commented Feb 1, 2019

Uh oh!

manishtomar commented Feb 4, 2019

Uh oh!

manishtomar commented Feb 5, 2019

Uh oh!

codecov bot commented Feb 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

manishtomar left a comment

Choose a reason for hiding this comment

Uh oh!

manishtomar Feb 5, 2019

Choose a reason for hiding this comment

Uh oh!

vishesh92 commented Feb 7, 2019

Uh oh!

manishtomar left a comment

Choose a reason for hiding this comment

Uh oh!

vishesh92 commented Mar 1, 2019

Uh oh!

caervs left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vishesh92 commented Mar 2, 2019

Uh oh!

caervs commented Mar 5, 2019

Uh oh!

Uh oh!

vishesh92 commented Jan 30, 2019 •

edited

Loading

codecov bot commented Feb 5, 2019 •

edited

Loading