Add support for file based secret #1007

hangyan · 2018-05-15T03:24:39Z

related to #296

Notes:

External secret is not supported
may have conflict with Add support for Config, endpoint_mode and 3.3 support #994
import a new package cast(https://github.com/spf13/cast)

hangyan · 2018-05-15T03:30:34Z

/assign cdrage
/assign kadel

cdrage · 2018-05-15T16:16:59Z

@hangyan Please separate the commits between code change and vendor update so we can review!

hangyan · 2018-05-16T00:33:20Z

@cdrage Done. Thanks!

cdrage

Hey @hangyan I see some still work-in-progress code, is this ready for review?

cdrage · 2018-05-16T13:20:54Z

pkg/loader/compose/v3.go

@@ -117,6 +117,7 @@ func parseV3(files []string) (kobject.KomposeObject, error) {
 			}
 		}
 	}
+	log.Debugf("fuck %+v", config)


I think this needs to be removed 🤣

@cdrage So sorry for this , totally forgot this debug info.

cdrage · 2018-05-16T13:21:02Z

pkg/kobject/kobject.go

+type OldSecretConfig struct {
+	File string
+}
+*/


Remove commented out code

cdrage · 2018-05-16T13:21:26Z

pkg/loader/compose/v3.go

+	}
+
+}*/
+


Remove commented out code?

@cdrage This is ready for review. But i have started too long ago and forgot to remove some useless code before this review. Sorry about that! Will be more carefully about this in the future

Since this code is no longer needed, please remove the commented out code 👍 @hangyan

cdrage

I will have to test this out, but other than a few comments, the code is great!

Just add more comments please to the code so future programmers can better understand what is happening, otherwise, looks amazing to me!

cdrage · 2018-07-05T17:37:52Z

pkg/loader/compose/v3.go

+	}
+
+}*/
+


Since this code is no longer needed, please remove the commented out code 👍 @hangyan

cdrage · 2018-07-05T17:38:36Z

pkg/transformer/kubernetes/kubernetes.go

+		if config.File != "" {
+			data, err := GetSecretDataFromFile(config.File, k.Opt)
+			if err != nil {
+				log.Fatal("unable to read secret from file:", config.File)


add spacing after "file:"

cdrage · 2018-07-05T17:39:01Z

pkg/transformer/kubernetes/kubernetes.go

+			}
+			objects = append(objects, secret)
+		} else {
+			log.Warnf("External secrets %s is not supported by now - ignoring", name)


small grammar mistake, should be "is not currently supported"

cdrage · 2018-07-05T17:39:40Z

pkg/transformer/kubernetes/kubernetes.go

+		}
+	}
+	return volumeMounts, volumes
+}


Are you able to comment on some of the code above as to what's happening?

cdrage · 2018-07-05T17:43:32Z

pkg/transformer/kubernetes/kubernetes.go

+	if komposeObject.Secrets != nil {
+		secrets, err := k.CreateSecrets(komposeObject)
+		if err != nil {
+			return nil, errors.Wrapf(err, "create secrets error")


Maybe add something different such as "Unable to create secrets" or something more descriptive? Not too sure what type of error message to add.

cdrage · 2018-07-05T17:46:36Z

pkg/transformer/kubernetes/kubernetes.go

@@ -449,6 +531,10 @@ func (k *Kubernetes) ConfigVolumes(name string, service kobject.ServiceConfig) (
 		useHostPath = true
 	}

+	secretsVolumeMounts, secretsVolumes := k.ConfigSecretVolumes(name, service)


Add comment above here explaining that we are adding secrets to volumes

cdrage · 2018-07-24T13:14:12Z

Progress @hangyan ?

hangyan · 2018-07-25T12:40:56Z

@cdrage Sorry, been very busy these days on my work. Working on this now.

hangyan · 2018-07-26T01:57:24Z

@cdrage Updated

hangyan · 2018-08-02T05:05:56Z

conflict resolved

jvitor83 · 2018-09-19T19:17:44Z

@cdrage is this good? will be merged?
I need that feature.

jvitor83 · 2018-09-20T16:04:01Z

pkg/transformer/kubernetes/kubernetes.go

+
+			volMount := api.VolumeMount{
+				Name:      vol.Name,
+				MountPath: "/run/secrets",


@hangyan
This shouldn't be MountPath: "/run/secrets/" + vol.Name, ???

Because without the MountPath targeting to the secret, it gives a error:

RunContainerError: failed to start container "f97d9e9354548939d15acae70758347538426fbb05c80f2a5cd316dc67e7bf58": Error response from daemon: oci runtime error: container_linux.go:247: starting container process caused "process_linux.go:359: container init caused \"rootfs_linux.go:54: mounting \\\"/opt/rke/var/lib/kubelet/pods/516c135b-bce8-11e8-8714-005056ae708d/volumes/kubernetes.io~secret/default-token-khnzc\\\" to rootfs \\\"/var/lib/docker/overlay/60fb5d9a4007bf8e1889898df737b275cb77431351b9168ab4d629fbcdf065b5/merged\\\" at \\\"/var/lib/docker/overlay/60fb5d9a4007bf8e1889898df737b275cb77431351b9168ab4d629fbcdf065b5/merged/run/secrets/kubernetes.io/serviceaccount\\\" caused \\\"mkdir /var/lib/docker/overlay/60fb5d9a4007bf8e1889898df737b275cb77431351b9168ab4d629fbcdf065b5/merged/run/secrets/kubernetes.io: read-only file system\\\"\""

Just like in this issue: kubernetes/kubernetes#65835

And concating the secret name, it work!

With some research i discover that:

kubernetes

kubectl create secret generic my_secret --from-literal=my_secret_key_1=my_secret_value_1 --from-literal=my_secret_key_2=my_secret_value_2

it create a file /run/secrets/my_secret/my_secret_key_1 (with the content my_secret_value_1)

docker-compose/swarm

with short syntax:

... secrets: - my_secret secrets: my_secret: file: my_secret.txt

it create a file /run/secrets/my_secret

don't allow to be compatible with kubernetes

with long syntax:

... secrets: - source: some_secret target: my_secret/my_secret_key secrets: some_secret: file: my_secret.txt

it create a file /run/secrets/my_secret/my_secret_key

allow to be compatible with kubernetes

I believe that this PR:

shouldn't allow to use short-syntax, or if allow, to at least concat the secret name as directory (as my previous comment suggest to avoid the error).

only allow long-syntax if has at least inside one directory to be compatible with kubernetes, or at least concat the secret name as directory

Maybe concat the secret name as directory (by default) for the path can be the solution!?!

@jvitor83 Sorry about the late response. I have re-checked the document of kubernetes and docker. I have different opinion on this problem. kompose is meant to translate compose to kubernetes as precision as possible. Because docker mount the secret to /run/secrets/<secret-name>, so this path should be the same in kubernetes, not /run/secrets/<secret-name>/<key>. I believe the right choice is to use subPath ( you can see a example in the issues link you provide)

I get it that "kompose is meant to translate compose to kubernetes as precision as possible", but it didn't work with the k8s generated!
The generated mapping gives error (mentioned before) and the pod didn't start!

By my understanding, the generated files should work in first place to then keep the translation as precise as possible.

I guess we should remove the short syntax once it is incompatible and don't allow to get the file at the same path.

The issue link you provide has a solution in the end:subPath. I think it's a better solution, i haven't test it out, but I think it should work

@hangyan
I am not understanding what you mean.

The thing is that this PR is generating:

volumeMounts: - name: secret-name mountPath: "/run/secrets"

which gives the error reported.
The issue (kubernetes/kubernetes#65835) was solved not because it put the subPath, but because the directory was added to the mountPath. (I think the issue author have been mistaken)
If the kompose generate the k8s without the mountPath with a directory (with secret using short-syntax), it will give error on start. I have tested it with openjdk, dotnet and others. All gives the same error.

What you are suggesting to do?
If is to: "modify the code to add the subPath", it will continue to give the error.

I think we have to at least put some warning saying that secrets with short-syntax is not recommended and can gives error.

jvitor83 · 2018-09-25T14:58:35Z

@hangyan , i create a PR ( https://github.com/hangyan/kompose/pull/3/files ) to your secret-support branch (PR) which:

Fix the issue reported at Add support for file based secret #1007 (comment)
Added support to long-syntax with absolute path

hangyan · 2018-09-27T07:05:28Z

@jvitor83 Great. Thanks very much. I will review this again ASAP. But since there are not so many active maintainers for this project, this PR may take a long time to be merged in. I'm afraid of you will have to build the binary yourself based on this branch.

jvitor83 · 2018-09-27T14:37:28Z

I'm afraid of you will have to build the binary yourself based on this branch.

Already did it!
Thanks for the feedback.

cdrage · 2018-10-01T14:58:08Z

@hangyan if you want @jvitor83 you can always do a branch of this PR, push your changes and open a new PR. I don't think @hangyan would mind if you continue on his awesome work 👍

k8s-ci-robot · 2018-10-30T15:42:02Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: cdrage

If they are not already assigned, you can assign the PR to them by writing /assign @cdrage in a comment when ready.

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

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

fejta-bot · 2019-01-28T15:46:40Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2019-02-27T16:05:14Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2019-03-29T16:51:22Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2019-03-29T16:51:29Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 15, 2018

hangyan requested review from cdrage, ngtuna and kadel and removed request for cdrage and ngtuna May 15, 2018 03:24

k8s-ci-robot assigned cdrage and kadel May 15, 2018

Import spf13/cast package

40c7f36

hangyan force-pushed the secret-support branch from fe708e9 to 18b840c Compare May 16, 2018 00:32

cdrage requested changes May 16, 2018

View reviewed changes

hangyan force-pushed the secret-support branch 2 times, most recently from 80648b6 to 75855dc Compare May 16, 2018 13:30

hangyan mentioned this pull request Jul 5, 2018

Fix environment with env_file bug #1020

Merged

cdrage requested changes Jul 5, 2018

View reviewed changes

hangyan force-pushed the secret-support branch from 8d19e32 to 5b12d9a Compare July 25, 2018 15:55

Support file based secret

0e503c9

hangyan force-pushed the secret-support branch from 5b12d9a to 0e503c9 Compare July 25, 2018 16:30

Merge branch 'master' into secret-support

98fb348

hangyan force-pushed the secret-support branch from d160aa8 to 422a766 Compare August 2, 2018 03:02

Fix merge errors

1adec75

hangyan force-pushed the secret-support branch from 422a766 to 1adec75 Compare August 2, 2018 03:22

jvitor83 reviewed Sep 20, 2018

View reviewed changes

jvitor83 mentioned this pull request Sep 25, 2018

Fix issue about read-only volume path and add support to absolute path at long syntax hangyan/kompose#3

Closed

Use subPath for secret based volume

7bd25b9

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 28, 2019

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 27, 2019

k8s-ci-robot closed this Mar 29, 2019

jvitor83 mentioned this pull request Aug 2, 2019

Add support for file based secret #1159

Merged

hangyan deleted the secret-support branch December 27, 2019 14:43

		}

		}*/

		}

		}*/

Add support for file based secret #1007

Add support for file based secret #1007

Conversation

hangyan commented May 15, 2018 • edited Loading

hangyan commented May 15, 2018

cdrage commented May 15, 2018

hangyan commented May 16, 2018

cdrage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage commented Jul 24, 2018

hangyan commented Jul 25, 2018

hangyan commented Jul 26, 2018

hangyan commented Aug 2, 2018

jvitor83 commented Sep 19, 2018

Choose a reason for hiding this comment

jvitor83 Sep 21, 2018 • edited Loading

Choose a reason for hiding this comment

kubernetes

docker-compose/swarm

hangyan Oct 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvitor83 Oct 4, 2018 • edited Loading

Choose a reason for hiding this comment

jvitor83 commented Sep 25, 2018 • edited Loading

hangyan commented Sep 27, 2018

jvitor83 commented Sep 27, 2018

cdrage commented Oct 1, 2018

k8s-ci-robot commented Oct 30, 2018

fejta-bot commented Jan 28, 2019

fejta-bot commented Feb 27, 2019

fejta-bot commented Mar 29, 2019

k8s-ci-robot commented Mar 29, 2019

hangyan commented May 15, 2018 •

edited

Loading

jvitor83 Sep 21, 2018 •

edited

Loading

hangyan Oct 2, 2018 •

edited

Loading

jvitor83 Oct 4, 2018 •

edited

Loading

jvitor83 commented Sep 25, 2018 •

edited

Loading