build: disable internet access during the build step

[esteve]

Description

This PR disables access to internet so that we make sure that packages do not download any artifacts during the build step.

Tests performed

Not applicable.

Effects on system behavior

Not applicable.

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

• I've confirmed the contribution guidelines.
• The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

• The PR follows the pull request guidelines.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

• There are no open discussions or they are tracked via tickets.

After all checkboxes are checked, anyone who has write access can merge the PR.

[esteve]

@mitsudome-r @xmfcx I don't know who should be assigned as a reviewer for this PR. The spell check test is failing because it's waiting for tier4/autoware-spell-check-dict#585

[HansRobo]

@esteve
sorry for late response.
The changes seem to be fine to me, but I am testing them just to be sure.
https://github.com/HansRobo/autoware.universe/actions/runs/6031849665

Please wait for a while

[HansRobo]

It seems that there are still packages in A that require external access during build.
ref: tensorrt_yolo tries to download a file via internet and its build fails

@esteve
How about extracting to the step of /etc/nsswitch.conf setting change and setting restoration workflow and adding an argument of colcon-build action that controls whether it can be executed or not, so as not to be a destructive change? (e.g. allow_internet_acess_during_building)

[esteve]

@HansRobo thanks for your review. I prefer not to add the option to disable this check, otherwise I worry that developers will bypass it and we won't notice (we don't have similar options for any of the other required checks). In any case, this PR is not meant to be merged as is right now, but once all the packages have been patched to not download artifacts by default (like tensorrt_yolo, for example), this PR can be merged and ensure that future packages don't access the network. The patch for tensorrt_yolo can be found at autowarefoundation/autoware.universe#3142, I'm tracking all the packages that need patching in autowarefoundation/autoware.universe#3137

I created a test PR (see autowarefoundation/autoware.universe#4375) to demonstrate how a PR that accesses the network would fail.

[HansRobo]

@esteve
I see. Thank you for providing the situation about this pull-request!

It's ok to merge this pull-request after finishing autowarefoundation/autoware.universe#3142 and autowarefoundation/autoware.universe#3137.
So, please don't mind my advice to add an option to allow internet connection.

Could you please let me know, when everything is complete?
Let's merge this pull-request then!

[esteve]

@HansRobo we have merged autowarefoundation/autoware#3375 and will merge autowarefoundation/autoware-documentation#345 soon. In the mean time between autowarefoundation/autoware#3375 was submitted and it was merged, autowarefoundation/autoware.universe#3143 has conflicts because new artifacts were added, so I propose we merge this PR as soon as possible to avoid new artifacts being added to packages.

@xmfcx @lexavtanke what do you think about merging this as soon as possible?

[xmfcx]

@HansRobo @mitsudome-r @kminoda is this repository being utilized by some internal TIER IV repositories?

Do any of those repositories download during the colcon build process?

If not I would like to merge this.

[esteve]

@xmfcx tensorrt_yolox currently downloads from the internet, but the list of artifacts has been updated since we first submitted autowarefoundation/autoware#3375, so I had to submit autowarefoundation/autoware#3848 to update the ansible scripts

[xmfcx]

Also I've asked ChatGPT for a solution to see if there are any other way of doing it and it recommended following:
https://chat.openai.com/share/10110835-5bbc-48b0-91b3-299c62290fcf

   - name: Disable Internet (DNS method)
     run: echo "nameserver 0.0.0.0" | sudo tee /etc/resolv.conf
     shell: bash

   - name: Build
     run: |
       colcon build --event-handlers console_cohesion+ \
         --packages-above-and-dependencies ${{ inputs.target-packages }} \
         --cmake-args -DCMAKE_BUILD_TYPE=${{ inputs.cmake-build-type }} \
         --mixin coverage-gcc coverage-pytest compile-commands
     shell: bash

   - name: Restore Internet (DNS method)
     if: always()
     run: |
       echo "nameserver 8.8.8.8" | sudo tee /etc/resolv.conf
     shell: bash

I didn't try it but separating the disable internet and restore internet into separate items might make it more readable. I'm not aware of further implications of this change, please just treat it as a suggestion.

[esteve]

@xmfcx I prefer using the nsswitch method because we make sure that even if /etc/resolv.conf gets updated (e.g. via dhcp), we won't be using it

[xmfcx]

@esteve I see. What do you think about separating "disable internet" and "restore internet" into new steps?

[esteve]

@xmfcx I don't think it's needed, because if colcon build fails, none of the other steps will run

[xmfcx]

I approve this to prevent additional changes being made which downloads artifacts during build process. Any repo will need to update their action version anyways to have these applied anyways.

[esteve]

@xmfcx thanks!