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

Update vendor of containers/common #5367

Merged
merged 2 commits into from
Mar 1, 2024

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented Feb 29, 2024

What type of PR is this?

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake
/kind other

What this PR does / why we need it:

How to verify it

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

@rhatdan rhatdan changed the title Update vendor of containers/(common,image,storage) Update vendor of containers/common Mar 1, 2024
@rhatdan
Copy link
Member Author

rhatdan commented Mar 1, 2024

@Luap99 are the errors about ipv6 being caused by pasta?

@Luap99
Copy link
Member

Luap99 commented Mar 1, 2024

Ipv6 is not an error, the problem is Couldn't open network namespace /proc/262717/ns/net: Permission denied which is caused by selinux. That is the issue I was talking to you about buildah not being container_runtime_exec_t.

If you add chcon -t container_runtime_exec_t in the Makefile after we build the binary I would assume it works.

@rhatdan
Copy link
Member Author

rhatdan commented Mar 1, 2024

Strange this would not happen before. Will update the Makefile to see if this fixes it.

@Luap99
Copy link
Member

Luap99 commented Mar 1, 2024

We never had running pasta tests on this repo enabled, ops. I actually added tests for but it required a new pasta version that was not in CI at that time so it got skipped.

Given nobody reported this in now over 6 months that seems to indicate nobody used buildah with pasta on a selinux enabled system. Or at least nobody reported it. I guess the vast majority just uses the default which is understandable.

Makefile Outdated
@@ -161,6 +161,7 @@ install.cni.sudo: gopath
.PHONY: install
install:
install -d -m 755 $(DESTDIR)/$(BINDIR)
test -z "${SELINUXOPT}" || chcon --verbose --reference=$(DESTDIR)$(BINDIR)/podman bin/buildah
Copy link
Member

Choose a reason for hiding this comment

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

SELINUXOPT is not defined in the buildah makefile so you also have to copy over
SELINUXOPT ?= $(shell test -x /usr/sbin/selinuxenabled && selinuxenabled && echo -Z)

@rhatdan
Copy link
Member Author

rhatdan commented Mar 1, 2024

No, the problem again is transition rules differing from testing and actual use.

In ordinary use people run buildah directly, which means they run as unconfined_t, which is able do to what it wants. When run in tests, it is being launched as init_t which is confined. So Buildah + Pasta run within a systemd unit file would be broken by default.

@Luap99
Copy link
Member

Luap99 commented Mar 1, 2024

In ordinary use people run buildah directly, which means they run as unconfined_t, which is able do to what it wants. When run in tests, it is being launched as init_t which is confined. So Buildah + Pasta run within a systemd unit file would be broken by default.

Well it is broken when you run from your shell as unconfined_t, the issue is that if it is started from unconfined pasta then transitions to pasta_t and pasta_t policy seems to broken I guess? Again I need to check with pasta maintainers to see what is up with that.
So we use container_runtime_t to prevent pasta policy from taking effect as pasta then runs under container_runtime_t if started there.

Makefile Outdated
@@ -162,6 +164,7 @@ install.cni.sudo: gopath
install:
install -d -m 755 $(DESTDIR)/$(BINDIR)
install -m 755 bin/buildah $(DESTDIR)/$(BINDIR)/buildah
test -z "${SELINUXOPT}" || chcon --verbose -t $(SELINUXTYPE) $(DESTDIR)/$(BINDIR)/buildah
Copy link
Member

Choose a reason for hiding this comment

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

you are only labelling the /usr/bin/buildah binary but the test run ./bin/buildah
The chcon should also be made on bin/buildah

rhatdan and others added 2 commits March 1, 2024 09:45
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
[NO NEW TESTS NEEDED]

Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
@Luap99
Copy link
Member

Luap99 commented Mar 1, 2024

Oh wow, it actually passed, lots of registry flakes though.

Also I find it rather concerning that a rootless buildah integration tests take over 80+ minutes. But seems like that is already the case on main so not something pasta would have caused.

Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

restarted the flakes

LGTM, now it is time to eat some real pasta for me :)

/lgtm
/hold

Copy link
Contributor

openshift-ci bot commented Mar 1, 2024

[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

@rhatdan
Copy link
Member Author

rhatdan commented Mar 1, 2024

/unhold

@openshift-merge-bot openshift-merge-bot bot merged commit 0e0676d into containers:main Mar 1, 2024
36 checks passed
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 31, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants