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

Support changing io.podman label domain namespace #1015

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Jul 31, 2024

Add global cli flag --label-domain and env var COMPOSE_LABEL_DOMAIN (default io.podman).
If set, it changes the out-of-spec root domain set and filtered for alongside the spec-mandated com.docker.compose.

Related:

Compose spec

@legobeat legobeat force-pushed the configurable-label-domain-namspace branch from bbcc79a to fb873f2 Compare July 31, 2024 02:07
@legobeat legobeat force-pushed the configurable-label-domain-namspace branch 2 times, most recently from 8b4ac31 to 9d7a7c9 Compare July 31, 2024 03:10
@p12tic
Copy link
Collaborator

p12tic commented Aug 1, 2024

I think this feature makes sense in principle, but instead of environment variable and command-line option I would add a customization point to the compose file. For example, we already have default_net_name_compat.

Environment variable and command line option are error prone, because a single compose file will need to have the same values passed on every invocation of podman-compose, otherwise it will break horribly.

@legobeat
Copy link
Contributor Author

legobeat commented Aug 1, 2024

I think this feature makes sense in principle, but instead of environment variable and command-line option I would add a customization point to the compose file. For example, we already have default_net_name_compat.

Environment variable and command line option are error prone, because a single compose file will need to have the same values passed on every invocation of podman-compose, otherwise it will break horribly.

My thinking on this matter:

  • Compose Spec does have some expectations around resource labels (specifically, ones under com.docker.compose)
    • The existing io.podman labels are already "undefined behavior"/"out of spec" (not in itself necessarily an issue!)
  • Your proposed approach sounds like an extension to the compose file schema and spec
  • If users prefer editing the Compose file, this can mostly already be achieved today by configuring labels on resources explicitly - adding the new feature here is really just sugar/convenience
    • (perhaps there's some unresolved case in filtering there - have not looked closer there)
  • There are actually a lot of places where podman-compose is yet to catch up to even some basics in the compose spec. I think it would be great to address some of the related ones before further extending the schema and diverging for the sake of new features or ergonomics.
  • If the feature turns out useful/widely desired, it can still be proposed as a spec extension according to your proposal as a follow-up or in parallel, of course

The motivation here is to allow transparently using the same compose files 1) together with other implementations that may differ in spec-compliance and how these labels are treated semantically, 2) using just podman-compose, but configured from an operations perspective (where compose files can typically be a shipped artifact, configured via env-files).

So doing it by env/cli would at least be preferred for us.

@legobeat
Copy link
Contributor Author

legobeat commented Aug 1, 2024

Environment variable and command line option are error prone, because a single compose file will need to have the same values passed on every invocation of podman-compose, otherwise it will break horribly.

I think that's a separate issue (combination of bugs / mismatch between documentation+spec and actual behavior), not an inherent limitation of the approach?

(I have local draft with fixes to some of the above but as they're based on a combination of this + #1014 + #1016, I'd like to see these through before submitting those to keep reviews simple and avoid messy rebase-churn)

@legobeat legobeat force-pushed the configurable-label-domain-namspace branch from 9d7a7c9 to 1483106 Compare August 1, 2024 23:36
@p12tic
Copy link
Collaborator

p12tic commented Aug 2, 2024

A couple of nitpicks:

The existing io.podman labels are already "undefined behavior"/"out of spec" (not in itself necessarily an issue!)

This is a conscious decision due to the fact that podman-compose will always be missing some features of docker-compose. Therefore it is impossible to claim full compatibility with docker-compose and thus podman-compose should not try to manage resources created by docker-compose.

If users prefer editing the Compose file, this can mostly already be achieved today by configuring labels on resources explicitly - adding the new feature here is really just sugar/convenience

I think it's not possible to set the labels with the prefix used by the compose itself. From the spec:

The com.docker.compose label prefix is reserved. Specifying labels with this prefix in the Compose file results in a runtime error.

Going back to whole argument I think I understand what you're saying, we are arriving to different conclusions due to different points of view.

I consider both command line arguments and the supported syntax of the compose file as the API provided by podman-compose. If we want to expose a feature to the users we should consider both command line arguments and the compose file as potential points where users could configure the feature. Flexibility, ease of use, whether API is error prone, maintainability and so on are arguments for and against a particular approach.

In this particular case, resources created using different domain namespace will be invisible to both podman-compose and docker-compose. It is easy to miss a command line argument in some invocation of podman-compose and this will result in error that is not immediately visible but may result in confusing errors later. Even podman-compose down has no chance to work properly. Therefore I view changing io.podman label domain namespace via command line option as error prone with high impact in terms of lost time in case of an error.

Putting the configuration of the feature into compose file as a vendor extension significantly reduces the chances of errors because all invocations of podman-compose will be guaranteed to use the same label domain namespace. Therefore this looks like a preferable way to expose the feature.

I am personally weakly against exposing this feature in general on the grounds of error-proneness. However I don't want to be a maintainer that limits a project to the limits of my personal imagination. Therefore I will accept this feature with some modifications to reduce impact that I don't like but preserving ability to do what you want. I think this should be a reasonable compromise.

Are there limitations in the approach that I proposed that would prevent your use case? Do you just want to configure com.docker.compose and that's it? Maybe exposing use_canonical_label_domain: (true|false) would be enough for your use case?

@legobeat
Copy link
Contributor Author

legobeat commented Aug 2, 2024

Thank you for the thoughtful follow-up!

This is a conscious decision due to the fact that podman-compose will always be missing some features of docker-compose. Therefore it is impossible to claim full compatibility with docker-compose

This makes perfect sense! Pointed it out for context, not problematic I think.

and thus podman-compose should not try to manage resources created by docker-compose.

I think this can be left to the user? Of course it would be up to the user to ensure they're not using unsupported features. Just like if they would be using two different instances/version of podman-compose interfacing with the same resources.

I'm mostly considering Compose spec. Which is separate from implementation parity with docker-compose.

Very much agree that the totality of files (with schemas and semantics), cli + env vars, are all part of the podman-compose API. But aside from the ergonomics perspective (as you say, coming from different situations most likely - for my situation, putting it the compose file defeats the purpose somewhat), I'm thinking of the Compose file as something we expect to conform to spec, whereas CLI flags is something podman-compose can change independently without considering spec-compliance in the same way?

I think it's not possible to set the labels with the prefix used by the compose itself. From the spec:

The com.docker.compose label prefix is reserved. Specifying labels with this prefix in the Compose file results in a runtime error.

Yes, so, this PR does not touch these labels, as it is it does not allow changing/removing/overriding these in any new way. What it concerns itself with is the io.podman counterparts.

Are there limitations in the approach that I proposed that would prevent your use case? Do you just want to configure com.docker.compose and that's it?

Actually, the main motivation was/is to workaround what I assume is some bug in compose where filtering of resources only look for io.podman.compose and ignores com.docker.compose. This PR helps to both work around the specific issue I'm facing (by setting COMPOSE_LABEL_DOMAIN=com.docker, which would fill in the gaps), and also debugging these classes of issues in the future.

But I can also imagine it being a useful feature on its own to namespace resources and use custom domains (which again, would not touch the implied com.docker ones, only allow replacing io.podman with something else).

Does any of this change the picture a bit? :)

@legobeat legobeat force-pushed the configurable-label-domain-namspace branch from 1483106 to 6ee405d Compare August 2, 2024 08:48
@legobeat legobeat marked this pull request as ready for review August 2, 2024 08:50
@p12tic
Copy link
Collaborator

p12tic commented Aug 2, 2024

I'm thinking of the Compose file as something we expect to conform to spec, whereas CLI flags is something podman-compose can change independently without considering spec-compliance in the same way?

This is where our point of view differs.

The fact is that the default io.podman domain namespace is already used. The reason why a value different from com.docker has been selected was most likely to not break docker-compose. Regardless of reasons, that's the current situation.

Whether default io.podman domain namespace is compatible with compose spec or not does not matter because we are not going to change it. Firstly, a lot of deployments rely on default io.podman domain namespace not being changed. Additionally, it is not good idea to change it right now because podman-compose is still lacking features compared to docker-compose. For the time being these are strong reasons.

You present a good argument why user may want to change the default domain namespace anyway. I agree with it.

The way how this is going to be exposed is completely up to podman-compose. Whether this is exposed as vendor setting in compose file or via command line option does not affect compatibility to compose spec in practice because out of the box podman-compose is incompatible anyway.

I agree that in theory it is a bit nicer to be able to specify a custom command line option to podman-compose and make it use the same domain namespace as docker-compose. However given practical considerations I think better idea is to have a setting in compose file instead.

Finally, the reason why compose exists at all was that it was inconvenient to setup a bunch of arguments to docker command when creating containers. Specifying them declaratively turned out way more convenient and less error prone. I think adding new setting keys to compose file (especially important ones such as domain namespace) is in the spirit of compose ecosystem as a whole.

Yes, so, this PR does not touch these labels, as it is it does not allow changing/removing/overriding these in any new way. What it concerns itself with is the io.podman counterparts.

Here I was responding to "If users prefer editing the Compose file, this can mostly already be achieved today by configuring labels on resources explicitly". I interpreted that you were suggesting that users may add com.docker.compose.project and similar labels. In any case, I think this is peripheral to the discussion.

Does any of this change the picture a bit? :)

Unfortunately no, sorry. Having said that, it was a very useful discussion.

I do wonder though, does my proposal not work in your use case for some reason? If it works then perhaps the best path forward is just to implement that solution, get it merged and move on. Compatibility of default domain namespace will become more important once podman-compose has relative feature parity with docker-compose.

@legobeat
Copy link
Contributor Author

legobeat commented Aug 3, 2024

Whether this is exposed as vendor setting in compose file or via command line option does not affect compatibility to compose spec in practice because out of the box podman-compose is incompatible anyway.

Hm, just to clarify on this, from README.md:

An implementation of [Compose Spec](https://compose-spec.io/) with [Podman](https://podman.io/) backend.
[...]
## References:
* [spec.md](https://github.com/compose-spec/compose-spec/blob/master/spec.md)
* [docker-compose compose-file-v3](https://docs.docker.com/compose/compose-file/compose-file-v3/)
* [docker-compose compose-file-v2](https://docs.docker.com/compose/compose-file/compose-file-v2/)

I would hope that this still reflects current intentions: for podman-compose to be an implementation of Compose Spec, and that any violations/deviations of the spec are to be treated as bugs?

(Which, AIUI, is not at conflict with the additional existing use of custom io.podman labels so unless I am overlooking something I don't think this is a situation where implementation-backwards-compat is at conflict with spec-conformance, though other such situation might exist I guess)

@p12tic
Copy link
Collaborator

p12tic commented Aug 3, 2024

I would hope that this still reflects current intentions: for podman-compose to be an implementation of Compose Spec, and that any violations/deviations of the spec are to be treated as bugs?

This is true with a temporary exception of default domain namespace. That bug will fixed once podman-compose is relatively compatible with docker-compose.

@p12tic
Copy link
Collaborator

p12tic commented Aug 3, 2024

By the way, one incompatibility between podman-compose and docker-compose has already been fixed by introducing a setting into the compose file. See https://github.com/containers/podman-compose/blob/main/docs/Extensions.md#compatibility-of-default-network-names-between-docker-compose-and-podman-compose

Compatibility between podman-compose and docker-compose in terms of default label domain namespace could be handled in a similar way. I'm sure we will find more similar incompatibilities in the future.

@legobeat legobeat force-pushed the configurable-label-domain-namspace branch from 6ee405d to 88f73bb Compare August 5, 2024 23:55
@legobeat legobeat force-pushed the configurable-label-domain-namspace branch from 88f73bb to 757d14e Compare October 16, 2024 11:21
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.

Support changing io.podman label domain namespace
2 participants