fix #4355: adding logic to set/validate the container name #4505
Conversation
| @@ -315,10 +315,10 @@ private URL getURL(String operation, String[] commands) throws MalformedURLExcep | |||
| */ | |||
| String validateOrDefaultContainerId(String name) { | |||
| Pod pod = this.require(); | |||
| List<Container> containers = pod.getSpec().getContainers(); | |||
| if (containers.isEmpty()) { | |||
| if (pod.getSpec() == null || pod.getSpec().getContainers() == null || pod.getSpec().getContainers().isEmpty()) { | |||
| throw new KubernetesClientException("Pod has no containers!"); | |||
There was a problem hiding this comment.
@manusa see also
if you want to guard against invalid Pods/specs, all of that logic makes the same assumptions.
There was a problem hiding this comment.
I'm not sure that this is possible, I added this after implementing a test that failed (using the mockserver).
If it is possible to create a Pod without spec or containers (in real k8s -which I doubt), then we should definitely handle this
There was a problem hiding this comment.
No, I don't believe it was possible. But I wasn't sure how pervasive you were trying to cover those cases.
There was a problem hiding this comment.
So my thoughts:
- I don't think this resource can exist in a real cluster (Pod | PodTemplate) -I did a very quick test-
- I added this here because in the MockServer this resource can exist.
- I don't think it hurts having these guards here, although the most correct approach would be to create a valid resource in the test and remove these.
- I think it's fine leaving this here, but I can remove if you think it's inconsistent or that this might be confusing.
Please let me know what do you prefer
There was a problem hiding this comment.
How about a comment explaining that it's for making testing easier. There's less danger it will be unnecessarily copied later that way.
Signed-off-by: Marc Nuri <[email protected]>
Signed-off-by: Marc Nuri <[email protected]>
|
SonarCloud Quality Gate failed. |
Description
Fixes #4355 - direct or indirect (copy, upload) to exec / attach will now validate or set the container parameter in the same was as the kubectl client. This requires always fetching the pod prior to the exec / attach call and will always add the container argument.
We do not yet need to do this for log operations as we don't have a similar option to specify "don't get from all" without specifying a container name.
Type of change
test, version modification, documentation, etc.)
Checklist