Fixes secret marshaling for kube play. Merge stringData with data for secrets.

[ancosma]

Fixes secret (un)marshaling for kube play.

Merges stringData into data for secrets as in k8s.
Fixes e2e tests, remove '\n' from base64 encoded data.
Correct test to check that data in secret mounted file is decoded.

Closes #16269
Closes #16625

Signed-off-by: Andrei Natanael Cosma andrei@intersect.ro

Does this PR introduce a user-facing change?

NO

Fixes secret unmarshaling for kube play.

[vrothberg]

Thanks for contributing!

Changes LGTM
@cdoern PTAL

[rhatdan]

@odra PTAL

[rhatdan]

@ygalblum PTAL

[rhatdan]

@ashley-cui @umohnani8 PTAL

[ygalblum]

This change mixes data and stringData together. But, according to the spec for Secret, the field data is base64 encoded while stringData isn't. Furthermore, in K8S stringData is used to create a secret, but its content is merged into data after the values are base64 encoded. In addition, when mounted as a volume, all values are decoded back. So, the application can read them as is.

I can see that you changed the test so that it looks for the decoded value, but I don't understand what made it work now

[odra]

I tried to run the pr locally but I got the following error:

$ ./bin/podman play kube --replace pod.yaml 
Error: failed to create volume "mysecret": invalid character 'a' looking for beginning of value

secret.yaml:

apiVersion: v1
kind: Secret
metadata:
  name: mysecret
type: Opaque
data:
  foo: YmFy

pod.yaml:

apiVersion: v1
kind: Pod
metadata:
  name: mypod
spec:
  containers:
    - name: app
      image: fedora
      command: ["/usr/bin/sleep", "infinity"]
      volumeMounts:
        - name: mysecret
          mountPath: "/etc/mysecret"
          readOnly: true
            # envFrom:
            # - secretRef:
            # name: mysecret
            # env:
            #   - name: FOO
            #     valueFrom:
            #       secretKeyRef:
            #         name: mysecret
            #         key: foo
  volumes:
    - name: mysecret
      secret:
        secretName: mysecret
        optional: false

[ancosma]

This change mixes data and stringData together. But, according to the spec for Secret, the field data is base64 encoded while stringData isn't. Furthermore, in K8S stringData is used to create a secret, but its content is merged into data after the values are base64 encoded. In addition, when mounted as a volume, all values are decoded back. So, the application can read them as is.

I can see that you changed the test so that it looks for the decoded value, but I don't understand what made it work now

The base64 decoding happens on unmarshaling.
https://github.com/containers/podman/blob/main/pkg/domain/infra/abi/play.go#L268

The problem was that the entire secret object was serialized and stored in the (podman) secret while only the data of it was expected to be loaded when using it to fill env vars or volumes - hence the error. Also K8s states that content of secrets in volumes and environment variables should be decoded and also that stringData will be put on top of data (which was no the case previously).
Since decoding on base64 data happens during unmarshaling, adding stringData on top doesn't have to be base64 encoded before adding it. We only store podman secret as storage medium for values of K8s secret (data and stringData) - we could store the entire secret yaml, but then the code has to be changed since it mixed storing the yaml and loading json (data).

[ancosma]

@odra
I've tried it and no issue. Please make sure that you run podman from the build and not local one (did same mistake while testing it previously).

podman on  fix-secret-unmarshal [?] via 🐹 v1.19.3 via 🐍 v3.10.8 via ⍱
❯ bin/podman kube play secret.yaml

podman on  fix-secret-unmarshal [?] via 🐹 v1.19.3 via 🐍 v3.10.8 via ⍱
❯ bin/podman kube play pod.yaml
Pod:
4dd786023d2a7c717b025d6cb97177f34c3bd1051566bfb88f660944d0ea3ef7
Container:
3f3e8a5033a595b91d611142773d4ee24f2ed8c9ad7ee261ec418dec13dc8396

podman on  fix-secret-unmarshal [?] via 🐹 v1.19.3 via 🐍 v3.10.8 via ⍱
❯ bin/podman exec -it mypod-app ls /etc/mysecret/
foo

podman on  fix-secret-unmarshal [?] via 🐹 v1.19.3 via 🐍 v3.10.8 via ⍱
❯ bin/podman exec -it mypod-app cat /etc/mysecret/foo
bar

[ancosma]

