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

Add defaulting and validation logic for EncryptionConfiguration type. #85363

Merged

Conversation

@immutableT
Copy link
Contributor

immutableT commented Nov 15, 2019

What type of PR is this?

/kind feature

What this PR does / why we need it:
Leverage built-in capabilities of k8s API to apply defaults and validation for EncryptionConfiguraiton type.
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

action required
1. Currently, if users were to explicitly specify CacheSize of 0 for KMS provider, they would end-up with a provider that caches up to 1000 keys. This PR changes this behavior.
Post this PR, when users supply 0 for CacheSize this will result in a validation error.

2. CacheSize type was changed from int32 to *int32. This allows defaulting logic to differentiate between cases where users explicitly supplied 0 vs. not supplied any value.
3. KMS Provider's endpoint (path to Unix socket) is now validated when the EncryptionConfiguration files is loaded. This used to be handled by the GRPCService.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 15, 2019

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@immutableT

This comment has been minimized.

Copy link
Contributor Author

immutableT commented Nov 15, 2019

/test pull-kubernetes-e2e-kind

@immutableT immutableT force-pushed the immutableT:encryption-config-defaulter branch from 9ac79c1 to 394fc74 Nov 15, 2019
@immutableT

This comment has been minimized.

Copy link
Contributor Author

immutableT commented Nov 15, 2019

/test pull-kubernetes-integration

@immutableT immutableT force-pushed the immutableT:encryption-config-defaulter branch 4 times, most recently from debfbe8 to fc0a59b Nov 15, 2019
@immutableT immutableT changed the title Add defaulting logic for EncryptionConfiguration type. Add defaulting and validation logic for EncryptionConfiguration type. Nov 19, 2019
@immutableT

This comment has been minimized.

Copy link
Contributor Author

immutableT commented Nov 19, 2019

/assign @awly

@immutableT

This comment has been minimized.

Copy link
Contributor Author

immutableT commented Nov 19, 2019

/assign @liggitt

@immutableT immutableT force-pushed the immutableT:encryption-config-defaulter branch from fc0a59b to 90ac250 Nov 19, 2019
@immutableT immutableT force-pushed the immutableT:encryption-config-defaulter branch from dc72c80 to cbcedae Nov 26, 2019
// See https://golang.org/pkg/crypto/aes/#NewCipher for details on supported key sizes for AES.
secretBoxKeySizes = []int{32}
// See https://godoc.org/golang.org/x/crypto/nacl/secretbox#Open for details on the supported key sizes for Secretbox.
root = field.NewPath("EncryptionConfiguration")

This comment has been minimized.

Copy link
@liggitt

liggitt Nov 26, 2019

Member

we don't specify the root type in the field path... the fieldPath should describe the json path from the root to the field in question (e.g. resources[0]...)

This comment has been minimized.

Copy link
@immutableT

immutableT Nov 26, 2019

Author Contributor

Done.

secretBoxKeySizes = []int{32}
// See https://godoc.org/golang.org/x/crypto/nacl/secretbox#Open for details on the supported key sizes for Secretbox.
root = field.NewPath("EncryptionConfiguration")
resource = root.Child("Resource")

This comment has been minimized.

Copy link
@liggitt

liggitt Nov 26, 2019

Member

make all field names match their serialized json tags (the error presented to the user should map to the config file field names they see)

Suggested change
resource = root.Child("Resource")
resource = root.Child("resources")

This comment has been minimized.

Copy link
@immutableT

immutableT Nov 26, 2019

Author Contributor

Done.

Copy link
Contributor

awly left a comment

LGTM, but please address liggitt's comments

base64EncodingErr = "secrets must be base64 encoded"
zeroOrNegativeErrFmt = "%s should be a positive value"
negativeValueErrFmt = "%s can't be negative"
encryptionConfigNilErr = "EncryptionConfiguration can't be nil"

This comment has been minimized.

Copy link
@awly

awly Nov 26, 2019

Contributor

re-run gofmt on this file

This comment has been minimized.

Copy link
@immutableT

immutableT Nov 26, 2019

Author Contributor

Done.

@immutableT immutableT force-pushed the immutableT:encryption-config-defaulter branch from cbcedae to e7fbd6e Nov 26, 2019
@immutableT immutableT force-pushed the immutableT:encryption-config-defaulter branch from e7fbd6e to a151aa3 Dec 2, 2019
@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Dec 2, 2019

adjust the release note to make sense from the perspective of a user writing a configuration file:

  • where the validation occurs does not matter to an end user
  • the go type does not matter
  • the fact that an explicit 0 is now disallowed instead of defaulting to 1000 matters
@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Dec 2, 2019

/lgtm
/approve
/priority important-longterm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 2, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: immutableT, liggitt

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

The pull request process is described 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

@immutableT

This comment has been minimized.

Copy link
Contributor Author

immutableT commented Dec 3, 2019

/test pull-kubernetes-e2e-kind

@k8s-ci-robot k8s-ci-robot merged commit 0810bc3 into kubernetes:master Dec 3, 2019
13 of 15 checks passed
13 of 15 checks passed
pull-kubernetes-e2e-kind Job triggered.
Details
tide Not mergeable. Job pull-kubernetes-e2e-kind has not succeeded.
Details
cla/linuxfoundation immutableT authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@k8s-ci-robot k8s-ci-robot added this to the v1.18 milestone Dec 3, 2019
@enj

This comment has been minimized.

Copy link
Member

enj commented Dec 9, 2019

@immutableT can we get an explicit action required release note for this change in regards to explicit 0 cache size now causing the master to not start?

I am also okay with using a negative cache size to imply caching is disabled (you can already set it to 1 so we might as well allow you to turn it off).

@immutableT

This comment has been minimized.

Copy link
Contributor Author

immutableT commented Dec 12, 2019

@enj
Added "action required" in the release note.

Regarding disabling cache, I feel that using 0 (to signal) this would be more intuitive to users.
I did propose to do this on this PR, but @liggitt had some concerns about this change, and we ended-up issuing a warning for cache size being equal or less than 0.

@enj

This comment has been minimized.

Copy link
Member

enj commented Dec 13, 2019

Regarding disabling cache, I feel that using 0 (to signal) this would be more intuitive to users.

There is not really a good option here. If you make 0 mean that the cache is disabled, you silently change the behavior of setting 0 from one release to the next. Having a hard validation error at least makes it clear that some action is required on part of the admin. I think that is an okay approach.

But I do think we should explicitly allow disabling of the cache if someone really wants to do that. And since we have:

nil = default
0 = error
> 0 = use this size cache

Then all we have left is to use < 0 to mean no caching at all. I do not really see a good reason for us to not provide that option. The performance of cache size 1 and no cache at all are both going to be incredibly bad.

@immutableT

This comment has been minimized.

Copy link
Contributor Author

immutableT commented Dec 13, 2019

I support the option to disable caching, since I think that there is a segment of users that expect credentials to be removed from memory after use (even if doing so implies taking some performance hit). By the way, the same should probably apply to kube-apiserver's cache.

With respect to what is the best option to signify that cache should be disabled. I still think it should be 0, but I see your point about the silent behaviour change.

Just to clarify the scenario:
If somebody set CacheSize = 0 prior to this PR, I would imagine that the only reason for doing so would be to disable cache. However, this setting would actually result in CacheSize = 1000 (a bug IMO).
Therefore, using 0 as a way to disable caching may be treated as a bug fix.
Users that set CacheSize to 0 (in hope of disabling caching) would actually end-up in configuration they probably expected all along. The flip side is a potential performance hit.

@enj

This comment has been minimized.

Copy link
Member

enj commented Dec 15, 2019

I am leaning towards using a negative value to mean disabled because setting a negative value has never worked before (thus we do not have to guess as to intent).

Also, if I was making a new API, I would prefer to use cahceSize int with negative meaning disabled, zero and unset meaning use default, and positive values to specify an explicit size (i.e. avoid having to use pointers).

@enj

This comment has been minimized.

Copy link
Member

enj commented Dec 16, 2019

Opened #86294

@immutableT

This comment has been minimized.

Copy link
Contributor Author

immutableT commented Dec 16, 2019

Interesting. I thought it was considered best practice in API design to use pointers even for primitive types, because doing so allows the defaulting logic to differentiate between cases where Go supplied types' default values vs. users providing defaults explicitly. In other words, the use of pointers allows the defaulting logic to assert that users left it up to the API to choose the default.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Dec 16, 2019

If the explicit zero value is considered an invalid value, pointers are preferred.

@immutableT

This comment has been minimized.

Copy link
Contributor Author

immutableT commented Dec 16, 2019

If type's default value value is invalid, then I don't think it really matters whether it was the user or Go that supplied it, we issue a validation error in either case.

I think the use of pointers is necessary when it is important to know when a field was set by the user and when it was not.

In the case of Encryption Config, I think it is important to know if users explicitly set CacheSize to 0. Hence, the use of *int.

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.