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

enable loopback adapter in hermetic unshare namespace #1469

Merged
merged 1 commit into from
Oct 23, 2024
Merged

enable loopback adapter in hermetic unshare namespace #1469

merged 1 commit into from
Oct 23, 2024

Conversation

brianwcook
Copy link
Contributor

@brianwcook brianwcook commented Sep 25, 2024 •
edited
Loading

Bazel uses a client server architecture to execute even when performing network isolated builds.It works fine as long as there is any adapter, even a loopback addapter. The default unshare env has a loopback device [lo] but it is DOWN by default. This PR brings lo UP in the unshare environment so that hermetic Bazel builds will work.

It is dependent on the 'ip' command being present in the buildah image so another PR is open for that:
konflux-ci/buildah-container#90

@chmeliik
Copy link
Contributor

Have you verified that this does not mess with the network isolation?

@brianwcook
Copy link
Contributor Author

Have you verified that this does not mess with the network isolation?

I did. I added debugging statements to ensure that during my testing only the 'lo' adapter was present (to ensure both commands were running in the unshare env) and I also included things in the Dockerfile which should fail in hermetic mode, and they did.

@brianwcook
Copy link
Contributor Author

I assume e2e tests are failing because the newest buildah-task image still doesn't container the 'ip' binary. Added hold label because I need to manually test even after that is fixed.

tkdchen
tkdchen reviewed Sep 30, 2024
View reviewed changes
Copy link
Contributor

@tkdchen tkdchen left a comment

Choose a reason for hiding this comment

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

It should be good to make this optional, not all builds require the lo adapter enabled in a hermetic build.

@brianwcook
Copy link
Contributor Author

it doesn't hurt anything to enable lo adapter and it still doesn't permit any actual network connectivity, so why complicate things by making it optional?

chmeliik
chmeliik previously approved these changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@chmeliik chmeliik left a comment

Choose a reason for hiding this comment

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

LGTM, would still appreciate a comment in the code about why it's needed

@chmeliik
Copy link
Contributor

chmeliik commented Oct 8, 2024

Btw, why the hold label?

@brianwcook brianwcook removed the hold label Oct 11, 2024
@brianwcook
Copy link
Contributor Author

/retest

@brianwcook
Copy link
Contributor Author

I had added the hold label to make sure that the buildah-task landed and this was retested with it.

@brianwcook
Copy link
Contributor Author

I cannot see why the e2e tests are failing - I don't seem to have rights to view the test logs.

@chmeliik
Copy link
Contributor

I cannot see why the e2e tests are failing - I don't seem to have rights to view the test logs.

Pretty much the only way to find the logs after the pod has been garbage collected is

kubectl tekton -n konflux-ci logs pipelinerun build-definitions-pull-request-hrhv4

Relevant part of the log:

[e2e-tests : e2e-test] [build-service-suite Build templates E2E test] HACBS pipelines should eventually finish successfully for component with Git source URL https://githu
b.com/konflux-qe-bd/retrodep and Pipeline docker-build [build, build-templates, HACBS, pipeline, build-templates-e2e, source-build-e2e]
[e2e-tests : e2e-test] /konflux-e2e/tests/build/build_templates.go:334
...
[e2e-tests : e2e-test]   Adding the entitlement to the build
[e2e-tests : e2e-test]   sh: line 1: ip: command not found
[e2e-tests : e2e-test] 
[e2e-tests : e2e-test]   pod: test-comp-rheb-on-pull-request-cm2l9-build-container-pod | container step-sbom-syft-generate: 
[e2e-tests : e2e-test]   2024/10/12 01:46:20 Skipping step because a previous step failed

@chmeliik
Copy link
Contributor

#1478 needs to go first

@chmeliik chmeliik self-requested a review October 15, 2024 08:16
@chmeliik chmeliik dismissed their stale review October 15, 2024 08:16

can't be merged yet

@brianwcook
Copy link
Contributor Author

/retest

@brianwcook
Copy link
Contributor Author

/retest

@chmeliik
Copy link
Contributor

Now it's this:

[e2e-tests : e2e-test] [build-service-suite Build templates E2E test] HACBS pipelines should eventually finish successfully for component with Git source URL https://github.com/konflux-qe-bd/devfile-sample-python-basic and Pipeline docker-build-oci-ta [build, build-templates, HACBS, pipeline, build-templates-e2e, source-build-e2e]
[e2e-tests : e2e-test] /konflux-e2e/tests/build/build_templates.go:334
...
[e2e-tests : e2e-test]   pod: test-comp-wyso-on-pull-request-jxjlg-build-container-pod | container step-build: 
[e2e-tests : e2e-test]   Adding the entitlement to the build
[e2e-tests : e2e-test]   RTNETLINK answers: Operation not permitted

@brianwcook
Copy link
Contributor Author

/retest

1 similar comment
@ralphbean
Copy link
Member

/retest

@ralphbean
Copy link
Member

It's failing in CI because it's breaking the non-hermetic scenarios. Notice the difference of --net:

sh-5.2# unshare -Uf --net --keep-caps -r --map-users 1,1,65536 --map-groups 1,1,65536 -w . -- sh -c "ip link set lo up && echo wat"
wat                                                                                                                                

sh-5.2# unshare -Uf  --keep-caps -r --map-users 1,1,65536 --map-groups 1,1,65536 -w . -- sh -c "ip link set lo up && echo wat"     
RTNETLINK answers: Operation not permitted

@brianwcook
Copy link
Contributor Author

I reproduced that error locally and fixed it. I can run test pipelines in both hermetic and non-hermetic mode, but the e2e tests are still failing.

@brianwcook
Copy link
Contributor Author

/retest

arewm
arewm previously requested changes Oct 22, 2024
View reviewed changes
task/buildah-oci-ta/0.2/buildah-oci-ta.yaml Outdated Show resolved Hide resolved
@brianwcook brianwcook dismissed arewm’s stale review October 23, 2024 02:44

change was made

@brianwcook
enable loopback adapter in hermetic unshare namespace
e5114a9 
Bazel uses a client server architecture to execute even when
performing network isolated builds.It works fine as long as there
is any adapter, even a loopback addapter. The default unshare env
has a loopback device [lo] but it is DOWN by default. This PR
brings lo UP in the unshare environment so that hermetic Bazel
builds will work.
@brianwcook brianwcook added this pull request to the merge queue Oct 23, 2024
Merged via the queue into konflux-ci:main with commit 9816995 Oct 23, 2024
14 checks passed
@brianwcook brianwcook deleted the bazel branch October 23, 2024 12:44
@rh-tap-build-team rh-tap-build-team bot mentioned this pull request Oct 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MartinBasti MartinBasti MartinBasti left review comments

@arewm arewm arewm left review comments

@tkdchen tkdchen tkdchen left review comments

@chmeliik chmeliik chmeliik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@brianwcook @chmeliik @ralphbean @tkdchen @arewm @MartinBasti @openshift-merge-robot