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

Clean up API key config in config/initalizers/production.rb #1380

Closed
pushcx opened this issue Nov 19, 2024 · 11 comments
Closed

Clean up API key config in config/initalizers/production.rb #1380

pushcx opened this issue Nov 19, 2024 · 11 comments

Comments

@pushcx
Copy link
Member

pushcx commented Nov 19, 2024

Following #1246

We have config/initializers/production.rb for setting API keys and site settings (see config/application.rb for the class Lobsters setup) that vary between the sister sites, but Rails style has long-since changed and I think there's a few new styles for better managing those.

To my comment we should look at reimplementing this config so that Lobsters and sister sites use a single way that plays well with deploying to a VPS, PaaS, and Kamal, and doesn't end up with require statements to kludge around zeitwerk.

It would be especially nice if this cleanup didn't break prod for us or sister sites! And removed the kludge in config/initializers/00_zeitwerk.rb.

(Maybe this is the kind of plumbing issue I can nerdsnipe @byroot into?)

pushcx added a commit that referenced this issue Nov 19, 2024
byroot added a commit to byroot/lobsters that referenced this issue Nov 21, 2024
Ref: lobsters#1380

- I see little point in using a core extension for this.
- It's only used in one place.
- The implementation itself can be simplified a lot.

Not 100% why but fixing this caused the `scripts/*.rb` to start
failing to load. But they were using a non-standard way to load
the application, so I simply cleaned them up too and the issue
disapeared, probably not worth on digging up the root cause.
This was referenced Nov 21, 2024
@byroot
Copy link
Contributor

byroot commented Nov 21, 2024

Could you expand a bit?

Looking at your pointers, I see you are basically forcing eager loading of app/extra in production, but I can't understand which files it's for. Is it for some uncommitted files?

@krydos
Copy link
Contributor

krydos commented Nov 21, 2024

If I may.
Here is related PR #1365 that solved one issue and introduced this one.

There is config/initializers/production.rb.sample file that you should rename to config/initializers/production.rb to see the issue.
As soon as you start rails with rails s -e production it's gonna fail because classes used in that initializer are not loaded yet. These classes are defined in extras folder and this folder is added to config.autoload_paths (see config/application.rb) but the folder's files are loaded AFTER the initializers.

I thought about splitting that initializer into a separate class(es?), move them to same extras folder and do ThatSeparateClass.initialize thing on them in config.after_initialize.
I feel like it may work but I haven't had a chance to try yet.

I hope it gives a bit more context on the issue.

@pushcx
Copy link
Member Author

pushcx commented Nov 21, 2024

After sleeping on it a couple days, I'm pretty sure the root cause is our pattern of having utility classes that an initializer sets class variables on. That starts this whole contention of whether the utility class is loaded before the initializer, which was compounded by not using the autoload_paths properly.

Maybe the better fix would be to have the utility classes (GitHub, Mastodon, Pushover, DiffBot) load their api keys directly from ENV vars? I do like keeping them together in one list, but maybe providing a .env.sample (or similar) would do it. I also prefer not to break sister sites again, but we don't really have a deprecation policy or a good list of them to notify, and I'm willing to do it for a long-term fix we have high confidence in.

@pushcx
Copy link
Member Author

pushcx commented Nov 21, 2024

@byroot Thanks for your two PRs cleaning up extras and lib/monkey.rb, it's nice to tidy up our junk drawers like this.

At this point it seems pretty likely that extras/ could be folded into lib/. There isn't a huge benefit, but there's also not a deep semantic separation that's apparent. Doing so also doesn't get us out of any of this initializer design/organization issue.

@pushcx
Copy link
Member Author

pushcx commented Nov 21, 2024

I was reading up on gmem's PR for Action Mailbox (#1285) and ran across mention of the rails credentials store, which is the feature we predate that we should probably adopt for to replace all this: https://edgeguides.rubyonrails.org/security.html#custom-credentials

@byroot
Copy link
Contributor

byroot commented Nov 21, 2024

Personally, I find Rails credentials quite inconvenient to work with, but I yes, it's the standard solution for this.

Personally I prefer the ergonomics of https://github.com/Shopify/ejson-rails

@pushcx
Copy link
Member Author

pushcx commented Nov 21, 2024

Do you prefer it because you're pulling from some kind of enterprisey shared secret store? I'm not very familiar, but rails credentials seems like the right size solution for Lobsters and ejson-rails looks like more flexibility than we need.

@byroot
Copy link
Contributor

byroot commented Nov 21, 2024

Do you prefer it because you're pulling from some kind of enterprisey shared secret store?

No, I prefer it because while the secrets are encrypted, the structure is still in clear text, so it's much easier to know which keys actually exist etc.

Less important for Lobsters, it also allows users to add encrypted secrets without having access to the decryption key, only the public key is needed.

But I was just mentioning it in passing, you operate the server, that's totally on you to chose whatever seem most convenient to you.

kernal053 pushed a commit to kernal053/lobsters that referenced this issue Nov 25, 2024
kernal053 pushed a commit to kernal053/lobsters that referenced this issue Nov 25, 2024
Ref: lobsters#1380

- I see little point in using a core extension for this.
- It's only used in one place.
- The implementation itself can be simplified a lot.

Not 100% why but fixing this caused the `scripts/*.rb` to start
failing to load. But they were using a non-standard way to load
the application, so I simply cleaned them up too and the issue
disapeared, probably not worth on digging up the root cause.
pushcx added a commit that referenced this issue Nov 26, 2024
@pushcx
Copy link
Member Author

pushcx commented Nov 26, 2024

I migrated us to the rails credentials feature in 92e8dc. Rails doesn't give us a way to templatize the file or give instructions at startup or anything, so the 00_zeitwerk.rb initializer is now different workaround to blows up the rails boot process, because I guess blowing up on boot is the only way to communicate mandatory setup and credential structure.

Of course, this broke the build. I'll add a workaround for the workaround for the incomplete rails feature when I'm less exasperated.

@pushcx
Copy link
Member Author

pushcx commented Nov 27, 2024

I’ve deployed this without hassle.

@pushcx
Copy link
Member Author

pushcx commented Nov 27, 2024

The build is green, I’m closing this.

On the off chance that anyone sees this, the missing functionality from credentials is the ability to provide an initial template for the credentials file, and revise it so that devs + sites get clear messages about config changes rather than obscure NameErrors when the code tries to access missing keys. It doesn’t rise to the level of importance that I am willing to put up with That Guy.

@pushcx pushcx closed this as completed Nov 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants