-
Notifications
You must be signed in to change notification settings - Fork 580
Add Imds data retriever as a service provider #8928
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
Conversation
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.
One point of clarification and one important typo fix.
@@ -49,8 +54,7 @@ public static boolean imdsAvailable(OciConfig config) { | |||
Duration timeout = config.imdsTimeout(); | |||
|
|||
try { | |||
URI imdsUri = config.imdsBaseUri() | |||
.orElse(IMDS_URI); | |||
URI imdsUri = imdsUri(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.
IIUC we want developers to use the IMDS provider (added in this PR) which will return an Optional<ImdsInstanceInfo>
. The provider nicely caches the info using LazyValue
.
With that in mind, do we still need HelidonOci.imdsAvailable
to be public
? Do we even need that method any more?
Do we even need this class itself to be public
now that the provider is added and that's what we want developers to use?
It's a bit confusing to have this additional public method, and doing so allows developers to trigger an access to the IMDS endpoint each time they check if IMDS data is available (given the provider they should not need to do this but, given the visibility of the class and method, they can) and then once more if they retrieve the data from the provider.
Can this method (if we need it at all) and this entire class become package private now?
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.
Really good point, however can we defer this question when @tomas-langer comes back from vacation as he recently added this class and that method as a public api? There might be a scenario where imdsAvailable() may be very useful without having to request ImdsInstanceInfo
from the service registry. Maybe, some may just need it to simply determine if they are running on an OCI Instance. If this is a relevant API, we can probably optimize it by caching the result of the 1st successful query to Imds, such that the next time the API is called, that it will not query anymore.
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, let's wait for Tomas to respond.
As implemented, the public imdsAvailable
method accesses the endpoint and retrieves the data. You're right, in the future the implementation might change so it does not do so. But as it stands the code will potentially retrieve the data multiple times.
And, yes, HelidonOci
could cache the retrieved data...but then both it and the provider would be caching it separately. We ought to cache it only once. And because the current imdsAvailable
method accepts an OciConfig
instance, would we need to cache possibly multiple instances of the retrieved data which could vary because of differences from one OciConfig
instance to the next?
Also, with that in mind, I noticed that the provider's constructor accepts an OciConfig
instance and sets up the lazy value using it.
Is there a valid use case with multiple instances of the provider, each with a different OciConfig
value? If so--and my ignorance of the new service provider infrastructure is about to show here--what would it mean to have multiple provider instances (each with different OciConfig
settings) and how would the service provider mechanism differentiate among them?
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.
There might be a scenario that multiple instances will be in play, but I apologize that I cannot give an example. There might be specific service provider where it makes sense to use multiple instances. However, my plan, specific to this service provider is to use GlobalServiceRegistry
so there is only a single global instance to use like how Tomas implemented it for secret-service module.
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.
My concern is not about multiple service registry instances but about multiple provider instances in the same service registry for one type--ImdsInstanceInfo
--but each created with a different OciConfig
instance.
It sounds like we expect only one instance of this particular provider to exist but if, somehow, more than one gets created the cached value will reflect only the first OciConfig
passed to the constructor. And I do not know how the service registry mechanism handles such a case.
This does not seem like something that should hold up this PR, but at some point we should clear this up.
For this PR, given that imdsAvailable
current does rely on fetching the data, maybe that method should use the MidsInstanceInfo
service to retrieve the data, thus ensuring we only fetch it once.
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 think a good example of where multiple instances will be created is regionProvider, because it has regionProviderConfig, and regionProviderSdk and RegionProviderAuthenticationMethod. As for ImdsInfoProvider, it only has that one provider, so it should not create multiple.
Your suggestion makes sense, however, imdsAvailable is called by another provider and I'm not sure if we run the risk of having circular dependency. Maybe not, but can we consider it a good optimization to do in the next iteration as that might require more testing.
Description
Add an Imds data retriever that provides some relevant fields related to the Instance