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 optional digest suffix to template locator #3021

Closed
wants to merge 2 commits into from

Conversation

jandubois
Copy link
Member

@jandubois jandubois commented Dec 14, 2024

This PR is stacked on top of #3009 and should be rebased once that one is merged.

Meanwhile look at 6a0d370 for the changes specific to this PR. See also discussion in #3019.

The suffix is @digest which may include an optional algorithm (defaults to sha256). The encoded digest must be at least 7 characters long.

Examples:

  • template://my@sha256:60a87371451eabcd211c929759db61746a7c6a1c068f59d868db6aa8dca637bd
  • template://my@sha256:60a87371451
  • template://my@60a8737

It was part of cmd/limactl/start.go but should be generally reusable code.
Moved related cmd/limactl/guessarg into limatmpl as well.

Also created a new `limactl template` command.

It takes a single template locator argument, loads the template,
and writes it to STDOUT.

This is currently not very interesting; it is in preparation of
implementing composable (multi-file) templates.

Signed-off-by: Jan Dubois <[email protected]>
@jandubois

This comment was marked as outdated.

@jandubois jandubois force-pushed the tmpldigest branch 2 times, most recently from 2ab5c20 to c288fcb Compare December 14, 2024 06:35
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Sorry, I don't think this is correct.
This is not useful for local template://, and is incorrect for HTTPS URLs as they may already contain "@" in the path part.

@jandubois
Copy link
Member Author

incorrect for HTTPS URLs as they may already contain "@" in the path part.

The digest is not part of the URL, it is part of the template locator:

LOCATOR := FILE_REF [ DIGEST ]
FILE_REF := [ RELATIVE_FILE | ABSOLUTE_FILE | FILE_URL | HTTPS_URL | TEMPLATE_URL ]
DIGEST := '@' [ ALGORITHM ':' ] HEX_STRING
…

Not all FILE_REFs are URLs. The notation means that you cannot use the @ character in the template name; it works fine everywhere else in the URL, including all paths segments, except for the last one, which is the template name (look for LastIndex('@') > LastIndex('/') logic in the implementation, which avoids matching in the middle of the URL).

I don't think being able to use @ in a template name is important, but I can tighten it up even further by just checking for a DIGEST suffix, so template names like tmpl@v1 or [email protected] would still work, but tmpl@1 would have to be written [email protected].

This is not useful for local template://

It is not, but with relative template locators you don't know if they are resolved locally or not. They work similar to href in HTML: the location is relative to the document that contains them.

So file.sh@deadbeef may turn out to be template://file.sh@deadbeef or https://example.com/file.sh@deadbeef, depending on how the template containing the reference is loaded. So the mechanism must work with any type of FILE_REF.

@jandubois
Copy link
Member Author

jandubois commented Dec 14, 2024

I can tighten it up even further by just checking for a DIGEST suffix,

I've updated the PR to only match valid digest suffixes. See the included test cases for the suffix splitter.

@AkihiroSuda PTAL (at 6a0d370) and let me know if there are any actual issues with this approach. I still think this is the most elegant way to provide the additional functionality without adding needless complexity for the regular user.

The suffix is "@digest" which may include an optional algorithm
(defaults to "sha256"). The encoded digest must be at least 7
characters long.

Examples:
- template://my@sha256:60a87371451eabcd211c929759db61746a7c6a1c068f59d868db6aa8dca637bd
- template://my@sha256:60a87371451
- template://my@60a8737

Signed-off-by: Jan Dubois <[email protected]>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

I still do not think that we should invent a new micro format.

https://example.com/foo@deadbeef should just send GET /foo@deadbeef HTTP/1.1, regardless to the length and format of the deadbeef part.

Why not just: #3009 (comment)

base:
  location: "https://example.com/a.yaml"
  digest: "sha256:deadbeef"

YAML also allows writing this in oneliner:

base: {location: "https://example.com/a.yaml", digest: "sha256:deadbeef"}

@jandubois
Copy link
Member Author

jandubois commented Dec 14, 2024

I still do not think that we should invent a new micro format.

We already do. There is no other application that understands template://fedora. You already can't feed a template locator to curl. (We messed it up too by leaving out the host part; it should have been template:///fedora).

I do not understand what problem you have with adding a suffix; as I pointed out above, it is not part of the URL, it is part of the locator. Adding --template-digest is also adding our own micro-format, just in a different way.

I'm mostly concentrating on usability, and having a single identifier that can describe an immutable resource seems much better to me than needing multiple pieces of information provided in separate places (an extra field in the YAML, another CLI option).

Why not just: #3009 (comment)

I assume you mean:

basedOn:
- location: "https://..."
  digest: "sha256:..."
- location: "template://alpine"
...

It just seems overly complex for something when most users just need:

basedOn: "template://fedora"

Which is already special-cased via custom marshaller to mean

basedOn: ["template://fedora"]

So this would add yet another possible marshaller to the mix, making the common use case complex for an edge case that virtually nobody will be using.

My real concern is that it makes composable templates "too complicated" when they are really quite simple conceptually.

Maybe we should ignore the whole digest stuff, and see how we and the users will actually use the new functionality, and then decide if adding more complexity is worth it.

As you said earlier, this is a side note to a side note, so we can revisit this at some later point in time. I just thought I could offer an elegant mechanism to provide this right away.

@jandubois
Copy link
Member Author

I still do not think that we should invent a new micro format.

BTW, and I know you are aware of this, but the format is not something specific to us; it is already used to specify container images. E.g. I can write:

$ docker build --build-context src=docker-image://alpine@sha256:2c43f33bd1502ec7818bce9eea60e062d04eeadc4aa31cad9dabecb1e48b647b .

So syntactically we use the same format. And the intent from the user's point of view is the same as well: to specify not just a location for the resource, but also a specific immutable content. The locator breaks if the content has changed1.

So this should provide a lot of familiarity for the user.

Now the implementation is of course subtly different: in our case the checksum is not used to locate the resource (so the FILE_REF part must already uniquely specify it), but just to make sure the request fails if the content in the response does not match the request.

But that is an implementation detail; from the user's view this is not a radically new/different format.

Footnotes

  1. Yes, this assumes there is only a single version of the content, and the old version becomes unavailable when it is being updated. But that is just details.

@AkihiroSuda
Copy link
Member

template://, docker-image://, etc. can be extended as we like, but the https:// URL cannot be extended because it has to be compatible with other applications (and has to conform to RFC XXXX).

@jandubois
Copy link
Member Author

Ok, let's not do this. It is premature anyways, as the code for multi-file templates has not been merged yet, and we have no experience how parameterized templates will be used, stored, shared, etc.

@jandubois jandubois closed this Dec 15, 2024
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