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

[WFCORE-5740] Add the ability to ignore unavailable realms for a distributed-realm #454

Merged
merged 5 commits into from
May 26, 2023

Conversation


== Requirements

=== Hard Requirements

Choose a reason for hiding this comment

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

What about (a switch enabling) emitting SecurityRealmUnavailableEvent to the corresponding SecurityDomain, like in case of failover-realm? See https://github.com/wildfly/wildfly-proposals/pull/261/files#diff-91dfd08fb2d5ac316c73e22e73a78b8816d43bc21c0cd707a6ba075fb8a65f9bR64

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @OndrejKotek , thank you for your review. To be honest I'm not sure if adding another attribute isn't a bit out of the issue scope.

Copy link

@OndrejKotek OndrejKotek Feb 8, 2022

Choose a reason for hiding this comment

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

Do you mean the event will be emitted, no option to disable it? Are there some requirements from the user side (as this RFE was brought by users AFAIK)? My suggestion aims to keep consistency with the failover-realm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @OndrejKotek , added.

=== Non-Requirements

== Test Plan
Tests will be added to the Elytron testsuite.

Choose a reason for hiding this comment

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

What about integration tests in WildFly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OndrejKotek Added

=== Testing By
[ ] Engineering

[x] QE
Copy link
Contributor

Choose a reason for hiding this comment

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

@lvydra Since you'll be writing tests for the WildFly Elytron / Core / WildFly testsuites, we can select Engineering here.

=== Hard Requirements

* It should be possible to configure an ```distributed-realm``` with the optional attribute ```ignore-unavailable-realms``` which
allow users to specify that the search should continue on to the next realm if a realm happens to be unavailable.
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to mention that the default value for the ignore-unavailable-realms attribute will be false.

WildFly integration tests will be added. Tests also will be added to the Elytron testsuite.

== Community Documentation

Copy link
Contributor

Choose a reason for hiding this comment

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

The Elytron documentation can be found here:
https://docs.wildfly.org/26/WildFly_Elytron_Security.html

The source for the documentation is in the WildFly repo:
https://github.com/wildfly/wildfly/tree/main/docs/src/main/asciidoc/_elytron

In the "Community Documentation" section, we should mention that documentation about the new attribute will be added to an appropriate section of the Elytron docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

A "Release Note" content section should also be added. As explained in design-doc-template.adoc, this section just includes a couple sentences that could be used when drafting the release notes for a release that includes this new feature.

allow users to specify that the search should continue on to the next realm if a realm happens to be unavailable.

When ```ignore-unavailable-realms``` is set to true and realm happens to be unavailable, SecurityRealmUnavailableEvent will be emitted to the corresponding SecurityDomain.
This can be turned off by adding `emit-events` attribute and setting it to `false`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding this in! It would be good to clarify here that emit-events is a new, optional attribute that will be added and we should also specify what it defaults to.

Copy link
Contributor

@fjuma fjuma left a comment

Choose a reason for hiding this comment

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

Thanks very much for working on this @lvydra! I've added some small comments.

@lvydra
Copy link
Contributor Author

lvydra commented Feb 10, 2022

Thanks for review @fjuma , updated.


== Release Note Content

* New Distributed Realm attribute ```ignore-unavailable-realms```enables user to switch to ignore unavailable realms during search and continue searching in subsequent realms.

Choose a reason for hiding this comment

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

ignore-unavailable-realmsenables

Missing space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @OndrejKotek, updated.

Copy link
Contributor

@fjuma fjuma left a comment

Choose a reason for hiding this comment

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

Thanks for the updates @lvydra, looks good!

@darranl darranl merged commit 5c8ec6a into wildfly:main May 26, 2023
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.

4 participants