Forwarding Play Kube HTTP API configmaps query parameter to the container engine

[jakub-dzon]

What this PR does / why we need it:

When I tried to pass correct configmap path via HTTP client while invoking Podman Play Kube API, there was an error returned and the following error reported in Podman logs:

Nov 09 18:45:33 fedora podman[2401215]: time="2021-11-09T18:45:33+01:00" level=info msg="Request Failed(Internal Server Error): error playing YAML file: Configmap device-config-map not found"
Nov 09 18:45:33 fedora podman[2401215]: @ - - [09/Nov/2021:18:45:30 +0100] "POST /v4.0.0/libpod/play/kube?configmaps=%2Fvar%2Flocal%2Fyggdrasil%2Fdevice%2Fdevice-config-map.yaml HTTP/1.1" 500 140 "" "Go-http-client/1.1"

As a result, the pod was not started and stuck in Created status. As it turns out, configmaps query parameter for /v4.0.0/libpod/play/kube was not forwared to the ContainerEngine.PlayKube with PlayKubeOptions.

This change addresses that problem by mapping incoming configmaps query parameter to options.ConfigMaps.

Signed-off-by: Jakub Dzon [email protected]

How to verify it

Create config map with some env variable; for example /tmp/device-config-map.yaml

apiVersion: v1
data:
  DEVICE_ID: 242e48d0-286b-4170-9b97-95502066e6ae
kind: ConfigMap
metadata:
  creationTimestamp: null
  name: device-config-map

Create pod YAML file (/tmp/pod.yaml):

kind: Pod
metadata:
  name: edgedeployment-sample
spec:
  containers:
  - envFrom:
    - configMapRef:
        name: device-config-map
    image: quay.io/jdzon/env-printer:latest
    name: printer

Execute following from a go program:

func main() {
	podmanConnection, err := bindings.NewConnection(context.Background(), "unix://run/podman/podman.sock")
	if err != nil {
		log.Fatal(err)
	}
	options := play.KubeOptions{
		ConfigMaps: &[]string{"/tmp/device-config-map.yaml"},
	}
	_, err = play.Kube(podmanConnection, "/tmp/pod.yaml", &options)
	if err != nil {
		log.Fatal(err)
	}
}

Without the fix, error described above would be reported.

Which issue(s) this PR fixes:

None

Special notes for your reviewer:

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jakub-dzon
To complete the pull request process, please assign tomsweeneyredhat after the PR has been reviewed.
You can assign the PR to them by writing /assign @tomsweeneyredhat in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[rhatdan]

@umohnani8 PTAL

[rhatdan]

@jakub-dzon you need to add a test.

[jwhonce]

Please update the swagger comments in pkg/api/server/register_play.go with the new query parameter.

[jakub-dzon]

/retest

[openshift-ci]

@jakub-dzon: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[Luap99]

This doesn't make sense why do we send the path to file over the API. It will fail when you run the client on a different system. Ideally this would send the config map content over the API like the actual kube yaml file.

[jakub-dzon]

@Luap99
The client API allows for that in that semantics - see https://github.com/containers/podman/pull/12243/files#diff-cef3dd63f572aaef036dce65d94140edbfa8c350c2fc836e864a6b1ef653db87R2874 and server without this change ignores that parameter.

In our use case we are trying to leverage that functionality while running client and podman server on the same host using unix://run/podman/podman.sock and this PR makes support for config maps work for us. In distributed situation the behavior would be unchanged.

I agree that sending content of the config map would resolve the problem in all situation, but that looks like serious change to the API.

[Luap99]

@jakub-dzon We are already on v4.0 on the main branch so we can accept breaking changes to the API if needed.

I understand that this will work on the same host but I think this is very ugly and not a good API. Given that this never worked I would rather change it right now when we still have the chance.

[jakub-dzon]

@Luap99 Makes sense. I will close this PR.

If it's OK with you, I'd like to work on a proper solution, starting probably in two weeks. What would be the best way to discuss ideas for implementing it?

[mheon]

@TomSweeneyRedHat When's the next community cabal? Could be a good topic.

[umohnani8]

The next community cabal is on November 18th, we can add this as a topic to discuss there.

[TomSweeneyRedHat]

Noted and added to my list of topics for the next Cabal meeting.

[TomSweeneyRedHat]

https://hackmd.io/gQCfskDuRLm7iOsWgH2yrg?both for the agenda. The meeting is next Thu Nov 18 at 11:00 a.m. EST UTC-5. Conference room link in the agenda.

[jakub-dzon]

Thank you!