Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Apiserver flowcontrol api models #83671

Open
wants to merge 3 commits into
base: master
from

Conversation

@yue9944882
Copy link
Member

commented Oct 9, 2019

NONE

/sig api-machinery
/kind feature

@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Oct 9, 2019

/hold cancel

Subjects []Subject `json:"subjects,omitempty" protobuf:"bytes,1,opt,name=subjects"`
// `rule` is the target verb, resource or the subresource the rule cares about. APIGroups, Resources, etc.
// Required.
Rule PolicyRule `json:"rule" protobuf:"bytes,2,opt,name=rule"`

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 10, 2019

Contributor

in RBAC we chose to use a slice of these because you want to make sure the group and resource tuple expansions are correct. You don't want to match deployments.rbac.authorization.k8s.io for instance.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 10, 2019

Member

We are keeping group and resource together, they are both in PolicyRule (copied from rbac).
What is different in this API is how rules are associated with subjects. In this API it is this struct (PolicyRuleWithSubjects) that does that binding. Where rbac has a slice of PolicyRule, this API has a slice of PolicyRuleWithSubjects.

Also, the field tag here is inconsistent between json and protobuf regarding optionality.

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 11, 2019

Contributor

That structure will require that subjects be defined multiple times to be able to grant access to resources in different groups. That is overly onerous.

// Rules can either apply to API resources (such as "pods" or "secrets") or non-resource URL paths (such as "/api"), but not both.
// NonResourceAll represents all non-resource urls.
// +optional
NonResourceURLs []string `json:"nonResourceURLs,omitempty" protobuf:"bytes,6,rep,name=nonResourceURLs"`

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 10, 2019

Contributor

I think this indicates we should have a union type for representing the rule. Back when I first wrote these, we hadn't formalized best practices.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 10, 2019

Member

And if we do not make that change then my comment on Resources applies here too.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 11, 2019

Member

The comment asserts more flexible patterns than the code implements.
The field tag does not say this is optional in protobuf.

// `queues` is a number of queues that belong to a non-exempt PriorityLevelConfiguration object. The queues exist
// independently at each apiserver. The value must be positive for a non-exempt priority level and setting it to 1
// disables shufflesharding and makes the distinguisher method irrelevant.
// TODO: sugguest a default or a way of deciding on a value.

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 10, 2019

Contributor

You must describe what happens if this is left at zero. What is a "good" value? People don't understand what this means, but my memory is that choosing 1 defeats the purpose of doing this.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 10, 2019

Member

@deads2k : You raise multiple points.

  1. "what happens if this is left at zero". I do not understand why you raise this question. The comment says "The value must be positive for a non-exempt priority level". Isn't it normally implied that when a constraint is stated in a comment, it is checked in validation?

  2. "What is a "good" value?" Do you mean what is a good default value (still thinking about the possiblity that this field can be omitted)? Or are you saying this comment should give the user guidance on how to choose a value for this field? If the former then this is moot since the field is not optional. If the latter then this makes some sense to me, but the question of guidance is not unique to this field and not even something that neatly divides among fields. The guidance we can give is the design discussion we have been having, and it is not small enough and final enough to put in an API file. I think we need to give guidance in the user-facing documentation and rely on people to look there. I think we have to settle for making the API define precisely what each field means.

  3. "People don't understand what this means, but my memory is that choosing 1 defeats the purpose of doing this". No, a value of 1 does not defeat the purpose of queuing or having a priority level, it merely says that everything of this priority level goes in the same queue. The comment already makes related points: "setting it to 1 disables shufflesharding and makes the distinguisher method irrelevant", relying on the reader to figure out that a value of 1 means one queue for the priority level. I do not understand what is difficult about that. Would the following rewording help?

Setting this field to 1 means that all requests of this priority level go in the same queue,
and this means there is no shuffle-sharding and the flow distinguisher method is moot.

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 11, 2019

Contributor

It's optional in your spec here, which means zero is allowed and you have to decide how to interpret it.

I don't think that a queue size of one by default is a good value.

Queues int32 `json:"queues,omitempty" protobuf:"varint,2,opt,name=queues"`
// `handSize` is a small positive number for applying shuffle sharding. When a request arrives at an apiserver the
// request flow identifier’s string pair is hashed and the hash value is used to shuffle the queue indices and deal
// a hand of the size specified here. If empty, the hand size will the be set to 1.

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 10, 2019

