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

oci: fix the file mode of the device #5028

Merged
merged 1 commit into from Feb 22, 2021
Merged

Conversation

Iceber
Copy link
Member

@Iceber Iceber commented Feb 10, 2021

The file mode of the device in the OCI runtime specification does not contain file type bits

unix.Stat_t.Mode contains the file type and mode,details are in "The file type and mode" section of inode

This issue causes some config.json to fail the runtime spec schema validation

For example, the config.json for the kube-proxy container

devices:
    [
            {
                "path":"/dev/sda",
                "type":"b",
                "major":8,
                "minor":0,
                "fileMode":25008,
                "uid":0,
                "gid":6
            },
           {
                "path":"/dev/null",
                "type":"c",
                "major":1,
                "minor":3,
                "fileMode":8630,
                "uid":0,
                "gid":0
            },
            {
                "path":"/dev/tty0",
                "type":"c",
                "major":4,
                "minor":0,
                "fileMode":8592,
                "uid":0,
                "gid":5
            },
            ...
    ]

Discussions about FileMode: opencontainers/runtime-spec#1082

Signed-off-by: Iceber Gu <wei.cai-nat@daocloud.io>
@k8s-ci-robot
Copy link

Hi @Iceber. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 10, 2021

Build succeeded.

@Iceber Iceber closed this Feb 10, 2021
@Iceber Iceber reopened this Feb 10, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 10, 2021

Build succeeded.

@thaJeztah
Copy link
Member

@AkihiroSuda does libcontainer/devices.DeviceFromPath need a similar change?

@Iceber
Copy link
Member Author

Iceber commented Feb 10, 2021

@thaJeztah @AkihiroSuda

# libcontainer/devices/device.go
type Device struct {
        Rule

        // Path to the device.
        Path string `json:"path"`

        // FileMode permission bits for the device.
        FileMode os.FileMode `json:"file_mode"`

        // Uid of the device.
        Uid uint32 `json:"uid"`

        // Gid of the device.
        Gid uint32 `json:"gid"`
}

mode &^ unix.S_IFMT should be added when setting device.FileMode.

@Iceber
Copy link
Member Author

Iceber commented Feb 10, 2021

I'm going to try to fix this issue in runc

@thaJeztah
Copy link
Member

I'm going to try to fix this issue in runc

Thank you!

@Iceber Iceber closed this Feb 17, 2021
@Iceber Iceber reopened this Feb 17, 2021
@theopenlab-ci
Copy link

theopenlab-ci bot commented Feb 17, 2021

Build succeeded.

@Iceber
Copy link
Member Author

Iceber commented Feb 17, 2021

@thaJeztah opencontainers/runc#2804 is approved, pls review.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@Iceber
Copy link
Member Author

Iceber commented Feb 22, 2021

@AkihiroSuda PTAL, Thanks

@Iceber
Copy link
Member Author

Iceber commented Feb 22, 2021

@AkihiroSuda Is it necessary to fix the previous version?

@AkihiroSuda
Copy link
Member

I'd prefer not to backport this unless there is a strong reason

@Iceber
Copy link
Member Author

Iceber commented Feb 22, 2021

This issue was found in the use of 1.2.10, the essence of the issue is that it does not comply with the OCI runtime specification, fixing the previous version may make sense
Of course, this issue does not cause program anomalies, but may cause confusion in the use of

Copy link
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

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

LGTM

@estesp estesp merged commit 08d765a into containerd:master Feb 22, 2021
@Iceber Iceber deleted the runtime-spec branch February 22, 2021 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants