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

Fix problem with authentication token expiration #457

Merged
merged 7 commits into from
Jul 17, 2024

Conversation

lagoan
Copy link
Contributor

@lagoan lagoan commented Jul 8, 2024

Context

We've been having multiple authentication token errors due to invalidation. This PR Fixes this

Related to #315

What's New

  • Set a parameter default value that makes the ruby-openstack gem retry to refresh the auth token.
  • Add an http header to refresh the token on each call.
  • Catch the exception when a token is invalid and recreate the connection.

One of the issues we've seen in production is errors on authentication
due to swift expiring the auth token.

Changes include:

- Set a parameter default value that makes the ruby-openstack gem retry
to refresh the auth token.
- Add a http header to refresh the token on each call.
- Catch the exception when a token is invalid and recreate the
connection.
Copy link
Member

@pgwillia pgwillia left a comment

Choose a reason for hiding this comment

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

This feels like two or three loosely related changes without any additional tests. I'm seeing if I can figure out a way to suggest some tests.

era_container = swift_connection.container(swift_container)
begin
era_container = @swift_connection.container(swift_container)
rescue OpenStack::Exception::ExpiredAuthToken => e
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. However, I could test that exception in our code, not the OpenStack gem, so I left our retry.

I am worried about the gem not being maintained. This us causing an error at the moment.

I am open to removing my code that catches the exception in order to have less things to maintain on our side. Let me know what you think.

deposited_file = era_container.object(file_base_name)
deposited_file.write(File.open(file_name), headers)
else
# for creating new: construct hash with symbols as keys, add metadata as a hash within the header hash
headers = { etag: checksum,
content_type: 'application/x-tar',
metadata: metadata }
metadata: metadata,
'X-Auth-New-Token' => 'true' }
Copy link
Member

Choose a reason for hiding this comment

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

https://stackoverflow.com/a/16525684 applies to swauth, and I can't find mention of it in https://github.com/openstack/swift/ or https://github.com/ruby-openstack/ruby-openstack. Do you know what auth we're using with local or OLRC Swift?

Copy link
Member

Choose a reason for hiding this comment

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

We're using keystone v1 for local swift and keystone v3 for OLRC

@pgwillia
Copy link
Member

pgwillia commented Jul 9, 2024

This feels like two or three loosely related changes without any additional tests. I'm seeing if I can figure out a way to suggest some tests.

Here's part of a test for the retry_auth config
8a0c21c

@lagoan
Copy link
Contributor Author

lagoan commented Jul 11, 2024

This feels like two or three loosely related changes without any additional tests. I'm seeing if I can figure out a way to suggest some tests.

I can add tests that fake the exception coming from the OpenStack gem. I've done it in the past.

Testing with authentication token revoked every second we found the
retry_auth parameter set to true was all that was needed to keep the
openstack gem original behaviour and avoid authentication errors.
Add a simple test to make sure the value for retry_auth is set to true
after setting its value as an initialization parameter
Simplify the test using inspect rather that digging into different
models.

Also remove byebug.
Copy link
Member

@pgwillia pgwillia left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -40,7 +40,7 @@

expect(first_deposit.name).to eq(second_deposit.name)
expect(first_deposit.container.name).to eq(second_deposit.container.name)
end.to change { swift_depositer.swift_connection.container('ERA').count.to_i }.by(1)
end.to change { swift_depositer.instance_variable_get('@swift_connection').container('ERA').count.to_i }.by(1)
Copy link
Member

Choose a reason for hiding this comment

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

I think it was easier to read before, but I agree that having an attribute accessor is unnecessary just for this purpose.

# rubocop:enable Layout/LineLength
end
end

it 'uses ruby-openstack gem behaviour to refresh authentication token' do
VCR.use_cassette('swift_new_deposit') do
Copy link
Member

Choose a reason for hiding this comment

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

For repeatability this might need to be a different tape than swift_new_deposit. Also maybe we should setup the docker container to set the token_life?

auth_url: 'http://127.0.0.1:8080/auth/v1.0',
retry_auth: true)

expect(swift_depositer.inspect).to include '@retry_auth=true' # retry_auth isn't exposed
Copy link
Member

Choose a reason for hiding this comment

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

Alternately, perhaps we include this test in the existing test above?

@lagoan lagoan merged commit 4d39b5a into master Jul 17, 2024
2 checks passed
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