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 support for an extra Rclone config file #2956

Merged
merged 2 commits into from
Sep 5, 2023

Conversation

robinkar
Copy link
Contributor

This PR is going to conflict with the other PR I have open, but submitting this already to get the discussion started. Will fix the conflicts once needed in either of the PRs.

The PR adds the possibility of having Rclone use two different config files, something that is not normally possible in Rclone. The idea behind this is that we would want to have both a system-managed config file (e.g. $HOME/.config/rclone/ood.conf) and a user-managed config file (Rclone default: $HOME/.config/rclone/rclone.conf). The system-managed config file could, for example, be generated on user login and deleted when the user's session ends, which is something we are attempting to accomplish. The user-managed one would use the default Rclone environment variable for configuring it, i.e. RCLONE_CONFIG, and the extra configuration added here would be set using OOD_RCLONE_EXTRA_CONFIG.

The way this works is that rclone config dump is used get the config in JSON format, e.g.

{
    "my_remote": {
        "access_key_id": "??????",
        "acl": "private",
        "env_auth": "false",
        "provider": "Other",
        "secret_access_key": "??????",
        "type": "s3"
    }
}

The JSON file is then parsed to create env vars used for configuring Rclone and passed to Rclone. While these env vars could of course be set for the whole Rails process, it might be best to limit them to only the Rclone process as I've done here in the PR, to avoid them accidentally being logged or exposed somewhere.

@johrstrom
Copy link
Contributor

I am considering what alternatives there could be.

Why not just invoke rclone with RCLONE_CONFIG=/some/other/location? We could add something like rclone_config_path and loop through /first/location:/some/other/location.

@robinkar
Copy link
Contributor Author

I am considering what alternatives there could be.

Why not just invoke rclone with RCLONE_CONFIG=/some/other/location? We could add something like rclone_config_path and loop through /first/location:/some/other/location.

That does seem like it would also be an option, hadn't thought about that.

@johrstrom
Copy link
Contributor

johrstrom commented Aug 11, 2023

I am considering what alternatives there could be.

Why not just invoke rclone with RCLONE_CONFIG=/some/other/location? We could add something like rclone_config_path and loop through /first/location:/some/other/location.

That does seem like it would also be an option, hadn't thought about that.

Yea I just feel like this is a bit complex. Especially as we have to call open3's original function to circumvent the initializer.

What's more is, if you're reading from a path, you can have several files and can apply FACLs to them to limit the visibility of one remote or another. (and you as the person who's writing the files don't need any translation between rclone config and json).

Make Rclone in OOD support an extra configuration file
(OOD_RCLONE_EXTRA_CONFIG), e.g. for allowing both user and OOD managed
configs. Extra config file is read into env vars passed to Rclone.
@robinkar robinkar force-pushed the extra-rclone-config branch from 9767e8f to 716acc0 Compare August 14, 2023 14:21
@robinkar
Copy link
Contributor Author

Rebased merge conflicts.

I am considering what alternatives there could be.

Why not just invoke rclone with RCLONE_CONFIG=/some/other/location? We could add something like rclone_config_path and loop through /first/location:/some/other/location.

That does seem like it would also be an option, hadn't thought about that.

Yea I just feel like this is a bit complex. Especially as we have to call open3's original function to circumvent the initializer.

What's more is, if you're reading from a path, you can have several files and can apply FACLs to them to limit the visibility of one remote or another. (and you as the person who's writing the files don't need any translation between rclone config and json).

I'll need to think a bit more how this could be implemented easily. One concern that came up while thinking about this is that handling the case where the two remotes are in different config files is going to be quite complicated. I.e. moving files from remote A (in $HOME/.config/rclone/rclone.conf) to remote B (in $HOME/.config/rclone/ood.conf), while only being able to configure Rclone to use either of the config files. This seems like it would be a relatively common scenario, so it would be good to support that.

@johrstrom
Copy link
Contributor

Yea it's definitely not totally thought through, I'm just trying to think what's simpler/easiest for use to manage. Seems like we'd have to pass both bits of information around the RCLONE_CONFIG as well as the name?

@robinkar
Copy link
Contributor Author

Yea it's definitely not totally thought through, I'm just trying to think what's simpler/easiest for use to manage. Seems like we'd have to pass both bits of information around the RCLONE_CONFIG as well as the name?

It feels like it in the end, that way would end up being more complex.
Coupling together and passing around RCLONE_CONFIG with the remote makes sense, although at some point some temporary file would have to be created, where both of those remote configurations exist, for executing the Rclone commands where two remotes are used.

@johrstrom
Copy link
Contributor

I know this is a little bit stale - I'm just thinking it through a bit on how we can enable your feature in a way that's easy to read/maintain.

@robinkar
Copy link
Contributor Author

Another potential solution that is somewhat simpler than this would be to just read the env vars directly from some file instead of having the format of the file as an Rclone config file. So it could either be a .yml or an env file that could be "sourced".

end

def rclone_popen(*args, stdin_data: nil, &block)
def rclone_popen(*args, stdin_data: nil, env: extra_config, **kwargs, &block)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see **kwargs being used. I assume it needs to be there, but I feel like they're being dropped if they are defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not needed in rclone_popen, only in the rclone method. Removed it.

Copy link
Contributor

@johrstrom johrstrom left a comment

Choose a reason for hiding this comment

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

OK - let's move forward with this. I guess the comment on the singleton method is the best we can do.

@johrstrom johrstrom merged commit ad71a83 into OSC:master Sep 5, 2023
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants