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

Pi-Hole Local DNS Support #2321

Closed
wants to merge 18 commits into from

Conversation

tinyzimmer
Copy link

@tinyzimmer tinyzimmer commented Sep 24, 2021

Description

Marking this WIP primarily until I get some feedback on design decisions I likely made poorly 😄, but also it needs tests and documentation.

Still need tests, but this is mostly ready.

This adds support for synchronizing DNS records with a Pi-Hole Local DNS instance. I've only put it through limited testing so far locally, so if there are others who want to try it out too to help catch edge cases that would be super helpful, but probably not an immediate priority.

I'm happy to help maintain this provider, but I can't make a guarantee on

It would be nice if the maintainers run the provider in production

Mainly, because I think I'd rather go sky diving out of a low-flying Cessna before attempting to actually do that. BUT! I am more than happy to continue to run this in my homelab if it continues to prove itself useful.

Addresses #1930

Checklist

  • Unit tests updated
  • End user documentation updated

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2021
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 24, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @tinyzimmer!

It looks like this is your first PR to kubernetes-sigs/external-dns 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/external-dns has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 24, 2021
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 26, 2021
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 27, 2021
@tinyzimmer tinyzimmer changed the title [WIP] Pi-Hole Local DNS Support Pi-Hole Local DNS Support Sep 27, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 27, 2021
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 28, 2021
@tinyzimmer
Copy link
Author

/assign @njuettner

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tinyzimmer
To complete the pull request process, please ask for approval from njuettner after the PR has been reviewed.

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

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tinyzimmer
Copy link
Author

I know this would never see production usage, but I do feel it serves as a good training piece for people running homelabs. I've been running it about a week in mine, and the combination of calico BGP with this makes for a pretty slick flat network across the house with automatic DNS resolution.

I gotta say, as ugly as the client code is, it works pretty well.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2022
@tinyzimmer
Copy link
Author

Any update on a potential review and/or merge?

@linuxmall
Copy link

When will this pr merge?

@pdmurray
Copy link

pdmurray commented Feb 2, 2022

Hey there @tinyzimmer - Thank you for this! I am testing it out in my homelab, working perfectly so far. It is very useful for me. I am using it along with metalLB. Looking forward to seeing it in an external-dns release.

@linuxmall
Copy link

@tinyzimmer and @pdmurray
How can i test?
I would like to run on my home

@tinyzimmer
Copy link
Author

@tinyzimmer and @pdmurray
How can i test?
I would like to run on my home

You'll need to clone and checkout the branch on my fork. Then you can use the Makefile to build a container image.

@pdmurray
Copy link

pdmurray commented Feb 3, 2022

@tinyzimmer and @pdmurray
How can i test?
I would like to run on my home

You'll need to clone and checkout the branch on my fork. Then you can use the Makefile to build a container image.

@linuxmall also check out the guide that @tinyzimmer wrote.

In my case I did exactly as @tinyzimmer suggested, I cloned his fork, built the Docker image and pushed it to my GCR registry and then followed his instructions. Just edit the K8S deployment template to point to your Docker image and you should be good to go!

@pdebuitlear
Copy link

Just wondering where this is at?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 21, 2022
@k8s-ci-robot
Copy link
Contributor

@tinyzimmer: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@adlnc
Copy link

adlnc commented Jun 12, 2022

Any chance this work (for nonproprietary, arm oriented solution) could be merged as it would greatly facilitate homelab & student environments setup ?

@rupertbenbrook
Copy link

rupertbenbrook commented Jul 1, 2022

I can confirm this is working perfectly on my homelab. I cloned tinyzimmer's fork, built an image locally using docker build . --build-arg ARCH=amd64 -t registry.local/kubernetes-sigs/external-dns:pihole-support, pushed this image to my homelab (registry.local) registry. Finally I deployed the currently released external-dns helm chart (1.9.0) using the deployment values:

image:
  repository: registry.local/kubernetes-sigs/external-dns
  tag: pihole-support
  pullPolicy: Always
policy: upsert-only
triggerLoopOnEvent: true
domainFilters:
- {{ .Values.urlSuffix }}
provider: pihole
registry: noop
env:
- name: EXTERNAL_DNS_PIHOLE_SERVER
  value: {{ .Values.pihole.url }}
- name: EXTERNAL_DNS_PIHOLE_PASSWORD
  value: {{ .Values.pihole.password }}
serviceMonitor:
  enabled: true

Hope this helps someone. :)

@davinkevin
Copy link

Please @tinyzimmer, can you provide a rebase for this? I'm dying to see it merged and release!

@tinyzimmer
Copy link
Author

@davinkevin It would be the second rebase I've done and at this point I'm kinda curious for an acknowledgment that it has merge potential first.

@davinkevin
Copy link

@davinkevin It would be the second rebase I've done and at this point I'm kinda curious for an acknowledgment that it has merge potential first.

Fair enough! So @njuettner and @seanmalloy are the only one able to validate this here. Again, this is a very awaited feature 😇

@anubisg1
Copy link
Contributor

anubisg1 commented Oct 7, 2022

@njuettner and @seanmalloy waiting for your final review please ! :)

@tinyzimmer you might need anothe rebase :(

@szuecs
Copy link
Contributor

szuecs commented Oct 8, 2022

Sorry that we are having that kind of backlog.
I already read it a while ago and I would merge it like it is in case rebase was done.

mekanics added a commit to mekanics/homelab that referenced this pull request Oct 8, 2022
@anubisg1
Copy link
Contributor

@tinyzimmer any chance you could rebase this? it would be finally merged!

@anubisg1
Copy link
Contributor

anubisg1 commented Oct 31, 2022

Sorry that we are having that kind of backlog. I already read it a while ago and I would merge it like it is in case rebase was done.

@szuecs

i went ahead and rebased this with #3125

if approved and merged, then this PR would be obsolete and could be closed

I made no changes, only rebased the few lines that needed to be moved around.

k8s-ci-robot added a commit that referenced this pull request Nov 10, 2022
Pi-Hole Local DNS Support - rebases #2321
@anubisg1
Copy link
Contributor

This was merged via #3125 and i think that this specific one can be closed

@szuecs
Copy link
Contributor

szuecs commented Nov 10, 2022

/close

@k8s-ci-robot
Copy link
Contributor

@szuecs: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.