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.

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

Add instuctions for updating cloudflared binary. #215

Merged
merged 5 commits into from
Aug 31, 2020

Conversation

iUnknwn
Copy link
Contributor

@iUnknwn iUnknwn commented Jan 16, 2020

Adds a section to the DNS-Over-HTTPS guide with instructions for how to ensure the cloudflared daemon is kept up-to-date.

This adds update instructions for cloudflared, which handles one part of issue #175

@dschaper
Copy link
Member

Thanks for the submission. I'm not a fan of automated updates without intervention so I'm not going to approve but the other team members may have different opinions.

@dschaper
Copy link
Member

Screenshot_2020-01-15 Command-line Arguments - Argo Tunnel(1)

@iUnknwn
Copy link
Contributor Author

iUnknwn commented Jan 16, 2020

The docs I added are simply calling that, actually (and then calling systemctl restart) - if you'd prefer, I can alter the docs to simply explain how to do an updating manually, and then just have a note you can automate it by placing it inside the weekly cron folder?

@dschaper
Copy link
Member

Yeah, that sounds like a better way to document it. Have the manual process be the main topic and then add a note for how to automate. Bonus if you want to do cron and systemds timers!

@iUnknwn
Copy link
Contributor Author

iUnknwn commented Feb 2, 2020

@dschaper Done - I've updated the docs so the manual method is described first, and then there's a note on how to do automatic updating via cron.

Also, I made a small comment earlier in the guide that cloudflared will also work with other DoH providers (so the same setup can be used for google DNS for example).

@dschaper dschaper closed this Feb 11, 2020
@dschaper dschaper reopened this Feb 11, 2020
@dschaper
Copy link
Member

@iUnknwn Can you rebase this on master? There have been some changes.

@netlify
Copy link

netlify bot commented Feb 17, 2020

Deploy preview for pihole-docs ready!

Built with commit 3337b2a

https://deploy-preview-215--pihole-docs.netlify.app

@iUnknwn
Copy link
Contributor Author

iUnknwn commented Feb 17, 2020

@dschaper Done - rebased onto master.

@dschaper
Copy link
Member

Thanks, some linting comments, I'll check them over in a bit.

 docs/guides/dns-over-https.md:65:99 MD034/no-bare-urls Bare URL used [Context: "https://8.8.8.8/dns-query"]
docs/guides/dns-over-https.md:156:91 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
docs/guides/dns-over-https.md:156:91 MD047/single-trailing-newline Files should end with a single newline character

@XhmikosR
Copy link
Contributor

I don't think this patch is correct. If one has installed cloudflared following the existent guide, then cloudflared won't run as root so updating it with its built-in command can mess things up.

I suggest that we focus on finishing #186 first and build on top of that.

@XhmikosR XhmikosR mentioned this pull request Feb 25, 2020
3 tasks
@XhmikosR
Copy link
Contributor

@iUnknwn can you fetch the latest master and check what still applies? I added some info in #186

@iUnknwn
Copy link
Contributor Author

iUnknwn commented Mar 29, 2020

This overlaps with the majority of my PR - the only thing that still might be useful is the note about how cloudflared can be configured to use other DoH providers.

Do you want me to update this PR, or open a new one just for that specific improvement?

@XhmikosR
Copy link
Contributor

I think you should try keeping most of your changes and we see the diff. There are some differences including the cron job (which I thought is not needed)

@iUnknwn
Copy link
Contributor Author

iUnknwn commented Apr 2, 2020

@XhmikosR OK - all of those changes should be in this PR - do you want me to do a rebase onto the current docs, or just leave what I currently have?

@XhmikosR
Copy link
Contributor

XhmikosR commented Apr 4, 2020

Just rebase this branch and add any more info and we'll review it.

iUnknwn added 3 commits April 4, 2020 14:51
Adds a section to the DNS-Over-HTTPS guide with instructions
for how to ensure the cloudflared daemon is kept up-to-date.
Changed documentation to first list manual method to
update the cloudflared binary, and then have a subsequent
paragraph on automatic updating if desired by the user.

Also, while small, added a note that cloudflared will
work with other DoH providers (was useful when there was
a cloudflare outage in my region).
Forgot to add sudo in front of the cloudflared/systemd command.
Fixed in this commit.
@iUnknwn
Copy link
Contributor Author

iUnknwn commented Apr 4, 2020

Done - just rebased on top of master.

CLOUDFLARED_OPTS=--port 5053 --upstream https://1.1.1.1/dns-query --upstream https://1.0.0.1/dns-query
```

**Note:** The `cloudflared` binary will work with other DoH providers (for example, you could use https://8.8.8.8/dns-query for Google DNS).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
**Note:** The `cloudflared` binary will work with other DoH providers (for example, you could use https://8.8.8.8/dns-query for Google DNS).
**Note:** The `cloudflared` binary will work with other DoH providers (for example, you could use `https://8.8.8.8/dns-query` for Google DNS).

sudo chown root:root /etc/cron.weekly/cloudflared-updater.sh
```

The system will now attempt to update the cloudflared binary automatically, once per week.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The system will now attempt to update the cloudflared binary automatically, once per week.
The system will now attempt to update the cloudflared binary automatically, once per week.

@iUnknwn
Copy link
Contributor Author

iUnknwn commented Apr 18, 2020

@XhmikosR done - changes made.

@iUnknwn
Copy link
Contributor Author

iUnknwn commented Apr 18, 2020

...Actually, I'm re-reading this and I notice we now have two updating cloudflared sections, should the section section be renamed "Keeping Cloudflared Up-To-Date" or something like that?

@XhmikosR
Copy link
Contributor

You need to merge your update changes with the current one.

@iUnknwn
Copy link
Contributor Author

iUnknwn commented Apr 20, 2020

OK - changes merged.

That said, I'm wondering if there might be a way to simply this guide considerably. The main reason to keep the manual configuration route is to give people an option to avoid running cloudflared as root, correct?

If so, wouldn't it make more sense to simply write an extension unit file (as described here). That would let people configure the binary to run as its own non-root user, and it would prevent future updates from overwriting the change.

If we did this, we'd be able to remove all the sections that refer to manual configuration, and I think it would make the guide considerably more readable (we'd no longer need two sub sections for each install, update, and delete step).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants