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

uidMappings: change order of fields for clarity #956

Merged
merged 1 commit into from
Mar 9, 2018

Conversation

alban
Copy link
Contributor

@alban alban commented Mar 8, 2018

"man 7 user_namespaces" explains the format of uid_map and gid_map:

    <containerID> <hostID> <mapSize>

The order of map entries in JSON does not matter. But for the clarity of
the spec, I find it easier to understand if the order of the JSON fields is
the same as the order of the fields in the underlying uid_map/gid_map
files.

I am about to file a PR in runtime-tools because the fields in
uid_map/gid_map were parsed in the wrong order.

Signed-off-by: Alban Crequy alban@kinvolk.io

alban added a commit to kinvolk-archives/runtime-tools that referenced this pull request Mar 8, 2018
The fields of /proc/$pid/uid_map and /proc/$pid/gid_map were parsed in
the wrong order.

Related: opencontainers/runtime-spec#956

Signed-off-by: Alban Crequy <alban@kinvolk.io>
@wking
Copy link
Contributor

wking commented Mar 8, 2018

Maybe swap them in the Go structure too? I think (but haven't tested) that Go's default JSON serializer uses the struct's property order for its output.

@alban
Copy link
Contributor Author

alban commented Mar 8, 2018

Just tested and Go's default JSON serializer indeed outputs things in order :)

I'll force-push with the swap.

"man 7 user_namespaces" explains the format of uid_map and gid_map:
    <containerID> <hostID> <mapSize>

The order of map entries in JSON does not matter. But for the clarity of
the spec, I find it easier to understand if the order of the JSON fields is
the same as the order of the fields in the underlying uid_map/gid_map
files.

I am about to file a PR in runtime-tools because the fields in
uid_map/gid_map were parsed in the wrong order.

Signed-off-by: Alban Crequy <alban@kinvolk.io>
@alban alban force-pushed the alban/uid-mapping branch from 1d4e354 to cd39042 Compare March 8, 2018 19:35
@tianon
Copy link
Member

tianon commented Mar 8, 2018

LGTM

(purely cosmetic change)

Approved with PullApprove

@mrunalp
Copy link
Contributor

mrunalp commented Mar 9, 2018

LGTM

Approved with PullApprove

@mrunalp mrunalp merged commit 6be516e into opencontainers:master Mar 9, 2018
@alban alban deleted the alban/uid-mapping branch March 13, 2018 15:10
@vbatts vbatts mentioned this pull request Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants