-
Notifications
You must be signed in to change notification settings - Fork 566
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
4.x: Adjusts CDI OciExtension to use runtime OciExtension for certain authentication tasks #7373
Conversation
The change in ordering looks like a gratuitous incompatibility. Is there a justification for changing the ordering? I think our principle should be to stay compatible unless we have a justification to break compatibility. |
I see two broad approaches:
I'm happy to make any changes that the community wants. |
@tomas-langer Thanks for the approval. Do you have any feedback on the behavior change? Or is your feedback: "the behavior change is fine"? |
Pausing this PR per request until changes are made to the "runtime" |
…rtain authentication tasks Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
d108b24
to
f56c827
Compare
Unpausing/undrafting now that, per consensus, I've amended this PR to make requested calls to newly available methods in the runtime OCI extension. |
TypeAndQualifiers serviceClient = serviceTaqs.serviceClient(); | ||
installServiceClientBuilder(event, bm, serviceClientBuilder, serviceClient, this.lenientClassloading); | ||
installServiceClient(event, bm, serviceClient, serviceTaqs.serviceInterface(), serviceClientBuilder); | ||
if (!this.serviceTaqs.isEmpty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test to ensure we are falling back to microprofile config?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like:
// in a unit test somewhere
io.helidon.config.Config c = toHelidonConfig(ConfigProvider.getConfig());
OciExtension.fallbackConfigSupplier(() -> c);
assertThat(OciExtension.configSupplier().get(), is(c));
…? Or did you have something else in mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, put something into the microprofile-config.properties file, and ensure that it got picked up by your code (which will internally use OciExtension, etc.). This will ensure that the fallback config is working to MP/CDI, and your code does not regress (i.e., confirms it is wired into the right OciExtension etc for auth)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK…I can do this, but isn't this testing the io.helidon.integrations.oci.sdk.runtime.OciExtension
class? Are you sure you want the test of how that class should behave in this Java module? There's no place in my code that could regress in this area (I just hand you a converted MicroProfile Config Config
object and you go from there).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test that hopefully gets at what you're looking for; not sure.
…ations.oci.sdk.runtime.OciExtension class per request Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
…er number of lines Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Per request, this PR adjusts the CDI
OciExtension
to use the runtimeOciExtension
for authentication tasks.(One thing I'd like feedback on is the change in behavior that will result in the absence of any configuration. In the existing CDI
OciExtension
, in the absence of configuration, a simpleAbstractAuthenticationDetailsProvider
(ADP) implementation will be tried first, followed by a configuration file-based ADP attempt, followed by an "instance principals"-based ADP attempt, followed, finally, by a "resource principal"-based ADP attempt. (This is governed by theenum
order and is currently documented).The runtime
OciExtension
changes the ordering.I'm happy to make any changes that need to be made, or to allow this (maybe?) backwards-incompatible change to occur. Just let me know. (If the new ordering is fine, then I'll need to change a lot of Javadoc documentation.)
The PR is simple. The bulk of it simply checks (as before) to see if the user has registered any custom CDI beans for certain pieces (very unlikely, but if she has, we must respect them). (In the "sunny day" case this will of course not happen.) Then, in this case, which will be the ordinary case, as requested I've changed things so that the runtime
OciExtension
'sociAuthenticationProvider
method will be called and its return value will be used.