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 regex domain filters #1504

Merged
merged 8 commits into from
Apr 5, 2021
Merged

Conversation

offzale
Copy link
Contributor

@offzale offzale commented Apr 9, 2020

In some use cases, you might need a more flexible way of filtering than the current suffix filtering for zones and domains.

I have added a new filter, RegexDomainFilter, which filters domains and zones by using a regular expression, and has precedence over DomainFilter. Its negative counterpart, RegexDomainExclusion, has been added as well since golang regexp package has some limitations and might be necessary in some use cases.

If RegexDomainFilter is not set, external-dns will work as usual filtering by DomainFilter.

Signed-off-by: Enrique Gonzalez <goga.enrique@gmail.com>
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 9, 2020
@k8s-ci-robot k8s-ci-robot requested review from linki and Raffo April 9, 2020 14:50
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 9, 2020
@offzale
Copy link
Contributor Author

offzale commented Apr 9, 2020

/assign @njuettner

@offzale
Copy link
Contributor Author

offzale commented Apr 23, 2020

@njuettner any thoughts on this?

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 22, 2020
@dsexton
Copy link

dsexton commented Aug 3, 2020

We may have a usecase for this as well. @linki @njuettner is there anything preventing this from being reviewed?

@offzale
Copy link
Contributor Author

offzale commented Aug 7, 2020

I have tested it and have been using it in production since I made the PR, it is working as designed. Let me know guys if there is anything you are missing or you think it should be done differently @njuettner @linki @Raffo

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 7, 2020
@Raffo
Copy link
Contributor

Raffo commented Aug 8, 2020

@offzale the change looks legit. Can you add a note in the unreleased section of the changelog? https://github.com/kubernetes-sigs/external-dns/blob/master/CHANGELOG.md

@offzale
Copy link
Contributor Author

offzale commented Aug 10, 2020

@Raffo thanks for the quick review, I have just added the change you mentioned

@seanmalloy
Copy link
Member

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Aug 19, 2020
@dsexton
Copy link

dsexton commented Aug 25, 2020

@Raffo can we move forward with this change?

@seanmalloy
Copy link
Member

/assign

@seanmalloy
Copy link
Member

@offzale can you please fix the merge conflicts? I will review after the merge conflicts are fixed. Thanks!

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 16, 2020
Signed-off-by: Enrique Gonzalez <goga.enrique@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2020
Signed-off-by: Enrique Gonzalez <goga.enrique@gmail.com>
@offzale
Copy link
Contributor Author

offzale commented Nov 3, 2020

@seanmalloy thanks for offering to review the PR yourself. I have just fixed the merge conflicts

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 1, 2021
@offzale
Copy link
Contributor Author

offzale commented Feb 1, 2021

@seanmalloy did you have a chance to have a look at the changes?

Copy link
Contributor

@Raffo Raffo left a comment

Choose a reason for hiding this comment

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

Sorry for the late re-review, I took a look at this code and it looks like there are several things missing for it to be mergeable. Please take a look at my comments, I will give this a short turnaround if you will address them.

endpoint/domain_filter.go Outdated Show resolved Hide resolved
endpoint/domain_filter.go Outdated Show resolved Hide resolved
endpoint/domain_filter.go Outdated Show resolved Hide resolved
endpoint/domain_filter.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 10, 2021
@offzale
Copy link
Contributor Author

offzale commented Feb 10, 2021

Hi @Raffo, Thanks for the review! I have just committed new changes addressing your comments above. Let me know if you find anything missing.

@offzale offzale requested a review from Raffo February 10, 2021 14:28
@njuettner njuettner assigned Raffo and unassigned njuettner Feb 25, 2021
@Raffo
Copy link
Contributor

Raffo commented Mar 3, 2021

@offzale Sorry for the hassle, we removed CHANGELOG.md because it was creating more problems than anything, can you drop your change? I will re-review this after that.

Signed-off-by: Enrique Gonzalez <goga.enrique@gmail.com>
@offzale
Copy link
Contributor Author

offzale commented Mar 3, 2021

@Raffo thanks for the shout, I have just dropped the change so the MR has no conflicts

@offzale
Copy link
Contributor Author

offzale commented Mar 3, 2021

/remove-lifecycle stale

@k8s-ci-robot k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 3, 2021
@Raffo
Copy link
Contributor

Raffo commented Apr 5, 2021

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 5, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: offzale, Raffo

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

The pull request process is described 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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 5, 2021
@k8s-ci-robot k8s-ci-robot merged commit 7d6eeed into kubernetes-sigs:master Apr 5, 2021
@yogeek
Copy link

yogeek commented May 4, 2021

Hi @offzale ! Thanks for this feature, very useful 👍
Is there any documentation update related to this please ?

@rafalkasa
Copy link

Hi @offzale !

I have two kubernetes one test and other production and I would like use your regex filters to filter ingress domains based on it:

Test configuration

      containers:
      - name: external-dns
        image: k8s.gcr.io/external-dns/external-dns:v0.8.0
        args:
        - --source=service
        - --source=ingress
        - --domain-filter=example.com
        - --regex-domain-filter=^(.*?(-test.example.com\b)[^$]*)$
        - --provider=azure
        - --azure-resource-group=some-group

Prod configuration

      containers:
      - name: external-dns
        image: k8s.gcr.io/external-dns/external-dns:v0.8.0
        args:
        - --source=service
        - --source=ingress
        - --domain-filter=example.com
        - --regex-domain-exclusion=^(.*?(-test.example.com\b)[^$]*)$
        - --provider=azure
        - --azure-resource-group=some-group

Unfortunately the production external-dns configuration is automatically deleting what was registered by the test external-dns.
This look like that:
- --regex-domain-exclusion=^(.*?(-test.example.com\b)[^$]*)$ not working or working improperly

@rafalkasa
Copy link

rafalkasa commented Jul 22, 2021

Correct Prod configuration should be and now everything is working:

  containers:
  - name: external-dns
    image: k8s.gcr.io/external-dns/external-dns:v0.8.0
    args:
    - --source=service
    - --source=ingress
    - --domain-filter=example.com
    - --regex-domain-filter=^(.*?(.example.com\b)[^$]*)$
    - --regex-domain-exclusion=^(.*?(-test.example.com\b)[^$]*)$
    - --provider=azure
    - --azure-resource-group=some-group

@offzale thank you for your work is awesome :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", 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
Development

Successfully merging this pull request may close these issues.

None yet

9 participants