Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are still many redundant CRUD tests part of this file, which don't validate anything.

  1. Lines 7-37: autoSizingReserved true/false/omitted - Basic CRUD, no validation
  2. Lines 40-50: logLevel = 4 - Not a boundary, no validation value
  3. Lines 51-59: logLevel omitted - Redundant
  4. Lines 98-136: machineConfigPoolSelector worker/master/omitted - No validation logic
  5. Lines 139-160: kubeletConfig with maxPods/omitted - Basic CRUD
  6. Lines 163-188: tlsSecurityProfile Old/Intermediate - Redundant (both valid values, no validation difference)
  7. Lines 220-228: tlsSecurityProfile omitted - Redundant
  8. Lines 231-253: Combined fields - No validation logic
  9. Lines 257-272: Update autoSizingReserved - No validation logic
  10. Lines 275-288: Remove logLevel - Redundant with onCreate omit

Tests that should be kept (validation logic):

  1. Lines 60-70: logLevel = 0 (minimum boundary)
  2. Lines 71-81: logLevel = 10 (maximum boundary)
  3. Lines 82-88: logLevel = -1 (reject below min)
  4. Lines 89-95: logLevel = 11 (reject above max)
  5. Lines 189-196: tlsSecurityProfile Modern (CEL XValidation rejection)
  6. Lines 197-208: tlsSecurityProfile Custom (CEL XValidation rejection)
  7. Lines 209-219: tlsSecurityProfile without type (CEL XValidation rejection)

Original file line number Diff line number Diff line change
@@ -0,0 +1,288 @@
apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this
name: "KubeletConfig"
crdName: kubeletconfigs.machineconfiguration.openshift.io
tests:
onCreate:
# AutoSizingReserved tests
- name: Should be able to set autoSizingReserved to true
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
autoSizingReserved: true
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
autoSizingReserved: true
- name: Should be able to set autoSizingReserved to false
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
autoSizingReserved: false
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
autoSizingReserved: false
- name: Should be able to omit autoSizingReserved
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec: {}
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec: {}

# LogLevel tests
- name: Should be able to set logLevel to 4
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
logLevel: 4
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
logLevel: 4
- name: Should be able to omit logLevel
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec: {}
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec: {}
- name: Should be able to set logLevel to 0
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
logLevel: 0
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
logLevel: 0
- name: Should be able to set logLevel to 10
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
logLevel: 10
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
logLevel: 10
- name: Should reject logLevel less than 0
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
logLevel: -1
expectedError: "Invalid value"
- name: Should reject logLevel greater than 10
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
logLevel: 11
expectedError: "Invalid value"

# MachineConfigPoolSelector tests
- name: Should be able to set machineConfigPoolSelector for worker pool
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
machineConfigPoolSelector:
matchLabels:
pools.operator.machineconfiguration.openshift.io/worker: ""
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
machineConfigPoolSelector:
matchLabels:
pools.operator.machineconfiguration.openshift.io/worker: ""
- name: Should be able to set machineConfigPoolSelector for master pool
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
machineConfigPoolSelector:
matchLabels:
pools.operator.machineconfiguration.openshift.io/master: ""
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
machineConfigPoolSelector:
matchLabels:
pools.operator.machineconfiguration.openshift.io/master: ""
- name: Should be able to omit machineConfigPoolSelector
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec: {}
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec: {}

# KubeletConfig tests
- name: Should be able to set kubeletConfig with maxPods
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
kubeletConfig:
maxPods: 250
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
kubeletConfig:
maxPods: 250
- name: Should be able to omit kubeletConfig
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec: {}
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec: {}

# TLSSecurityProfile tests
- name: Should be able to set tlsSecurityProfile to Old
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
tlsSecurityProfile:
type: Old
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
tlsSecurityProfile:
type: Old
- name: Should be able to set tlsSecurityProfile to Intermediate
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
tlsSecurityProfile:
type: Intermediate
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
tlsSecurityProfile:
type: Intermediate
- name: Should reject tlsSecurityProfile with Modern type
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
tlsSecurityProfile:
type: Modern
expectedError: "only Old and Intermediate TLS profiles are supported for kubelet"
- name: Should reject tlsSecurityProfile with Custom type
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
tlsSecurityProfile:
type: Custom
custom:
ciphers:
- ECDHE-ECDSA-AES128-GCM-SHA256
minTLSVersion: VersionTLS12
expectedError: "only Old and Intermediate TLS profiles are supported for kubelet"
- name: Should reject tlsSecurityProfile without type field
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
tlsSecurityProfile:
custom:
ciphers:
- ECDHE-ECDSA-AES128-GCM-SHA256
minTLSVersion: VersionTLS12
expectedError: "only Old and Intermediate TLS profiles are supported for kubelet"
- name: Should be able to omit tlsSecurityProfile
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec: {}
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec: {}

# Combined fields tests
- name: Should be able to set multiple fields together
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
autoSizingReserved: true
logLevel: 2
machineConfigPoolSelector:
matchLabels:
pools.operator.machineconfiguration.openshift.io/worker: ""
kubeletConfig:
maxPods: 250
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
autoSizingReserved: true
logLevel: 2
machineConfigPoolSelector:
matchLabels:
pools.operator.machineconfiguration.openshift.io/worker: ""
kubeletConfig:
maxPods: 250

onUpdate:
# AutoSizingReserved update tests
- name: Should be able to update autoSizingReserved from true to false
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
autoSizingReserved: true
updated: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
autoSizingReserved: false
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
autoSizingReserved: false

# LogLevel update tests
- name: Should be able to remove logLevel
initial: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec:
logLevel: 2
updated: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec: {}
expected: |
apiVersion: machineconfiguration.openshift.io/v1
kind: KubeletConfig
spec: {}
34 changes: 24 additions & 10 deletions machineconfiguration/v1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -737,27 +737,41 @@ type KubeletConfig struct {
Status KubeletConfigStatus `json:"status"`
}

// KubeletConfigSpec defines the desired state of KubeletConfig
// KubeletConfigSpec configures the kubelet running on cluster nodes.
type KubeletConfigSpec struct {
// autoSizingReserved controls whether system-reserved CPU and memory are automatically
// calculated based on each node's installed capacity. When enabled, prevents node failure
// from resource starvation of system components (kubelet, CRI-O) without manual configuration.
// When unset, defaults to true for worker nodes and false for control plane nodes.
// Set to false to disable and use manual settings.
// +optional
AutoSizingReserved *bool `json:"autoSizingReserved,omitempty"`
// logLevel sets the kubelet log verbosity, controlling the amount of detail in kubelet logs.
// Valid values range from 0 (minimal logging) to 10 (maximum verbosity with trace-level detail).
// Higher log levels may impact node performance. When omitted, the platform chooses a reasonable default,
// which is subject to change over time. The current default is 2 (standard informational logging).
// +kubebuilder:validation:Minimum=0
// +kubebuilder:validation:Maximum=10
Comment on lines +753 to +754
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we have not enforced this in production clusters? Hm, then it would be a breaking change and we can't apply it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good opportunity to fix this. I understand it's a breaking change from an API perspective, but it's already validated in the MCO: https://github.com/openshift/machine-config-operator/blob/main/pkg/controller/kubelet-config/helpers.go#L353
We are effectively adding a first line of defense to reject values < 0 or > 10 immediately.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good, but two things to clarify:

  • Does the cluster degrade when this is being enforced now and users put in an invalid value?
  • Let's compare the scenarios of putting an invalid value in a running cluster. What are the error messages and is the user feedback now more or less useful?

// +optional
LogLevel *int32 `json:"logLevel,omitempty"`

// machineConfigPoolSelector selects which pools the KubeletConfig shoud apply to.
// A nil selector will result in no pools being selected.
// machineConfigPoolSelector selects which pools the KubeletConfig should apply to.
// A nil selector results in no pools being selected, meaning this kubelet configuration
// will not be applied to any nodes in the cluster.
// +optional
MachineConfigPoolSelector *metav1.LabelSelector `json:"machineConfigPoolSelector,omitempty"`
// kubeletConfig fields are defined in kubernetes upstream. Please refer to the types defined in the version/commit used by
// OpenShift of the upstream kubernetes. It's important to note that, since the fields of the kubelet configuration are directly fetched from
// upstream the validation of those values is handled directly by the kubelet. Please refer to the upstream version of the relevant kubernetes
// for the valid values of these fields. Invalid values of the kubelet configuration fields may render cluster nodes unusable.
// kubeletConfig contains upstream Kubernetes kubelet configuration fields.
// Values are validated by the kubelet itself. Invalid values may render nodes unusable.
// Refer to OpenShift documentation for the Kubernetes version corresponding to your
// OpenShift release to find valid kubelet configuration options.
// +optional
KubeletConfig *runtime.RawExtension `json:"kubeletConfig,omitempty"`

// If unset, the default is based on the apiservers.config.openshift.io/cluster resource.
// Note that only Old and Intermediate profiles are currently supported, and
// the maximum available minTLSVersion is VersionTLS12.
// tlsSecurityProfile configures TLS settings for the kubelet.
// When omitted, the TLS configuration defaults to the value from apiservers.config.openshift.io/cluster.
// When specified, the type field is required and must be set to either "Old" or "Intermediate".
// Modern and Custom TLS profiles are not supported for kubelet; maximum minTLSVersion is VersionTLS12.
// +kubebuilder:validation:XValidation:rule="has(self.type) && (self.type == 'Old' || self.type == 'Intermediate')",message="only Old and Intermediate TLS profiles are supported for kubelet"
// +optional
TLSSecurityProfile *configv1.TLSSecurityProfile `json:"tlsSecurityProfile,omitempty"`
}
Expand Down
Loading