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 additional listen-address to CRC dnsmasq service #1761

Conversation

lewisdenny
Copy link
Collaborator

Due to this change [1] introduced in CRC 2.32.0[2] the dnsmasq service is now running as a systemd service rather than in a container.

The behavior of the dnsmasq listen-address has now changed, the CRC dnsmasq only listens on the default address provided by Zuul. The Ansible controller is configured to query the interface we control on CRC, ci-private-network.

This breaks domain name resolution as dns queries are blocked.

This patch:

  • Updates the CRC dnsmasq listen-address configuration to include the ci-private-network address allowing the Ansible controller to query it.

  • Removes the hard coded ci-private-network address from the Ansible controllers default connection dns configuration to ensure they always match.

[1] crc-org/crc@5f49891
[2] https://github.com/crc-org/crc/releases/tag/v2.32.0

As a pull request owner and reviewers, I checked that:

  • Appropriate testing is done and actually running

This comment was marked as outdated.

@lewisdenny lewisdenny force-pushed the configure_listen-address branch 3 times, most recently from 5765fda to 9521796 Compare May 27, 2024 04:07

This comment was marked as outdated.

@lewisdenny lewisdenny force-pushed the configure_listen-address branch 3 times, most recently from ce1b86c to 2af85ca Compare May 27, 2024 05:26
become: true
vars:
_original_listen_address: "listen-address={{ hostvars['crc'].ansible_host }}"
ansible.builtin.replace:
Copy link
Contributor

Choose a reason for hiding this comment

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

iirc, we could just use ansible.builtin.lineinfile and add the new listen-address - dnsmasq should listen on both. Might be even safer?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So in the end have two listen-address= lines defined? I can quickly test that

Copy link
Contributor

Choose a reason for hiding this comment

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

that's what we did for the reproducer a while ago, and it seems to work

- name: Ensure dnsmasq listens on correct interfaces

Copy link
Contributor

Choose a reason for hiding this comment

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

yup, imho that's probably better and safer than editing the default interface.

Copy link
Collaborator Author

@lewisdenny lewisdenny May 27, 2024

Choose a reason for hiding this comment

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

Sorry guys, could you expand on this?

The link from @cescgina seems to match what I'm doing here. My patch as an example searches for listen-address=192.168.25.178 and replaces it with listen-address=192.168.25.178,192.168.122.10

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for the clarification on Slack, I confirmed lineinfile works so if your happy to go that direction I'll update this patch

This comment was marked as outdated.

@lewisdenny lewisdenny marked this pull request as ready for review May 27, 2024 10:59
@lewisdenny

This comment was marked as outdated.

This comment was marked as outdated.

@lewisdenny lewisdenny force-pushed the configure_listen-address branch from 2af85ca to f398f62 Compare May 27, 2024 13:32

This comment was marked as outdated.

@lewisdenny lewisdenny force-pushed the configure_listen-address branch from f398f62 to f57840d Compare May 27, 2024 14:30
@lewisdenny lewisdenny force-pushed the configure_listen-address branch from f57840d to 812f51a Compare May 30, 2024 02:51
@lewisdenny lewisdenny force-pushed the configure_listen-address branch from 812f51a to f723622 Compare May 30, 2024 03:43
@lewisdenny
Copy link
Collaborator Author

lewisdenny commented May 30, 2024

Testing here: #1730

Proof adding two lines for listen-address in config for works:

[core@crc ~]$ sudo ss -tpnl | grep dnsmasq
LISTEN 0      32     192.168.122.10:53         0.0.0.0:*    users:(("dnsmasq",pid=32452,fd=7))
LISTEN 0      32      192.168.26.86:53         0.0.0.0:*    users:(("dnsmasq",pid=32452,fd=5))

[core@crc ~]$ grep listen /etc/dnsmasq.d/crc-dnsmasq.conf
listen-address=192.168.26.86
listen-address=192.168.122.10

Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/0662376f0e0440d1a0ea07cb76522cfb

✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 32m 45s
podified-multinode-edpm-deployment-crc RETRY_LIMIT in 24m 41s
✔️ cifmw-crc-podified-edpm-baremetal SUCCESS in 1h 15m 40s
✔️ noop SUCCESS in 0s
✔️ cifmw-pod-ansible-test SUCCESS in 8m 25s
✔️ cifmw-pod-pre-commit SUCCESS in 7m 27s

Due to this change [1] introduced in CRC 2.32.0[2] the dnsmasq service is now running as a systemd service rather than in a container.

The behavior of the dnsmasq listen-address has now changed, the CRC dnsmasq only listens on the default address provided by Zuul. The Ansible controller is configured to query the interface we control on CRC, `ci-private-network`.

This breaks domain name resolution as dns queries are blocked.

This patch:
- Adds additional dnsmasq listen-address line to CRC node include the `ci-private-network` address allowing the Ansible controller to query it.

- Removes the hard coded `ci-private-network` address from the Ansible controllers default connection dns configuration.

[1] crc-org/crc@5f49891
[2] https://github.com/crc-org/crc/releases/tag/v2.32.0
@lewisdenny lewisdenny force-pushed the configure_listen-address branch from f723622 to c3e3517 Compare May 30, 2024 10:26
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://review.rdoproject.org/zuul/buildset/0be4b3200d194facaf52bd73e01b61bf

✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 42m 24s
✔️ podified-multinode-edpm-deployment-crc SUCCESS in 1h 23m 15s
✔️ cifmw-crc-podified-edpm-baremetal SUCCESS in 1h 22m 03s
✔️ noop SUCCESS in 0s
cifmw-pod-ansible-test FAILURE in 5m 55s
✔️ cifmw-pod-pre-commit SUCCESS in 7m 52s

