Upgrade VolumeSubpathEnvExpansion to Beta #65769

kevtaylor · 2018-07-03T15:17:53Z

What this PR does / why we need it:
Promotes the alpha feature gate VolumeSubpathEnvExpansion to Beta for milestone 1.12
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #64604
Special notes for your reviewer:

Release note:

Upgrade VolumeSubpathEnvExpansion feature gate to Beta

k8s-ci-robot · 2018-07-03T15:18:05Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

Assign the PR to them by writing /assign @soltysh 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:

pkg/features/OWNERS
test/OWNERS

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

thockin · 2018-07-03T16:22:01Z

Strictly speaking this is a breaking change. A directory named $(foo) or even $(rm -rf ..) is technically valid. This feature breaks that.

I didn't see the original PR, so I apologize for not reviewing it then. This is the sort of change that needs sig-arch to review (@jdumars @kubernetes/sig-architecture-api-reviews) (the process is in development, so no indictment here :). It's a good example of an API change that DOESN'T touch a types.go file, because we don't document alpha features in the comments.

The compatible way to do this would be to add a new field, e.g. subPathFrom as was done for EnvVar.ValueFrom. I'd say this PR should NOT go in until that is changed.

I didn't track all the original work - I assume this properly handles $$ escape and that we validate that FOO="../../../../../proc is not allowed?

Edit: for future, the way to make sure I see a PR is to ASSIGN it to me :)

kevtaylor · 2018-07-04T06:15:09Z

Not sure why you think it is a breaking change as it doesn't alter the existing subpath creation or validation

 setting env to "" gives /logs/hello.txt
 setting env to " " gives /logs/ /hello.txt
 setting env variable to $(PODX_NAME) gives /logs/$(PODX_NAME)/hello.txt

There are tests for backticks and absolute paths to ensure no validation was violated.

So can't see the need for introduction of a new element but I guess that's your perogative as code owners.

kevtaylor · 2018-07-04T06:20:05Z

/cc @msau42

idealhack · 2018-07-04T07:46:33Z

/ok-to-test

thockin · 2018-07-06T04:52:03Z

My point was that someone COULD be using a subpath that was ACTUALLY the literal `$(FOOBAR)`, since that is a legitimate path name. After this change, that can change meaning to expand the variable, breaking users. Another example would be someone using a subpath with `$$` in it - that will also get changed and break users. In short, the entirety of the namespace of subpath strings belongs to the end user. You can't intrude on it in any way without being a breaking change. Sorry :(

…

On Tue, Jul 3, 2018 at 11:15 PM Kevin Taylor ***@***.***> wrote: Not sure why you think it is a breaking change as it doesn't alter the existing subpath creation or validation setting env to "" gives /logs/hello.txt setting env to " " gives /logs/ /hello.txt setting env variable to $(PODX_NAME) gives /logs/$(PODX_NAME)/hello.txt There are tests for backticks and absolute paths to ensure no validation was violated. So can't see the need for introduction of a new element but I guess that's your perogative as code owners. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#65769 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVHphtXMUI5oueKF479UBpzYqWWouks5uDF2NgaJpZM4VBJlw> .

kevtaylor · 2018-07-06T08:05:28Z

@thockin Thanks for the clarity and yes I see your point on this edge case but that would be a very unconventional use of file systems to include $ in files/folders

However, in our defence the massive workarounds that we had to do to implement this and the subsequent vulnerabilities that the team uncovered as a result of it are in my opinion a greater justification for this feature than the edge case(s) that you highlight.

Also, not sure if you saw this but Michelle Au raised #64045 for a wider discussion

liggitt · 2018-07-06T14:09:03Z

@thockin Thanks for the clarity and yes I see your point on this edge case but that would be a very unconventional use of file systems to include $ in files/folders

However, in our defence the massive workarounds that we had to do to implement this and the subsequent vulnerabilities that the team uncovered as a result of it are in my opinion a greater justification for this feature than the edge case(s) that you highlight.

I agree with @thockin that it isn't acceptable to break the meaning of the existing field. That doesn't preclude the introduction of a mutually exclusive parallel field with expansion semantics.

thockin

Strictly speaking this is a breaking change, and should not be allowed. $(FOO) is a valid (if awkward) subPath today and after this PR it changes meaning (for all users, by default). I swear I said this before but I can't find proof of that via github's awesome UIs.

How should we proceed? It's probably unlikely to be a real problem in practice, but on principle we SHOULD NOT be doing changes like this.

I scanned the alpha PR - I don't think we considered a whole lot of other options. It's likely that whatever we do here will be looked at as precedent for further changes, so we need to be careful.

I'm holding this PR until discussion, sorry.

thockin · 2018-08-23T17:56:03Z

pkg/features/kube_features.go

