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.

Sign up for GitHub

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

Jump to bottom

nginx router based on template #13840

Merged
merged 2 commits into from
Sep 21, 2017
Merged

nginx router based on template #13840

merged 2 commits into from
Sep 21, 2017

Conversation

rajatchopra
Copy link
Contributor

template based nginx router implementation

/cc @jawnsy

@smarterclayton
Copy link
Contributor

Nice

@louyihua
Copy link
Contributor

nginx supports include conf files, so it maybe better to split routes into individual conf files, one file per route.

@openshift-merge-robot openshift-merge-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 24, 2017
@tdawson tdawson removed their assignment Jul 25, 2017
@openshift-merge-robot openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 28, 2017
@openshift-merge-robot openshift-merge-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 28, 2017
@rajatchopra rajatchopra changed the title [WIP] [DO NOT MERGE] nginx router based on template. initial commit. nginx router based on template Aug 29, 2017
@rajatchopra
Copy link
Contributor Author

@openshift/networking Review please

@pweil-
Copy link
Contributor

pweil- commented Aug 29, 2017

/assign @knobunc

@pweil- pweil- removed their assignment Aug 29, 2017
pecameron
pecameron reviewed Aug 30, 2017
View reviewed changes
Copy link
Contributor

@pecameron pecameron left a comment

Choose a reason for hiding this comment

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

/lgtm
Is there going to be a doc change PR for this?

images/router/nginx/Dockerfile
#
FROM openshift/origin

RUN INSTALL_PKGS="nginx" && \
Copy link
Contributor

Choose a reason for hiding this comment

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

@rajatchopra Just curious here. How do we specify which version of nginx is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not specify-able yet. This is the base image being built. And we will pick the standard install from openshift/origin image's configured repositories.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 30, 2017
@lihongan
Copy link
Contributor

lihongan commented Sep 1, 2017

@rajatchopra How to start the nginx router ? Does it need other special options or same to haproxy router? e.g. just using oadm router --images=registry/openshift3/ose-nginx-router

lihongan
lihongan reviewed Sep 1, 2017
View reviewed changes
images/router/nginx/reload-nginx Outdated

config_file=/var/lib/nginx/conf/nginx.config
if [ -f /var/lib/nginx/logs/nginx.pid ]; then
/usr/sbin/nginx -c ${config_file} -s reload
Copy link
Contributor

Choose a reason for hiding this comment

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

indent ?

@rajatchopra
Copy link
Contributor Author

/test cmd

@rajatchopra
Copy link
Contributor Author

@lihongan
Yes start with 'oadm router --images=openshift/nginx-router', but we need to edit the dc for working-dir also. So the deployment config should have:

 - images: ...
 - command:
 - /usr/bin/openshift-router
 - --loglevel=2
 - --working-dir=/var/lib/nginx/router

How to start the nginx router ? Does it need other special options or same to haproxy router? e.g. just using oadm router --images=registry/openshift3/ose-nginx-router

knobunc
knobunc approved these changes Sep 6, 2017
View reviewed changes
Copy link
Contributor

@knobunc knobunc left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks Rajat!

We just need to make it clear that this is not going to be supported in Enterprise (at least for a while).

@knobunc
Copy link
Contributor

knobunc commented Sep 6, 2017

/lgtm

@rajatchopra
Copy link
Contributor Author

/assign @smarterclayton
Clayton, with this change the nginx router image will also be built. Do we need a 'community'/'contrib'/'vendor' kind of setup for such images? At least until this is fully supported.

@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2017
@rajatchopra
Copy link
Contributor Author

@eparis Removed the default pub keys file (was initially meant to provide a default cert for tls connections).

tls edge support
f0bc6b4 
add nginx to build local images script
@eparis
Copy link
Member

eparis commented Sep 21, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 21, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eparis, knobunc, pecameron, rajatchopra

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 21, 2017
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue

@openshift-merge-robot openshift-merge-robot merged commit dc11ae9 into openshift:master Sep 21, 2017
@smarterclayton
Copy link
Contributor

smarterclayton commented Sep 23, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pecameron pecameron pecameron left review comments

@lihongan lihongan lihongan left review comments

@knobunc knobunc knobunc approved these changes

Assignees

@smarterclayton smarterclayton

@eparis eparis

@knobunc knobunc

@pecameron pecameron

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet