get stored secrets and configmaps from packet without dependency on c… #2500
Conversation
| @@ -80,12 +79,12 @@ public NextAction onSuccess(Packet packet, CallResponse<V1SecretList> callRespon | |||
| } | |||
|
|
|||
| static List<V1Secret> getSecrets(Packet packet) { | |||
| return Optional.ofNullable(getSecretsIfContinue(packet)).orElse(new ArrayList<>()); | |||
| return Optional.ofNullable(getSecretsFromPacket(packet)).orElse(new ArrayList<>()); | |||
There was a problem hiding this comment.
We can just inline (List) packet.get(SECRETS) here and remove the getSecretsFromPacket method.
| @@ -105,12 +104,12 @@ public NextAction onSuccess(Packet packet, CallResponse<V1ConfigMapList> callRes | |||
| } | |||
|
|
|||
| static List<V1ConfigMap> getConfigMaps(Packet packet) { | |||
| return Optional.ofNullable(getConfigMapsIfContinue(packet)).orElse(new ArrayList<>()); | |||
| return Optional.ofNullable(getConfigMapsFromPacket(packet)).orElse(new ArrayList<>()); | |||
|
Please add a unit test that requires this change, to make it clear why and to ensure that it is not inadvertently undone. |
|
So, with this change do you see the aggregate list? How did this regress? I think we have a slightly different pattern for other lists. |
Yes, with this change we see the aggregate list. The issue is that retrieval of the list from subsequent query is dependent on the CONTINUE flag in the packet. Unfortunately, for the subsequent request the CONTINUE flag is removed from the packet, in AsyncRequestStep.apply() (see line 263), and hence we return a new empty list for the SECRETS instead of the list from the packet. We then overwrite the SECRETS entry in the packet with the last result set, losing the previous result set. I tried looking at other lists for resources, like namespace, and it looks very similar to the proposed change. Can you clarify the pattern used for other lists in comparison to my proposed change? I would like to align it so that we’re handling lists the same. Also, our unit tests pass since the unit test support stubs out the AsyncRequestStep and we aren't removing the CONTINUE flag from the packet. I have a change that incorporates the same behavior (removing CONTINUE flag from packet) and this exposed the same failure in the unit tests that Rakshit was seeing. |
Everything is fine with your change. I see that the SECRETS and CONFIGMAP Packet entries are populated with code that follows the pattern I was expecting. |
| private static List<V1ConfigMap> getConfigMapsIfContinue(Packet packet) { | ||
| return packet.get(AsyncRequestStep.CONTINUE) != null ? (List<V1ConfigMap>) packet.get(CONFIGMAPS) : null; | ||
| private static List<V1ConfigMap> getConfigMapsFromPacket(Packet packet) { | ||
| return packet.get(CONFIGMAPS) != null ? (List<V1ConfigMap>) packet.get(CONFIGMAPS) : null; |
There was a problem hiding this comment.
It looks like Dongbo's comment above is correct -- this code could be simplified or just inlined as it tests a key and then returns either the same value or null -- it could have just returned the packet.get() straight away.
|
So the change lets the tests work for secrets? I don't see the new test. |
the change to KubernetesTestSupport replicates the behavior in AsyncRequestStep which then caused the unit tests to fail (failure was masked by not removing the CONTINUE flag). This replicates the failure that Rakshit was seeing. The change(s) to DomainValidationSteps then resolves the issue and the current unit tests cover the feature for reading in multiple results, hence I didn't add a new unit test. |
When listing secrets and configmaps, greater than the configured callRequestLimit (default 50), the previous list in the packet is overridden with the last set of results causing DomainValidation to fail. When reading multiple results, a 'continue' flag is placed in the packet but is cleared at the beginning of the next async request which results in an empty list returned instead of the aggregated list in the packet. It is unnecessary to rely on the 'continue' flag to return the results in the packet and hence this fix is to just return the secrets/configmaps list if it exists in the packet.
Unit tests already exist for handling listing of secrets and configmaps greater than the configured callRequestLimit.