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

Allow for file mark tokens within arguments for file marks #468

Closed
kdvolder opened this issue Jul 29, 2021 · 9 comments
Closed

Allow for file mark tokens within arguments for file marks #468

kdvolder opened this issue Jul 29, 2021 · 9 comments
Labels
discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution enhancement This issue is a feature request priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done.

Comments

@kdvolder
Copy link

The documentation here: https://carvel.dev/ytt/docs/latest/file-marks/#available-marks

Shows that you can specify file marks on CLI as follows:

-file-mark '<path>:path=<new-path>'

This means that characters such as ':' and '=' have a special meaning in the notation (they are used to separate different 'parts' of the file mark argument. But if we wanted to use these characters in some of those values as well? And yes it is valid for example to have file names with a ':' or '=' as part of their path/name.

Does the notation allow for escaping these special characters when we don't want them to be interpreted as a 'seperator'? If no, perhaps that is a situation that should be rectified. If yes, then the docs should probably explain how.

@kdvolder kdvolder added the carvel triage This issue has not yet been triaged for relevance label Jul 29, 2021
@cppforlife
Copy link
Contributor

Does the notation allow for escaping these special characters when we don't want them to be interpreted as a 'seperator'? If no, perhaps that is a situation that should be rectified.

it does not today, given that it's very rare use case that was not previously requested. do you have some concrete examples where you are relying on :s and =s in paths? i think if we ever decide to add support for such characters in paths, then we would probably add --file-mark-json flag that allows to specify such values unambiguously.

current state might still be worth documenting though.

@cppforlife cppforlife added discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution and removed carvel triage This issue has not yet been triaged for relevance labels Aug 3, 2021
@kdvolder
Copy link
Author

kdvolder commented Aug 9, 2021

I do not have a specific use case. However we have a tool that wraps YTT and passes options/values from the wrapping tool into YTT to filemarks. The question on how to encode special characters thus came up in that context. As it is now, we just pass the options as is and don't do anything about any special characters in them. That may have unpredictable effects but it seems to be the best that we can do. Also I suppose as long as users of our tool aren't using any special characters (which is probably the case) then there's no problem.

@pivotaljohn pivotaljohn added enhancement This issue is a feature request priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done. labels Nov 5, 2021
@pivotaljohn pivotaljohn changed the title Clarify in the docs how to handle special characters in file marks Allow for file mark tokens within arguments for file marks Nov 5, 2021
@pivotaljohn
Copy link
Contributor

pivotaljohn commented Nov 5, 2021

i think if we ever decide to add support for such characters in paths, then we would probably add --file-mark-json flag that allows to specify such values unambiguously.

What do you think of simple quoting?

--file-mark '"<path>":path=<new-path>'
--file-mark '<path>:path="<new-path>"'

... and combinations thereof.

(edit: removed suggestion to quote file mark names; its not necessary as this is an enumeration, not a wide-open string)

@pivotaljohn
Copy link
Contributor

@kdvolder: two questions:

  1. Did the folks who brought up this question...

    The question on how to encode special characters thus came up in that context.

    do they have any specific strings in mind?

  2. Would the approach of quoting, noted above, be feasible from your perspective?

@kdvolder
Copy link
Author

kdvolder commented Nov 24, 2021

  1. Did the folks who brought up this question...

The 'folks' here was just me. Sorry if that wasn't clear.

do they have any specific strings in mind?

Not really. We are just passing through a string that comes from 'some user'. Did not have a particular user in mind, nor any particular use-case that requires specific characters. However when our tool is asked to pass along information a user provided to us as an argument / input, I think it is our tool's responsibility in turn that this string is passed faithfully to ytt. This means essentially that it should work regardless of any particulars about the string in question or whether or not it contained any 'special ' characters. So what I meant when I said that 'the question came up in that context' is that I considered the question 'how do I pass a string faithfully down to ytt, regardless of what might be in that string exactly?'. I did not have an answer to that question so I came here and asked about it.

Nevertheless, I can tell you that the characters I was thinking of specifically where ':' and '=' because they clearly have special meaning in the 'filemark' syntax. And so therefore that probably means that if they are used as 'just characters in a string' they might need to be escaped.

What do you think of simple quoting?

Simpler is better in my view. I.e. from my point of view I just have to know two things to answer my question:

  1. what characters are to be treated specially and need some kind of escaping (e.g. just provide a list of characters that 'need escaping')
  2. what is the mechanism for 'escaping' such characters (e.g. if a character is special you can escape it by preceding it with a \, this implies backslashes themselves are 'special' and need to be escaped by using \\).

I do not particularly care about the exact mechanics, just that they are clearly defined so I can implement an 'escape special characters when passing a string to ytt' function.

From your point of view things may be more complex since you may also need to consider how it interferes with shell escape mechanics (i.e. I can imagine scenarios where you would need double escaping or other 'funny business' because the two escape mechanism interact and overlap). But from my point of view this doesn't matter because the scenario is just one tool (ours) calling another tool (ytt) and we are passing the tool arguments as a string array.

In your proposal, I think you may have some issues with using quotes to escape things, because that is also what a typical shell does. So if we wanted to use quotes to escape a '=' character we would have to include quotes in the string to escape the '='. But that means we would have to also tell the shell not to treat those quotes as special so escape them for the shell. I don't even want to think of what happens when we want to escape both kinds of quotes in the same string as well as some special '=' and ':' characters.

Note: In some ways maybe this is more of a 'academic' than a practical issue. So I woudn't be offended at all if you decided its not something you wanted to fix. As you have rightfully observed, it is questionable whether this causes any real problems in practical / real use-cases. Then again, these kinds of 'improper escape handling' issue are also somewhat notorious for turning into security bugs (SQL injection is probably the most prominent example this kind of thing I guess).

These are just my thoughts on the matter. Feel free to use your own best judgement on whether this is important enough to fix and or what kind of escaping mechanism would fit best with how YTT works, if you do decide to address it.

@kdvolder
Copy link
Author

kdvolder commented Nov 24, 2021

More random thoughts... To avoid interaction between 'shell' escapes and 'ytt escapes' (which is rather tricky)... Maybe the simplest solution would be not to have any special characters part of ytt syntax and only use shell escape syntax. What this means essentially is that ytt cannot use ':' and '=' characters within a single shell argument as 'dividers' between multiple bits of information. Instead we would have to just rely on the shell to pass the 3 bits of required information for a 'file mark' as separate shell arguments.

So instead of something like:

--file-mark 'generated.go.txt:path=gen.go.txt'

You would do instead:

--file-mark 'generated.go.txt' 'path' 'gen.go.txt'

Drawbacks:

  • less readable / intuirive (i.e. the special characters help somewhat to understand / parse command for human readers, and we'd loose that).
  • this would be a breaking change (unless you keep the old filemark option and provide a new one with a different name using this new syntax)

Advantages:

  • ytt doesn't need to worry about any escaping, neither does another tool passing filemark arguments to ytt
  • users use shell escape mechanics in the normal way that they are probably familiar with

@pivotaljohn
Copy link
Contributor

So what I meant when I said that 'the question came up in that context' is that I considered the question 'how do I pass a string faithfully down to ytt, regardless of what might be in that string exactly?'. I did not have an answer to that question so I came here and asked about it.

Appreciate you bringing it up and having the conversation, @kdvolder. 🙏🏻

I'm better getting a sense of the scope of what you're aiming at: tooling integration.

Given all this, I'm returning to @cppforlife's observation and suggestion:

... i think if we ever decide to add support for such characters in paths, then we would probably add --file-mark-json flag that allows to specify such values unambiguously.

Let's:

  • not introduce a breaking change or complicate the existing flag;
  • document that : is not a supported character in paths for this flag (in fact, = works just fine for file mark path values);
  • recast this issue as a feature request to provide a new flag: one that puts no restriction on path values: Accept File Marks for any valid path (via --file-marks-yaml) #553

In this way, we can prioritize creating a new flag that supports the use case that's at the heart of this issue.

@pivotaljohn
Copy link
Contributor

For clarity's sake, we'll close this issue. It's referenced by the recast feature request, so remains discoverable.

@kdvolder
Copy link
Author

kdvolder commented Nov 29, 2021

FWIW: I think that you made a very reasonable decision and I'm pretty happy about the thoughtfullness with which you have handled this request/ticket. And yes, I can confirm that if such a json or yaml option, would have existed, I would have used that and it would have served our needs just fine.

@github-actions github-actions bot added the carvel triage This issue has not yet been triaged for relevance label Nov 29, 2021
@aaronshurley aaronshurley removed the carvel triage This issue has not yet been triaged for relevance label Nov 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This issue is not a bug or feature and a conversation is needed to find an appropriate resolution enhancement This issue is a feature request priority/awaiting-more-evidence Lowest priority. Possibly useful, but not yet enough support to actually get it done.
Projects
None yet
Development

No branches or pull requests

4 participants