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

coredns: response message always set recursion available (RA) when RD is set in request #206

Merged
merged 1 commit into from
Sep 5, 2022
Merged

coredns: response message always set recursion available (RA) when RD is set in request #206

merged 1 commit into from
Sep 5, 2022

Conversation

flouthoc
Copy link
Collaborator

Set recursion available in all response message by setting RA flag in
response packet.

Closes: #204

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 26, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

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

@flouthoc
Copy link
Collaborator Author

/hold need to verify if we should check RD (Recursion Desired) flag in the request packet and not always set RA.

@flouthoc flouthoc mentioned this pull request Aug 26, 2022
Closed
@flouthoc flouthoc changed the title coredns: response message always set recursion available (RA) coredns: response message always set recursion available (RA) Aug 26, 2022
@flouthoc flouthoc changed the title coredns: response message always set recursion available (RA) coredns: response message always set recursion available (RA) when RD is set in request Aug 29, 2022
@flouthoc
Copy link
Collaborator Author

PR should be good but lets wait for #208

@flouthoc
Copy link
Collaborator Author

PR is good for review and merge. @mheon @baude PTAL

mheon
mheon reviewed Aug 30, 2022
View reviewed changes
test/100-basic-name-resolution.bats Outdated
@@ -49,10 +53,14 @@ load helpers
a1_pid=$CONTAINER_NS_PID
run_in_container_netns "$a1_pid" "dig" "+short" "aone" "@$gw"
assert "$ip_a1"
# Set recursion bit
assert "$output" !~ "WARNING: recursion requested but not available"
Copy link
Member

Choose a reason for hiding this comment

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

Nit: spacing on these asserts is weird - doesn't match the rest of the file

Copy link
Collaborator Author

@flouthoc flouthoc Aug 31, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

My bad, i used spaces instead of tabs and added extra space. Fixed it now.

@mheon
Copy link
Member

mheon commented Aug 30, 2022

One nit otherwise LGTM

@flouthoc flouthoc requested a review from mheon August 31, 2022 09:12
@mheon
Copy link
Member

mheon commented Aug 31, 2022

CI seems broken, but it doesn't look like your PR; could be a more general thing?

@flouthoc
Copy link
Collaborator Author

@cevich Looks like the new VM is missing configured rust stable, could it happen cause we update the VM.

@cevich
Copy link
Member

cevich commented Aug 31, 2022

could it happen cause we update the VM.

Most probably. Here's what happes at image-build time:

https://github.com/containers/automation_images/blob/main/cache_images/fedora-netavark_packaging.sh#L72-L76

So if I understand correctly, we need to add 'default' to the end of that? Better yet, since y'all probably know better than I, just open up a PR in that repo. to fix it. It will build new images in about an hour that you can then test in this PR.

@flouthoc
coredns: response message set recursion available if RD is true
8e02e2e 
Set recursion available message by setting `RA` flag in
response packet if `RD` in request is set.

Closes: containers#204

Signed-off-by: Aditya R <arajan@redhat.com>
@flouthoc
Copy link
Collaborator Author

flouthoc commented Sep 3, 2022

This is good to merge now.

@baude
Copy link
Member

baude commented Sep 4, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Sep 4, 2022
@flouthoc
Copy link
Collaborator Author

flouthoc commented Sep 5, 2022

/hold cancel

@openshift-merge-robot openshift-merge-robot merged commit 2fb9fb7 into containers:main Sep 5, 2022
@flouthoc
Copy link
Collaborator Author

flouthoc commented Sep 8, 2022

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2124416

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mheon mheon Awaiting requested review from mheon

Assignees

@baude baude

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Recursion Available bit is not set in response header
5 participants
@flouthoc @mheon @cevich @baude @openshift-merge-robot