odsds: fix repeated starts and warming/non-warming overlap - #46473
odsds: fix repeated starts and warming/non-warming overlap#46473kyessenov wants to merge 3 commits into
Conversation
Change-Id: Ibb6a47a6d43e2366d4c49b7425e99ce874e90448 Signed-off-by: Kuat Yessenov <kuat@google.com>
kyessenov
changed the title
Change-Id: I61e7a2cc6937889ffc6de9fc78485eb6b1fb122d Signed-off-by: Kuat Yessenov <kuat@google.com>
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks, overall LGTM, although it is unclear to me why the provider is dependent on the warming status.
| OptRef<Init::Manager> init_manager, bool warm) { | ||
| const std::string map_key = | ||
| absl::StrCat(MessageUtil::hash(sds_config_source), ".", config_name); | ||
| absl::StrCat(MessageUtil::hash(sds_config_source), ".", config_name, warm); |
There was a problem hiding this comment.
It is not clear to me why is the warming status part of the provider key. It seems to be a wrong move, as the provider should not change regardless of the warming state.
Please add a comment explaining this.
There was a problem hiding this comment.
The init target belongs the provider. If warm set, then the init target is marked ready when a secret is fetched, otherwise, the init target is marked ready immediately.
There was a problem hiding this comment.
Added a comment. The alternate way is to add two init targets per provider. Do you want that option to be implemented?
There was a problem hiding this comment.
I don't understand why the warming and non-warming have 2 different init-targets, and not just one.
This is my understanding so feel free to correct me: When a resource is added it may be added in an "active-state" if all of its dependencies are ready, or in a "warming-state" as it waits for its dependencies. Once its dependencies are ready, it will become "active", and notify anything that was dependent on it.
The resource should be the same, regardless of its warming/active state.
It may be that "warm" in this case has a different meaning.
There was a problem hiding this comment.
This for the "prefetch mode". A parent resource (listener) initiates the on-demand request because we anticipate an on-demand request on the data path. So it needs to register into the parent resource init manager as if it were warming, but it does not block the parent resource init manager, since it's on-demand.
There was a problem hiding this comment.
Sorry, I might be missing something in the design here.
Is the issue that a listener can have both on-demand and non-on-demand secret paths? If so, I assume the listener only waits for the non-on-demand secrets, correct?
Also the on-demand request should only unblock the data-path request, no? why does it need to have an init-manager?
There was a problem hiding this comment.
This is a bit tricky, consider the following setup:
Listener 1 { using regular SDS secret X }
Listener 2 { using on-demand discovery using a discovered name X }
Listener 3 { using on-demand discovery with a prefetched name X }
The behavior is as follows:
| Listener | When does SDS for X start | What is blocked on SDS for X fetch |
|---|---|---|
| Listener 1 | LDS load | LDS start |
| Listener 2 | Connection accepted | Connection proceeds |
| Listener 3 | LDS load | Connection proceeds |
To handle listener 3, we use its init manager to start the fetch for SDS. But we do not block, so cannot re-use the subscription from the listener 1.
There was a problem hiding this comment.
The requesting but non-blocking is an interesting use-case. Do you have links to where/when this was added? I want to learn more about the design there in order to give a better response.
There was a problem hiding this comment.
| EXPECT_CALL(*subscription_factory_.subscription_, start(_)); | ||
| sds_api.start(); | ||
| // Validate that starting twice only calls subscription start once. | ||
| sds_api.start(); |
There was a problem hiding this comment.
How does this validate, only via logs?
In this case it would be better to use mocks to make sure that it is only invoked once. Otherwise, this can be removed.
There was a problem hiding this comment.
There is an expectation mock on line 104.
| config_source, "abc.com", secret_context.server_context_, {}, true); | ||
|
|
||
| EXPECT_NE(secret_provider2, secret_provider3); | ||
| } |
There was a problem hiding this comment.
I'm assuming that there isn't an integration test that can test the regression, but if you are aware of one, consider add it.
There was a problem hiding this comment.
The only visible effect of the first bug is the noise about the init fetch failures. We can try to insert an assert into gRPC subscription, but that might trigger under other circumstances.
Change-Id: I12bb189a4da6c982e9f89707aeb74086c52ce6a0 Signed-off-by: Kuat Yessenov <kuat@google.com>
2 participants
Change-Id: Ibb6a47a6d43e2366d4c49b7425e99ce874e90448
Commit Message: Fix repeated gRPC subscription starts in OdSDS caused when the secret is subscribed from multiple places. Fixed overlap of the secret providers between warming and non-warming users (a warming user waits for the secret, a non-warming doesn't).
Additional Description:
Risk Level: low (only OdSDS is impacted, e.g. on_demand TLS cert provider)
Testing: updated
Docs Changes: none
Release Notes: yes