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

Fix default CNI paths #3697

Merged
merged 1 commit into from
Jan 10, 2022
Merged

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Jan 10, 2022

What type of PR is this?

/kind bug

What this PR does / why we need it:

We need to use the default from containers.conf and not hardcode them in
buildah. This fixes an issue with the cni network backend since it would
try to access /etc/cni/net.d/ even as rootless user. This regression was
introduced in commit f9cff07.

Also hide the cni flags as we do not expect users to change this. The
recommended way is to change them in containers.conf.

How to verify it

run buildah bud rootless

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?


@openshift-ci openshift-ci bot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 10, 2022
@Luap99
Copy link
Member Author

Luap99 commented Jan 10, 2022

@flouthoc This should fix the rootless issue with cni.

@rhatdan
Copy link
Member

rhatdan commented Jan 10, 2022

LGTM
Add [NO NEW TESTS NEEDED] to move forward on this.

Or better yet add a test

buildah build --help | grep -- --cni-config-dir
buildah build --help | grep -- --cni-config-plugin-path
Make sure it does not have Default.

Should we hide these options, are these really options we expect users to play with?

@Luap99
Copy link
Member Author

Luap99 commented Jan 10, 2022

I think it would make sense to remove these options. The fields from the bud/run options structs should also be removed IMO but this would be a breaking change. I am not sure if these options are used anywhere (I hope not).

@rhatdan
Copy link
Member

rhatdan commented Jan 10, 2022

Just hide the options and remove the documentation from the man pages, should be sufficient.

@flouthoc
Copy link
Collaborator

@Luap99 Thanks as @rhatdan suggested NO NEW TESTS NEEDED is needed.

We need to use the default from containers.conf and not hardcode them in
buildah. This fixes an issue with the cni network backend since it would
try to access /etc/cni/net.d/ even as rootless user. This regression was
introduced in commit f9cff07.

Also hide the cni flags as we do not expect users to change this. The
recommended way is to change them in containers.conf.

[NO NEW TESTS NEEDED]

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Jan 10, 2022

/approve
/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 10, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, rhatdan

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-merge-robot openshift-merge-robot merged commit da67f97 into containers:main Jan 10, 2022
@Luap99 Luap99 deleted the fix-cni branch January 10, 2022 16:51
flouthoc added a commit to flouthoc/podman that referenced this pull request Jan 12, 2022
Following PR allows vendoring in latest buildah. Since some of the
essential constants were removed from buildah hence they cannot be
used any more. Use `c/common` instead.

Constants were removed in following PR: containers/buildah#3697

[NO NEW TESTS NEEDED]

Signed-off-by: Aditya Rajan <arajan@redhat.com>
flouthoc added a commit to flouthoc/podman that referenced this pull request Jan 12, 2022
Following PR allows vendoring in latest buildah. Since some of the
essential constants were removed from buildah hence they cannot be
used any more. Use `c/common` instead.

Constants were removed in following PR: containers/buildah#3697

[NO NEW TESTS NEEDED]
[NO TESTS NEEDED]

Signed-off-by: Aditya Rajan <arajan@redhat.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved kind/bug Categorizes issue or PR as related to a bug. lgtm locked - please file new issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants