[WIP] Switch from secrets to rails credentials #253

jrafanie · 2024-11-05T17:40:20Z

UPDATE: The code has been updated to use fallbacks of default values in tests so CI and users don't need to actually specify Rails.application.credentials unless they are recording new cassettes.

TODO: Verify we can rerecord the cassettes using this PR.

Here's what you have to do:

To run the tests, do nothing, we have defaults specified to match the cassettes.

To record updated cassettes:

Apply the commit here to switch to credentials
Edit rails credentials in the rails application (manageiq) in the test environment:
IMPORTANT: cd to manageiq and not the rails engine directory. It looks for the encryption key and encrypted
credentials file relative to the rails application, not the engine.

EDITOR=vi be rails credentials:edit --environment test

Specify the real values in your editor(change the values below):

autosde:
  appliance_host: autosde-appliance-host
  site_manager_user: autosde
  site_manager_password: change_me

Save this. It should generate the following files in the rails app:

config/credentials/test.key (if not previously created)
an encrypted credentials file in config/credentials/test.yml.enc

Both files should be .gitignored.

You can now run record updated cassettes.

agrare · 2024-11-06T18:44:41Z

@jrafanie yeah we need some way to have default credentials for vcrs but be able to override that in order to re-record the VCRs using real credentials.

jrafanie · 2024-11-06T19:16:13Z

@jrafanie yeah we need some way to have default credentials for vcrs but be able to override that in order to re-record the VCRs using real credentials.

Default credentials feels like something that's separated from rails credentials. You could theoretically run EDITOR=vi be rails credentials:edit --environment test to set defaults like I did above, then ship the config/credentials/test.key and config/credentials/test.yml.enc where the .key is like our v2_key and the .yml.enc is the encrypted yaml of the credentials for use in CI or similar.

There are problems with this though. You wouldn't be able to run the tests at all without first following those steps. I wonder if it makes more sense changing usage of Rails.application.secrets or Rails.application.credentials to have a default value if nil in the code/test that uses it so you only need it when re-recording VCRs.

In other words:

Rails.application.credentials.dig(:autosde, :appliance_host) || "autosde-appliance-host"

jrafanie · 2024-11-06T19:50:59Z

In other words:

Rails.application.credentials.dig(:autosde, :appliance_host) || "autosde-appliance-host"

I'll just do it as additional commits... it's easier to talk about code once it's in front of you

jrafanie · 2024-11-07T16:39:25Z

spec/spec_helper.rb

+  }
+  (Rails.application.credentials.autosde_defaults || defaults).each do |key, value|
+    config.define_cassette_placeholder(key) do
+      Rails.application.credentials.dig(:autosde, key) || value


Note, I don't know exactly what it's doing here and realize we have the methods just below... but this needs to work in two scenarios:

User or CI with no Rails credentials defined:

no config/master.key, no config/credentials.yml.enc

Or based on env, no config/credentals/test.key or config/credentials/test.yml.enc

User or CI with rails credentials defined with the same format as seen here.

In other words, if someone can run this with actual credentials to update cassettes, we can refactor it to minimize the duplication. I didn't want to change more than I had to.

Since we won't have Rails.application.credentials.autosde_defaults I think we can simplify this to just defaults.each the rest looks right 👍 use a credentials value if it exists otherwise use the default value.

I don't know exactly what it's doing here

This is what will replace the "real" secret values with the default values when writing out the vcr file.

Ok, I'll push that change. Thanks for the information. It didn't look they you'd change the autosde_defeaults in secrets but wasn't sure if we'd change our settings in the future.

jrafanie · 2024-11-07T16:47:23Z

@agrare @nasark No rush but if one of you is able to rerecord cassettes, it would be great if you could verify the process I listed above where you run rails credentials:edit in the main app repo to set the correct actual credentials works when you try to update the cassettes.

jrafanie · 2024-11-07T16:49:47Z

spec/models/manageiq/providers/autosde/storage_manager/refresher_v2_spec.rb

@@ -4,10 +4,10 @@
  let(:ems) do
    FactoryBot.create(:autosde_storage_manager,
                      :with_autosde_credentials,
-                      :hostname => Rails.application.secrets.autosde[:appliance_host])
+                      :hostname => credentials_autosde_host)


I needed to change this and realized the test file's name was incorrect and was never run in CI or locally unless you specifically ran the path to the file.

jrafanie · 2024-11-07T16:50:12Z

spec/models/manageiq/providers/autosde/storage_manager/refresher_v2_spec.rb

  end

-  describe "#refresh - autosde gem v2" do
+  xcontext "#refresh - autosde gem v2 - TODO: Did this ever work? Committed with incorrect filename in 23db5ed" do


Disabled the whole test for now since it's not been run, see above.

agrare · 2024-11-12T14:45:42Z

spec/spec_helper.rb

+    "appliance_host"        => "autosde-appliance-host",
+    "site_manager_user"     => "autosde",
+    "site_manager_password" => "change_me"


If you didn't want to "duplicate" the values you could always

Suggested change