Ran it with --replace and no issue.
podman on  fix-secret-unmarshal [?] via 🐹 v1.19.3 via 🐍 v3.10.8 via ⍱
❯ bin/podman kube play --replace pod.yaml
Pods stopped:
4dd786023d2a7c717b025d6cb97177f34c3bd1051566bfb88f660944d0ea3ef7
Pods removed:
4dd786023d2a7c717b025d6cb97177f34c3bd1051566bfb88f660944d0ea3ef7
Volumes removed:
Pod:
2ed36a9f4b51467e07fa60ef127808ed236256c43ce8150e62d83261446b8335
Container:
48114384883ca0d04442c9fa36055913d56b251b348211293c7aa2f81b03898a

[ygalblum]

Sorry for nagging, I'm just trying to understand why before the change the values in the container were encoded and now they are not.

Your change I understand. Instead of storing the entire K8S secret object as Podman secret, you store only its data by combining data and stringData into a single map and marshaling it into a json.

Furthermore, I now understand that the decoding is handled by the Unmarshal method and as a result when you merge data and stringData the values in both maps are decoded and hence there is no issue in combining them.

But, what I fail to understand is why before this change the values mounted into the container were encoded and now they are decoded. Now, don't get me wrong, I agree they should be decoded to align with K8S. I just don't understand how it happened.

Thanks in advance

[ancosma]

I haven't really checked it but might come from the fact that play used "github.com/ghodss/yaml" for marshaling while volume used "gopkg.in/yaml.v3" for unmarshaling.

[ygalblum]

I haven't really checked it but might come from the fact that play used "github.com/ghodss/yaml" for marshaling while volume used "gopkg.in/yaml.v3" for unmarshaling.

OK, thanks

[ygalblum]

LGTM, but I guess @odra should first see what's not working for him

[ashley-cui]

Will this break existing kube secrets? Since this changes the secret storage schema?

I don't remember if kube secrets persist across plays though, so this might not be a problem..

[ygalblum]

I think @ashley-cui is right. K8S secrets are stored into Podman secrets and hence persist. Since this change breaks the schema, podman will fail to load secrets that were created with an older version

[ancosma]

Maybe that's the issue @odra is having.
I personally wouldn't care much about backward compatibility since the secrets were not usable in the last versions of podman. If needed I can modify it to store/load the entire K8s secret instead of only the data, not sure if make sense.

[odra]

Yeah, that's the issue I had, storing secrets with "play kube" works wihile storing a raw kubernetes secret with "podman secret create.." breaks it (altough that was working before this change) but creating it wth "play kube" works.

I thought that creating a k8s secret using "play kube" would store the raw yaml/json secret as podman secret but that doesn't seem to be the case right?

Using "play kube" to store kubernetes secrets sound a bit weird to me since there but I may not have the full context of a podman secret.

[rhatdan]

If the kube.yaml file includes a secret then the secret should be created for the run of the podman kube play command and removed when the command exits, which I believe follows the k8s standard.

[vrothberg]

I personally wouldn't care much about backward compatibility since the secrets were not usable in the last versions of podman. If needed I can modify it to store/load the entire K8s secret instead of only the data, not sure if make sense.

Yes, let's restore to the previous behavior. We must remain backwards compatible unless there's an urgent need not to (e.g., security fix). Thanks for pointing that out, @ashley-cui! (And sorry for not spotting that in my earlier review)

[ancosma]

@vrothberg Previous behavior restored for backward compatibility.

[vrothberg]

Thanks, @andrei-n-cosma! The tests seem unhappy.

[ancosma]

@vrothberg Fixed the read of k8s secret from podman secret to support both JSON and YAML. Should pass now.

[vrothberg]

Thanks!

@ashley-cui PTAL

[ashley-cui]

LGTM, thanks @andrei-n-cosma!

[vrothberg]

/lgtm
/hold

I restarted the flaked job.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrei-n-cosma, vrothberg

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [vrothberg]

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

[vrothberg]

/hold cancel

[MartinX3]

will there be a v4.3.2 bugfix release?
I sadly don't see a podman roadmap or a milestone anywhere.

[vrothberg]

There are no plans to cut a v4.3.2. But 4.4 is scheduled early next year.

[MartinX3]

Thank you for the info
But also it's sad to hear, that bug fixes don't get backported to patch releases until the next feature release comes :(

[vrothberg]

Thank you for the info But also it's sad to hear, that bug fixes don't get backported to patch releases until the next feature release comes :(

They do get backported. v4.3 had two bug-fix releases already. We didn't plan for another patch-release in this specific case as we're close to the end of the year; many folks are on vacation or shortly going to be.