Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

Proposal: Add Discriminators in All Unions/OneOf APIs #229

Closed
mengqiy opened this issue Dec 22, 2016 · 26 comments
Closed

Proposal: Add Discriminators in All Unions/OneOf APIs #229

mengqiy opened this issue Dec 22, 2016 · 26 comments

Comments

@mengqiy
Copy link
Member

mengqiy commented Dec 22, 2016 •
edited
Loading

Overview

We have a number of cases in the API where only one of a set of fields is allowed to be specified, aka undiscriminated union / oneof.

VolumeSource is the canonical example: it has fields such as emptyDir, gcePersistentDisk, awsElasticBlockStore and other 20 fields. Only one of these fields can be specified.

We should add discriminators to union / oneof APIs, since it has several advantages.

Original issue is described in kubernetes/kubernetes#35345

Advantages

Adding discriminators to all unions/oneof cases would have multiple advantages:

  1. Clients could effectively implement a switch instead of if-else trees to inspect the resource -- look at discriminator and lookup the corresponding field in a map (though differences in capitalization of the first letter in the API convention currently prevents the discriminator value from exactly matching the field name).

  2. The API server could automatically clear non-selected fields, which would be convenient for kubectl apply and other cases.

Analysis

List of Impacted APIs

In pkg/api/v1/types.go:

In pkg/authorization/types.go:

In pkg/apis/extensions/v1beta1/types.go:

Behavior

If the discriminator were set, we'd require that the field corresponding to its value were set and the APIServer (registry) could automatically clear the other fields.

If the discriminator were unset, behavior would be as before -- exactly one of the fields in the union/oneof would be required to be set and the operation would otherwise fail validation.

We should set discriminators by default. This means we need to change it accordingly when the corresponding union/oneof fields were set and unset. If so, clients can rely on this for purpose (1).

Proposed Changes

API

Add a discriminator field in all unions/oneof APIs.

The discriminator should be optional for backward compatibility. There is an example below, the field Type works as a discriminator.

type PersistentVolumeSource struct {
	// +optional
	GCEPersistentDisk *GCEPersistentDiskVolumeSource `json:"gcePersistentDisk,omitempty" protobuf:"bytes,1,opt,name=gcePersistentDisk"`
	// +optional
	AWSElasticBlockStore *AWSElasticBlockStoreVolumeSource `json:"awsElasticBlockStore,omitempty" protobuf:"bytes,2,opt,name=awsElasticBlockStore"`
	
...

	// Discriminator for PersistentVolumeSource, it can be "gcePersistentDisk", "awsElasticBlockStore" and etc.
	// +optional
	Type *string `json:"type,omitempty" protobuf:"bytes,24,opt,name=type"`
}

API Server

We need to add defaulting logic described in the Behavior section.

kubectl

No change required on kubectl.

Example Discriminators

Discriminator are Set by Default

Assume we have added a field as discussed in section API changes

We first use kubectl apply -f create a PersistentVolume using the following config:

apiVersion: v1
kind: PersistentVolume
metadata:
  name: pv0001
  annotations:
    volume.beta.kubernetes.io/storage-class: "slow"
spec:
  capacity:
    storage: 5Gi
  accessModes:
    - ReadWriteOnce
  persistentVolumeReclaimPolicy: Recycle
  hostPath:
    path: /tmp

The subsequent kubectl get should have the discriminator field set by API server:

apiVersion: v1
kind: PersistentVolume
metadata:
  annotations:
    volume.beta.kubernetes.io/storage-class: slow
  creationTimestamp: 2016-12-22T00:56:31Z
  name: pv0001
  namespace: ""
  resourceVersion: "1059564"
  selfLink: /api/v1/persistentvolumespv0001
  uid: 7a57e42b-c7e1-11e6-aa89-42010a800002
spec:
  accessModes:
  - ReadWriteOnce
  capacity:
    storage: 5Gi
  hostPath:
    path: /tmp
  persistentVolumeReclaimPolicy: Recycle
  # Discriminator showing the type is "hostPath"
  type: hostPath
status:
  phase: Available

Automatically Clear Unselected Fields

Issue kubernetes/kubernetes#34292 will be fixed if spec.strategy.type is treated as a discriminator.

Create a deployment using kubectl apply-f

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  name: nginx-deployment
spec:
  replicas: 3
  template:
    metadata:
      labels:
        app: nginx
    spec:
      containers:
      - name: nginx
        image: nginx
        ports:
        - containerPort: 80

Get the deployment back. Fields spec.strategy.type and spec.strategy.rollingUpdate have been defaulted.

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  annotations:
    deployment.kubernetes.io/revision: "1"
    kubectl.kubernetes.io/last-applied-configuration: |
      {"kind":"Deployment","apiVersion":"extensions/v1beta1","metadata":{"name":"nginx-deployment","creationTimestamp":null},"spec":{"replicas":1,"template":{"metadata":{"creationTimestamp":null,"labels":{"app":"nginx"}},"spec":{"containers":[{"name":"nginx","image":"nginx","ports":[{"containerPort":80}],"resources":{}}]}},"strategy":{}},"status":{}}
  creationTimestamp: 2016-12-22T01:23:34Z
  generation: 1
  labels:
    app: nginx
  name: nginx-deployment
  namespace: default
  resourceVersion: "1062013"
  selfLink: /apis/extensions/v1beta1/namespaces/default/deployments/nginx-deployment
  uid: 416ef165-c7e5-11e6-aa89-42010a800002
spec:
  replicas: 1
  selector:
    matchLabels:
      app: nginx
  strategy:
    # Defaulted by API server
    rollingUpdate:
      maxSurge: 1
      maxUnavailable: 1
    # Defaulted by API server
    type: RollingUpdate
...

Then we update the config file by adding

  strategy:
    type: Recreate

Apply the new config by kubectl apply -f.

The operation should succeed now, because now the API server knows to clear field spec.strategy.rollingUpdate after updating spec.strategy.type.

@kubernetes/sig-api-machinery-misc @kubernetes/api-reviewers @kubernetes/kubectl
@thockin
Copy link
Member

thockin commented Dec 22, 2016 via email

@0xmichalis
Copy link
Contributor

The API server could automatically clear non-selected fields

It's still not clear to me which is the appropriate place in the api server for doing this sort of defaulting. The rest handlers or where we set defaults?

cc: @kubernetes/sig-api-machinery-misc

@liggitt
Copy link
Member

liggitt commented Dec 22, 2016

seems like repeating information in two places, and I agree that I'd rather get an error than have fields auto-cleared

@pwittrock
Copy link
Member

Thanks for the comments.

The driving use case for this is the api server defaulting one-of-union values. The current behavior of providing an error message breaks kubectl apply's ability to update one-of fields defaulted by the server, as the values are missing from the original config and won't be cleared by apply.

We could make the union behavior configurable by a parameter to the api call so that apply can safely auto-clear server-defaulted union fields.

Thoughts on other ways can address the use case of clearing one-of fields defaulted by the server?

@thockin
Copy link
Member

thockin commented Jan 3, 2017 via email

@pwittrock
Copy link
Member

Are there any other cases where defaulting sets a value which the user
might legitimately want to un-set it later? I can't think of any off the
top of my head.

I am not aware of any, but @ymqytw did an audit of all fields that are defaulted by the server that we could look at.

@pwittrock
Copy link
Member

I also think it would be helpful to have this requirement defined as part of object / field definition instead of implementing in the object validation. Not a huge win, but would provide more consistency.

@lavalamp
Copy link
Member

lavalamp commented Jan 6, 2017 

  hostPath:
    path: /tmp
  persistentVolumeReclaimPolicy: Recycle
  # Discriminator showing the type is "hostPath"
  type: hostPath

We can't just call the discriminators "type" if there's other fields mixed in. The above snippet is super confusing. These need to be nested one level deeper, or the name has to be relevant (e.g. mountType).

@thockin
Copy link
Member

thockin commented Jan 7, 2017 via email

@lavalamp
Copy link
Member

lavalamp commented Jan 7, 2017 via email

@thockin
Copy link
Member

thockin commented Jan 7, 2017 via email

@smarterclayton
Copy link
Contributor

I'm a little uncomfortable with magic unsetting of values - I think this should be context sensitive:

  1. PATCH setting two of a one of is an error
  2. PATCH should replace a union when you specify a single member
  3. UPDATE should behave like PATCH
  4. 3-way should happen on the server (so naive clients don't have to implement union clearing)
  5. No need for magic clearing?

@thockin
Copy link
Member

thockin commented Jan 7, 2017 via email

@pwittrock
Copy link
Member

@smarterclayton

That makes a lot of sense to me. Similarly, we could introduce a struct tag patchStrategy:"replace" for maps and structs (as opposed to patchStrategy:"merge" for lists). Unions could be implemented by defining a field containing the set of Union values, and a replace strategy. This doesn't provide "1.", but should provide 2, 3 & 5. Semantically, this would be less meaningful than a tag denoting a "Union" specifically, but it is a simpler approach than trying to validate that a struct is in-fact a Union.

@thockin openapi supports extensions, so yes would could denote this through openapi. @mbohlool is more familiar with the details around this.

@thockin
Copy link
Member

thockin commented Jan 10, 2017 via email

@pwittrock pwittrock mentioned this issue Jan 10, 2017
Closed
@pwittrock
Copy link
Member

pwittrock commented Jan 10, 2017 •
edited
Loading

@thockin I was thinking there are a several different ways we could solve the underlying issue.

Option A: Add Union Support to the Api Server

Server Changes

Generalize support for Unions in the server. Instead of having the union-ness of the field hard coded into the validation logic on a per-field basic, add first class support for validating unions.

  • Validate that neither PATCH nor UPDATE set multiple values within a Union
    • Maybe add first class support for validating discriminators
  • When doing a PATCH to set one of the fields in a union, clear any other field in the union that was previously set. This is consistent with how we treat single-primitive-fields (e.g. we don't make you clear a "string" field before you change its value when patching.)
  • UPDATE replaces the entire object, so there should be no need to clear anything in this case

Go Struct (Strawman)

type Volume struct {
	...
	VolumeSource `json:",inline" protobuf:"bytes,2,opt,name=volumeSource" union:"oneof"`
}

type DeploymentSpec struct {
	...
       Strategy DeploymentStrategy `json:"strategy,omitempty" protobuf:"bytes,4,opt,name=strategy" union:"discriminator"`
       ...
}

Open Api

We would need to add extensions to the openapi spec we publish. This is something we already need to do for the patchStrategy and mergeKey struct tags, so I imagine adding 1 more will be trivial.

Option B: Add Tag Indicating To Replace Union Fields in Patch

This approach could also be used to fix the issues with kubectl apply simply by having apply always replace union fields instead of patching them. E.g. instead of setting spec.strategy.rolloutstrategy without touching the rest of spec.strategy, pathces should always replace the entire spec.strategy.

Server / Client Changes

Support a struct-tag / openapi value telling clients to replace an entire field when doing a PATCH. Does not required changes on the server, this could be only a directive to clients performing patches. Alternatively, the server could enforce that the entire field is replaced during a PATCH.

Go Struct (Strawman)

type Volume struct {
	Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
	VolumeSource `json:",inline" protobuf:"bytes,2,opt,name=volumeSource" patchStrategy:"replace"`
}

type DeploymentSpec struct {
	...
       Strategy DeploymentStrategy `json:"strategy,omitempty" protobuf:"bytes,4,opt,name=strategy" patchStrategy:"replace"`
       ...
}

Open Api

Similar to Option A

Summary

  • Option A provides richer api semantics to clients and generalizes validation of unions
  • Option B is a simpler and more flexible solution not specific to Union fields

@pwittrock
Copy link
Member

@lavalamp expressed a preference for option B when I spoke him him today. It is minimally invasive and is able to address the specific issue.

@thockin
Copy link
Member

thockin commented Jan 11, 2017 via email

@lavalamp
Copy link
Member

lavalamp commented Jan 12, 2017 via email

@pwittrock
Copy link
Member

Since it is inlined into Volume, which just has the name and inlined VolumeSource, can we treat Volume as a Union / patchStrategy=replace and get the desired behavior?

@liggitt
Copy link
Member

liggitt commented Jan 12, 2017

can we treat Volume as a Union / patchStrategy=replace and get the desired behavior?

Pretty sure we can't… "union except for the special name field" seems like it would be painful

@lavalamp
Copy link
Member

lavalamp commented Jan 12, 2017 via email

@pwittrock
Copy link
Member

I think patchStrategy=replace might work for this case then - e.g. the patch always must contain the full Volume struct, which includes the name + the union

This was referenced Jan 18, 2017
Closed
Merged
@mengqiy
Copy link
Member Author

mengqiy commented Jan 20, 2017

Created PR #278. Please review. Thanks.

@mbohlool
Copy link
Contributor

@thockin @pwittrock OpenAPI 3.0 draft has oneof support.

@mengqiy
Copy link
Member Author

mengqiy commented Mar 8, 2017

Closing this in favor of #278.

@mengqiy mengqiy closed this as completed Mar 8, 2017
shyamjvs pushed a commit to shyamjvs/community that referenced this issue Sep 22, 2017
@calebamiles
Merge pull request kubernetes#229 from kubernetes/release-go-signal
028a244 
2017-03-24 release status report
@dhellmann dhellmann mentioned this issue Sep 2, 2021
Merged
@justinsb justinsb mentioned this issue Nov 5, 2021
Closed
danehans pushed a commit to danehans/community that referenced this issue Jul 18, 2023
@geeknoid
Update to latest container. (kubernetes#229)
df50437
# for free to join this conversation on GitHub. Already have an account? # to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@lavalamp @liggitt @smarterclayton @mbohlool @mengqiy @0xmichalis @thockin @pwittrock