Contributor

With hand size one, don't I defeat the purpose of the shuffle? It seems like you'd want a percentage of the queues.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 10, 2019

Member

Yes, a hand size of one means there is no shuffle sharding. I think it is a poor default value. In fact, I think there is no good default value. Have a look at #76846 (comment) and propose a specific default value or formula for computing one, if you think there is a good way to compute a default value.

@yue9944882 yue9944882 force-pushed the yue9944882:flow-control-api-model branch from 3ae3dc7 to 6c433a4 Oct 15, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Oct 15, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yue9944882
To complete the pull request process, please assign lavalamp
You can assign the PR to them by writing /assign @lavalamp in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2019

/test pull-kubernetes-integration

1 similar comment
@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2019

/test pull-kubernetes-integration

@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Oct 15, 2019

/test pull-kubernetes-conformance-kind-ipv6

@MikeSpreitzer

This comment has been minimized.

Copy link
Member

commented Oct 15, 2019

See the types PriorityLevelConfigurationSpec and NonExemptPriorityLevelConfiguration in https://github.com/MikeSpreitzer/kubernetes/blob/better-union/staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go for a suggested way to use a more usual union structure.

@yue9944882 yue9944882 force-pushed the yue9944882:flow-control-api-model branch from 6c433a4 to fe2db66 Oct 16, 2019
Kind string `json:"kind" protobuf:"bytes,1,opt,name=kind"`
// `name` of the object being referenced. To match regardless of name, use '*'.
// Required.
Name string `json:"name" protobuf:"bytes,2,opt,name=name"`

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 16, 2019

Contributor

in a true union type, this and namespace would be grouped by stanza and distinct.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 17, 2019

Member

Are you suggesting something like the following?

// +union
type Subject struct {
    // Required
    // +unionDiscriminator
    Kind SubjectKind `json:"kind" protobuf:"bytes,1,opt,name=kind"`
    // +optional
    User *NamedSubject `json:"user,omitempty" protobuf:"bytes,2,opt,name=user"`
    // +optional
    Group *NamedSubject `json:"group,omitempty" protobuf:"bytes,3,opt,name=group"`
    // +optional
    ServiceAccount *NamespaceNamedSubject `json:"serviceAccount,omitempty" protobuf:"bytes,4,opt,name=serviceAccount"`
}

type SubjectKind string
const SubjectKindUser SubjectKind = "User"
const SubjectKindGroup SubjectKind = "Group"
const SubjectKindServiceAccount SubjectKind = "ServiceAccount"

type NamedSubject struct {
    // Required
    Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
}

type NamespaceNamedSubject struct {
    // Required
    Namespace string `json:"namespace" protobuf:"bytes,1,opt,name=namespace"`
    // Required
    Name string `json:"name" protobuf:"bytes,2,opt,name=name"`
}

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Oct 21, 2019

Author Member

i updated the subject to union type.

// at least one member of rules matches the request.
// There must be at least one rule for this field, or it will be rejected by the api validation.
// Required.
Rules []PolicyRulesWithSubjects `json:"rules" protobuf:"bytes,4,rep,name=rules"`

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 16, 2019

Contributor

because I can have this without any rules in it, this is logically optional. I suggest making it optional here too.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 17, 2019

Member

I am not sure which sense of "can" you mean. The current comments and validation code say you can not. You might be saying that's an inferior choice on our part, and I would tend to agree (on the general principle of not making unnecessary restrictions). The counter-argument that I understand is that a flow schema with an empty set of rules matches no requests and is thus completely inconsequential and thus we suspect it may be that way due to a client bug and we will be doing the client a favor by rejecting it.

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Oct 21, 2019

Author Member

+1 the restriction is unnecessary. updated to // +optional

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 21, 2019

Member

The JSON part of the field tag does not say omitempty --- but it should.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 21, 2019

Member

I think it's a bit weird for this to be zero length; if there's a use for that, say what it is in the comment, otherwise I think we should enforce that there be at least one entry for the reason Mike gives.