@@ -397,7 +397,7 @@ var defaultKubernetesFeatureGates = map[utilfeature.Feature]utilfeature.FeatureS
 	BalanceAttachedNodeVolumes:                  {Default: false, PreRelease: utilfeature.Alpha},
 	DynamicProvisioningScheduling:               {Default: false, PreRelease: utilfeature.Alpha},
 	PodReadinessGates:                           {Default: false, PreRelease: utilfeature.Beta},
-	VolumeSubpathEnvExpansion:                   {Default: false, PreRelease: utilfeature.Alpha},
+	VolumeSubpathEnvExpansion:                   {Default: true, PreRelease: utilfeature.Beta},


The comments around the SubPath field need to be updated to indicate that it supports variable expansion.

msau42 · 2018-08-23T18:09:12Z

Yes I agree we should consider an alternative API.

thockin · 2018-08-23T18:11:47Z

I couldn't find it because it was in this thread, LOL.

Jordan's suggestion is better - can we add a new field? We ruled out subPathFrom because we need catenated variables. What about subPathExpr or subPathTemplate which is explicitly an env-substituted value and mutually exclusive with subPath?

kevtaylor · 2018-08-23T18:57:01Z

@thockin I think the use of the word template implies a lot more functionality than is actually occurring so I prefer subPathExpr

k8s-ci-robot · 2018-08-23T21:56:41Z

@kevtaylor: PR needs rebase.

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.

thockin · 2018-08-24T00:30:11Z

Please feel free to find a better name - those were just seeds...

fejta-bot · 2018-11-22T01:21:07Z

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

msau42 · 2018-11-22T01:56:26Z

Can this be closed? We have a KEP for the new proposal that we will pursue in 1.14: kubernetes/community#2871

kevtaylor · 2018-11-22T08:35:08Z

/close

k8s-ci-robot · 2018-11-22T08:35:16Z

@kevtaylor: Closed this PR.

In response to this:

/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.

Upgrade VolumeSubpathEnvExpansion to Beta

c260eb3

k8s-ci-robot requested review from shyamjvs and thockin July 3, 2018 15:18

k8s-ci-robot added sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API labels Jul 3, 2018

thockin self-assigned this Jul 3, 2018

k8s-ci-robot requested a review from msau42 July 4, 2018 06:20

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 4, 2018

kevtaylor mentioned this pull request Jul 24, 2018

Provide environment variables expansion in sub path mount kubernetes/enhancements#559

Closed

BrendanThompson mentioned this pull request Jul 30, 2018

Pod Volume Mount with subPath causes container creation to fail #66583

Closed

thockin reviewed Aug 23, 2018

View reviewed changes

thockin added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2018

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2018

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

k8s-ci-robot closed this Nov 22, 2018

kevtaylor deleted the feature/volumesubpathenvexpansion-beta branch March 28, 2019 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade VolumeSubpathEnvExpansion to Beta #65769

Upgrade VolumeSubpathEnvExpansion to Beta #65769

kevtaylor commented Jul 3, 2018

k8s-ci-robot commented Jul 3, 2018

thockin commented Jul 3, 2018 •

edited

Loading

kevtaylor commented Jul 4, 2018

kevtaylor commented Jul 4, 2018

idealhack commented Jul 4, 2018

thockin commented Jul 6, 2018 via email

kevtaylor commented Jul 6, 2018

liggitt commented Jul 6, 2018

thockin left a comment

thockin Aug 23, 2018

msau42 commented Aug 23, 2018

thockin commented Aug 23, 2018

kevtaylor commented Aug 23, 2018

k8s-ci-robot commented Aug 23, 2018

thockin commented Aug 24, 2018

fejta-bot commented Nov 22, 2018

msau42 commented Nov 22, 2018

kevtaylor commented Nov 22, 2018

k8s-ci-robot commented Nov 22, 2018

Upgrade VolumeSubpathEnvExpansion to Beta #65769

Upgrade VolumeSubpathEnvExpansion to Beta #65769

Conversation

kevtaylor commented Jul 3, 2018

k8s-ci-robot commented Jul 3, 2018

thockin commented Jul 3, 2018 • edited Loading

kevtaylor commented Jul 4, 2018

kevtaylor commented Jul 4, 2018

idealhack commented Jul 4, 2018

thockin commented Jul 6, 2018 via email

kevtaylor commented Jul 6, 2018

liggitt commented Jul 6, 2018

thockin left a comment

Choose a reason for hiding this comment

thockin Aug 23, 2018

Choose a reason for hiding this comment

msau42 commented Aug 23, 2018

thockin commented Aug 23, 2018

kevtaylor commented Aug 23, 2018

k8s-ci-robot commented Aug 23, 2018

thockin commented Aug 24, 2018

fejta-bot commented Nov 22, 2018

msau42 commented Nov 22, 2018

kevtaylor commented Nov 22, 2018

k8s-ci-robot commented Nov 22, 2018

thockin commented Jul 3, 2018 •

edited

Loading