Skip to content

Commit

Permalink
feat: convert dependencies to use workspaces
Browse files Browse the repository at this point in the history
Update package description and some dependencies to use `workspace`
inheritance.

* Also updates circleci to produce dockerhub image.

Closes #1461
Issue #1362

* f touch to retry circleci

* f try to pre-emptively lock protobuf to the older 2.25.2 version

* f force Cargo.lock to use protobuf 2.25.2, dammit

* f move protobuf pin to syncstorage-spanner

* f revert cargo.lock

* f revert #448

* f update base64 / tickle circleci

* f update cadence (and kick circleci)

* f clippy

* f switch to latest google-cloud-rust

* f update to use google-cloud-rust
  • Loading branch information
jrconlin authored Aug 29, 2023
1 parent ef0fbfb commit 1f9323b
Show file tree
Hide file tree
Showing 31 changed files with 729 additions and 424 deletions.
5 changes: 4 additions & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ jobs:
command: |
export UTILS_DOCKERHUB_REPO=mozilla/sync-spanner-py-utils
if [ "${CIRCLE_BRANCH}" == "master" ]; then
DOCKER_TAG="${CIRCLE_SHA1}"
DOCKER_TAG="latest"
fi
if echo "${CIRCLE_BRANCH}" | grep '^feature\..*' > /dev/null; then
Expand Down Expand Up @@ -428,6 +428,9 @@ workflows:
filters:
tags:
only: /.*/
branches:
only: master
# touch: 1676417203
- deploy-python-utils:
requires:
- mysql-e2e-tests
Expand Down
Loading

12 comments on commit 1f9323b

@ThatOneCalculator
Copy link

Choose a reason for hiding this comment

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

Are you psychic?? I was just talking about this repo when you made this commit... https://firefish.social/notes/9j0cu5pnoibiqwr8

@jrconlin
Copy link
Member Author

Choose a reason for hiding this comment

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

Heh, no. Just able to do some cleanup.

Working on a bit more cleanup, and we've got a ticket for fixing up the crate dependencies this quarter.

@AndresJ551
Copy link

Choose a reason for hiding this comment

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

@jrconlin
I'm sorry to use this to ask a somewhat unrelated question...
I'm looking into https://hub.docker.com/r/mozilla/syncstorage-rs and got the ENV "SYNC_DATABASE_URL" but that is pretty much it... I have the feeling that "TOKENSERVER_DATABASE_URL" is missing and that's why my Firefox browser throws an error when... Calling the token server.
Any idea who can I ask what are the variables?
Thanks!

@jrconlin
Copy link
Member Author

Choose a reason for hiding this comment

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

I'll admit, syncstorage-rs isn't my primary server (I mostly work on autopush / autopush-rs)
I know enough about it that I can do maintenance and can do some checks.

With the new rust server, there are two ways that you can specify configuration options. Either by a prefixed environment variable or a stand-alone configuration file. It's a bit more complicated since we rolled the Tokenserver into Sync. Having them as two apps was a bit silly since they both are heavily interdependent.

The config file name can be specified with the --config option, which uses the config crate. (yes, I know I'm probably going into more details than you want. I'm kinda hoping that this is a first draft for a new page about setting up those variables.)

It's a bit more complicated for the incorporated tokenserver since that prefix is now SYNC_TOKENSERVER, followed by a double underscore (__) and the name of the option.

In your case, you probably want SYNC_TOKENSERVER__DATABASE_URL="mysql://user:pass@host:3306/tokenserver" where
SYNC_TOKENSERVER is the prefix, the double underline __ and DATABASE_URL is one of the tokenserver arguments (see the list of settings in the code.
You add more __ breaks when going down additional structures,
(e.g. SYNC_TOKENSERVER__FXA_OAUTH_PRIMARY_JWK__KTY="RSA" to set the JWK variables. )

For the syncserver, most of those options have the simple SYNC_ prefix (note, single _).

For lots of reasons, there are three groups of settings currently. There are the syncserver-settings which are the general settings needed to get the server running (e.g. SYNC_HOST=0.0.0.0).
There are the syncstorage-settings which are about how you connect up to the database. (e.g. SYNC_SPANNER_EMULATOR_HOST="http://localhost:9020)
And there are the tokenserver settings mentioned above with the weird separator, because those are going to a sub-section of the settings. (Some of this is driven by how rust's config crate wants to do things.)

FWIW, I usually use a config file for all of this since those are generally easier to deal with than tons and tons of Environment variables.

(Have I mentioned that I've run out of environment memory on some systems? Because that's a thing that can happen.)

@AndresJ551
Copy link

Choose a reason for hiding this comment

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

@jrconlin
Thanks for the insights!
I feel I'm almost there, but still no luck with the deploy.
I'm deifying "SYNC_TOKENSERVER__DATABASE_URL" as a different table than "SYNC_DATABASE_URL"
The token server is no populating the tables/rows correctly, but the sync server does...
This is the Firefox "about:sync-log" error:

1693404071786	FirefoxAccounts	DEBUG	_updateAccountData with items: ["oauthTokens"]
1693404071786	FirefoxAccounts	DEBUG	writing plain storage: ["email","sessionToken","uid","verified","device","encryptedSendTabKeys","oauthTokens","profileCache"]
1693404071786	FirefoxAccounts	TRACE	starting write of json user data: ["email","sessionToken","uid","verified","device","encryptedSendTabKeys","oauthTokens","profileCache"]
1693404071786	Services.Common.TokenServerClient	DEBUG	Beginning OAuth token exchange: https://*********************/token/1.0/sync/1.5
1693404071787	Services.Common.RESTRequest	DEBUG	GET request to https://******************/token/1.0/sync/1.5
1693404071788	FirefoxAccounts	TRACE	finished write of json user data - took: 2
1693404071788	FirefoxAccounts	DEBUG	writing secure storage: ["scopedKeys","kSync","kXCS","kExtSync","kExtKbHash"]
1693404071788	FirefoxAccounts	TRACE	starting write of user data to the login manager
1693404071791	FirefoxAccounts	TRACE	finished write of user data to the login manager
1693404071813	Services.Common.RESTRequest	DEBUG	GET https://*******************/token/1.0/sync/1.5 401
1693404071814	Services.Common.TokenServerClient	DEBUG	Got token response: 401
1693404071814	Services.Common.TokenServerClient	WARN	Invalid JSON returned by server: Err: Invalid Authorization

The heartbeat is responding with:
{"status":"Ok","database":"Ok","version":"0.2.5"}
At this point I'm clueless and don't know Rust enough to make a crate config file.

Let me know if there is a better place to post this questions and thanks again!

@jrconlin
Copy link
Member Author

Choose a reason for hiding this comment

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

AH! I think because (in our case at least) we migrated the database as a whole. That means that the rust tokenserver presumed one was there and doesn't bother trying to create one.

Give me a bit and I'll try and get the schema to create one (it might also be buried in the code somewhere, but I can't check right now.)

@jrconlin
Copy link
Member Author

Choose a reason for hiding this comment

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

I did find these migrations which you should be able to use to get to the latest iteration of the table.

@AndresJ551
Copy link

Choose a reason for hiding this comment

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

You sir are a godsend.
I will report back with the results.

@AndresJ551
Copy link

Choose a reason for hiding this comment

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

@jrconlin
Still no luck, I have the database with the tables for the token server, but having the same messages at the about:sync-log
I have the feeling the "SYNC_PUBLIC_URL" is not the correct ENV.
Here is the complete list:

  • SYNC_DATABASE_URL
  • SYNC_HOST
  • SYNC_MASTER_SECRET
  • SYNC_PUBLIC_URL
  • SYNC_TOKENSERVER__DATABASE_URL
  • SYNC_TOKENSERVER__RUN_MIGRATIONS

Maybe some of those have changed or I'm missing one?

@jrconlin
Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, so I collected up the core settings and added them to a wiki page which should help.
To your question, SYNC_DATABASE_URL should become SYNC_SYNCSTORAGE__DATABASE_URL
SYNC_PUBLIC_URL is no longer used.

@AndresJ551
Copy link

Choose a reason for hiding this comment

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

@jrconlin
Again, thanks for all the help!
I have now defined "SYNC_TOKENSERVER__ENABLED" as "true" since it seems logical to have it enabled, but had no change on FF logs, still same errors.
I changed as you suggested from "SYNC_DATABASE_URL" to "SYNC_SYNCSTORAGE__DATABASE_URL" but now the container doesn't start and throws the error:

Bad configuration: "missing field `database_url`"
Please set in config file or use environment variable.
For example to set `database_url` use env var `SYNC_DATABASE_URL`
Error: configuration property "missing field `database_url`" not found

@jrconlin
Copy link
Member Author

Choose a reason for hiding this comment

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

Well, there's at least one bug. SYNC_DATABASE_URL is definitely wrong, and that's a hand-built error message, so we ought to change that.

We do specify the setting for our internal dev environment as SYNC_SYNCSTORAGE__DATABASE_URL, and it all starts up and the /__lbheartbeat__ reports it as OK. Maybe the environment parameter is getting dropped or changed as it passes through the docker interface? 🤷🏻‍♂️

If I get time later, I can try poking around a bit more at this.

Please sign in to comment.