[WIP] Introduce kubeapiserver.config.k8s.io/v1 and use standard method for parsing encryption config file by php-coder · Pull Request #63032 · kubernetes/kubernetes

php-coder · 2018-04-23T19:38:19Z

What this PR does / why we need it:
This PR reworks handling of the configuration file for encryption at rest. Now it uses a standard approach for parsing and also it supports versioning.

Which issue(s) this PR fixes:
Fixes #61599

php-coder · 2018-04-23T19:38:57Z

/release-note-none
CC @simo5 @marrrvin

php-coder · 2018-04-23T19:47:04Z

/sig auth

At this time it doesn't work yet as compilation fails with:

import cycle not allowed
package k8s.io/kubernetes/cmd/hyperkube
	imports k8s.io/kubernetes/cmd/kube-apiserver/app
	imports k8s.io/apiserver/pkg/server/options/encryptionconfig
	imports k8s.io/apiserver/pkg/server/options/encryptionconfig/scheme
	imports k8s.io/apiserver/pkg/server/options/encryptionconfig

@liggitt @smarterclayton We need to move encryption configuration somewhere, but I'm not sure where its place. If we want to use apiserver.config.k8s.io API Group then we should move encryption config to https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/apiserver/pkg/apis/apiserver but it's in v1alpha1 version. I'm not sure that we want and can downgrade api version from v1 to v1alpha1.

How we can handle this?

php-coder · 2018-04-23T19:49:13Z

@liggitt This type looks suspicious. Did we decide some time ago to always use integers with a fixed size in APIs, do we?

the field in the external type should be a fixed size int, yes

php-coder · 2018-04-23T19:50:53Z

I decided to make some functions private, it doesn't relate to my change. Let me know if it's inappropriate change at all or if I should submit it separately.

there's no real reason to make these private, so let's revert this change.

php-coder · 2018-04-23T19:52:40Z

@liggitt Do we have a requirement to fully support old format of the config file? Perhaps we should show a warning in this case?

I assume that no, we don't have such requirement.

liggitt · 2018-04-23T20:07:52Z

I'm not sure that we want and can downgrade api version from v1 to v1alpha1.

The documentation says to use v1:

https://kubernetes.io/docs/tasks/administer-cluster/encrypt-data/#understanding-the-encryption-at-rest-configuration

Because we were using a non-standard load that ignored version, this wasn't caught before.

I'd actually be in favor of promoting the config version to v1. The types are minimal, there's been no desired changes since it was created, and anyone following our documentation is already using v1.

php-coder · 2018-04-24T12:19:02Z

I'd actually be in favor of promoting the config version to v1.

@liggitt Do I understand you correctly, that we should do this promotion before moving encryption config there? In other words, we have one more dependency for promoting encryption config to beta.

I'll be able to prepare and submit PR next week.

php-coder · 2018-04-30T16:16:22Z

I'd actually be in favor of promoting the config version to v1.

I'll be able to prepare and submit PR next week.

Done: #63304

php-coder · 2018-05-03T17:01:05Z

I'm not sure about this change because v1 has one type and v1alpha1 has others, in other words, these aren't different versions of the same API but different APIs in fact.

The admissionregistration seems to do something similar where different versions of the API hold different object.

https://github.com/kubernetes/api/blob/kubernetes-1.10.2/admissionregistration/v1alpha1/register.go#L45-L48
https://github.com/kubernetes/api/blob/kubernetes-1.10.2/admissionregistration/v1beta1/register.go#L45-L50

Any apimachinery folk care to comment here? It looks fine to me.

php-coder · 2018-05-03T17:03:23Z

I've just casted int32 to int here but let me know if there is a better way or perhaps we should use int32 everywhere.

php-coder · 2018-05-03T17:05:39Z

Do we have a better way for parsing YAML? I couldn't find a way of getting a scheme.

CC @sttts

This is what we do for the audit API policy file

kubernetes/staging/src/k8s.io/apiserver/pkg/audit/policy/reader.go

Line 47 in 484f62a

func LoadPolicyFromFile(filePath string) (*auditinternal.Policy, error) {

php-coder · 2018-05-03T17:06:55Z

Now we don't need these checks as they will be performed by deserializer.

php-coder · 2018-05-03T17:10:36Z

This PR makes the old configs unusable. If we have to be backward compatible and you know how to handle such cases, please, share this knowledge with me or, at least, point me to the right direction. Thanks!

Definitely needs a release note.

We could try to parse out the kind and apiVersion beforehand, then conditionally use the old parsing logic. Don't know how much work that is.

We can register the types under the legacy core group in the scheme used for decoding. Why is that no option?

Probably this was just never considered. Would that mean there would be two different API groups for one functionality? Could the legacy one ever be deprecated? Should that be done at a different time than when promoting from alpha to unfortunate stable?

yes, same types, multiple groups. Technically possible.

Good question about deprecation. Formally we will probably have to stick with this forever.

php-coder · 2018-05-04T14:02:52Z

@liggitt PTAL
@deads2k @sttts Your feedback would be welcomed!

…ig/BUILD

…ig/config.go

…/apis/webhookadmission/register.go

k8s-ci-robot · 2018-06-15T11:12:52Z

@php-coder: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel-test	`7cdc116`	link	`/test pull-kubernetes-bazel-test`
pull-kubernetes-integration	`7cdc116`	link	`/test pull-kubernetes-integration`
pull-kubernetes-verify	`7cdc116`	link	`/test pull-kubernetes-verify`

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.

Details

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.

php-coder · 2018-06-15T11:22:46Z

W0604 14:07:59.325] vendor/k8s.io/apiserver/pkg/admission/plugin/webhook/config/apis/webhookadmission/register.go:49:36: cannot use kubeapiserverconfig.EncryptionConfiguration literal (type *kubeapiserverconfig.EncryptionConfiguration) as type runtime.Object in argument to scheme.AddKnownTypes:

Looks like, I forgot to run some script or maybe I need to "register" a path with new type somewhere so the scripts will pick it up?

I've fixed this by adding doc.go, register.go and install/install.go files for an internal type.

At this moment, I faced with another verify import violation:

2018/06/15 10:18:27 Inspecting imports under ./vendor/k8s.io/apiserver/...
2018/06/15 10:18:28 - validating imports for 130 packages in the tree
2018/06/15 10:18:28 -- found forbidden imports for k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/admission/plugin/webhook/config/apis/webhookadmission:
2018/06/15 10:18:28 --- k8s.io/kubernetes/pkg/kubeapiserver/apis/kubeapiserverconfig
2018/06/15 10:18:28 -- found forbidden imports for k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/admission/plugin/webhook/config/apis/webhookadmission/install:
2018/06/15 10:18:28 --- k8s.io/kubernetes/pkg/kubeapiserver/apis/kubeapiserverconfig/v1
2018/06/15 10:18:28 -- found forbidden imports for k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/server/options/encryptionconfig:
2018/06/15 10:18:28 --- k8s.io/kubernetes/pkg/kubeapiserver/apis/kubeapiserverconfig/v1
2018/06/15 10:18:28 --- k8s.io/kubernetes/pkg/kubeapiserver/apis/kubeapiserverconfig
2018/06/15 10:18:28 - FAIL

Not sure how to fix it, perhaps, I need to relax this check.

stlaz · 2018-07-30T07:28:48Z

@liggitt @sttts Would the following patch help moving this PR forward? I have this plus the commits from this PR squashed and rebased on the current (~week old) master in https://github.com/stlaz/kubernetes/tree/slava_encryptconfig. I believe @php-coder won't have much time to carry on with his work on this one.

From ceae7ac28d3a29d064aaccad2c6c59e23b7937d7 Mon Sep 17 00:00:00 2001
From: Stanislav Laznicka <slaznick@redhat.com>
Date: Mon, 30 Jul 2018 09:22:04 +0200
Subject: [PATCH] Loosen ./vendor/k8s.io/apiserver/ import verification

Loosen ./vendor/k8s.io/apiserver/ import verification so that it allows
imports from k8s.io/pkg/kubeapiserver so that it can use the
EncryptConfiguration objects
---
 hack/import-restrictions.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hack/import-restrictions.yaml b/hack/import-restrictions.yaml
index f9749300a8..f5194bb839 100644
--- a/hack/import-restrictions.yaml
+++ b/hack/import-restrictions.yaml
@@ -79,6 +79,7 @@
   - k8s.io/apiserver
   - k8s.io/client-go
   - k8s.io/kube-openapi
+  - k8s.io/kubernetes/pkg/kubeapiserver
 
 - baseImportPath: "./vendor/k8s.io/metrics/"
   allowedImports:
-- 
2.18.0

luxas · 2018-07-30T15:23:44Z

This proposal is probably a bit related to this PR: kubernetes/community#2354

stlaz · 2018-08-10T12:57:52Z

@luxas I finally got to read the design and compare it to this PR.

If I understand the KEP correctly, the kubeapiserver package would eventually get its own repository. That action still needs to be done. Therefore the only implication for this PR right now is that I should move the changes for the staging package to kubernetes/apiserver repository, am I correct?

luxas · 2018-08-12T17:36:30Z

kubeapiserver package would eventually get its own repository

Yes, eventually, but that's not a near-term goal for that KEP specifically.

Therefore the only implication for this PR right now is that I should move the changes for the staging package to kubernetes/apiserver repository, am I correct?

If this is generic apiserver code I guess it can live in k8s.io/apiserver if it's not I'd prefer k8s.io/kube-apiserver. The k8s.io/apiserver/pkg/apis/config{,v1alpha1} packages are already created FWIW.

stlaz · 2018-08-13T06:35:48Z

