Merge pull request #587 from jgehrcke/jp/strict-decode-on-prepres

kubelet plugins: re-introduce strict decoding in NodePrepareResources and webhook

Author: jgehrcke

api/nvidia.com/resource/v1beta1/api.go

@@ -41,8 +41,17 @@ type Interface interface {
 	Validate() error
 }
 
-// Decoder implements a decoder for objects in this API group.
-var Decoder runtime.Decoder
+// StrictDecoder implements a decoder for objects in this API group. Fails upon
+// unknown fields in the input. Is the preferable choice when processing input
+// directly provided by the user (example: opaque config JSON provided in a
+// resource claim, validated only in the NodePrepareResources code path when no
+// validating webhook is deployed).
+var StrictDecoder runtime.Decoder
+
+// NonstrictDecoder implements a decoder for objects in this API group. Silently
+// drops unknown fields in the input. Used for deserializing checkpoint data
+// (JSON that may have been created by older or newer versions of this driver).
+var NonstrictDecoder runtime.Decoder
 
 func init() {
 	// Create a new scheme and add our types to it. If at some point in the
@@ -63,20 +72,23 @@ func init() {
 	)
 	metav1.AddToGroupVersion(scheme, schemeGroupVersion)
 
-	// Set up a json serializer to decode our types.
-	Decoder = json.NewSerializerWithOptions(
+	// Note: the strictness applies to all types defined above via
+	// AddKnownTypes(), i.e. it cannot be set per-type. That is OK in this case.
+	// Unknown fields will simply be dropped (ignored) upon decode, which is
+	// what we want. This is relevant in a downgrade case, when a checkpointed
+	// JSON document contains fields added in a later version (workload defined
+	// with a new version of this driver).
+	NonstrictDecoder = json.NewSerializerWithOptions(
+		json.DefaultMetaFactory,
+		scheme,
+		scheme,
+		json.SerializerOptions{Strict: false},
+	)
+
+	StrictDecoder = json.NewSerializerWithOptions(
 		json.DefaultMetaFactory,
 		scheme,
 		scheme,
-		json.SerializerOptions{
-			// Note: the strictness applies to all types defined above via
-			// AddKnownTypes(), i.e. it cannot be set per-type. That is OK in
-			// this case. Unknown fields will simply be dropped (ignored) upon
-			// decode, which is what we want. This is relevant in a downgrade
-			// case, when a checkpointed JSON document contains fields added in
-			// a later version (workload defined with a new version of this
-			// driver).
-			Pretty: true, Strict: false,
-		},
+		json.SerializerOptions{Strict: true},
 	)
 }

cmd/compute-domain-kubelet-plugin/device_state.go

@@ -277,8 +277,10 @@ func (s *DeviceState) updateCheckpoint(f func(*Checkpoint)) error {
 }
 
 func (s *DeviceState) prepareDevices(ctx context.Context, claim *resourceapi.ResourceClaim) (PreparedDevices, error) {
-	// Generate a mapping of each OpaqueDeviceConfigs to the Device.Results it applies to
-	configResultsMap, err := s.getConfigResultsMap(&claim.Status)
+	// Generate a mapping of each OpaqueDeviceConfigs to the Device.Results it
+	// applies to. Strict-decode: data is provided by user and may be completely
+	// unvalidated so far (in absence of validating webhook).
+	configResultsMap, err := s.getConfigResultsMap(&claim.Status, configapi.StrictDecoder)
 	if err != nil {
 		return nil, fmt.Errorf("error generating configResultsMap: %w", err)
 	}
@@ -366,8 +368,11 @@ func (s *DeviceState) prepareDevices(ctx context.Context, claim *resourceapi.Res
 }
 
 func (s *DeviceState) unprepareDevices(ctx context.Context, cs *resourceapi.ResourceClaimStatus) error {
-	// Generate a mapping of each OpaqueDeviceConfigs to the Device.Results it applies to
-	configResultsMap, err := s.getConfigResultsMap(cs)
+	// Generate a mapping of each OpaqueDeviceConfigs to the Device.Results it
+	// applies to. Non-strict decoding: do not error out on unknown fields (data
+	// source is checkpointed JSON written by potentially newer versions of this
+	// driver).
+	configResultsMap, err := s.getConfigResultsMap(cs, configapi.NonstrictDecoder)
 	if err != nil {
 		return fmt.Errorf("error generating configResultsMap: %w", err)
 	}
@@ -496,10 +501,10 @@ func (s *DeviceState) applyComputeDomainDaemonConfig(ctx context.Context, config
 	return &configState, nil
 }
 
-func (s *DeviceState) getConfigResultsMap(rcs *resourceapi.ResourceClaimStatus) (map[runtime.Object][]*resourceapi.DeviceRequestAllocationResult, error) {
+func (s *DeviceState) getConfigResultsMap(rcs *resourceapi.ResourceClaimStatus, decoder runtime.Decoder) (map[runtime.Object][]*resourceapi.DeviceRequestAllocationResult, error) {
 	// Retrieve the full set of device configs for the driver.
 	configs, err := GetOpaqueDeviceConfigs(
-		configapi.Decoder,
+		decoder,
 		DriverName,
 		rcs.Allocation.Devices.Config,
 	)

cmd/gpu-kubelet-plugin/device_state.go

@@ -265,7 +265,7 @@ func (s *DeviceState) prepareDevices(ctx context.Context, claim *resourceapi.Res
 
 	// Retrieve the full set of device configs for the driver.
 	configs, err := GetOpaqueDeviceConfigs(
-		configapi.Decoder,
+		configapi.StrictDecoder,
 		DriverName,
 		claim.Status.Allocation.Devices.Config,
 	)

cmd/webhook/main.go

@@ -249,7 +249,8 @@ func admitResourceClaimParameters(ar admissionv1.AdmissionReview) *admissionv1.A
 		}
 
 		fieldPath := fmt.Sprintf("%s.devices.config[%d].opaque.parameters", specPath, configIndex)
-		decodedConfig, err := runtime.Decode(nvapi.Decoder, config.Opaque.Parameters.Raw)
+		// Strict-decode: do not allow for users to provide unknown fields.
+		decodedConfig, err := runtime.Decode(nvapi.StrictDecoder, config.Opaque.Parameters.Raw)
 		if err != nil {
 			errs = append(errs, fmt.Errorf("error decoding object at %s: %w", fieldPath, err))
 			continue