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.

Sign up for GitHub

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

Jump to bottom

Allow StorageFactory to wrap encoders and decoders #40624

Merged
merged 1 commit into from Feb 2, 2017
Merged

Allow StorageFactory to wrap encoders and decoders #40624

merged 1 commit into from Feb 2, 2017

Conversation

smarterclayton
Copy link
Contributor

Prepares for allowing encryption at rest of resources as well as any
other lower level optimization we might chose to implement.

Also cleans up a bunch of ugly code.

@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 27, 2017
@smarterclayton
Copy link
Contributor Author

@enj since you also touched this code recently. This is to enable secret encryption and put the right hook in at the bottom of the stack.

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jan 27, 2017
@smarterclayton smarterclayton added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jan 27, 2017
@smarterclayton smarterclayton added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jan 30, 2017
sttts
sttts reviewed Jan 30, 2017
View reviewed changes
pkg/genericapiserver/server/storage_factory.go Outdated
@@ -95,6 +107,34 @@ type groupResourceOverrides struct {
// of exposing one set of concepts. autoscaling.HPA and extensions.HPA as a for instance
// The order of the slice matters! It is the priority order of lookup for finding a storage location
cohabitatingResources []schema.GroupResource
// encoderChainFn is optional and may wrap the provided encoder prior to being serialized.
encoderChainFn func(runtime.Encoder) runtime.Encoder
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ChainFn/Decorator/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense

@k8s-github-robot k8s-github-robot assigned deads2k and unassigned liggitt Jan 30, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2017
@smarterclayton
Copy link
Contributor Author

Any other comments?

sttts
sttts reviewed Feb 1, 2017
View reviewed changes
pkg/genericapiserver/server/storage_factory.go Outdated
}

// StorageCodecOptions are the arguments passed to newStorageCodecFn
type StorageCodecOptions struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be called StorageCodecConfig, compare the other structs in config.go.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have the Options concept as well inside the options subfolder.

sttts
sttts reviewed Feb 1, 2017
View reviewed changes
pkg/genericapiserver/server/storage_factory.go Outdated
if len(exactResourceOverride.mediaType) != 0 {
etcdMediaType = exactResourceOverride.mediaType
// operate on copy
config := s.StorageConfig
Copy link
Contributor

Choose a reason for hiding this comment

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

storageConfig
codecConfig

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@sttts
Copy link
Contributor

sttts commented Feb 1, 2017

For naming consistency in genericapiserver: StorageCodec{Options -> Config}

Otherwise, lgtm

@smarterclayton
Allow StorageFactory to wrap encoders and decoders
494eeaa 
Prepares for allowing encryption at rest of resources as well as any
other lower level optimization we might chose to implement.

Also cleans up a bunch of ugly code.
@smarterclayton
Copy link
Contributor Author

updated, applying label.

@smarterclayton smarterclayton added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2017
@enj
Copy link
Member

enj commented Feb 1, 2017

Seems reasonable. I am assuming sometime in the future we will add a serializer that does encryption and decryption of specific objects?

@smarterclayton
Copy link
Contributor Author

smarterclayton commented Feb 1, 2017 •
edited by apelisse

There are other issues afloat, but other things that might happen here would be an interceptor for fixes to apiVersion on disk, or potentially fixing issues as they come out of the objects

@smarterclayton
Copy link
Contributor Author

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: smarterclayton

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@smarterclayton smarterclayton removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2017
@smarterclayton
Copy link
Contributor Author

@k8s-bot gce etcd3 e2e test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 39217, 40624)

@k8s-github-robot k8s-github-robot merged commit c92f29a into kubernetes:master Feb 2, 2017
@liggitt liggitt mentioned this pull request Feb 2, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

Assignees

@lavalamp lavalamp

@liggitt liggitt

@deads2k deads2k

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@smarterclayton @k8s-reviewable @sttts @enj @k8s-github-robot @lavalamp @liggitt @deads2k @k8s-ci-robot