Allow setting spec.template.spec.automountServiceAccountToken in PodSpec #11723

psschwei · 2021-07-26T15:26:29Z

Proposed Changes

Allows users to set spec.template.spec.automountServiceAccountToken in the PodSpec for a kservice

Release Note

Users can set `spec.template.spec.automountServiceAccountToken` to `false` in a PodSpec in order to opt-out of Kubenetes' default behaviour of mounting a ServiceAccount token in that Pod's containers.

codecov · 2021-07-26T15:31:43Z

Codecov Report

Merging #11723 (1d7d8fc) into main (0703daa) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #11723      +/-   ##
==========================================
- Coverage   87.83%   87.81%   -0.03%     
==========================================
  Files         196      196              
  Lines        9364     9365       +1     
==========================================
- Hits         8225     8224       -1     
- Misses        886      887       +1     
- Partials      253      254       +1

Impacted Files	Coverage Δ
pkg/apis/serving/fieldmask.go	`95.03% <100.00%> (+0.01%)`	⬆️
pkg/autoscaler/statforwarder/leases.go	`75.53% <0.00%> (-1.44%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0703daa...1d7d8fc. Read the comment docs.

julz · 2021-07-26T15:34:29Z

pkg/apis/serving/fieldmask.go

@@ -183,6 +183,7 @@ func PodSpecMask(ctx context.Context, in *corev1.PodSpec) *corev1.PodSpec {
 	out.Volumes = in.Volumes
 	out.ImagePullSecrets = in.ImagePullSecrets
 	out.EnableServiceLinks = in.EnableServiceLinks
+	out.AutomountServiceAccountToken = in.AutomountServiceAccountToken


I thought I remembered we were going to have this only allow false, since true could theoretically make things less secure (if an administrator had set AutomountServiceAccountToken = false on the service account), but looks like that was never written on the issue 🤔. TBH maybe this is fine, though, interested in opinions @dprotaso @markusthoemmes

Let's just allow setting this only to false for now

dprotaso · 2021-07-28T13:59:57Z

I think you're missing some updates to the schema file which is why verify is failing

markusthoemmes · 2021-07-28T15:45:14Z

pkg/apis/serving/fieldmask.go

@@ -183,6 +183,10 @@ func PodSpecMask(ctx context.Context, in *corev1.PodSpec) *corev1.PodSpec {
 	out.Volumes = in.Volumes
 	out.ImagePullSecrets = in.ImagePullSecrets
 	out.EnableServiceLinks = in.EnableServiceLinks
+	// Only allow setting AutomountServiceAccountToken to false
+	if !*in.AutomountServiceAccountToken {


This'll panic because of a nil pointer deref, you'll want to do if in.AutomountServiceAccountToken != nil && !*in.AutomountServiceAccountToken

markusthoemmes · 2021-07-28T15:58:38Z

pkg/apis/serving/fieldmask.go

@@ -183,6 +183,10 @@ func PodSpecMask(ctx context.Context, in *corev1.PodSpec) *corev1.PodSpec {
 	out.Volumes = in.Volumes
 	out.ImagePullSecrets = in.ImagePullSecrets
 	out.EnableServiceLinks = in.EnableServiceLinks
+	// Only allow setting AutomountServiceAccountToken to false
+	if !*in.AutomountServiceAccountToken && in.AutomountServiceAccountToken != nil {


Now it's backwards, will still panic. The && short-circuits, so if you turn this around, the second !*in.AutomountServiceAccountToken won't even be executed if in.AutomountServiceAccountToken != nil is false.

dprotaso · 2021-07-28T18:51:50Z

pkg/apis/serving/k8s_validation_test.go

+	}, {
+		name: "with automountServiceAccountToken (ok)",
+		ps: corev1.PodSpec{
+			AutomountServiceAccountToken: &falseValue,


we have a helper ptr.Bool(false)

dprotaso · 2021-07-28T18:53:27Z

pkg/apis/serving/fieldmask.go

+	// Only allow setting AutomountServiceAccountToken to false
+	if in.AutomountServiceAccountToken != nil {
+		if *in.AutomountServiceAccountToken {
+			out.AutomountServiceAccountToken = nil
+		}
+	}


I'd say just delete this block - it shouldn't be necessary

dprotaso · 2021-07-28T19:29:24Z

/lgtm
/approve

knative-prow-robot · 2021-07-28T19:29:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, psschwei

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:

~~config/OWNERS~~ [dprotaso]
~~hack/OWNERS~~ [dprotaso]
~~pkg/apis/OWNERS~~ [dprotaso]

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

allow setting spec.template.spec.automountServiceAccountToken in PodSpec

bfa179d

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Jul 26, 2021

knative-prow-robot added area/API API objects and controllers size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 26, 2021

julz reviewed Jul 26, 2021

View reviewed changes

only allow setting to false

ab4bddf

markusthoemmes reviewed Jul 28, 2021

View reviewed changes

preventing panic

be3ae98

markusthoemmes reviewed Jul 28, 2021

View reviewed changes

psschwei added 2 commits July 28, 2021 12:01

actually preventing panic

7053c60

updating schema

1537f1a

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Jul 28, 2021

fixing logic to deny when set to true

6666037

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 28, 2021

gofmt

bacdc1b

dprotaso reviewed Jul 28, 2021

View reviewed changes

using helper function for &false, removing unneeded disallowed block

1d7d8fc

knative-prow-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 28, 2021

knative-prow-robot assigned dprotaso Jul 28, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 28, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2021

knative-prow-robot merged commit 45dda6b into knative:main Jul 28, 2021

psschwei mentioned this pull request Jul 29, 2021

Adding test for automountServiceAccountToken=true #11743

Merged

psschwei deleted the automount-serviceaccount-token branch October 15, 2021 15:18

KauzClay mentioned this pull request Jun 26, 2023

Serving webhook should allow changing the automountServiceAccountToken to true #14124

Closed

hernan-abi mentioned this pull request Nov 25, 2024

Allow configuration of securityContext.Privileged explicitly to default value #15628

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow setting spec.template.spec.automountServiceAccountToken in PodSpec #11723

Allow setting spec.template.spec.automountServiceAccountToken in PodSpec #11723

psschwei commented Jul 26, 2021 •

edited

Loading

codecov bot commented Jul 26, 2021 •

edited

Loading

julz Jul 26, 2021

dprotaso Jul 28, 2021 •

edited

Loading

markusthoemmes Jul 28, 2021

dprotaso commented Jul 28, 2021

markusthoemmes Jul 28, 2021

markusthoemmes Jul 28, 2021

dprotaso Jul 28, 2021

dprotaso Jul 28, 2021

dprotaso commented Jul 28, 2021

knative-prow-robot commented Jul 28, 2021

Allow setting spec.template.spec.automountServiceAccountToken in PodSpec #11723

Allow setting spec.template.spec.automountServiceAccountToken in PodSpec #11723

Conversation

psschwei commented Jul 26, 2021 • edited Loading

Proposed Changes

codecov bot commented Jul 26, 2021 • edited Loading

Codecov Report

julz Jul 26, 2021

Choose a reason for hiding this comment

dprotaso Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

markusthoemmes Jul 28, 2021

Choose a reason for hiding this comment

dprotaso commented Jul 28, 2021

markusthoemmes Jul 28, 2021

Choose a reason for hiding this comment

markusthoemmes Jul 28, 2021

Choose a reason for hiding this comment

dprotaso Jul 28, 2021

Choose a reason for hiding this comment

dprotaso Jul 28, 2021

Choose a reason for hiding this comment

dprotaso commented Jul 28, 2021

knative-prow-robot commented Jul 28, 2021

psschwei commented Jul 26, 2021 •

edited

Loading

codecov bot commented Jul 26, 2021 •

edited

Loading

dprotaso Jul 28, 2021 •

edited

Loading