"appliance_host" => "autosde-appliance-host",

"site_manager_user" => "autosde",

"site_manager_password" => "change_me"

"appliance_host" => credentials_autosde_host,

"site_manager_user" => credentials_autosde_user,

"site_manager_password" => credentials_autosde_password

Yeah, I guess I wanted to see what the usage was down below to better understand if i could eliminate it entirely and do it inline.

jrafanie · 2024-11-12T17:57:53Z

Ok, removing WIP. I still need to record updated cassettes using this procedure but I think all of the concerns raised here have been addressed.

agrare · 2024-11-12T18:52:41Z

@jrafanie and just to make sure I understand how the new credentials system works would we have to use rails credentials:edit --environment test here? Can that be per-plugin or is it for the whole app? We should probably include something in the plugin/providers developers guide on how to re-record vcrs

jrafanie · 2024-11-12T19:31:09Z

@jrafanie and just to make sure I understand how the new credentials system works would we have to use rails credentials:edit --environment test here? Can that be per-plugin or is it for the whole app? We should probably include something in the plugin/providers developers guide on how to re-record vcrs

rails credentials are for the rails application so I guess you'd have some .gitignore(d) credentials for each plugin and they'd reside in config/credentials/ so several in the same encrypted file. I need to add the .gitignore for the main app. I'll do that next.

Fryguy · 2024-11-12T19:56:03Z

We also need to update the plugin generator.

For some reason I'm not a fan of the defaults in the code as opposed to the YAML file, but in the end it doesn't make much difference, and I won't let that hold up a merge. In the end as long as @agrare is happy then I'm happy 😅

jrafanie · 2024-11-12T20:05:46Z

We also need to update the plugin generator.

Yes, it's on the list. I wanted to get a concrete example done and then it's on to all the other places we use secrets. I'm going to try to find an environment I can try to rerecord cassettes at some point.

For some reason I'm not a fan of the defaults in the code as opposed to the YAML file, but in the end it doesn't make much difference, and I won't let that hold up a merge. In the end as long as @agrare is happy then I'm happy 😅

I don't like it either. One alternative is requiring CI/devs create their credentials for use with the test but that seems cumbersome.

Fryguy · 2024-11-12T20:08:22Z

However I would like to have the "real" secrets gitignored ahead of time in core.

jrafanie · 2024-11-12T20:46:23Z

However I would like to have the "real" secrets gitignored ahead of time in core.

On the way...

We're switching to rails credentials to keep current with rails 7.1 and the future. Note, we're assuming we won't want to commit and share encrypted credentials. If we want to share them, such as for recording cassettes, the comments describe how to switch to only ignoring the plain text encryption key files. Followup to: ManageIQ#23254 Required for: ManageIQ/manageiq-providers-autosde#253

jrafanie · 2024-11-12T21:18:39Z

@agrare @Fryguy see: ManageIQ/manageiq#23263

We're switching to rails credentials to keep current with rails 7.1 and the future. Note, we're assuming we won't want to commit and share encrypted credentials. If we want to share them, such as for recording cassettes, the comments describe how to switch to only ignoring the plain text encryption key files. Followup to: ManageIQ#23254 Required for: ManageIQ/manageiq-providers-autosde#253

Here's what you have to do: 1) To run the tests, do nothing, we have defaults specified to match the cassettes. To record updated cassettes: 1) Apply the commit here to switch to credentials 2) Edit rails credentials in the rails application (manageiq) in the test environment: IMPORTANT: cd to manageiq and not the rails engine directory. It looks for the encryption key and encrypted credentials file relative to the rails application, not the engine. EDITOR=vi be rails credentials:edit --environment test Specify the real values in your editor(change the values below): autosde: appliance_host: autosde-appliance-host site_manager_user: autosde site_manager_password: change_me Save this. It should generate the following files in the rails app: * config/credentials/test.key (if not previously created) * an encrypted credentials file in config/credentials/test.yml.enc Both files should be .gitignored. You can now run record updated cassettes.

It's a common pattern in ruby to run *_test.rb for test/unit/minitest, *_spec.rb for rspec, etc.

The spec file was named incorrectly in 23db5ed as: spec/models/manageiq/providers/autosde/storage_manager/refresher_spec_v2.rb Spec files should match the glob pattern "*_spec.rb". We need to come back and make these tests work since the file has been renamed correctly. Before: ``` Finished in 7.6 seconds (files took 3.76 seconds to load) 35 examples, 0 failures ``` After: ``` Finished in 8.35 seconds (files took 4.11 seconds to load) 44 examples, 0 failures, 9 pending ```

Rails 7.1 removes access to modifying secrets as we should be moved over to rails credentials. Here we describe how to setup these rails crednetials for the purpose of recording VCR cassettes. See also: ManageIQ/manageiq-providers-autosde#253

jrafanie · 2024-11-13T16:51:07Z

Leaving as wip until I can run through recording cassettes along with the updated guide: ManageIQ/guides#553

Based on changes from: ManageIQ/guides#553 ManageIQ/manageiq-providers-autosde#253