// `subjects` is the list of normal user, serviceaccount, or group that this rule cares about.
// There must be at least one subject for this field, or it will be rejected by the api validation.
// Required.
Subjects []Subject `json:"subjects" protobuf:"bytes,1,rep,name=subjects"`

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 16, 2019

Contributor

because I can have this without any rules in it, this is logically optional. I suggest making it optional here too.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 17, 2019

Member

If we make this field optional then this field becomes a regularity singularity. This semantics of this struct are, at the outmost level, an AND (i.e., of (1) subjects and (2) the union of the two sets of rules). Normally removing an item from an AND means that you don't care about the issue addressed by that item. Accordingly, omitting subjects should mean this PolicyRulesWithSubjects matches a request regardless of the request's subject. OTOH, the semantics of the subjects field itself are an OR. Normally an OR among the members of an empty list is trivially false. Accordingly, when subjects is present but empty this PolicyRulesWithSubjects should match no requests. In JSON there is a distinction between a list-valued field being omitted vs. present and empty. But our unmarshalling system erases that distinction IF the list-valued field is marked "optional" --- meaning that we can not provide the natural (regular) meaning to both JSON sources. We do not want the admins to be able to write something that they think (because of regularity) means one thing but actually gets interpreted in exactly the opposite way. That is why we have chosen to avoid that situation, by requiring that this field always be present with at least one entry.

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Oct 21, 2019

Author Member

Normally removing an item from an AND means that you don't care about the issue addressed by that item.

+1. the field cannot be an empty list here. to match all subject, we agree to use the disjunction of system:authenticated and system:unauthenticated i remember..

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 21, 2019

Member

How about we update the comment to explain how to say "match everything"? Perhaps something like the following.

// subjects is the list of normal user, serviceaccount, or group that this rule cares about.
// There must be at least one member in this slice.
// A slice that includes both the system:authenticated and system:unauthenticated user groups matches every request.
// Required.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 21, 2019

Member

I agree w/ Mike & Min.

We must update the comment to describe how to select all subjects, though.

//
// ACV(l) = ceil( SCL * ACS(l) / ( sum[priority levels k] ACS(k) ) )
//
// Required.

This comment has been minimized.

Copy link
@deads2k

deads2k Oct 16, 2019

Contributor

This is the last required field here. By scaling a default here to be a reasonable percentage of our "bootstrap policies", I think we can make this optional with a default set.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 17, 2019

Member

I do not understand what you are suggesting. Can you please elaborate?

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 21, 2019

Member

@deads2k: on re-reading I think maybe I understand what you mean. The "last" in your first sentence refers to progression in development time. You are suggesting that we define a default value for this field, by choosing a value that bears a non-sucky relation with the ACS values in the cluster initial configuration. I think that makes sense. We have two versions of the cluster initial configuration: one in the KEP (https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/20190228-priority-and-fairness.md#example-configuration) and one in the feature branch (https://github.com/kubernetes/kubernetes/blob/feature-rate-limiting/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/bootstrap/default-objects.go#L95). For both of them, I think a good default value would be 30.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 21, 2019

Member

I think David is suggesting a default, and I agree that this is useful, if for no other reason than to anchor people on the right order-of-magnitude. 100? 1000? It depends on the rest of the objects in the system, which is the default objects at first.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 21, 2019

Member

It sounds like @lavalamp is agreeing with my interpretation of @deads2k's comment. I think a default of 30 does anchor readers in the right order-of-magnitude, I chose it after reviewing the cluster initial configuration ("default objects", if you will --- but I won't, because we have used the words "default objects" to mean various things, see the KEP updates about that).

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 21, 2019

Member

30 sounds fine for our first take.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 21, 2019

Member

(I hadn't refreshed my page and hadn't seen your 2nd comment when I wrote my comment.)

This comment has been minimized.

Copy link
@yue9944882

yue9944882 Oct 22, 2019

Author Member

applied 30 as its default value, now there's no required value in the priority-level's spec

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 22, 2019

Member

The comments that I see now still say this required rather than defaulted. Did you push your latest changes?

// at least one member of rules matches the request.
// There must be at least one rule for this field, or it will be rejected by the api validation.
// Required.
Rules []PolicyRulesWithSubjects `json:"rules" protobuf:"bytes,4,rep,name=rules"`

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 17, 2019

Member

I am not sure which sense of "can" you mean. The current comments and validation code say you can not. You might be saying that's an inferior choice on our part, and I would tend to agree (on the general principle of not making unnecessary restrictions). The counter-argument that I understand is that a flow schema with an empty set of rules matches no requests and is thus completely inconsequential and thus we suspect it may be that way due to a client bug and we will be doing the client a favor by rejecting it.

// `subjects` is the list of normal user, serviceaccount, or group that this rule cares about.
// There must be at least one subject for this field, or it will be rejected by the api validation.
// Required.
Subjects []Subject `json:"subjects" protobuf:"bytes,1,rep,name=subjects"`

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 17, 2019

Member

If we make this field optional then this field becomes a regularity singularity. This semantics of this struct are, at the outmost level, an AND (i.e., of (1) subjects and (2) the union of the two sets of rules). Normally removing an item from an AND means that you don't care about the issue addressed by that item. Accordingly, omitting subjects should mean this PolicyRulesWithSubjects matches a request regardless of the request's subject. OTOH, the semantics of the subjects field itself are an OR. Normally an OR among the members of an empty list is trivially false. Accordingly, when subjects is present but empty this PolicyRulesWithSubjects should match no requests. In JSON there is a distinction between a list-valued field being omitted vs. present and empty. But our unmarshalling system erases that distinction IF the list-valued field is marked "optional" --- meaning that we can not provide the natural (regular) meaning to both JSON sources. We do not want the admins to be able to write something that they think (because of regularity) means one thing but actually gets interpreted in exactly the opposite way. That is why we have chosen to avoid that situation, by requiring that this field always be present with at least one entry.

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
Kind string `json:"kind" protobuf:"bytes,1,opt,name=kind"`
// `name` of the object being referenced. To match regardless of name, use '*'.
// Required.
Name string `json:"name" protobuf:"bytes,2,opt,name=name"`

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 17, 2019

Member

Are you suggesting something like the following?

// +union
type Subject struct {
    // Required
    // +unionDiscriminator
    Kind SubjectKind `json:"kind" protobuf:"bytes,1,opt,name=kind"`
    // +optional
    User *NamedSubject `json:"user,omitempty" protobuf:"bytes,2,opt,name=user"`
    // +optional
    Group *NamedSubject `json:"group,omitempty" protobuf:"bytes,3,opt,name=group"`
    // +optional
    ServiceAccount *NamespaceNamedSubject `json:"serviceAccount,omitempty" protobuf:"bytes,4,opt,name=serviceAccount"`
}

type SubjectKind string
const SubjectKindUser SubjectKind = "User"
const SubjectKindGroup SubjectKind = "Group"
const SubjectKindServiceAccount SubjectKind = "ServiceAccount"

type NamedSubject struct {
    // Required
    Name string `json:"name" protobuf:"bytes,1,opt,name=name"`
}

type NamespaceNamedSubject struct {
    // Required
    Namespace string `json:"namespace" protobuf:"bytes,1,opt,name=namespace"`
    // Required
    Name string `json:"name" protobuf:"bytes,2,opt,name=name"`
}
staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
// `nonResourceURLs` is a set of partial urls that a user should have access to. *s are allowed, but only as the full, final step in the path
// Rules can either apply to API resources (such as "pods" or "secrets") or non-resource URL paths (such as "/api"), but not both.
// "*" represents all non-resource urls.
// Required.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 17, 2019

Member

Since the more extensive commentary has been removed, it is no longer clear what is meant by "partial URL". But any reasonable interpretation of that phrase is inconsistent with the implementation we have so far.

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
//
// ACV(l) = ceil( SCL * ACS(l) / ( sum[priority levels k] ACS(k) ) )
//
// Required.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 17, 2019

Member

I do not understand what you are suggesting. Can you please elaborate?

@yue9944882 yue9944882 force-pushed the yue9944882:flow-control-api-model branch 3 times, most recently from d89b620 to fc3e9d6 Oct 21, 2019
@yue9944882

This comment has been minimized.

Copy link
Member Author

commented Oct 21, 2019

/test pull-kubernetes-integration

// at least one member of rules matches the request.
// There must be at least one rule for this field, or it will be rejected by the api validation.
// Required.
Rules []PolicyRulesWithSubjects `json:"rules" protobuf:"bytes,4,rep,name=rules"`

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 21, 2019

