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

Match environment input to mirror Gitea's #6

Merged
merged 5 commits into from
Feb 27, 2023
Merged

Match environment input to mirror Gitea's #6

merged 5 commits into from
Feb 27, 2023

Conversation

kousu
Copy link
Member

@kousu kousu commented Feb 24, 2023

See the individual commits!

And probably rebase not squash this!

@kousu kousu requested a review from mguaypaq February 24, 2023 16:43
@kousu kousu mentioned this pull request Feb 24, 2023
@kousu
Copy link
Member Author

kousu commented Feb 24, 2023

I like the names GITEA_ROOT_URL and GITEA_TOKEN because I think the terminology is easier to follow when you're cross-referencing the Gitea docs, but I'm having second thoughts about GITEA_PUBLIC_URL.

If we allow configuring GITEA_PUBLIC_PATH then we have to allow configuring GITEA_PUBLIC_URL, because it could be anywhere: maybe we stick it on a different partition and set up an nginx location /bids-validator-results { root /path/to/other/partition } to glue in that path; maybe we use something like s3fs to stick it up on a CDN and then have to set GITEA_PUBLIC_URL=https://our-bucket.s3.amazonaws.com/bids-validator.

On the other hand, maybe we don't need to allow configuring GITEA_PUBLIC_PATH either. Instead we can set GITEA_WORK_DIR (see ref) and derive GITEA_PUBLIC_PATH from that; we need that anyway, because part of why #5 is still drafted is it needs some way to compute GITEA_REPOSITORY_PATH, which is easy to do from GITEA_WORK_DIR but less reliable to do from GITEA_PUBLIC_PATH.

EDIT: I went with adding GITEA_CUSTOM, which is something Gitea itself reads, and generating the output directory based on that. This assumes that your custom/ will always be, which I think is true unless you're doing some weird nginx/apache hacks. And I think we're okay (are we?) assuming that the servers we're aiming at are not going to grow so large to need anything like that.

This shortens the code, but requires upgrading to go1.19,
which Gitea itself upgraded to recently.
This is easier to get right, since it mirrors Gitea's app.ini.

Gitea also has a STATIC_URL_PREFIX which is similar to what GITEA_PUBLIC_URL
was; but we have little motivation to use it, since we are designing
that bids-hook runs _on the same server_ as gitea. Also, if we did keep
it, I would want to more closely mirror app.ini's config behaviour.
This is an environment variable Gitea also can use.
This isn't an environment variable Gitea uses,
but it is close to a name in its config file:
[repository].ROOT.

The worker will need this to find its input.

This mirrors the rules for that's default value.
@kousu kousu changed the title Combine GITEA_PUBLIC_URL + GITEA_API_URL -> GITEA_ROOT_URL Match environment input to mirror Gitea's Feb 25, 2023
go.mod Show resolved Hide resolved
bids-hook.go Show resolved Hide resolved

# this replicates the default location logic from https://docs.gitea.io/en-us/config-cheat-sheet/
# any setting can be overridden just by setting its variable before calling this script
: ${GITEA_APP_PATH:=../gitea/gitea}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is awkward!

In our deployments, this will be /srv/gitea/gitea. On my dev machine I have ~/src/gitea/ and ~/src/bids-hook.

I don't know. Maybe this should be a mandatory input? But then again, if you on a server where you're explicitly setting GITEA_CUSTOM for gitea, you can also set it for bids-hook and then GITEA_APP_PATH is irrelevant.

Copy link
Member

Choose a reason for hiding this comment

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

I realize now that I didn't mention this before, but: the start script is meant to disappear from the repo at some point! It's just scaffolding for manual, local testing on my own machine (or yours!) while we develop a v1. Hence all the hardcoded values and dummy secret and token.

So actual deployment situations are out of scope for this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. In that case, the part that mirrors Gitea's defaults logic should probably make its way into Go instead of shell. I could probably copy it straight out of their codebase.

And the environment variables that remain would migrate to a systemd .service file? Is it weird to put credentials in a .service file?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like systemd has a whole system in place for credentials. According to System and Service Credentials:

Within unit files, there are four settings to configure service credentials.

  1. LoadCredential= may be used to load a credential from disk, from an AF_UNIX socket, or propagate them from a system credential.
  2. SetCredential= may be used to set a credential to a literal string encoded in the unit file. Because unit files are world-readable (both on disk and via D-Bus), this should only be used for credentials that aren’t sensitive, i.e. public keys/certificates – but not private keys.
  3. LoadCredentialEncrypted= is similar to LoadCredential= but will load an encrypted credential, and decrypt it before passing it to the service. For details on credential encryption, see below.
  4. SetCredentialEncrypted= is similar to SetCredential= but expects an encrypted credential to be specified literally. Unlike SetCredential= it is thus safe to be used even for sensitive information, because even though unit files are world readable, the ciphertext included in them cannot be decoded unless access to TPM2/encryption key is available.

So, systemd wants to make credentials available as files, and currently bids-hook wants to accept credentials as environment variables. I guess I can change the environment variable to contain a path to the secret, rather than the secret itself, and that should be sufficiently flexible for manual testing and also for systemd-controlled execution.

Copy link
Member

@mguaypaq mguaypaq 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! As you said, for a first version it's definitely ok to assume that we're not doing fancy nginx stuff. I don't think it would be hard to support if/when we feel the need, so let's ignore it for now.

@mguaypaq mguaypaq merged commit a37ccf2 into main Feb 27, 2023
@mguaypaq mguaypaq deleted the shorten-env branch February 27, 2023 20:57
kousu added a commit that referenced this pull request May 5, 2023
Gitea is now testing against this, so I think we should too, as in #6.
@kousu kousu mentioned this pull request May 5, 2023
mguaypaq pushed a commit that referenced this pull request May 8, 2023
Gitea is now testing against this, so I think we should too, as in #6.
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.

2 participants