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: extension env-injector #4925

Open
wants to merge 41 commits into
base: main
Choose a base branch
from

Conversation

locnnil
Copy link

@locnnil locnnil commented Jul 17, 2024

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox run -m lint?
  • Have you successfully run tox run -e test-py310? (supported versions: py39, py310, py311, py312)

Add extension to inject environment variables into snaps by its command-chain.
This extension takes snap options and transform them into environment variables

snapcraft/extensions/env_injector.py Outdated Show resolved Hide resolved
snapcraft/extensions/env_injector.py Outdated Show resolved Hide resolved
snapcraft/extensions/env_injector.py Outdated Show resolved Hide resolved
snapcraft/extensions/env_injector.py Outdated Show resolved Hide resolved
snapcraft/extensions/env_injector.py Outdated Show resolved Hide resolved
snapcraft/extensions/env_injector.py Outdated Show resolved Hide resolved
@locnnil locnnil force-pushed the env-vars-extension branch 2 times, most recently from 28e698c to 1f9db53 Compare July 23, 2024 18:56
@locnnil locnnil marked this pull request as ready for review July 24, 2024 16:24
extensions/env-injector/src/main.rs Outdated Show resolved Hide resolved
extensions/env-injector/Cargo.toml Outdated Show resolved Hide resolved
extensions/env-injector/src/main.rs Outdated Show resolved Hide resolved
snapcraft/extensions/env_injector.py Show resolved Hide resolved
Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

Thanks, I've tested and everything works as expected.

I have a few concerns, on the testing, error handling, and documentation.

tests/spread/extensions/env-injector/task.yaml Outdated Show resolved Hide resolved
snapcraft/extensions/env_injector.py Outdated Show resolved Hide resolved
extensions/env-injector/src/main.rs Outdated Show resolved Hide resolved
tests/spread/extensions/env-injector/task.yaml Outdated Show resolved Hide resolved
tests/spread/extensions/env-injector/task.yaml Outdated Show resolved Hide resolved
snapcraft/extensions/env_injector.py Show resolved Hide resolved
extensions/env-injector/src/main.rs Outdated Show resolved Hide resolved
extensions/env-injector/src/main.rs Outdated Show resolved Hide resolved
extensions/env-injector/src/main.rs Outdated Show resolved Hide resolved
snapcraft/extensions/env_injector.py Outdated Show resolved Hide resolved
tests/spread/extensions/snaps/env-injector-hello/hello.c Outdated Show resolved Hide resolved
tests/spread/extensions/env-injector/task.yaml Outdated Show resolved Hide resolved
tests/spread/extensions/env-injector/task.yaml Outdated Show resolved Hide resolved
tests/spread/extensions/env-injector/task.yaml Outdated Show resolved Hide resolved
@locnnil locnnil requested a review from farshidtz August 2, 2024 21:58
Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes.
I've just discovered another issue while testing.

extensions/env-injector/src/main.rs Outdated Show resolved Hide resolved
@locnnil locnnil requested a review from farshidtz August 4, 2024 23:05
@locnnil locnnil force-pushed the env-vars-extension branch 2 times, most recently from d6ca9e4 to c2bd475 Compare August 5, 2024 18:45
Copy link
Member

@farshidtz farshidtz left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks a lot.

Nevertheless, I do have concerns over the licensing of snaps using this extension. The extension adds a binary distribution to the snap which uses the following libraries:

We need to take a deeper look into this before flagging the extension as stable. I believe a snap using this extension needs to list MIT as one of its licenses and bundle a copy of the license files for all the above. A simpler solution would be to go back to the Bash implementation (at the expense of performance) with a royalty-free license such as CC0.

Copy link
Collaborator

@mr-cal mr-cal left a comment

Choose a reason for hiding this comment

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

Overall, I think the extension and spread test look good, but I'm not sure how it will work without a configure hook.

Also, this will need a tests/unit/extensions/test_env_injector.py.

I would like to return to one of the original criteria. We still need a snapcraft.io forum post about this idea to get input and feedback on this approach. We can adjust this PR so it gets published to a branch for testing.

tests/spread/extensions/env-injector/task.yaml Outdated Show resolved Hide resolved
@locnnil locnnil requested a review from mr-cal August 8, 2024 17:57
@farshidtz
Copy link
Member

@mr-cal

I would like to return to one of the original criteria. We still need a snapcraft.io forum post about this idea to get input and feedback on this approach. We can adjust this PR so it gets published to a branch for testing.

We have done extensive research on the existing practices. The plan was to get this added as an experimental extension to allow widespread testing (assuming this is the whole point of experimental extensions) and validate the proposed configuration schema for the snap options keys. A forum post is a great idea to request community feedback. Will publishing to a branch make this extension available for testing via stable snapcraft?

@mr-cal
Copy link
Collaborator

mr-cal commented Aug 9, 2024

We have done extensive research on the existing practices. The plan was to get this added as an experimental extension to allow widespread testing (assuming this is the whole point of experimental extensions) and validate the proposed configuration schema for the snap options keys. A forum post is a great idea to request community feedback. Will publishing to a branch make this extension available for testing via stable snapcraft?

It would be available in a branch along the lines of edge/PR-4925. From a conversation between Gustavo and @cmatsuoka last week, I believe that feedback and agreement is needed prior to landing the PR. I think we have a misalignment/miscommunication - @cmatsuoka, can you chime in here?

update: we could also land this and #4926 into a feature/env-injector-extension branch.

Copy link
Contributor

@cmatsuoka cmatsuoka left a comment

Choose a reason for hiding this comment

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

Thanks! I think we need more discussion on this before landing to main, but even for a feature branch it would be interesting to refactor the test case to avoid splitting the extension test between the spread task scriptlet and verifications done inside C code, ideally both sides should reside in the task scriptlet (and maybe the C code can be entirely dropped).

Also compressing the rust binary can be pointless since the snap payload is already compressed (we should investigate other ways to reduce the binary size if a binary is to be used at all.)

tests/spread/extensions/snaps/env-injector-hello/hello.c Outdated Show resolved Hide resolved
tests/spread/extensions/env-injector/task.yaml Outdated Show resolved Hide resolved
Comment on lines 116 to 118
# compress the binary
upx --best --lzma target/{toolchain}/release/env-exporter
cp target/{toolchain}/release/env-exporter $SNAPCRAFT_PART_INSTALL/bin/command-chain/env-exporter-upx
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be simply unecessary as snap payloads are already compressed. Additionally, we can have other ways to produce leaner binaries that should be investigated (e.g. can we go the no_std way?)

Also we need the reasoning to implement this as a binary (as opposed to e.g. a shell script) explained somewhere. Having it as a script could make the need for an additional part unnecessary, and the whole extension would end up being very trivial, maybe to the point that any changes to snapcraft.yaml could be done manually by the user.

In the binary case, it's better to have the injector source in a separate repository instead of bundling it with snapcraft, and just point the part source to it.

Copy link
Member

Choose a reason for hiding this comment

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

This could be simply unecessary as snap payloads are already compressed.

Good point regarding the redundant compression. Need to do some comparison.

Additionally, we can have other ways to produce leaner binaries that should be investigated (e.g. can we go the no_std way?)

A few libraries were added to simplify HTTP query over Unix Socket, JSON parsing, env file parsing so we don't have to rewrite all that logic in the experimentation phase.

Also we need the reasoning to implement this as a binary (as opposed to e.g. a shell script) explained somewhere. Having it as a script could make the need for an additional part unnecessary, and the whole extension would end up being very trivial, maybe to the point that any changes to snapcraft.yaml could be done manually by the user.

This started as a Bash script but was rewritten in Rust due to performance concerns, mostly contributed to the number of queries to SnapD and JSON/string parsing. I do believe this was done prematurely.

In the binary case, it's better to have the injector source in a separate repository instead of bundling it with snapcraft, and just point the part source to it.

If the code gets moved to another repository, how can we maintain compatibility with the snap's base? Git branch per base? Which project / team will own that code?

Comment on lines +79 to +82
"command-chain": ["bin/command-chain/env-exporter"],
"environment": {
"env_alias": f"{app_name}",
},
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a very simple snippet and snap authors are quite used to apply even more complex patterns to parts or applications in order to implement certain features, so this alone may not warrant the existence of an extension.

Copy link
Member

Choose a reason for hiding this comment

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

The selected snippet adds the env exporter program and the alias. The alias gets added only when the user hasn't set it (this is intended and based on snapcraft's internal handling of the dictionary).

The extension does more than that: It adds the source code for the exporter program, builds it and places it in the right location. The design also envisions additional functions, such as adding a configure hook when it isn't a present, or adding configuration validation.

If this was to be done manually, without the extension, the developers would have to:

  1. Fetch the exporter source from trusted location, pick and appropriate version, and hope that it stays compatible with the snap. *
  2. Add build logic to compile and place the executable in a suitable location *
  3. For each application that needs the functionality:
    i. Add the exporter program to the command chain
    ii. Set the application alias

In comparison, when the extension is used:

  1. Add the extension to each application that needs the functionality
  2. Add alias if an app's name needs to be overridden

The manual way requires average snapping knowledge, which early adopters lack. I'd emphasize that the goal is to simplify the snapping of apps and all this complexity thrown at the developer defeats its purpose.


* We could make a snap or a debian package and let the developer stage it, but then we need to separately maintain that package.

@cmatsuoka
Copy link
Contributor

Let's not forget that we still need this to be presented and discussed in the forum before landing it to main. In any case the injector code should reside in a separate repository instead of being part of the snapcraft codebase, and we must carefully consider if the extension is needed at all (the changes to snapcraft.yaml look too simple and can be easily added manually by the snap author).

locnnil and others added 29 commits September 27, 2024 11:40
Co-authored-by: Callahan <callahan.kovacs@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Permission problems are happening because the test snap doesn't have
acess to the envfiles.
As the confinement isn't something related to test, I'm changing this to classic

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
- refact: Change comments to log messages
- refact: Changed install confinement to devmode
- refact: error messages

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Co-authored-by: Farshid Tavakolizadeh <email@farshid.ws>
- exporter program moved to:
https://github.com/canonical/snappy-env

Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
Signed-off-by: Lincoln Wallace <lincoln.wallace@canonical.com>
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.

6 participants