Cloudflare's dns hard coded

Buzze · September 13, 2022, 6:40pm

I recently came across the fact that supervisor/tests/api/ contains a file called test_network.py which uses the 1.1.1.1 dns from Coudflare. The problem here is that why the Domain Name Server has to be provided by Cloudflare, why can’t it be the default from the router or the one that you assigned from your router like Pihole, AdGuard Home, etc. The other i found was in supervisor/tests/resolution/check/test_check_dns_server_failure.py which prioritized the Cloudflare domain as opposed to the main 192.168.30.1 . I could be wrong though, but using a DNS provider hard coded is not a good practice, TV and IoT manufacturers do these kind of shady practices to avoid your set of rules again by your router or your pihole install…What are your thoughts on this?

CentralCommand · September 13, 2022, 7:16pm

Those are tests. That code isn’t run in deployed supervisor instances, its only run in CI and on developer machines to confirm their code works. Hell that code doesn’t even exist in deployed supervisor instances, the only files copied from the supervisor repo into supervisor’s built images are here:

github.com

home-assistant/supervisor/blob/d195f19fa811c45b46c1e9d7e8dcf6c1d9179d00/Dockerfile#L38-L54

    
      
          COPY requirements.txt .
          RUN \
              export MAKEFLAGS="-j$(nproc)" \
              && pip3 install --no-cache-dir --no-index --only-binary=:all: --find-links \
                  "https://wheels.home-assistant.io/musllinux/" \
                  -r ./requirements.txt \
              && rm -f requirements.txt
          
          
# Install Home Assistant Supervisor
          COPY . supervisor
          RUN \
              pip3 install --no-cache-dir -e ./supervisor \
              && python3 -m compileall ./supervisor/supervisor
          
          

          
WORKDIR /
          COPY rootfs /

Notice tests is intentionally omitted. Test code has no place in a production image.

So yea, my thoughts are there is absolutely nothing wrong with this. Hard-coded values are a best practice in test code. And the code you’re calling out affects literally no one. Even if you run the tests it won’t actually reach out to 1.1.1.1, it just uses that as a mock value for an ip address to confirm the tests works.

Amusingly you actually missed the only one I do have an issue with and that’s this one:

github.com

home-assistant/supervisor/blob/d195f19fa811c45b46c1e9d7e8dcf6c1d9179d00/tests/utils/test_check_port.py#L7-L14

    
      
          def test_exists_open_port():

              """Test a exists network port."""

              assert check_port(ip_address("8.8.8.8"), 53)

          

          

          def test_not_exists_port():

              """Test a not exists network service."""

              assert not check_port(ip_address("192.0.2.1"), 53)

Again, to be clear, this is test code, no one runs this code other then supervisor developers. But I am a supervisor developer and so this one bugs me. Because test_exists_open_port always fails for me since it actually tries to reach out to 8.8.8.8 and I block that on my network