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

Feature: Fix miekgdns resolver LookupSRV function to work with CNAME records too #5716

Merged
merged 7 commits into from Oct 31, 2022
Merged

Feature: Fix miekgdns resolver LookupSRV function to work with CNAME records too #5716

merged 7 commits into from Oct 31, 2022

Conversation

Atharva-Shinde
Copy link
Contributor

Signed-off-by: Atharva Shinde atharvashinde179@gmail.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Fixes: #5679
Added case support to resolve intermediate CNAME records for LookupSRV function

Verification

@Atharva-Shinde
feat: cname handling in lookupsrv
9ec0483 
Signed-off-by: Atharva Shinde <atharvashinde179@gmail.com>
@Atharva-Shinde
Copy link
Contributor Author

cc: @matej-g @bwplotka

@Atharva-Shinde Atharva-Shinde marked this pull request as ready for review September 23, 2022 11:35
matej-g
matej-g reviewed Sep 23, 2022
View reviewed changes
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

Thanks for this @Atharva-Shinde! Looking pretty good, I have only few suggestions.

Also if you could add a CHANGELOG entry please.

pkg/discovery/dns/miekgdns/resolver.go Show resolved Hide resolved
pkg/discovery/dns/miekgdns/resolver.go Outdated Show resolved Hide resolved
@Atharva-Shinde
Copy link
Contributor Author

Atharva-Shinde commented Sep 23, 2022 •
edited

hey @matej-g am I supposed to create a CHANGELOG entry under Unreleased Added Unreleased Changed sub-type?

@matej-g
Copy link
Collaborator

matej-g commented Sep 26, 2022

hey @matej-g am I supposed to create a CHANGELOG entry under Unreleased Added Unreleased Changed sub-type?

Yes please 👍

@Atharva-Shinde
Merge branch 'main' into feature-cname-dnssrv-resolution
4ffcb19
@Atharva-Shinde
deleted: redundant var
994622b 
Signed-off-by: Atharva Shinde <atharvashinde179@gmail.com>
@Atharva-Shinde
Merge branch 'thanos-io:main' into feature-cname-dnssrv-resolution
3772d9f
@Atharva-Shinde
Update CHANGELOG.md
56d7de7 
Signed-off-by: Atharva Shinde <atharvashinde179@gmail.com>
@Atharva-Shinde
Update CHANGELOG.md
9c96e18 
Signed-off-by: Atharva Shinde <atharvashinde179@gmail.com>
matej-g
matej-g previously approved these changes Sep 28, 2022
View reviewed changes
Copy link
Collaborator

@matej-g matej-g left a comment

Choose a reason for hiding this comment

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

Thank you @Atharva-Shinde 🙇

@stale
Copy link

stale bot commented Oct 30, 2022

Hello 👋 Looks like there was no activity on this amazing PR for the last 30 days.
Do you mind updating us on the status? Is there anything we can help with? If you plan to still work on it, just comment on this PR or push a commit. Thanks! 🤗
If there will be no activity in the next week, this issue will be closed (we can always reopen a PR if you get back to this!). Alternatively, use remind command if you wish to be reminded at some point in future.

@stale stale bot added the stale label Oct 30, 2022
@matej-g matej-g removed the stale label Oct 31, 2022
@matej-g
Merge branch 'main' into feature-cname-dnssrv-resolution
d199875 
Signed-off-by: Matej Gera <38492574+matej-g@users.noreply.github.com>
@matej-g
Copy link
Collaborator

matej-g commented Oct 31, 2022

Looks like there are no more comments or objections since this has been opened, merging

@matej-g matej-g merged commit 7968915 into thanos-io:main Oct 31, 2022
bwplotka
bwplotka approved these changes Oct 31, 2022
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

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

Thanks! Do you mind adding unit test for this? Otherwise LGTM

@Atharva-Shinde
Copy link
Contributor Author

sure @bwplotka, I'll open a PR soon :)

@Atharva-Shinde Atharva-Shinde deleted the feature-cname-dnssrv-resolution branch November 1, 2022 07:14
@douglascamata douglascamata mentioned this pull request Jan 5, 2023
Closed
ngraham20 pushed a commit to ngraham20/thanos that referenced this pull request May 18, 2023
@Atharva-Shinde @matej-g
Feature: Fix miekgdns resolver LookupSRV function to work with CNAME …
9d6abc8 
…records too (thanos-io#5716)

* feat: cname handling in lookupsrv

Signed-off-by: Atharva Shinde <atharvashinde179@gmail.com>

* deleted: redundant var

Signed-off-by: Atharva Shinde <atharvashinde179@gmail.com>

* Update CHANGELOG.md

Signed-off-by: Atharva Shinde <atharvashinde179@gmail.com>

* Update CHANGELOG.md

Signed-off-by: Atharva Shinde <atharvashinde179@gmail.com>

Signed-off-by: Atharva Shinde <atharvashinde179@gmail.com>
Signed-off-by: Matej Gera <38492574+matej-g@users.noreply.github.com>
Co-authored-by: Matej Gera <38492574+matej-g@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwplotka bwplotka bwplotka approved these changes

@matej-g matej-g matej-g approved these changes

Assignees
No one assigned
Labels
size/S
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

query: Error with "dnssrv+" service discovery with intermediate CNAME
3 participants
@Atharva-Shinde @matej-g @bwplotka