Make some OAuth2 settings optional #12258
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
MarcialRosales
force-pushed
the
make-some-oauth2-settings-optional
branch
3 times, most recently
from
043520f
to
f6dd1e0
Compare
MarcialRosales
force-pushed
the
make-some-oauth2-settings-optional
branch
8 times, most recently
from
ecac15f
to
3a210d7
Compare
MarcialRosales
force-pushed
the
make-some-oauth2-settings-optional
branch
5 times, most recently
from
378bbb2
to
697b5a2
Compare
This was referenced Oct 2, 2024
michaelklishin
requested changes
Oct 4, 2024
There was a problem hiding this comment.
@MarcialRosales we cannot use very generic modules names such as rar and oauth2, or keycloak.
We should continue using the rabbit_oauth2_ prefix in this plugin to avoid potential code path conflicts with external libraries, other plugins, and so on.
|
@MarcialRosales and I have agreed to move the AWS suite to use Make as part of this PR. |
Improve logging Fix an issue running selenium tests locally WIP modify schema to configure queryParameters for oauth2 endpoints
And fix selenium script to run rabbitrmq locally
And location of cert files when running multioauth test suites locally
- Use rabbit_oauth2 prefix for modules which do not have it - Ensure most lines stick to 80 column
MarcialRosales
force-pushed
the
make-some-oauth2-settings-optional
branch
from
0ac9e5f
to
d98eb17
Compare
|
Erlang 27 and AWS peer discovery jobs need some work, they are not related to these changes. |
It was still possible, although rare, to have message store files lose message data, when the following conditions were met: * the message data contains byte values 255 (255 is used as an OK marker after a message) * the message is located after a 0-filled hole in the file * the length of the data is at least 4096 bytes and if we misread it (as detailed below) we encounter a 255 byte where we expect the OK marker The trick for the code to previously misread the length can be explained as follow: A message is stored in the following format: <<Len:64, MsgIdAndMsg:Len/unit:8, 255>> With MsgId always being 16 bytes in length. So Len is always at least 16, if the message data Msg is empty. But technically it never is. Now if we have a zero filled hole just before this message, we may end up with this: <<0, Len:64, MsgIdAndMsg:Len/unit:8, 255>> When we are scanning we are testing bytes to see if there is a message there or not. We look for a Len that gives us byte 255 after MsgIdAndMsg. Len of value 4096 looks like this in binary: <<0:48, 16, 0>> Problem is if we have leading zeroes, Len may look like this: <<0, 0:48, 16, 0>> If we take the first 64 bits we get a potential length of 16. We look at the byte after the next 16 bytes. If it is 255, we think this is a message and skip by this amount of bytes, and mistakenly miss the real message. Solving this by changing the file format would be simple enough, but we don't have the luxury to afford that. A different solution was found, which is to combine file scanning with checking that the message exists in the message store index (populated from queues at startup, and kept up to date over the life time of the store). Then we know for sure that the message above doesn't exist, because the MsgId won't be found in the index. If it is, then the file number and offset will not match, and the check will fail. There remains a small chance that we get it wrong during dirty recovery. Only a better file format would improve that.
|
All suites pass on Erlang 26 and the failures on 27 are well known => merging. |
michaelklishin
approved these changes
Oct 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed Changes
Implements features:
management.metadata_urland move it toauth_oauth2.discovery_endpoint_path#12237It is accompanied by this docs PR rabbitmq/rabbitmq-website#2056
Tasks:
oauth_providerandresource_servertypes rather than asking for each setting to the deprecated config module. Split config module into oauth_provider and resource_server modules.discovery_endpoint_pathanddiscovery_endpoint_paramsoauth_authorization_endpoint_paramsandoauth_token_endpoint_paramsaccess_token_requestanddiscovery_endpointto theoauth_providertype. (TODO the WSR plugin should be updated to read these extra params and pass them to theaccess_token_request)Types of Changes
What types of changes does your code introduce to this project?
Put an
xin the boxes that apply