Skip to content

Comments

Add whitelisting docker registries#1742

Merged
tpoitras merged 1 commit intoopenshift:masterfrom
tpoitras:whitelisting-BZ1307210
Mar 22, 2016
Merged

Add whitelisting docker registries#1742
tpoitras merged 1 commit intoopenshift:masterfrom
tpoitras:whitelisting-BZ1307210

Conversation

@tpoitras
Copy link

Tagging @rhatdan for Tech Review:

Added details on whitelisting docker registries as per https://bugzilla.redhat.com/show_bug.cgi?id=1307210

Pretty rendered docs build:

http://file.bne.redhat.com/tpoitras/2016/Mar15/openshift-enterprise/whitelisting-BZ1307210/install_config/install/docker_registry.html#whitelisting-docker-registries

Questions:

  1. Would this apply to OpenShift Origin as well?
  2. Do we really need the = sign in between the --add-registry command and the specified registry? Example from my docs update, based on comment#1 in the BZ:

ADD_REGISTRY='--add-registry=registry.access.redhat.com

Because on my test machine, my /etc/sysconfig/docker file already has this line:

ADD_REGISTRY='--add-registry registry.access.redhat.com'

@tpoitras tpoitras added this to the Next Release milestone Mar 15, 2016
@tpoitras tpoitras force-pushed the whitelisting-BZ1307210 branch from cd783e5 to 3f4165d Compare March 15, 2016 05:14
@tpoitras tpoitras changed the title Add whitelisting docker registries [TECH REV] Add whitelisting docker registries Mar 15, 2016
@runcom
Copy link
Member

runcom commented Mar 16, 2016

Would this apply to OpenShift Origin as well?

I guess this is up to the docker daemon running on the instances. I'll defer the answer to who know this better since I'm not familiar with Origin.

Do we really need the = sign in between the --add-registry command and the specified registry? Example from my docs update, based on comment#1 in the BZ:

No, we don't really need the equal sign at all, but docker options take it, just a matter of style, but both with and without sign are accepted options

cat /proc/`pidof docker`/cmdline 
/usr/bin/docker daemon --add-registry localhost:5000 --block-registry all

@runcom
Copy link
Member

runcom commented Mar 16, 2016

changes LGTM

@tpoitras tpoitras added branch/dedicated-3.2 peer-review-in-progress Signifies that the peer review team is reviewing this PR and removed tech_review branch/dedicated-3.2 labels Mar 16, 2016
@tpoitras
Copy link
Author

Tagging team for peer review: @adellape @bfallonf @tnguyen-rh @ahardin-rh

Assuming this applies to Origin as well as Enterprise, maybe @adellape knows since he's a bit closer to Origin than me?

@tpoitras tpoitras changed the title [TECH REV] Add whitelisting docker registries Add whitelisting docker registries Mar 16, 2016

Choose a reason for hiding this comment

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

I think this would need ===='s around it? Example tags?

@tnguyen-rh
Copy link

@tpoitras One nit, otherwise LGTM.

@tpoitras tpoitras force-pushed the whitelisting-BZ1307210 branch from 3f4165d to 1ddbc20 Compare March 21, 2016 02:33
@tpoitras
Copy link
Author

Updated as per peer review. Thank you, @bfallonf & @tnguyen-rh :octocat:

Good to merge once Travis is done the build confirmation.

@tpoitras tpoitras removed the peer-review-in-progress Signifies that the peer review team is reviewing this PR label Mar 21, 2016
@tpoitras
Copy link
Author

tpoitras commented Mar 22, 2016

[rev_history]
|link:../install_config/install/docker_registry.html[Installing -> Deploying a Docker Registry]
|Added details on whitelisting Docker registries.
%

tpoitras pushed a commit that referenced this pull request Mar 22, 2016
Add whitelisting docker registries
@tpoitras tpoitras merged commit 8faabd3 into openshift:master Mar 22, 2016
@tpoitras tpoitras deleted the whitelisting-BZ1307210 branch March 22, 2016 00:04
@tpoitras tpoitras modified the milestones: Future Release, Next Release Mar 22, 2016
@adellape adellape modified the milestones: OSE 3.2, Future Release May 3, 2016
@adellape adellape modified the milestones: Staging, OSE 3.2, OSE 3.2 (Picked) May 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants