Add field-level warnings for deprecated / known bad values

[liggitt]

• Determine approach and format for warnings:
  • determine when to emit warnings: on creation, on update that would change generation / spec
  • Use field.Path format
  • for deprecated values, indicate replacement, deprecation and target removal release
• Add warnings for deprecated fields
  • Pod templates
    • beta os/arch/zone/region labels in node selectors / affinity - Add field-level warning plumbing and add pod spec warnings #101688
    • alpha seccomp annotations (Add warning messages about deprecated seccomp annotation usage #94365) - Add field-level warning plumbing and add pod spec warnings #101688
    • alpha sysctl annotations (Promote sysctl annotations to fields #63717) - Add field-level warning plumbing and add pod spec warnings #101688
  • PersistentVolumeSpec
    • deprecated node labels in volume node affinity - add deprecated warning for node beta labels in pv/sc/rc/csi storage capacity #108554
  • StorageClass
    • deprecated node labels - add deprecated warning for node beta labels in pv/sc/rc/csi storage capacity #108554
  • CSIStorageCapacity
    • deprecated node labels - add deprecated warning for node beta labels in pv/sc/rc/csi storage capacity #108554
  • RuntimeClass
    • deprecated node labels - add deprecated warning for node beta labels in pv/sc/rc/csi storage capacity #108554
• Add warnings for known bad values
  • PVC, PV, PVC template in statefulset and inline ephemeral volume
    • fractional byte storage requests (PVCs specified with quantity containing suffix m are not counted in ResourceQuota and block ResourceQuota updates #94313)
  • Pod templates
    • fractional byte requests/limits - Add field-level warning plumbing and add pod spec warnings #101688
    • duplicate hostAliases (kubectl apply drops all hostAlias entries when removing a duplicate entry from an existing object #91670, kubectl apply (client-side) removes all entries when attempting to remove a single duplicated entry in a persisted object #58477) - Add field-level warning plumbing and add pod spec warnings #101688
    • duplicate imagePullSecrets (kubectl apply removes all imagePullSecrets when user attempts to remove duplicate secrets #91629, kubectl apply (client-side) removes all entries when attempting to remove a single duplicated entry in a persisted object #58477) - Add field-level warning plumbing and add pod spec warnings #101688
    • duplicate containers[*].env (this issue, Failed to create three way merge patch when container environment variable specified multiple times #86163, Deployment objects using optional envs with the same name are unexpectedly removed during updates - but exist out of the box #93266, kubectl apply (client-side) removes all entries when attempting to remove a single duplicated entry in a persisted object #58477) - Add field-level warning plumbing and add pod spec warnings #101688
    • duplicate volumes (kubectl apply admits Deployment with duplicate volumes then fails when renaming duplicate #78266, kubectl apply (client-side) removes all entries when attempting to remove a single duplicated entry in a persisted object #58477) - Add field-level warning plumbing and add pod spec warnings #101688
    • duplicate containers[*].ports (Container Ports for pods vanishing #86273, Duplicate ports in deployment results in non-deterministic port assignment #93952, kubectl apply (client-side) removes all entries when attempting to remove a single duplicated entry in a persisted object #58477)
    • pods setting both linux and windows options? (if multi-OS images are not possible. xref fix windows container root validate #92355 (comment))
    • imagePullSecrets or volume source entries with empty secret or configmap name (Kubelet fails to retrieve secret with empty name #99454 (comment))
  • Service
    • duplicate {port,protocol} items in ports list (apiserver allows duplicate service port #59119, kubectl apply (client-side) removes all entries when attempting to remove a single duplicated entry in a persisted object #58477, PATCH merges Services with same port, different protocol #47249)
  • ... issues linked from Validation Tightening is not Generally Possible #64841 (comment)

See:

• Warning mechanism for use of deprecated APIs enhancements#1693
• https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1693-warnings
• https://kubernetes.io/blog/2020/09/03/warnings/#future-possibilities
• kubectl apply (client-side) removes all entries when attempting to remove a single duplicated entry in a persisted object #58477

/assign
/sig api-machinery
/milestone v1.20
See:

• Warning mechanism for use of deprecated APIs enhancements#1693
• https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1693-warnings
• https://kubernetes.io/blog/2020/09/03/warnings/#future-possibilities
• kubectl apply (client-side) removes all entries when attempting to remove a single duplicated entry in a persisted object #58477

/assign
/sig api-machinery
/milestone v1.20
See:

• Warning mechanism for use of deprecated APIs enhancements#1693
• https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1693-warnings
• https://kubernetes.io/blog/2020/09/03/warnings/#future-possibilities
• kubectl apply (client-side) removes all entries when attempting to remove a single duplicated entry in a persisted object #58477

/assign
/sig api-machinery
/milestone v1.20

[caesarxuchao]

cc @deads2k

Do we have a KEP discussing how to emit the warnings?

[liggitt]

The mechanism was put in place in 1.19. See https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1693-warnings

[deads2k]

The mechanism was put in place in 1.19. See https://github.com/kubernetes/enhancements/tree/master/keps/sig-api-machinery/1693-warnings

I'm thinking more about finding a way to make it easy to discover these. Are you thinking about placing the checks in validating admission or storage or somewhere else? Validating admissions seems like a reasonable place that could be mirrored for out of tree providers as well.

[deads2k]

I think that deprecated fields could/should be labeled by tag in the API itself, similar to how we handle it for the structs themselves.

[liggitt]

I think that deprecated fields could/should be labeled by tag in the API itself, similar to how we handle it for the structs themselves.

I'd be fine with that, though the immediate ones I want to warn about are specific selector label values and alpha seccomp annotations, so there's not really a place to attach declarative deprecations for those

[jsturtevant]

Another area for warnings related to "pods setting both linux and windows options" is #93220

[liggitt]

Another area for warnings related to "pods setting both linux and windows options" is #93220

is that always invalid? are multi-os images possible?

[jsturtevant]

AppArmor is a linux only security module so it is not valid on windows.

I might have misunderstood the item. What is a multi-os image? It is possible to have multi-arch image manifests.

[liggitt]

What is a multi-os image?

e.g. https://www.docker.com/blog/docker-official-images-now-multi-platform/

[jsturtevant]

Thanks for confirming. Since it is possible to have multi-os image (multi-arch) I don't think that we would be able to effectively warn for pods that set both linux and windows options. In azure we use a mutli-os/multi arch image for the pause container: mcr.microsoft.com/oss/kubernetes/pause:1.4.0 which has a manifest for linux and windows:

docker manifest inspect --verbose mcr.microsoft.com/oss/kubernetes/pause:1.4.0 | jq '.[] .Descriptor.platform'   
{
  "architecture": "amd64",
  "os": "windows",
  "os.version": "10.0.17763.1282"
}
{
  "architecture": "amd64",
  "os": "windows",
  "os.version": "10.0.18362.900"
}
{
  "architecture": "amd64",
  "os": "windows",
  "os.version": "10.0.18363.900"
}
{
  "architecture": "amd64",
  "os": "windows",
  "os.version": "10.0.19041.329"
}
{
  "architecture": "amd64",
  "os": "linux"
}

[erismaster]

@saschagrunert should this issue be milestoned to v1.23? Basing this off of kubernetes/enhancements#135

[liggitt]

next set of tasks expected to be done in 1.21

looks like 1.22

[fejta-bot]

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-contributor-experience at kubernetes/community.
/lifecycle stale

[desponda]

Hello! 👋 This issue has not been updated for a long time, so I'd like to check what's the status. The code freeze is starting March 9th, 2021 (about 1 week from now). As the Issue is tagged for 1.21, is it still planned for this release?

[voigt]

Hey there (@pacoxu, @liggitt), Bug-Triage here 👋 ,
As we already approached code freeze for 1.22 and this issue is frozen. I'd like to suggest moving this issue to 1.23 release. wdyt?

[pacoxu]

Agree.

[voigt]

Hey there (@pacoxu, @liggitt), Bug-Triage here 👋 ,
There hasn't been any activity on this issue for a couple of months now. Just want to make sure this issue is still on track for the 1.23?

[jyotimahapatra]

Since we're in 1.23 code freeze, I'm moving this to milestone 1.24. @liggitt Let me know if this is an error.

/milestone v1.24

[DiptoChakrabarty]

Hi @pacoxu @liggitt Bug Triage for v1.24 here . I wanted to know if this issue is still targeting the 1.24 release. If yes could you update its priority field.

[dims]

kick the can down the road

/milestone v1.25

[liggitt]

updated description with work completed in 1.25

[pacoxu]

• imagePullSecrets or volume source entries with empty secret or configmap name (Kubelet fails to retrieve secret with empty name #99454 (comment))

kubernetes/pkg/api/pod/warnings.go

Lines 179 to 184 in f3ae27f

	// imagePullSecrets with empty name (#99454#issuecomment-787838112)
	for i, item := range podSpec.ImagePullSecrets {
	if len(item.Name) == 0 {
	warnings = append(warnings, fmt.Sprintf("%s: invalid empty name %q", fieldPath.Child("spec", "imagePullSecrets").Index(i).Child("name"), item.Name))
	}
	}

• imagePullSecrets with empty secret or configmap name Add field-level warning plumbing and add pod spec warnings #101688

[root@paco ~]# cat po-secret.yaml
apiVersion: v1
kind: Pod
metadata:
  name: mypod
spec:
  containers:
  - name: mypod
    image: redis
    volumeMounts:
    - name: foo
      mountPath: "/etc/foo"
      readOnly: true
  volumes:
  - name: foo
    secret:
      secretName: ""
      optional: false # default setting; "mysecret" must exist
[root@paco ~]# kubectl create -f po-secret.yaml
The Pod "mypod" is invalid:
* spec.volumes[0].secret.secretName: Required value
* spec.containers[0].volumeMounts[0].name: Not found: "foo"

As the Pod spec.volumes[0].secret.secretName is required value. This is also not a problem

• volume source entries with empty secret or configmap name

[pacoxu]

I opened 2 PR to add bad values warnings.

• pvc storage request warning for fractional byte value #113238 for pvc's request storage value warning.
• add warning for dup ports in containers[*].ports #113245 for duplicate {port,protocol} items in service and container's ports.

[pacoxu]

#118547 will add a warn for env dup.

[pacoxu]

#72593 (comment) is another case that we may warn user or refuse it during creation.

A proposal would be adding a warning for pod creation with sysctl that would most likely be rejected by kubelet.

• add a warning for pod with unsafe sysctl.
• add a warning for pod with hostNetwork=true and net.ipv4.ip_local_port_range(even if it is privileged pod, it will be rejected. See ReplicaSet controller continuously creating pods failing due to SysctlForbidden #72593.)

[LucasViniciusp]

Another important case that should be addressed is when attempting to set a nodeName on the deployment spec.template.spec for NoExecute or NoSchedule tainted nodes.

#75913