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

Enable protobuf in Origin for server-to-server #9814

Merged
merged 12 commits into from Jul 25, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
Expand Up @@ -126,6 +126,7 @@ test-cmd: build
# Example:
# make test-end-to-end
test-end-to-end: build
hack/env hack/verify-generated-protobuf.sh # Test the protobuf serializations when we know Docker is available
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this wired to check the upstream ones too? Actually, given protobuf rules, we probably need additional checks to ensure that we're compatible, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does not check upstream protobuf, what do you mean by "additional
checks"?

On Fri, Jul 22, 2016 at 1:16 PM, David Eads notifications@github.com
wrote:

In Makefile
#9814 (comment):

@@ -126,6 +126,7 @@ test-cmd: build

Example:

make test-end-to-end

test-end-to-end: build

  • hack/env hack/verify-generated-protobuf.sh # Test the protobuf serializations when we know Docker is available

Is this wired to check the upstream ones too? Actually, given protobuf
rules, we probably need additional checks to ensure that we're compatible,
right?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9814/files/f605a623322421bf84d1623a3037b41470c530d3..32e60e560f6d18abd70f9903411b694f01c2d539#r71913255,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p7bmzohK3-BKp6GOvCyUZtVCi2hRks5qYPsGgaJpZM4JLFc7
.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does not check upstream protobuf, what do you mean by "additional
checks"?

We had a couple API carries since shipped a v1 first. We need to make sure that never happens again or if it does, make sure that its not serialized to protobuf.

I guess we could all promise to do no evil.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That applies to conversions, clients, and deep copies too. Agree we should
have something to test, but should be done together.

On Fri, Jul 22, 2016 at 2:09 PM, David Eads notifications@github.com
wrote:

In Makefile
#9814 (comment):

@@ -126,6 +126,7 @@ test-cmd: build

Example:

make test-end-to-end

test-end-to-end: build

  • hack/env hack/verify-generated-protobuf.sh # Test the protobuf serializations when we know Docker is available

It does not check upstream protobuf, what do you mean by "additional
checks"?

We had a couple API carries since shipped a v1 first. We need to make
sure that never happens again or if it does, make sure that its not
serialized to protobuf.

I guess we could all promise to do no evil.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9814/files/f605a623322421bf84d1623a3037b41470c530d3..32e60e560f6d18abd70f9903411b694f01c2d539#r71921067,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p9exoCq9ixRWXbfQpjQ600E1jgaVks5qYQdugaJpZM4JLFc7
.

hack/test-end-to-end.sh
.PHONY: test-end-to-end

Expand Down
2 changes: 1 addition & 1 deletion api/swagger-spec/oapi-v1.json
Expand Up @@ -21648,7 +21648,7 @@
"items": {
"$ref": "v1.DeploymentTriggerPolicy"
},
"description": "Triggers determine how updates to a DeploymentConfig result in new deployments. If no triggers are defined, a new deployment can only occur as a result of an explicit client update to the DeploymentConfig with a new LatestVersion."
"description": "Triggers determine how updates to a DeploymentConfig result in new deployments. If no triggers are defined, a new deployment can only occur as a result of an explicit client update to the DeploymentConfig with a new LatestVersion. If null, defaults to having a config change trigger."
},
"replicas": {
"type": "integer",
Expand Down
11 changes: 9 additions & 2 deletions hack/update-generated-protobuf.sh
Expand Up @@ -19,9 +19,16 @@ if [[ -z "$(which protoc)" || "$(protoc --version)" != "libprotoc 3.0."* ]]; the
exit 1
fi

if [[ -z "$(which goimports)" ]]; then
echo "goimports is required to be present on your path in order to format the generated"
echo "protobuf files"
echo
exit 1
fi

os::build::setup_env

hack/build-go.sh tools/genprotobuf vendor/k8s.io/kubernetes/cmd/libs/go2idl/go-to-protobuf/protoc-gen-gogo
"${OS_ROOT}/hack/build-go.sh" tools/genprotobuf vendor/k8s.io/kubernetes/cmd/libs/go2idl/go-to-protobuf/protoc-gen-gogo
genprotobuf="$( os::build::find-binary genprotobuf )"

if [[ -z "${genprotobuf}" ]]; then
Expand All @@ -32,4 +39,4 @@ if [[ -z "${genprotobuf}" ]]; then
exit 1
fi

${genprotobuf} --output-base="${GOPATH}/src" "$@"
PATH="$( dirname "${genprotobuf}" ):${PATH}" ${genprotobuf} --output-base="${GOPATH}/src" "$@"
20 changes: 10 additions & 10 deletions pkg/authorization/api/deep_copy_generated.go
Expand Up @@ -14,7 +14,7 @@ import (

func init() {
if err := api.Scheme.AddGeneratedDeepCopyFuncs(
DeepCopy_api_AuthorizationAttributes,
DeepCopy_api_Action,
DeepCopy_api_ClusterPolicy,
DeepCopy_api_ClusterPolicyBinding,
DeepCopy_api_ClusterPolicyBindingList,
Expand Down Expand Up @@ -48,7 +48,7 @@ func init() {
}
}

func DeepCopy_api_AuthorizationAttributes(in AuthorizationAttributes, out *AuthorizationAttributes, c *conversion.Cloner) error {
func DeepCopy_api_Action(in Action, out *Action, c *conversion.Cloner) error {
out.Namespace = in.Namespace
out.Verb = in.Verb
out.Group = in.Group
Expand Down Expand Up @@ -77,7 +77,7 @@ func DeepCopy_api_ClusterPolicy(in ClusterPolicy, out *ClusterPolicy, c *convers
}
if in.Roles != nil {
in, out := in.Roles, &out.Roles
*out = make(map[string]*ClusterRole)
*out = make(ClusterRolesByName)
for key, val := range in {
if newVal, err := c.DeepCopy(val); err != nil {
return err
Expand Down Expand Up @@ -106,7 +106,7 @@ func DeepCopy_api_ClusterPolicyBinding(in ClusterPolicyBinding, out *ClusterPoli
}
if in.RoleBindings != nil {
in, out := in.RoleBindings, &out.RoleBindings
*out = make(map[string]*ClusterRoleBinding)
*out = make(ClusterRoleBindingsByName)
for key, val := range in {
if newVal, err := c.DeepCopy(val); err != nil {
return err
Expand Down Expand Up @@ -260,7 +260,7 @@ func DeepCopy_api_LocalResourceAccessReview(in LocalResourceAccessReview, out *L
if err := unversioned.DeepCopy_unversioned_TypeMeta(in.TypeMeta, &out.TypeMeta, c); err != nil {
return err
}
if err := DeepCopy_api_AuthorizationAttributes(in.Action, &out.Action, c); err != nil {
if err := DeepCopy_api_Action(in.Action, &out.Action, c); err != nil {
return err
}
return nil
Expand All @@ -270,7 +270,7 @@ func DeepCopy_api_LocalSubjectAccessReview(in LocalSubjectAccessReview, out *Loc
if err := unversioned.DeepCopy_unversioned_TypeMeta(in.TypeMeta, &out.TypeMeta, c); err != nil {
return err
}
if err := DeepCopy_api_AuthorizationAttributes(in.Action, &out.Action, c); err != nil {
if err := DeepCopy_api_Action(in.Action, &out.Action, c); err != nil {
return err
}
out.User = in.User
Expand Down Expand Up @@ -309,7 +309,7 @@ func DeepCopy_api_Policy(in Policy, out *Policy, c *conversion.Cloner) error {
}
if in.Roles != nil {
in, out := in.Roles, &out.Roles
*out = make(map[string]*Role)
*out = make(RolesByName)
for key, val := range in {
if newVal, err := c.DeepCopy(val); err != nil {
return err
Expand Down Expand Up @@ -338,7 +338,7 @@ func DeepCopy_api_PolicyBinding(in PolicyBinding, out *PolicyBinding, c *convers
}
if in.RoleBindings != nil {
in, out := in.RoleBindings, &out.RoleBindings
*out = make(map[string]*RoleBinding)
*out = make(RoleBindingsByName)
for key, val := range in {
if newVal, err := c.DeepCopy(val); err != nil {
return err
Expand Down Expand Up @@ -468,7 +468,7 @@ func DeepCopy_api_ResourceAccessReview(in ResourceAccessReview, out *ResourceAcc
if err := unversioned.DeepCopy_unversioned_TypeMeta(in.TypeMeta, &out.TypeMeta, c); err != nil {
return err
}
if err := DeepCopy_api_AuthorizationAttributes(in.Action, &out.Action, c); err != nil {
if err := DeepCopy_api_Action(in.Action, &out.Action, c); err != nil {
return err
}
return nil
Expand Down Expand Up @@ -624,7 +624,7 @@ func DeepCopy_api_SubjectAccessReview(in SubjectAccessReview, out *SubjectAccess
if err := unversioned.DeepCopy_unversioned_TypeMeta(in.TypeMeta, &out.TypeMeta, c); err != nil {
return err
}
if err := DeepCopy_api_AuthorizationAttributes(in.Action, &out.Action, c); err != nil {
if err := DeepCopy_api_Action(in.Action, &out.Action, c); err != nil {
return err
}
out.User = in.User
Expand Down
32 changes: 22 additions & 10 deletions pkg/authorization/api/types.go
Expand Up @@ -105,6 +105,8 @@ type RoleBinding struct {
RoleRef kapi.ObjectReference
}

type RolesByName map[string]*Role

// +genclient=true

// Policy is a object that holds all the Roles for a particular namespace. There is at most
Expand All @@ -117,9 +119,11 @@ type Policy struct {
LastModified unversioned.Time

// Roles holds all the Roles held by this Policy, mapped by Role.Name
Roles map[string]*Role
Roles RolesByName
}

type RoleBindingsByName map[string]*RoleBinding

// PolicyBinding is a object that holds all the RoleBindings for a particular namespace. There is
// one PolicyBinding document per referenced Policy namespace
type PolicyBinding struct {
Expand All @@ -133,7 +137,7 @@ type PolicyBinding struct {
// PolicyRef is a reference to the Policy that contains all the Roles that this PolicyBinding's RoleBindings may reference
PolicyRef kapi.ObjectReference
// RoleBindings holds all the RoleBindings held by this PolicyBinding, mapped by RoleBinding.Name
RoleBindings map[string]*RoleBinding
RoleBindings RoleBindingsByName
}

// SelfSubjectRulesReview is a resource you can create to determine which actions you can perform in a namespace
Expand Down Expand Up @@ -171,8 +175,10 @@ type ResourceAccessReviewResponse struct {
// Namespace is the namespace used for the access review
Namespace string
// Users is the list of users who can perform the action
// +genconversion=false
Users sets.String
// Groups is the list of groups who can perform the action
// +genconversion=false
Groups sets.String

// EvaluationError is an indication that some error occurred during resolution, but partial results can still be returned.
Expand All @@ -187,7 +193,7 @@ type ResourceAccessReview struct {
unversioned.TypeMeta

// Action describes the action being tested
Action AuthorizationAttributes
Action
Copy link
Contributor

Choose a reason for hiding this comment

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

anonymous inclusion makes me sad. We can't get autogeneration and keep this named?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Name has to be the same as the type if on one side it's anonymous (which it
has to be to work correctly in JSON). What I'll do is a follow up to make
the internal be Action Action.

On Fri, Jul 22, 2016 at 1:17 PM, David Eads notifications@github.com
wrote:

In pkg/authorization/api/types.go
#9814 (comment):

@@ -189,7 +189,7 @@ type ResourceAccessReview struct {
unversioned.TypeMeta

// Action describes the action being tested
  • Action AuthorizationAttributes
  • Action

anonymous inclusion makes me sad. We can't get autogeneration and keep
this named?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9814/files/07c9f15316a1b3bd1893ee35004b6e2423917143..c983121be658b72bc96062df167bbb3a9160a1a6#r71913346,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_pwg3pUbwSx6yAe1yBxdjQQuYP_Ewks5qYPsqgaJpZM4JLFc7
.

}

// SubjectAccessReviewResponse describes whether or not a user or group can perform an action
Expand All @@ -207,10 +213,11 @@ type SubjectAccessReview struct {
unversioned.TypeMeta

// Action describes the action being tested
Action AuthorizationAttributes
Action
// User is optional. If both User and Groups are empty, the current authenticated user is used.
User string
// Groups is optional. Groups is the list of groups to which the User belongs.
// +genconversion=false
Groups sets.String
// Scopes to use for the evaluation. Empty means "use the unscoped (full) permissions of the user/groups".
// Nil for a self-SAR, means "use the scopes on this request".
Expand All @@ -223,27 +230,28 @@ type LocalResourceAccessReview struct {
unversioned.TypeMeta

// Action describes the action being tested
Action AuthorizationAttributes
Action
}

// LocalSubjectAccessReview is an object for requesting information about whether a user or group can perform an action in a particular namespace
type LocalSubjectAccessReview struct {
unversioned.TypeMeta

// Action describes the action being tested. The Namespace element is FORCED to the current namespace.
Action AuthorizationAttributes
Action
// User is optional. If both User and Groups are empty, the current authenticated user is used.
User string
// Groups is optional. Groups is the list of groups to which the User belongs.
// +genconversion=false
Groups sets.String
// Scopes to use for the evaluation. Empty means "use the unscoped (full) permissions of the user/groups".
// Nil for a self-SAR, means "use the scopes on this request".
// Nil for a regular SAR, means the same as empty.
Scopes []string
}

// AuthorizationAttributes describes a request to be authorized
type AuthorizationAttributes struct {
// Action describes a request to be authorized
type Action struct {
// Namespace is the namespace of the action being requested. Currently, there is no distinction between no namespace and all namespaces
Namespace string
// Verb is one of: get, list, watch, create, update, delete
Expand Down Expand Up @@ -327,6 +335,8 @@ type ClusterRoleBinding struct {
RoleRef kapi.ObjectReference
}

type ClusterRolesByName map[string]*ClusterRole

// ClusterPolicy is a object that holds all the ClusterRoles for a particular namespace. There is at most
// one ClusterPolicy document per namespace.
type ClusterPolicy struct {
Expand All @@ -338,9 +348,11 @@ type ClusterPolicy struct {
LastModified unversioned.Time

// Roles holds all the ClusterRoles held by this ClusterPolicy, mapped by Role.Name
Roles map[string]*ClusterRole
Roles ClusterRolesByName
}

type ClusterRoleBindingsByName map[string]*ClusterRoleBinding

// ClusterPolicyBinding is a object that holds all the ClusterRoleBindings for a particular namespace. There is
// one ClusterPolicyBinding document per referenced ClusterPolicy namespace
type ClusterPolicyBinding struct {
Expand All @@ -354,7 +366,7 @@ type ClusterPolicyBinding struct {
// ClusterPolicyRef is a reference to the ClusterPolicy that contains all the ClusterRoles that this ClusterPolicyBinding's RoleBindings may reference
PolicyRef kapi.ObjectReference
// RoleBindings holds all the RoleBindings held by this ClusterPolicyBinding, mapped by RoleBinding.Name
RoleBindings map[string]*ClusterRoleBinding
RoleBindings ClusterRoleBindingsByName
}

// ClusterPolicyList is a collection of ClusterPolicies
Expand Down