@rlandy
Copy link
Collaborator

rlandy commented May 30, 2024

recheck

@lewisdenny
Copy link
Collaborator Author

Testing passed here and #1730 meaning it fixes 4.15 and doesn't break 4.14, we should be fine to merge now :)

@cescgina
Copy link
Contributor

Testing here: #1730

Proof adding two lines for listen-address in config for works:

[core@crc ~]$ sudo ss -tpnl | grep dnsmasq
LISTEN 0      32     192.168.122.10:53         0.0.0.0:*    users:(("dnsmasq",pid=32452,fd=7))
LISTEN 0      32      192.168.26.86:53         0.0.0.0:*    users:(("dnsmasq",pid=32452,fd=5))

[core@crc ~]$ grep listen /etc/dnsmasq.d/crc-dnsmasq.conf
listen-address=192.168.26.86
listen-address=192.168.122.10

@lewisdenny I thought crc by default configures dnsmasq to listen only to localhost, do we set the 192.168.26.86 ip ?

@lewisdenny
Copy link
Collaborator Author

Testing here: #1730
Proof adding two lines for listen-address in config for works:

[core@crc ~]$ sudo ss -tpnl | grep dnsmasq
LISTEN 0      32     192.168.122.10:53         0.0.0.0:*    users:(("dnsmasq",pid=32452,fd=7))
LISTEN 0      32      192.168.26.86:53         0.0.0.0:*    users:(("dnsmasq",pid=32452,fd=5))

[core@crc ~]$ grep listen /etc/dnsmasq.d/crc-dnsmasq.conf
listen-address=192.168.26.86
listen-address=192.168.122.10

@lewisdenny I thought crc by default configures dnsmasq to listen only to localhost, do we set the 192.168.26.86 ip ?

I'm not sure mate, before we add 192.168.122.10, 192.168.26.86 is already there in the config. I'm not sure where else in the pipeline we are setting the listen-address.

@lewisdenny
Copy link
Collaborator Author

lewisdenny commented Jun 2, 2024

Testing here: #1730
Proof adding two lines for listen-address in config for works:

[core@crc ~]$ sudo ss -tpnl | grep dnsmasq
LISTEN 0      32     192.168.122.10:53         0.0.0.0:*    users:(("dnsmasq",pid=32452,fd=7))
LISTEN 0      32      192.168.26.86:53         0.0.0.0:*    users:(("dnsmasq",pid=32452,fd=5))

[core@crc ~]$ grep listen /etc/dnsmasq.d/crc-dnsmasq.conf
listen-address=192.168.26.86
listen-address=192.168.122.10

@lewisdenny I thought crc by default configures dnsmasq to listen only to localhost, do we set the 192.168.26.86 ip ?

I'm not sure mate, before we add 192.168.122.10, 192.168.26.86 is already there in the config. I'm not sure where else in the pipeline we are setting the listen-address.

Yeah so CRC sets the listen-address to the IP of the VM [1] or in our case "instance" of crc. We need to access it on the internal network we create as part of the network bootstrap which is why we need this change.

So [1] is passed through a bunch of functions and ends up being .IP here in the template[2], you can trace back from [2] to [1].

I think your thought about CRC listening on localhost comes from the reproducer scenario, I guess there we start crc differently and it picks up 127.0.0.1 as it's IP? :)

[1] https://github.com/crc-org/crc/blob/813c80a473046247eb232bb7e9e3da79a954118b/pkg/crc/machine/start.go#L399C1-L403C65
[2] https://github.com/crc-org/crc/blob/813c80a473046247eb232bb7e9e3da79a954118b/pkg/crc/services/dns/template.go#L11

@cescgina
Copy link
Contributor

cescgina commented Jun 3, 2024

Testing here: #1730
Proof adding two lines for listen-address in config for works:

[core@crc ~]$ sudo ss -tpnl | grep dnsmasq
LISTEN 0      32     192.168.122.10:53         0.0.0.0:*    users:(("dnsmasq",pid=32452,fd=7))
LISTEN 0      32      192.168.26.86:53         0.0.0.0:*    users:(("dnsmasq",pid=32452,fd=5))

[core@crc ~]$ grep listen /etc/dnsmasq.d/crc-dnsmasq.conf
listen-address=192.168.26.86
listen-address=192.168.122.10

@lewisdenny I thought crc by default configures dnsmasq to listen only to localhost, do we set the 192.168.26.86 ip ?

I'm not sure mate, before we add 192.168.122.10, 192.168.26.86 is already there in the config. I'm not sure where else in the pipeline we are setting the listen-address.

Yeah so CRC sets the listen-address to the IP of the VM [1] or in our case "instance" of crc. We need to access it on the internal network we create as part of the network bootstrap which is why we need this change.

So [1] is passed through a bunch of functions and ends up being .IP here in the template[2], you can trace back from [2] to [1].

I think your thought about CRC listening on localhost comes from the reproducer scenario, I guess there we start crc differently and it picks up 127.0.0.1 as it's IP? :)

[1] https://github.com/crc-org/crc/blob/813c80a473046247eb232bb7e9e3da79a954118b/pkg/crc/machine/start.go#L399C1-L403C65 [2] https://github.com/crc-org/crc/blob/813c80a473046247eb232bb7e9e3da79a954118b/pkg/crc/services/dns/template.go#L11

yes, you're right, I was thinking about the reproducer, where I looked at the stuff, thanks for digging!

@raukadah
Copy link
Contributor

raukadah commented Jun 3, 2024

/approve

Copy link
Contributor

openshift-ci bot commented Jun 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: raukadah

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

@openshift-ci openshift-ci bot added the approved label Jun 3, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 6a7e9fb into openstack-k8s-operators:main Jun 3, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants