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

Add new refescape module #107

Merged
merged 1 commit into from
Sep 30, 2021
Merged

Add new refescape module #107

merged 1 commit into from
Sep 30, 2021

Conversation

cgwalters
Copy link
Member

Prep for work on the new container module, where we want to store
container image references (e.g. docker://quay.io/coreos/fedora)
as ostree refs. Several bits of that are not valid in ostree refs,
such as the : or the double // (which would be an empty filesystem path).

This escaping scheme uses _ in a similar way as a \ character is
used in other syntax. For example, : is _3A_ (hexadecimal).
// is escaped as /_2F_ (i.e. the second / is escaped).

@@ -0,0 +1,156 @@
//! Escape strings for use in ostree refs.
Copy link
Member Author

Choose a reason for hiding this comment

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

Now, perhaps this should go into https://github.com/ostreedev/ostree-rs - but that takes us down the new road of adding Rust-native API. Which...well, we'll visit ostreedev/ostree#2427 at some point.

//!
//! This escaping scheme uses `_` in a similar way as a `\` character is
//! used in Rust unicode escaped values. For example, `:` is `_3A_` (hexadecimal).
//! Because the empty path is not valid, `//` is escaped as `/_2F_` (i.e. the second `/` is escaped).
Copy link
Member

@lucab lucab Sep 29, 2021

Choose a reason for hiding this comment

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

I fear that the rules for handling / are too weird and will lead to future footguns, as you can see trying to escape a// vs a/b/ vs a/b/c (and similar).

I think the problem is that this logic is conflating an input character / with the ostree fragments separator /. Per ostree regexp, those two are not the same thing. We do introduce ambiguity here.

My suggestions here would be:

  • always escape all / from the input string
  • clarify that the input stream is a single fragment, not a full ref (i.e. one can't do docker_name/my_ostree_branch)
  • if there is a need to encode a ref with multiple fragments, take them as already split inputs through a string slice (i.e. ["docker_name", "my_ostree_branch"].

Same for the unescape counterpart. With the current logic, once you get back an unescaped a/b/c it's ambiguous whether the initial input was a single docker name or three ostree fragments or something inbetween.

Copy link
Member Author

@cgwalters cgwalters Sep 29, 2021

Choose a reason for hiding this comment

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

The way the container code uses this, it is unambiguous - see this https://github.com/ostreedev/ostree-rs-ext/pull/99/files#diff-d618b05deec14d83f95c25b128171241e11614efec182d232e6ae46a8f4c09feR21 - basically because we have a static string as prefix, and the rest is assumed defined/required to be escaped.

I think it's at least somewhat nicer to see / literally, you just learn to read _3A_ as :. Here's a random paste from my test VM:

[root@cosa-devsh tmp]# ostree refs
ostree/container/blob/sha256_3A_0f79a34559f64a74b08fd5bbad34072f8c1a7784f33bd750d878c7f387f1b5a1
ostree/container/blob/sha256_3A32d62d73008776158de3532b560bb4d1c087f9600eecc732594a5a31cdccaf5b
ostree/container/blob/sha256_3A_32d62d73008776158de3532b560bb4d1c087f9600eecc732594a5a31cdccaf5b
ostree/container/blob/sha256_3A0f79a34559f64a74b08fd5bbad34072f8c1a7784f33bd750d878c7f387f1b5a1
ostree/container/blob/sha256_3A655734cf6267d3ba1506d4dac15329679216c9169093a5a99b60b7484e4320d4
ostree/1/1/2
ostree/1/1/1
ostree/container/image/docker_3A_/_2F_quay_2E_io/cgwalters/fcos-derivation-example_3A_latest
ostree/1/1/0
ostree/container/blob/sha256_3A_809c66ca43fa2ac72c15a83adb27b1e1794a7f5ae8463b3a8c559164a9201d8b
ostree/container/blob/sha256_3A_655734cf6267d3ba1506d4dac15329679216c9169093a5a99b60b7484e4320d4
ostree/container/blob/sha256_3A809c66ca43fa2ac72c15a83adb27b1e1794a7f5ae8463b3a8c559164a9201d8b
ostree/container/blob/sha256_3A5707006e0d4d228ea1011b85c431727af86bf46309615b04555e95fb3849dab7
test
[root@cosa-devsh tmp]# 

With your proposal of escaping individual fragments...the calling app would need to have rules/schema for which portions of a ref are escaped. I guess I could imagine doing that, but I am having trouble thinking of a strong use case for that versus the "prefixing" approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually we should canonicalize docker:// to registry: I think instead of the other way around. Then it'd be:
ostree/container/image/registry_3A_quay_2E_io/cgwalters/fcos-derivation-example_3A_latest
which I think is nicer than reading
ostree/container/image/registry_3A_quay_2E_io_2F_cgwalters_2F_fcos-derivation-example_3A_latest

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the additional context! It looks the API you are aiming for is perhaps something like $input -> prefix/$escaped-input and back? But then is there anything preventing people to do prefix/$escaped-input/arbitrarystuff?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I reworked this to require a prefix to better demonstrate best practice.

@cgwalters
Copy link
Member Author

Perhaps it'd be better to fully enshrine this as an APIs which accept a prefix, and escape/unescape the rest.

The list_images() code could be generalized a bit to list_escaped_refs() or so too.

@cgwalters
Copy link
Member Author

Nevermind still more to do

@cgwalters cgwalters marked this pull request as ready for review September 29, 2021 17:44
@cgwalters
Copy link
Member Author

OK, this should be better now.

@cgwalters cgwalters force-pushed the ref-escape branch 2 times, most recently from c291a76 to 452215c Compare September 29, 2021 18:50
Prep for work on the new container module, where we want to store
container image references (e.g. `docker://quay.io/coreos/fedora`)
as ostree refs.  Several bits of that are not valid in ostree refs,
such as the `:` or the double `//` (which would be an empty filesystem path).

This escaping scheme uses `_` in a similar way as a `\` character is
used in other syntax.  For example, `:` is `_3A_` (hexadecimal).
`//` is escaped as `/_2F_` (i.e. the second `/` is escaped).
@cgwalters
Copy link
Member Author

OK, assuming comments here were addressed, if anyone has anything further we can do post-merge fixes!

@cgwalters cgwalters merged commit 0a9c777 into ostreedev:main Sep 30, 2021
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.

2 participants