KEP-4369: Promote to beta (allow ~all ASCII characters in env vars) #4805

HirazawaUi · 2024-08-26T15:42:32Z

One-line PR description: promote feature gate to beta

Issue link:

Other comments:

HirazawaUi · 2024-08-26T15:44:43Z

/cc @thockin @jpbetz

HirazawaUi · 2024-08-26T15:57:28Z

/cc @BenTheElder

keps/sig-node/4369-allow-special-characters-environment-variable/README.md

kannon92 · 2024-08-26T17:46:20Z

keps/sig-node/4369-allow-special-characters-environment-variable/README.md

 ###### How can a rollout or rollback fail? Can it impact already running workloads?

-When a feature gate is closed, already running workloads are not affected in any way, but update fields for workload will cause the workload to fail.
+When a feature gate is disabled, already running workloads are not affected in any way, but update fields for workload will cause the workload to fail.


What fields would change in an update for this?

Any update to the workload's fields results in failure because pods need to be recreate.

Maybe I missed this in the KEP but I don't understand how that is related.

If we update any field of workloads that use an environment variable with relaxed validation when disabling feature gate, the workload might fail to recreate pods or ReplicaSets because it cannot pass the validation logic of the Apiserver.

I see. So you are saying that if you have expanded environment variable support and then you downgrade it could be possible to start failing validation.

Your phrase above seems to be more general than what you expanded here.

This is, unfortunately, true of any feature on pod - if you enable, use in Deployment, disable, then the ".. or already in use" logic doesn't matter much.

I used more friendly words to describe it :)

thockin · 2024-09-16T21:42:49Z

Thanks!

/lgtm
/approve

thockin · 2024-09-16T21:44:02Z

This really needs a sig-node approver

HirazawaUi · 2024-09-17T15:54:12Z

/cc @kubernetes/sig-node-leads Could anyone review and approve it?

SergeyKanzhelev · 2024-09-17T16:55:22Z

/cc @kubernetes/sig-node-leads Could anyone review and approve it?

We will discuss KEPs and assign approvers at today's meeting that starts in a few minutes

kannon92 · 2024-09-18T17:44:14Z

/assign @mrunalp

SergeyKanzhelev

/lgtm
/approve

SergeyKanzhelev

/lgtm
/approve

keps/sig-node/4369-allow-special-characters-environment-variable/kep.yaml

SergeyKanzhelev

/lgtm

HirazawaUi · 2024-09-19T07:04:44Z

/cc @jpbetz Could you review for it as PRR reviewer? It has already been approved by sig-node.

jpbetz

A few comments about ratcheting validation for downgrade/rollback. LGTM for PRR once those are addressed.

jpbetz · 2024-09-19T13:56:14Z

keps/sig-node/4369-allow-special-characters-environment-variable/README.md

 ###### Can the feature be disabled once it has been enabled (i.e. can we roll back the enablement)?

-If close the feature gate, already running workloads will not be affected in any way, 
+If disable the feature gate, already running workloads will not be affected in any way, 


Should we also updated the above "Downgrade" section ("users need to reset their environment variables for special characters to normal characters."). I was expecting it to say something like "After downgrade, environment variables containing special characters will continue to work as expected, but any writes to resources to add or change environment variables must set the environment variable names to only use normal characters."

Modified as suggested.

jpbetz · 2024-09-19T13:59:46Z

keps/sig-node/4369-allow-special-characters-environment-variable/README.md

 ###### How can a rollout or rollback fail? Can it impact already running workloads?

-When a feature gate is closed, already running workloads are not affected in any way, but update fields for workload will cause the workload to fail.
+When the feature gate is disabled, workloads that are already running will not be affected. However, if user update the workloads, they may fail to recreate pods or ReplicaSets due to failing the Apiserver's validation logic, which could cause the workloads to fail.


If a environment variable has special characters, and the cluster is rolled back, do updates that change other fields (but that do not modify the fields containing environment variables) fail? I am expecting updates like this to be allowed.

The trick here is usually to modify the validation rules to compare the old value with the new value for updates, and allow the less restrictive rule (use of special characters in this situation) so long as the field value is not changing. This way, only controllers that are actually changing the environment fields are at risk of being impacted by a rollback.

Modified as suggested.

Apologies for my hindsight. I suddenly realized that we can't achieve this. Modifying any controller will delete the old pods and create new ones, and we can't retrieve the spec of the old pods during pod creation validation. So, after disabling the feature gate, we won't be able to update other fields of the workload.

Ah, I follow. That limitation makes sense to me.

Thank you for confirming! @HirazawaUi I think what is implemented should be OK than

jpbetz · 2024-09-19T14:00:24Z

keps/sig-node/4369-allow-special-characters-environment-variable/README.md

+
+```
+kubectl get pods --all-namespaces -o json | jq -r '.items[] | select(.spec.containers[].env[]?.name | test("^[a-zA-Z_][a-zA-Z0-9_]*$") | not) | [.metadata.namespace, .metadata.name, .spec.containers[].env[]?.name] | @tsv'
+```


Thanks for including a command!

HirazawaUi · 2024-09-29T14:50:47Z

ping @jpbetz

jpbetz · 2024-09-29T15:39:21Z

/approve
For PRR

k8s-ci-robot · 2024-09-29T15:39:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: HirazawaUi, jpbetz, SergeyKanzhelev, thockin

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:

~~keps/prod-readiness/OWNERS~~ [jpbetz]
~~keps/sig-node/4369-allow-special-characters-environment-variable/OWNERS~~ [SergeyKanzhelev]

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

HirazawaUi · 2024-09-29T16:02:09Z

@SergeyKanzhelev Could you lgtm for it again?

thockin · 2024-09-29T22:31:47Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 26, 2024

k8s-ci-robot requested a review from dchen1107 August 26, 2024 15:42

k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Aug 26, 2024

k8s-ci-robot requested a review from mrunalp August 26, 2024 15:42

k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 26, 2024

HirazawaUi force-pushed the promote-4396-to-beta branch from 26ff8ac to 1090b25 Compare August 26, 2024 15:44

k8s-ci-robot requested review from jpbetz and thockin August 26, 2024 15:44

k8s-ci-robot requested a review from BenTheElder August 26, 2024 15:57

HirazawaUi force-pushed the promote-4396-to-beta branch from 1090b25 to ee1d6c5 Compare August 26, 2024 16:09

kannon92 reviewed Aug 26, 2024

View reviewed changes

keps/sig-node/4369-allow-special-characters-environment-variable/README.md Show resolved Hide resolved

kannon92 reviewed Aug 26, 2024

View reviewed changes

HirazawaUi force-pushed the promote-4396-to-beta branch from ee1d6c5 to 1c0d1f4 Compare September 3, 2024 16:21

kannon92 mentioned this pull request Sep 9, 2024

Allow almost all printable ASCII characters in environment variables #4369

Closed

22 tasks

thockin changed the title ~~KEP-4396: Promote to beta~~ KEP-4369: Promote to beta Sep 10, 2024

thockin changed the title ~~KEP-4369: Promote to beta~~ KEP-4369: Promote to beta (allow ~all ASCII characters in env vars) Sep 10, 2024

HirazawaUi force-pushed the promote-4396-to-beta branch from 1c0d1f4 to 56045f6 Compare September 11, 2024 14:59

k8s-ci-robot assigned thockin Sep 16, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2024

thockin assigned jpbetz and SergeyKanzhelev and unassigned thockin Sep 16, 2024

k8s-ci-robot assigned mrunalp Sep 18, 2024

SergeyKanzhelev approved these changes Sep 18, 2024

View reviewed changes

SergeyKanzhelev approved these changes Sep 19, 2024

View reviewed changes

SergeyKanzhelev reviewed Sep 19, 2024

View reviewed changes

keps/sig-node/4369-allow-special-characters-environment-variable/kep.yaml Show resolved Hide resolved

HirazawaUi force-pushed the promote-4396-to-beta branch from 56045f6 to f161bdd Compare September 19, 2024 06:57

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2024

SergeyKanzhelev approved these changes Sep 19, 2024

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2024

jpbetz reviewed Sep 19, 2024

View reviewed changes

promote 4396 to beta

a658078

HirazawaUi force-pushed the promote-4396-to-beta branch from f161bdd to a658078 Compare September 19, 2024 14:41

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2024

jpbetz approved these changes Sep 29, 2024

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 29, 2024

k8s-ci-robot assigned thockin Sep 29, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2024

k8s-ci-robot merged commit e85182b into kubernetes:master Sep 29, 2024

k8s-ci-robot added this to the v1.32 milestone Sep 29, 2024

This was referenced Oct 16, 2024

KEP-4369: promote RelaxedEnvironmentVariableValidation feature gate to beta kubernetes/kubernetes#126897

Merged

KEP-4396 Fix beta stage description #4926

Merged

KEP-4369: Promote to beta (allow ~all ASCII characters in env vars) #4805

KEP-4369: Promote to beta (allow ~all ASCII characters in env vars) #4805

Uh oh!

Conversation

HirazawaUi commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HirazawaUi commented Aug 26, 2024

Uh oh!

HirazawaUi commented Aug 26, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thockin commented Sep 16, 2024

Uh oh!

thockin commented Sep 16, 2024

Uh oh!

HirazawaUi commented Sep 17, 2024

Uh oh!

SergeyKanzhelev commented Sep 17, 2024

Uh oh!

kannon92 commented Sep 18, 2024

Uh oh!

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

Uh oh!

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SergeyKanzhelev left a comment

Choose a reason for hiding this comment

Uh oh!

HirazawaUi commented Sep 19, 2024

Uh oh!

jpbetz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HirazawaUi commented Sep 29, 2024

Uh oh!

jpbetz commented Sep 29, 2024

Uh oh!

k8s-ci-robot commented Sep 29, 2024

Uh oh!

HirazawaUi commented Sep 29, 2024

Uh oh!

thockin commented Sep 29, 2024

Uh oh!

Reviewers

HirazawaUi commented Aug 26, 2024 •

edited

Loading