protobuf loses distinction between nil and empty slice #13419

liggitt · 2017-03-16T21:31:26Z

Origin has more resources that make use of that distinction than kube does

Need to resolve this before we release with protobuf storage enabled (or ship with controllers speaking protobuf)

Interesting fields

In this table, "Differentiated" indicates whether Origin treats nil and [] interchangeably.

Field	Shipped	`omitempty`	Nil-persistable	Differentiated	Fix	Notes
`v1.BrokerTemplateInstanceSpec.BindingIDs`	No	No	No	No	Add `omitempty`	Enforced by generated internal→external conversion
`v1.BuildConfigSpec.Triggers`	Yes	No	No	No		False positive; Enforced by custom external→internal conversion
`v1.BuildRequest.TriggeredBy`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.BuildSpec.TriggeredBy`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.ClusterRole.Rules`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.ClusterRoleBinding.Subjects`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.ClusterRoleScopeRestriction.Namespaces`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.ClusterRoleScopeRestriction.RoleNames`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.DeploymentDetails.Causes`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.EgressNetworkPolicySpec.Egress`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.ExecNewPodHook.Command`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.GroupRestriction.Groups`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.GroupRestriction.Selectors`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.Image.DockerImageLayers`	Yes	No	Yes	No	TBD	None
`v1.ImageSignature.Content`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.ImageSource.Paths`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.LocalSubjectAccessReview.GroupsSlice`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.PodSecurityPolicyReviewStatus.AllowedServiceAccounts`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.PolicyRule.APIGroups`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.PolicyRule.Resources`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.PolicyRule.Verbs`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.ResourceAccessReviewResponse.GroupsSlice`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.ResourceAccessReviewResponse.UsersSlice`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.Role.Rules`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.RoleBinding.Subjects`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.RouteStatus.Ingress`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.ServiceAccountRestriction.Namespaces`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.ServiceAccountRestriction.ServiceAccounts`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.SubjectAccessReview.GroupsSlice`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.SubjectRulesReviewSpec.Groups`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.SubjectRulesReviewStatus.Rules`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.Template.Objects`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.TemplateInstanceStatus.Conditions`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.User.Groups`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.User.Identities`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.UserRestriction.Groups`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.UserRestriction.Users`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion
`v1.UserRestriction.Selectors`	Yes	No	No	No		False positive; Enforced by generated internal→external conversion

The text was updated successfully, but these errors were encountered:

liggitt · 2017-03-21T03:59:07Z

Security Context Constraints needs the protobuf wrapper fix to make this nullable

	// assume a nil volume slice is allowing everything for backwards compatibility
	if defaultAllowedVolumes == nil {
		defaultAllowedVolumes = sets.NewString(string(FSTypeAll))
	}

liggitt · 2017-03-21T04:00:37Z

type SelfSubjectRulesReviewSpec struct {
	// Scopes to use for the evaluation.  Empty means "use the unscoped (full) permissions of the user/groups".
	// Nil for a self-SubjectRulesReview, means "use the scopes on this request".
	// Nil for a regular SubjectRulesReview, means the same as empty.
	Scopes []string
}

edit: nevermind, already has proto nullable fix

liggitt · 2017-03-21T04:39:21Z

func SetDefaults_DeploymentConfigSpec(obj *DeploymentConfigSpec) {
	if obj.Triggers == nil {
		obj.Triggers = []DeploymentTriggerPolicy{
			{Type: DeploymentTriggerOnConfigChange},
		}
	}

edit: nevermind, already has proto nullable fix

0xmichalis · 2017-03-22T01:40:08Z

This seems like a problem for triggers because empty slice means "no triggers". Will DCs with "no triggers" end up being defaulted to ConfigChange once they do the first round trip with the new storage format?

liggitt · 2017-03-22T02:26:45Z

triggers aliases the slice to a different type, and annotates it as nullable for protobuf, which does crazy things to make protobuf be able to distinguish between null and []

0xmichalis · 2017-03-22T02:37:35Z

Ok, that change makes sense now.

mfojtik · 2017-03-24T09:36:25Z

@liggitt i guess we need to pick kubernetes/kubernetes#43422 for 1.6?

@sttts @ncdc can you pick this for the rebase?

ncdc · 2017-03-24T10:40:07Z

Yes

…

On Fri, Mar 24, 2017 at 5:36 AM Michal Fojtik ***@***.***> wrote: @liggitt <https://github.com/liggitt> i guess we need to pick kubernetes/kubernetes#43422 <kubernetes/kubernetes#43422> for 1.6? @sttts <https://github.com/sttts> @ncdc <https://github.com/ncdc> can you pick this for the rebase? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#13419 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAABYkhqS86jqdj5_tEZn3AM1l1U3DpIks5ro46fgaJpZM4Mf59C> .

mfojtik · 2017-03-27T10:16:36Z

@ncdc feel free to close it via rebase PR merge

ncdc · 2017-03-28T14:49:43Z

Not a release blocker right now (but it will be once the 1.6 rebase lands). Lowering priority to get this off the blocker list in the immediate term.

ncdc · 2017-04-19T15:31:55Z

From build's CommonSpec:

        // NodeSelector is a selector which must be true for the build pod to fit on a node
	// If nil, it can be overridden by default build nodeselector values for the cluster.
	// If set to an empty map or a map with any values, default build nodeselector values
	// are ignored.
	NodeSelector map[string]string

liggitt · 2017-05-13T02:45:50Z

SCC Volumes field is still missing the nullable fix

liggitt · 2017-05-13T18:01:16Z

here's the list of all slice fields in origin that do not specify omitempty. we need to review this list and:

mark as false positives any which validation does not allow to be persisted empty
add omitempty to as many as we can compatibly
add wrapper types to marshal the rest to [], even when nil (and figure out if that breaks protobuf for old clients)

liggitt · 2017-05-15T20:12:16Z

@bparees is it important for the build instantiate API to treat nil TriggeredBy fields differently than set-but-empty TriggeredBy fields?

// Create instantiates a new build from a build configuration
func (s *InstantiateREST) Create(ctx apirequest.Context, obj runtime.Object) (runtime.Object, error) {
	if err := rest.BeforeCreate(Strategy, ctx, obj); err != nil {
		return nil, err
	}

	request := obj.(*buildapi.BuildRequest)
	if request.TriggeredBy == nil {
		buildTriggerCauses := []buildapi.BuildTriggerCause{}
		request.TriggeredBy = append(buildTriggerCauses,
			buildapi.BuildTriggerCause{
				Message: buildapi.BuildTriggerCauseManualMsg,
			},
		)
	}
	return s.generator.Instantiate(ctx, request)
}

bparees · 2017-05-15T20:15:29Z

No, I don't think so. If it's set-but-empty we'd want to do the same thing as we're doing for nil here, add in the "manual cause" triggercause.

0xmichalis · 2017-05-15T20:34:55Z

v1.DeploymentDetails.Causes is a slice of type []v1.DeploymentCause

doesn't care about nil vs empty

sdodson · 2017-05-30T14:24:46Z

@ingvagabund relevant to your interests

liggitt · 2017-06-20T15:07:55Z

fixed SCC.Volumes field, release noted the behavior change for other fields in openshift/openshift-docs#4021 (comment)

liggitt added this to the 1.6.0 milestone Mar 16, 2017

pweil- added component/restapi kind/bug Categorizes issue or PR as related to a bug. priority/P1 labels Mar 20, 2017

pweil- assigned mfojtik Mar 20, 2017

pweil- added the area/compatibility label Mar 20, 2017

mfojtik assigned ncdc and unassigned mfojtik Mar 27, 2017

sttts mentioned this issue Mar 27, 2017

WIP: Rebase 1.6.0 beta3 #13483

Closed

2 tasks

liggitt mentioned this issue Mar 28, 2017

support for gitlab and bitbucket webhooks #13389

Merged

ncdc added priority/P2 and removed priority/P1 labels Mar 28, 2017

sttts mentioned this issue Apr 6, 2017

Rebase 1.6.1 #13653

Merged

13 tasks

ncdc added the kind/post-rebase label Apr 18, 2017

sttts referenced this issue in sttts/origin Apr 19, 2017

REVISIT/TODO: fix-test: semantic equality for pkg/deploy roundtrip tests

766b1de

deads2k closed this as completed in #13653 Apr 27, 2017

liggitt reopened this May 13, 2017

liggitt added the priority/P1 label May 13, 2017

liggitt removed the priority/P2 label May 13, 2017

liggitt self-assigned this May 23, 2017

liggitt mentioned this issue May 30, 2017

Decide when to switch to etcd3, protobuf #14029

Closed

liggitt assigned ironcladlou and unassigned ncdc Jun 5, 2017

ironcladlou mentioned this issue Jun 8, 2017

Eliminate nil/empty distinction for new v1 field #14532

Merged

liggitt mentioned this issue Jun 14, 2017

Allow setting volumes:["none"] to disallow all volume types in SCC #14625

Merged

liggitt closed this as completed Jun 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

protobuf loses distinction between nil and empty slice #13419

protobuf loses distinction between nil and empty slice #13419

liggitt commented Mar 16, 2017 •

edited by ironcladlou

Loading

liggitt commented Mar 21, 2017 •

edited

Loading

liggitt commented Mar 21, 2017 •

edited

Loading

liggitt commented Mar 21, 2017 •

edited

Loading

0xmichalis commented Mar 22, 2017

liggitt commented Mar 22, 2017 •

edited

Loading

0xmichalis commented Mar 22, 2017

mfojtik commented Mar 24, 2017

ncdc commented Mar 24, 2017 via email

mfojtik commented Mar 27, 2017

ncdc commented Mar 28, 2017

ncdc commented Apr 19, 2017

liggitt commented May 13, 2017

liggitt commented May 13, 2017 •

edited

Loading

liggitt commented May 15, 2017

bparees commented May 15, 2017

0xmichalis commented May 15, 2017

sdodson commented May 30, 2017

liggitt commented Jun 20, 2017

protobuf loses distinction between nil and empty slice #13419

protobuf loses distinction between nil and empty slice #13419

Comments

liggitt commented Mar 16, 2017 • edited by ironcladlou Loading

Interesting fields

liggitt commented Mar 21, 2017 • edited Loading

liggitt commented Mar 21, 2017 • edited Loading

liggitt commented Mar 21, 2017 • edited Loading

0xmichalis commented Mar 22, 2017

liggitt commented Mar 22, 2017 • edited Loading

0xmichalis commented Mar 22, 2017

mfojtik commented Mar 24, 2017

ncdc commented Mar 24, 2017 via email

mfojtik commented Mar 27, 2017

ncdc commented Mar 28, 2017

ncdc commented Apr 19, 2017

liggitt commented May 13, 2017

liggitt commented May 13, 2017 • edited Loading

liggitt commented May 15, 2017

bparees commented May 15, 2017

0xmichalis commented May 15, 2017

sdodson commented May 30, 2017

liggitt commented Jun 20, 2017

liggitt commented Mar 16, 2017 •

edited by ironcladlou

Loading

liggitt commented Mar 21, 2017 •

edited

Loading

liggitt commented Mar 21, 2017 •

edited

Loading

liggitt commented Mar 21, 2017 •

edited

Loading

liggitt commented Mar 22, 2017 •

edited

Loading

liggitt commented May 13, 2017 •

edited

Loading