Fix issue where /exec/init calls put kubeconfig in first container #129

amisevsk · 2021-02-09T18:27:02Z

Description

When /exec/init is called (e.g. in the web terminal operator), it resolves the first container that supports a shell, but does not use this container for injecting kubeconfig. When no container name is provided, the kubeconfig is injected into the first container, which causes kubeconfig injection to fail when the first container in the pod does not support a shell.

Issue

Fixes devfile/devworkspace-operator#258

sleshchenko · 2021-02-10T09:10:48Z

api/rest/execInit.go

@@ -52,7 +52,7 @@ func HandleInit(c *gin.Context) {
 		return
 	}

-	err := HandleKubeConfigCreation(c, &initConfigParams, token)
+	err := HandleKubeConfigCreation(&initConfigParams.KubeConfigParams, token, execRequest.ContainerName)


I believe it could work in more optimal way.
execRequest already has pod name, while HandleKubeConfigCreation resolves it again in https://github.com/eclipse/che-machine-exec/blob/35dff49aa0ac5db3d847569a24e9f97ff7b7bf66/filter/kubernetes_filter.go#L104

So, the whole ContainerInfo could be propagated instead of just container name.

The issue is that the other flow (execKubeConfig) currently doesn't do the resolve step and uses containerName from initConfigParams, so we would have to change that process as well.

sleshchenko

Consider updating https://github.com/eclipse/che-machine-exec/blob/cd2b12bb456cb720fbe32b5486e50ae458496a0c/api/model/model_types.go#L102
to say something like // first suitable (with shell available)

sleshchenko · 2021-02-10T09:13:41Z

On more thing that we would need to do for WebTerminal and DevWorkspace on Che - make che-machine-exec independent from CHE_MACHINE_NAME env var. But it's for sure deserves a dedicated issue

sleshchenko

The changes solve the faced issue. So, I'm approving it and I'm OK with leaving as is currently, non-optimal way.
Though optimization is welcome if it's going to be a quick fix.

JPinkney

I'm OK with leaving it as is as well

amisevsk · 2021-02-10T19:16:17Z

Consider updating
che-machine-exec/api/model/model_types.go
to say something like // first suitable (with shell available)

Technically neither doc is accurate -- InitConfigParams is used for exec/config as well where container name is not optional :P

amisevsk · 2021-02-10T19:29:57Z

@sleshchenko I tried to rework the flow slightly to avoid having to filter twice, but I'm not sure it's clearer. I think long-term a large refactor pass is necessary as there's some confusing constructs.

Please re-review

sleshchenko · 2021-02-11T15:57:36Z

api/rest/execInit.go

 	execRequest := handleContainerResolve(c, token, initConfigParams.ContainerName)
 	if execRequest == nil {
 		rest.WriteResponse(c, http.StatusInternalServerError, "Could not retrieve exec request")
 		return
 	}

-	err := HandleKubeConfigCreation(&initConfigParams.KubeConfigParams, token, execRequest.ContainerName)
+	err := HandleKubeConfigCreation(&kubeConfigParams, execRequest)


Does that func really needs execRequest? I think it's better to convert it to containerInfo as soon as possible. Or even avoid converting like in the following sleshchenko@5de74ca

^ that's not tested, maybe I missed some point and it's better to avoid extending. If you have any concern about it, feel free to just ignore the proposal

Mostly just me avoiding making too many changes for a simple fix :)

I did find it strange we had two almost identical structs.

sleshchenko · 2021-02-11T16:16:46Z

api/rest/execKubeConfig.go

+		return
+	}
+
+	err := HandleKubeConfigCreation(&kubeConfigParams, execRequest)


As far as I understand you dropped token argument to make args list shorter, but now you kind of re-purposing kubeConfigParams.BearerToken, because in addition to propagate into kubeconfig, you also use it to create K8s API and make API call.
Ideally, I think K8sAPI should be created per request(on demand since there is requests in exec manager which does not need it) and reused in places where it's needed, without need to propagate the token.

If I made this PR I would introduce one more argument without dropping token, but I'm not against the made change here as well.

Perhaps I misunderstood the existing code -- my understanding is that token is derived from the same value, i.e.

if initConfigParams.KubeConfigParams.BearerToken is set, it is used

if it's unset, we log in via username/password and get the token

Both current flows explictly set initConfigParams.BearerToken = token in HandleKubeConfigCreation, so I thought I woudl move this step earlier in the call stack and avoid having to pass initConfigParams and the token only to apply the token to initConfigParams.

amisevsk · 2021-02-22T22:04:27Z

I can't tell if this s390x issue is my fault or not.

sleshchenko · 2021-02-24T07:44:43Z

I can't tell if this s390x issue is my fault or not.

I can tell for sure, it's not your fault. It's the same instability as Dashboard used to have
Here is a PR that fixes PR check #133

When /exec/init is called (e.g. in the web terminal operator), it resolves the first container that supports a shell, but does not use this container for injecting kubeconfig. When no container name is provided, the kubeconfig is injected into the first container, which causes kubeconfig injection to fail when the first container in the pod does not support a shell. Signed-off-by: Angel Misevski <[email protected]>

Signed-off-by: Angel Misevski <[email protected]>

amisevsk requested review from AndrienkoAleksandr, evidolob and l0rd as code owners February 9, 2021 18:27

amisevsk mentioned this pull request Feb 9, 2021

Enable referencing plugin components by ID (internal-registry only) devfile/devworkspace-operator#263

Merged

sleshchenko reviewed Feb 10, 2021

View reviewed changes

sleshchenko approved these changes Feb 10, 2021

View reviewed changes

JPinkney approved these changes Feb 10, 2021

View reviewed changes

AndrienkoAleksandr approved these changes Feb 10, 2021

View reviewed changes

amisevsk requested a review from sleshchenko February 10, 2021 19:31

sleshchenko approved these changes Feb 11, 2021

View reviewed changes

sleshchenko approved these changes Feb 22, 2021

View reviewed changes

JPinkney approved these changes Feb 22, 2021

View reviewed changes

amisevsk added 3 commits February 24, 2021 11:06

Rework CreateKubeConfig to avoid filtering containers twice

6073db5

Signed-off-by: Angel Misevski <[email protected]>

Make ResolvedExec inline ContainerInfo to avoid duplicating information

22b830a

Signed-off-by: Angel Misevski <[email protected]>

amisevsk force-pushed the fix-exec-init branch from a07de39 to 22b830a Compare February 24, 2021 16:07

amisevsk merged commit a143084 into eclipse-che:master Feb 26, 2021

amisevsk deleted the fix-exec-init branch February 26, 2021 04:43

che-bot added this to the 7.27 milestone Feb 26, 2021

JPinkney mentioned this pull request Jul 22, 2021

Cherry-pick commits needed from che-machine-exec redhat-developer/web-terminal-exec#26

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix issue where /exec/init calls put kubeconfig in first container #129

Fix issue where /exec/init calls put kubeconfig in first container #129

Uh oh!

amisevsk commented Feb 9, 2021

Uh oh!

sleshchenko Feb 10, 2021

Uh oh!

amisevsk Feb 10, 2021

Uh oh!

sleshchenko left a comment

Uh oh!

sleshchenko commented Feb 10, 2021

Uh oh!

sleshchenko left a comment

Uh oh!

JPinkney left a comment

Uh oh!

amisevsk commented Feb 10, 2021

Uh oh!

amisevsk commented Feb 10, 2021 •

edited

Loading

Uh oh!

sleshchenko Feb 11, 2021

Uh oh!

sleshchenko Feb 11, 2021

Uh oh!

amisevsk Feb 12, 2021

Uh oh!

sleshchenko Feb 11, 2021

Uh oh!

amisevsk Feb 12, 2021

Uh oh!

amisevsk commented Feb 22, 2021

Uh oh!

sleshchenko commented Feb 24, 2021 •

edited

Loading

Uh oh!

Uh oh!

Fix issue where /exec/init calls put kubeconfig in first container #129

Fix issue where /exec/init calls put kubeconfig in first container #129

Uh oh!

Conversation

amisevsk commented Feb 9, 2021

Description

Issue

Uh oh!

sleshchenko Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

amisevsk Feb 10, 2021

Choose a reason for hiding this comment

Uh oh!

sleshchenko left a comment

Choose a reason for hiding this comment

Uh oh!

sleshchenko commented Feb 10, 2021

Uh oh!

sleshchenko left a comment

Choose a reason for hiding this comment

Uh oh!

JPinkney left a comment

Choose a reason for hiding this comment

Uh oh!

amisevsk commented Feb 10, 2021

Uh oh!

amisevsk commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sleshchenko Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

sleshchenko Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

amisevsk Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

sleshchenko Feb 11, 2021

Choose a reason for hiding this comment

Uh oh!

amisevsk Feb 12, 2021

Choose a reason for hiding this comment

Uh oh!

amisevsk commented Feb 22, 2021

Uh oh!

sleshchenko commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

amisevsk commented Feb 10, 2021 •

edited

Loading

sleshchenko commented Feb 24, 2021 •

edited

Loading