This PR originally implemented the changes as a part of apiserver but after series of decisions the kubeapiserver package was chosen. I'd stick to that unless there is a specific reason to move it back or to kube-apiserver instead.
Also, this is going to be v1 due to #63032 (comment)

stlaz · 2018-08-13T07:07:19Z

@luxas I just went through the code again, we could probably really go and contain this change just for apiserver since we're not touching neither kube-apiserver nor federation-apiserver codebase. I suspect kubeapiserver/apis/kubeapiserverconfig was chosen simply for the sake of no other better location which now hopefulyl exists with the k8s.io/apiserver/pkg/apis/config you're mentioning. It seems it would be a code-wise cleaner change, too.

php-coder · 2018-09-07T11:11:11Z

Superseded by #67383

/close

k8s-ci-robot · 2018-09-07T11:11:12Z

@php-coder: Closing this PR.

Details

In response to this:

Superseded by #67383

/close

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.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 23, 2018

php-coder changed the title ~~EncryptionConfig: use standard method for parsing config file~~ [WIP] EncryptionConfig: use standard method for parsing config file Apr 23, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 23, 2018

k8s-ci-robot requested review from brendandburns and liggitt April 23, 2018 19:39

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Apr 23, 2018

php-coder commented Apr 23, 2018

View reviewed changes

php-coder mentioned this pull request Apr 30, 2018

Promote apiserver.config.k8s.io to v1 #63304

Closed

php-coder force-pushed the gh61599_encryption_config branch from ced4b1d to d097747 Compare May 3, 2018 16:58

php-coder commented May 3, 2018

View reviewed changes

php-coder force-pushed the gh61599_encryption_config branch 2 times, most recently from 837aaaf to 0384b7e Compare May 4, 2018 13:27

php-coder force-pushed the gh61599_encryption_config branch from 0384b7e to d1868f5 Compare May 4, 2018 14:24

php-coder changed the title ~~[WIP] EncryptionConfig: use standard method for parsing config file~~ EncryptionConfig: use standard method for parsing config file May 4, 2018

Slava Semushin added 11 commits June 15, 2018 10:55

revert staging/src/k8s.io/apiserver/pkg/server/options/encryptionconf…

4f49c22

…ig/BUILD

update staging/src/k8s.io/apiserver/pkg/server/options/encryptionconf…

ffb06aa

…ig/config.go

edit staging/src/k8s.io/apiserver/pkg/admission/plugin/webhook/config…

79eab80

…/apis/webhookadmission/register.go

revert test/integration/master/BUILD

bd56cc6

update test/integration/master/secrets_transformation_test.go

7e57c92

update test/integration/master/transformation_testcase.go

1ce938f

Run hack/update-codegen.sh

0b3cbd6

Run hack/update-bazel.sh

890b1c0

add missing files for internal type

785f4bc

./hack/update-bazel.sh

090a9e4

run hack/codegen.sh

94fb043

php-coder force-pushed the gh61599_encryption_config branch from 78cccd0 to 94fb043 Compare June 15, 2018 09:54

Slava Semushin added 3 commits June 15, 2018 12:00

fix group name

375556b

hack/update-bazel.sh

53aaadd

fix golint

7cdc116

php-coder changed the title ~~Introduce kubeapiserver.config.k8s.io/v1 and use standard method for parsing encryption config file~~ [WIP] Introduce kubeapiserver.config.k8s.io/v1 and use standard method for parsing encryption config file Jun 15, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 15, 2018

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2018

stlaz mentioned this pull request Aug 14, 2018

Introduce apiserver.config.k8s.io/v1 and use standard method for parsing encryption config file #67383

Merged

k8s-ci-robot closed this Sep 7, 2018

Conversation

php-coder commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

php-coder commented Apr 23, 2018

Uh oh!

php-coder commented Apr 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liggitt commented Apr 23, 2018

Uh oh!

php-coder commented Apr 24, 2018

Uh oh!

php-coder commented Apr 30, 2018

Uh oh!

php-coder May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

php-coder commented May 4, 2018

Uh oh!

k8s-ci-robot commented Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

php-coder commented Jun 15, 2018

Uh oh!

stlaz commented Jul 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luxas commented Jul 30, 2018

Uh oh!

stlaz commented Aug 10, 2018

Uh oh!

luxas commented Aug 12, 2018

Uh oh!

stlaz commented Aug 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stlaz commented Aug 13, 2018

Uh oh!

php-coder commented Sep 7, 2018

Uh oh!

k8s-ci-robot commented Sep 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

php-coder commented Apr 23, 2018 •

edited

Loading

php-coder commented Apr 23, 2018 •

edited

Loading

php-coder May 3, 2018 •

edited

Loading

k8s-ci-robot commented Jun 15, 2018 •

edited

Loading

stlaz commented Jul 30, 2018 •

edited

Loading

stlaz commented Aug 13, 2018 •

edited

Loading