jrafanie added wip rails7.1 labels Nov 5, 2024

jrafanie requested review from agrare, Fryguy, ereztheodorou, erantzabari, llivne and gregoryb-at-ibm as code owners November 5, 2024 17:40

jrafanie force-pushed the switch-from-secrets-to-credentials branch 2 times, most recently from 55d3ba3 to 733c49d Compare November 6, 2024 19:29

jrafanie force-pushed the switch-from-secrets-to-credentials branch from 733c49d to 441d36b Compare November 7, 2024 16:27

jrafanie commented Nov 7, 2024

View reviewed changes

jrafanie force-pushed the switch-from-secrets-to-credentials branch from 441d36b to 539dc2b Compare November 7, 2024 16:52

Fryguy changed the title ~~WIP: Switch from secrets to rails credentials~~ [WIP] Switch from secrets to rails credentials Nov 11, 2024

agrare reviewed Nov 12, 2024

View reviewed changes

jrafanie force-pushed the switch-from-secrets-to-credentials branch 2 times, most recently from 5285fed to e3ea008 Compare November 12, 2024 17:56

jrafanie changed the title ~~[WIP] Switch from secrets to rails credentials~~ Switch from secrets to rails credentials Nov 12, 2024

jrafanie removed the wip label Nov 12, 2024

agrare self-assigned this Nov 12, 2024

jrafanie mentioned this pull request Nov 12, 2024

Add gitignores for rails credentials ManageIQ/manageiq#23263

Merged

jrafanie mentioned this pull request Nov 13, 2024

Support rails 7.1 ManageIQ/manageiq#23200

Open

43 tasks

jrafanie force-pushed the switch-from-secrets-to-credentials branch from e3ea008 to a914a63 Compare November 13, 2024 16:33

jrafanie added 3 commits November 13, 2024 11:36

The default runs *_spec.rb for spec, so this test never ran!

2bd0162

It's a common pattern in ruby to run *_test.rb for test/unit/minitest, *_spec.rb for rspec, etc.

jrafanie force-pushed the switch-from-secrets-to-credentials branch from a914a63 to eedd571 Compare November 13, 2024 16:36

jrafanie changed the title ~~Switch from secrets to rails credentials~~ [WIP] Switch from secrets to rails credentials Nov 13, 2024

jrafanie added the wip label Nov 13, 2024

jrafanie mentioned this pull request Nov 13, 2024

[WIP] Use Rails credentials instead of deprecated and removed secrets ManageIQ/guides#553

Open

jrafanie added a commit to jrafanie/manageiq that referenced this pull request Nov 13, 2024

Update plugin generator to switch from rails secrets to credentials

07c0181

Based on changes from: ManageIQ/guides#553 ManageIQ/manageiq-providers-autosde#253

This was referenced Nov 13, 2024

[WIP] Update plugin generator to switch from rails secrets to credentials ManageIQ/manageiq#23266

Open

[WIP] Use rails credentials instead of secrets ManageIQ/manageiq-providers-vmware#928

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Switch from secrets to rails credentials #253

[WIP] Switch from secrets to rails credentials #253

jrafanie commented Nov 5, 2024 •

edited

Loading

agrare commented Nov 6, 2024

jrafanie commented Nov 6, 2024 •

edited

Loading

jrafanie commented Nov 6, 2024

jrafanie Nov 7, 2024

jrafanie Nov 7, 2024

agrare Nov 12, 2024

agrare Nov 12, 2024

jrafanie Nov 12, 2024

jrafanie commented Nov 7, 2024

jrafanie Nov 7, 2024

jrafanie Nov 7, 2024

agrare Nov 12, 2024

jrafanie Nov 12, 2024

jrafanie commented Nov 12, 2024

agrare commented Nov 12, 2024

jrafanie commented Nov 12, 2024 •

edited

Loading

Fryguy commented Nov 12, 2024

jrafanie commented Nov 12, 2024

Fryguy commented Nov 12, 2024

jrafanie commented Nov 12, 2024

jrafanie commented Nov 12, 2024

jrafanie commented Nov 13, 2024

[WIP] Switch from secrets to rails credentials #253

Are you sure you want to change the base?

[WIP] Switch from secrets to rails credentials #253

Conversation

jrafanie commented Nov 5, 2024 • edited Loading

agrare commented Nov 6, 2024

jrafanie commented Nov 6, 2024 • edited Loading

jrafanie commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrafanie commented Nov 12, 2024

agrare commented Nov 12, 2024

jrafanie commented Nov 12, 2024 • edited Loading

Fryguy commented Nov 12, 2024

jrafanie commented Nov 12, 2024

Fryguy commented Nov 12, 2024

jrafanie commented Nov 12, 2024

jrafanie commented Nov 12, 2024

jrafanie commented Nov 13, 2024

jrafanie commented Nov 5, 2024 •

edited

Loading

jrafanie commented Nov 6, 2024 •

edited

Loading

jrafanie commented Nov 12, 2024 •

edited

Loading