Member

The JSON part of the field tag does not say omitempty --- but it should.

// `subjects` is the list of normal user, serviceaccount, or group that this rule cares about.
// There must be at least one subject for this field, or it will be rejected by the api validation.
// Required.
Subjects []Subject `json:"subjects" protobuf:"bytes,1,rep,name=subjects"`

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 21, 2019

Member

How about we update the comment to explain how to say "match everything"? Perhaps something like the following.

// subjects is the list of normal user, serviceaccount, or group that this rule cares about.
// There must be at least one member in this slice.
// A slice that includes both the system:authenticated and system:unauthenticated user groups matches every request.
// Required.

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
//
// ACV(l) = ceil( SCL * ACS(l) / ( sum[priority levels k] ACS(k) ) )
//
// Required.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 21, 2019

Member

@deads2k: on re-reading I think maybe I understand what you mean. The "last" in your first sentence refers to progression in development time. You are suggesting that we define a default value for this field, by choosing a value that bears a non-sucky relation with the ACS values in the cluster initial configuration. I think that makes sense. We have two versions of the cluster initial configuration: one in the KEP (https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/20190228-priority-and-fairness.md#example-configuration) and one in the feature branch (https://github.com/kubernetes/kubernetes/blob/feature-rate-limiting/staging/src/k8s.io/apiserver/pkg/util/flowcontrol/bootstrap/default-objects.go#L95). For both of them, I think a good default value would be 30.

Copy link
Member

left a comment

Here is a pass over the API types, I haven't looked at the rest yet.

// at least one member of rules matches the request.
// There must be at least one rule for this field, or it will be rejected by the api validation.
// Required.
Rules []PolicyRulesWithSubjects `json:"rules" protobuf:"bytes,4,rep,name=rules"`

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 21, 2019

Member

I think it's a bit weird for this to be zero length; if there's a use for that, say what it is in the comment, otherwise I think we should enforce that there be at least one entry for the reason Mike gives.

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
// `subjects` is the list of normal user, serviceaccount, or group that this rule cares about.
// There must be at least one subject for this field, or it will be rejected by the api validation.
// Required.
Subjects []Subject `json:"subjects" protobuf:"bytes,1,rep,name=subjects"`

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 21, 2019

Member

I agree w/ Mike & Min.

We must update the comment to describe how to select all subjects, though.

staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
staging/src/k8s.io/api/flowcontrol/v1alpha1/types.go Outdated Show resolved Hide resolved
//
// ACV(l) = ceil( SCL * ACS(l) / ( sum[priority levels k] ACS(k) ) )
//
// Required.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 21, 2019

Member

I think David is suggesting a default, and I agree that this is useful, if for no other reason than to anchor people on the right order-of-magnitude. 100? 1000? It depends on the rest of the objects in the system, which is the default objects at first.

yue9944882 added 3 commits Oct 21, 2019
versioned

versioned

versioned
internal

rule list

internal

internal

rule
generated

generated
@yue9944882 yue9944882 force-pushed the yue9944882:flow-control-api-model branch from fc3e9d6 to b37bad7 Oct 22, 2019
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2019

@yue9944882: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kind b37bad7 link /test pull-kubernetes-e2e-kind
pull-kubernetes-e2e-gce b37bad7 link /test pull-kubernetes-e2e-gce

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

DistinguisherMethod *FlowDistinguisherMethod `json:"distinguisherMethod,omitempty" protobuf:"bytes,3,opt,name=distinguisherMethod"`
// `rules` describes which requests will match this flow schema. This FlowSchema matches a request if and only if
// at least one member of rules matches the request.
// if it is an empty slice, there will be no requests matching the FlowSchema.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 22, 2019

Member

nit: s/if/If/

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 22, 2019

Member

... and therefore we forbid it being empty?

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 23, 2019

Member

The earlier discussion of this has evaporated from the "Files Changed" view of this PR. To summarize:

  1. It is not necessary to forbid an empty slice of rules because an empty slice of rules has a perfectly well-defined meaning. It means the FlowSchema matches no requests, and thus has no consequences. The FlowSchema might as well not exist.

  2. The question is whether we want to make a restriction that is not "necessary" but may be helpful in preventing user mistakes.

Subjects []Subject `json:"subjects" protobuf:"bytes,1,rep,name=subjects"`
// `resourceRules` is a slice of ResourcePolicyRules that identify matching requests according to their verb and the
// resource they request to act upon.
// At least one of `resourceRules` and `nonResourceRules` has to be non-empty.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 22, 2019

Member

Our validation logic insists that exactly one of those two is non-empty. I think that is the right check, and it should be accurately documented in the comments in this file. It's a little asymmetric to document it in a comment on one of the two fields involved. Better to put this remark in the type's comment or the comments of both fields, I think.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 22, 2019

Member

hm, why? What bad thing happens if both are non-empty?

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 23, 2019

Member

Hmm, now that you mention it, there is nothing wrong with both lists being non-empty. That prohibition was inherited from the earlier way of doing the union. With the normal union structure, there is no longer a problem with both lists being non-empty. So I agree with leaving the comment as it is and making the validation follow that.

// of resources matches the request.
type ResourcePolicyRule struct {
// `verbs` is a list of matching verbs and may not be empty.
// "*" represents all verbs. if it is present, it must be the only entry.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 22, 2019

Member

How about "*" matches all verbs rather than "*" represents all verbs? Similar suggestion for similar comments.

In English writing, "it" refers to the most recent noun phrase. In the current text, that's "all verbs". But that is not what we mean. I suggest rewriting like the following.

// "*" matches all verbs and, if present, must be the only entry.

Also, please review all new English sentences and make sure each one starts with a captial letter (unless, of course, the sentence starts with a technical symbol).

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 22, 2019

Member

agree "matches" is better than "represents"

// Required.
Verbs []string `json:"verbs" protobuf:"bytes,1,rep,name=verbs"`
// `nonResourceURLs` is a set of url prefixes that a user should have access to.
// For example, ["/apis", "/apis/*"].

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 22, 2019

Member

The examples so far raise the question of whether "/api*" and "/apis/" would be allowed and have the obvious interpretations. Note that the original rbac comment answers the corresponding question about wildcards. Note also that the current implementation does not handle the exhibited prefixes as claimed in the comment.

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 22, 2019

Member

Also we need to be clear about whether partial path elements are permitted, I'd go for "no"; you have to specify complete path elements, and * must be its own element, and the final one. E.g.

  • "/ap*" is illegal
  • "/ap" is legal but matches nothing
  • "/ap/*" also matches nothing
  • "/api/*" matches all core/v1 api paths

We should document what happens if you use a prefix which covers a resource, such as "/api/"; I expect resource accesses not to ever pass through here?

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 22, 2019

Member

Maybe /metrics, /healthz/*, etc would be better examples.


// PriorityLevelConfigurationSpec is specification of a priority level
type PriorityLevelConfigurationSpec struct {
// `queuingType` indicates whether this priority level does

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 22, 2019

Member

The comment needs to be updated to match the new field name.

//
// ACV(l) = ceil( SCL * ACS(l) / ( sum[priority levels k] ACS(k) ) )
//
// bigger numbers of ACS mean more reserved concurrent requests (at the

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 22, 2019

Member

Note that everywhere else we say "assured" rather than "reserved".

This comment has been minimized.

Copy link
@lavalamp

lavalamp Oct 22, 2019

Member

Although reserved is more accurate, no? At least to me, reserved implies I have an exclusive claim while assured just means I won't be denied if I try to use it.

I think it is fine to continue to use the word assured, since I may be unique in that interpretation and we may change the behavior. But we could change this line to be "Larger values of ACS mean more assured (think: reserved) concurrent requests " since otherwise this isn't really explaining anything, it's just repeating the A in ACS.

This comment has been minimized.

Copy link
@MikeSpreitzer

MikeSpreitzer Oct 23, 2019

Member

I think that for our first release, "reserved" is a better word than "assured". I think we are using "assured" because we started with more ambitious ideas, for which "assured" is the better word.

I do not have a lot of enthusiasm for doing a comprehensive s/assured/reserved/, both because it is a lot of work and because we have ambitions that may lead us to wanting to reverse that reversal.

If you think the word "assured" needs some explanation, let's do that in its own sentence or more rather than sliding it into one parenthetic remark.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.