The Wayback Machine - https://web.archive.org/web/20201028043310/https://github.com/metakgp/metakgp-wiki/pull/96
Skip to content

/ metakgp-wiki

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.

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

Added Ingress specific changes (backwards compatible) #96

Open
wants to merge 2 commits into
base: master
from
Open

Added Ingress specific changes (backwards compatible) #96

wants to merge 2 commits into from

Conversation

@grapheo12
Copy link
Contributor

@grapheo12 grapheo12 commented Aug 22, 2020

After @thealphadollar 's changes to ingress, there were some problems which I corrected.

After testing, I came up with this simple config:

Now we do not need to specify ports for wiki. It can listen to port 80. However, we need a fixed container name (which this PR fixes) and a fixed network name (also ensured by this PR and partly ensured by the naming convention of docker-compose).

See the current ingress docker-compose for the use of metakgp-wiki_nginx-network.

This makes the changes backwards compatible, meaning, even without Ingress, wiki would run without any change to the docker-compose file. Just then port forwarding has to be done manually.

If this is merged, #95 would become stale.
@grapheo12
Added Ingress specific changes (backward compatible)
Loading status checks…
4649a90
@grapheo12
Copy link
Contributor Author

@grapheo12 grapheo12 commented Aug 22, 2020 •
edited

@icyflame @amrav @thealphadollar Please review.
@thealphadollar
thealphadollar approved these changes Aug 23, 2020
View changes
Copy link
Contributor

@thealphadollar thealphadollar left a comment

Thanks @grapheo12 for the PR and all the amazing work on Ingress 🚀
@icyflame
icyflame reviewed Sep 1, 2020
View changes
Copy link
Member

@icyflame icyflame left a comment

This PR is removing the exposure of one container port to the host. So, deploying this would bring the wiki down? Or does nginx_network a network created by the Ingress repository?

image

This is the current network diagram. In this diagram, this PR will remove the link between host and Nginx container.

This makes the changes backwards compatible, meaning, even without Ingress, wiki would run without any change to the docker-compose file. Just then port forwarding has to be done manually.

Can you elaborate what you are trying to say here? We should avoid any manual step (even one-time steps) to ensure that we can bring the wiki up from scratch with just docker-compose.

I think we should expose to some port on the host and the ingress can expose that host port to the world. We can use UFW to ensure that only one world facing port exists.
docker-compose.override.yml
ports:
- "${SERVER_PORT:-8080}:80"
networks:
nginx-network:
Comment on lines -10 to +12

This comment has been minimized.

Sign in to view
@icyflame

icyflame Sep 1, 2020
Member

I am not sure how this is backward compatible? I think we have to stop the wiki server. Implement this change, then bring the ingress up and restart service?

This comment has been minimized.

Sign in to view
@grapheo12

grapheo12 Sep 1, 2020
Author Contributor

Yeah actually I meant backwards compatible in a loose sense as this will not do much change to the way wiki works now.
@icyflame
icyflame requested changes Sep 1, 2020
View changes
Copy link
Member

@icyflame icyflame left a comment

Changing review state;
@grapheo12
Copy link
Contributor Author

@grapheo12 grapheo12 commented Sep 1, 2020

Can you elaborate what you are trying to say here? We should avoid any manual step (even one-time steps) to ensure that we can bring the wiki up from scratch with just docker-compose.

I meant just as you restart the containers now, after this PR, there would be almost no change to how that happens.
Only change you need to do, if Ingress isn't being used, is to specify which port to forward to in the command line with -p8000:80 etc. flags.
@grapheo12
Copy link
Contributor Author

@grapheo12 grapheo12 commented Sep 1, 2020

In our current plan, there is only one port exposed to the host, and that is the port 80 of Ingress.
Instead of leaving different ports of Wiki and IQPS open in the host environment, we are using Docker networks for the containers to connect with each other.

Please check out: grapheo12/ingress@a069047
for the recent changes in the working of Ingress.

If you think this might not be a good idea, do tell us what to change.
@thealphadollar thealphadollar requested a review from icyflame Sep 2, 2020
@icyflame
Copy link
Member

@icyflame icyflame commented Sep 3, 2020 •
edited

@grapheo12 Yes, sorry, I don't think it's a good idea to make the addition of a command line option required. This requires quite a bit of explanation and I would like to avoid those kind of gotchas if possible.

This is my proposal:

  1. Host port exposed to the world is only port 80 for HTTPs, 443 for SSH
  2. All other host ports are blocked to world using UFW
  3. The nginx container inside metakgp-wiki exposes it's port 80 to host port 8001
  4. The nginx container inside iqps exposes it's port 80 to host port 8002 https://github.com/grapheo12/iqps/blob/master/docker-compose.yml.template#L8-L17
  5. Ingress's nginx container connects to port 80, 3001, 3002 on the host
  6. Incoming traffic on port 80 is proxy passed to the localhost's 3001 port in case it's wiki.metakgp.org request https://github.com/grapheo12/ingress/blob/a069047b4a900f47dc1734c6e29e91e2c2e78a8b/wiki.conf#L12
  7. Incoming traffic on port 80 is proxy passed to the localhost's 8002 port in case it's iqps.metakgp.org request https://github.com/grapheo12/ingress/blob/master/iqps.conf#L12

Here's a list of pros and cons I could think of. I can't think of any other cons, please add any that of 🙇‍♂️

Pros Cons
Host is safe as the only outward facing port is 80 UFW has to be set-up properly
Adding a new service can be done by adding a new conf to the nginx conf inside ingress
We don't depend on host names on the docker network being the same
We don't have to keep docker network names in sync across wiki, iqps, ingress to ensure routing. This is ensured by port numbers alone which will be kept in sync between wiki and ingress OR iqps and ingress
All configuration is inside each repo's docker-compose files; no command line options are required
Local testing is easy, simply connect to localhost:8001 etc on local PC

What do you think about this approach?
@grapheo12
Copy link
Contributor Author

@grapheo12 grapheo12 commented Sep 3, 2020

This was what Ingress initially did. And then we moved to docker networks. We then have to rollback ingress. 😅
@thealphadollar
Copy link
Contributor

@thealphadollar thealphadollar commented Sep 4, 2020 •
edited

@icyflame Do you think it is pro for the network that they ingress cannot be started without wiki and iqps containers running? I think this loose coupling will help avoid issues.

One con, a trivial one for our case, for networks is that bridge networks are slower compared to host network.

Anyways, it was my suggestion to use docker networks since I preferred to not expose ports to the system for this loose coupling and in general to avoid usage of ports that could be used by some other application. My apologies @grapheo12 for increasing the work for you.

I especially like the tests part and then these containers can be run independently as well. Can we please roll the docker-networks back? I'm really sorry for the extra work you had to do 😢
@thealphadollar
Merge branch 'master' into ingress-add
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
0b4ff92
@icyflame
Copy link
Member

@icyflame icyflame commented Sep 7, 2020

Ah, I see, I didn't follow the conversation closely about moving to docker networks closely. Sorry about the back and forth on this, Shubham! 🙇‍♂️

Do you think it is pro for the network that they ingress cannot be started without wiki and iqps containers running?

The ingress can be started, it won't work because the underlying service is not up but the ingress will be up and do it's job. Essentially, the ingress, wiki and iqps are completely independent.

  1. Without the ingress, no traffic from world can get in ✅
    1. This is very useful; during maintenance, we can simply bring the network down and forward host port 3001 to local port 3001 for testing 😌
  2. With ingress, world traffic will be proxy_passed to the appropriate port
    1. If the destination service (wiki, iqps, etc) is down, ingress doesn't really care about this and will not be affected by it (Decoupling)

I think this loose coupling will help avoid issues.

Yes, exactly 👍

Yes, I agree with your assessment that this will not affect us significantly.

image

I can't tell what unit the latency is in here, but either way, it doesn't matter because the difference is about 10-20% 🙆‍♂️
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants
@grapheo12 @icyflame @thealphadollar
You can’t perform that action at this time.