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

Respect core.add-remotes-config-dir configuration option when adding remotes #1665

Open
pwithnall opened this issue May 11, 2018 · 4 comments
Labels

Comments

@pwithnall
Copy link
Collaborator

pwithnall commented May 11, 2018

There are currently a couple of places in common/flatpak-dir.c where ostree_repo_write_config() is called to add per-remote configuration to the OSTree configuration. This won’t work if the repository is configured with core.add-remotes-config-dir=true. For the moment, OSTree sets core.add-remotes-config-dir=false by default for non-system repositories (such as flatpak repositories). However, on systems where the OSTree and flatpak repositories are shared (such as Endless OS), this can cause problems if core.add-remotes-config-dir=false isn’t explicitly configured. Problems like flatpak remote-add failing:

$ sudo flatpak remote-add new-remote http://example.com/
error: Remote "new-remote" already defined in /etc/flatpak/remotes.d/new-remote.conf

The problem is that ostree_repo_remote_[add|change]() will preferentially write remote config to /etc/ostree/remotes.d/ (or wherever the configured remote-config-dir is), if core.add-remotes-config-dir=true. But ostree_repo_write_config() will always modify $repo_dir/config.

Since late last year, calls to ostree_repo_write_config() will error out if core.add-remotes-config-dir=true is set, but the config provided to it contains remotes which are already defined in /etc/ostree/remotes.d. This prevents the system getting into a bad state, but it does mean that flatpak remote-add is broken (it will not succeed, and will leave a partially-configured remote behind when it fails).

Two functions in common/flatpak-dir.c are currently affected:

  • flatpak_dir_modify_remote()
  • flatpak_dir_install_bundle()

For example, if I have:

$ cat /ostree/repo/config && ls -laR /etc/*/remotes.d
[core]
repo_version=1
mode=bare
add-remotes-config-dir=true
min-free-space-percent=0

[remote "flathub"]
url=https://dl.flathub.org/repo/
xa.title=Flathub
gpg-verify=true
gpg-verify-summary=true

/etc/ostree/remotes.d:
total 8
drwxr-xr-x 2 root root 4096 May 11 10:59 .
drwxr-xr-x 3 root root 4096 May 11 10:59 ..

Then I run:

$ sudo flatpak remote-add new-remote http://example.com/
error: Remote "new-remote" already defined in /etc/flatpak/remotes.d/new-remote.conf

I end up with:

$ cat /ostree/repo/config && ls -laR /etc/*/remotes.d
[core]
repo_version=1
mode=bare
add-remotes-config-dir=true
min-free-space-percent=0

[remote "flathub"]
url=https://dl.flathub.org/repo/
xa.title=Flathub
gpg-verify=true
gpg-verify-summary=true

/etc/flatpak/remotes.d:
total 12
drwxr-xr-x 2 root root 4096 May 11 14:23 .
drwxr-xr-x 3 root root 4096 May 11 14:18 ..
-rw-r--r-- 1 root root   46 May 11 14:23 new-remote.conf

/etc/ostree/remotes.d:
total 8
drwxr-xr-x 2 root root 4096 May 11 10:59 .
drwxr-xr-x 3 root root 4096 May 11 10:59 ..
$ cat /etc/flatpak/remotes.d/new-remote.conf 
[remote "new-remote"]
url=http://example.com/
@dbnicholson
Copy link
Contributor

Another option is to finish off ostreedev/ostree#1166 so that flatpak can do sane remote modification instead of doing the "add remote with just URL if it doesn't exist then set options via ostree_repo_copy_config()/ostree_repo_write_config() dance".

pwithnall added a commit to endlessm/flatpak that referenced this issue May 11, 2018
See the comment added in the code for an explanation: if
core.add-remotes-config-dir=true, we must avoid adding configuration in
both /etc/flatpak/remotes.d/ and /ostree/repo/config. In lieu of
actually supporting adding the configuration in the right place, bail
early to avoid leaving partial configuration in the wrong place.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

flatpak/flatpak#1665
pwithnall added a commit that referenced this issue May 11, 2018
See the comment added in the code for an explanation: if
core.add-remotes-config-dir=true, we must avoid adding configuration in
both /etc/flatpak/remotes.d/ and /ostree/repo/config. In lieu of
actually supporting adding the configuration in the right place, bail
early to avoid leaving partial configuration in the wrong place.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

#1665
@pwithnall
Copy link
Collaborator Author

That would be the best option. I don’t have time to work on that at the moment though. :-(

rh-atomic-bot pushed a commit that referenced this issue May 14, 2018
See the comment added in the code for an explanation: if
core.add-remotes-config-dir=true, we must avoid adding configuration in
both /etc/flatpak/remotes.d/ and /ostree/repo/config. In lieu of
actually supporting adding the configuration in the right place, bail
early to avoid leaving partial configuration in the wrong place.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

#1665

Closes: #1666
Approved by: alexlarsson
mwleeds pushed a commit to endlessm/flatpak that referenced this issue May 17, 2018
See the comment added in the code for an explanation: if
core.add-remotes-config-dir=true, we must avoid adding configuration in
both /etc/flatpak/remotes.d/ and /ostree/repo/config. In lieu of
actually supporting adding the configuration in the right place, bail
early to avoid leaving partial configuration in the wrong place.

Signed-off-by: Philip Withnall <withnall@endlessm.com>

flatpak/flatpak#1665

Closes: #1666
Approved by: alexlarsson

(cherry picked from commit d87d5ed)
@mwleeds
Copy link
Collaborator

mwleeds commented May 25, 2018

@pwithnall this can be closed, right?

@pwithnall
Copy link
Collaborator Author

@pwithnall this can be closed, right?

Nope, the right fix is to modify the file which contains the particular remote config, rather than unconditionally modifying $repo_dir/config. The ‘fix’ I pushed in 21e219b just returns an error message in the dodgy case, rather than actually doing the right thing.

The long-term fix here is probably ostreedev/ostree#1166, but I’ll leave it for Alex to decide whether to defer to 1166, or whether to fix this in flatpak in the interim.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants