kubeapiserver: rename '--experimental-encryption-provider-config' to … by marrrvin · Pull Request #61592 · kubernetes/kubernetes

marrrvin · 2018-03-23T11:22:19Z

…'--encryption-provider-config'.

This change renames the '--experimental-encryption-provider-config' flag to '--encryption-provider-config'. The old flag is accepted but generates a warning.

In 1.12, we will drop support for '--experimental-encryption-provider-config' entirely.

What this PR does / why we need it:

Encryption at rest with static configs has been promoted to beta.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Updates #61420

Special notes for your reviewer:

Release note:

Encryption at rest with static configs has been promoted to beta.
API server flag `--experimental-encryption-provider-config` has been renamed to `--encryption-provider-config`. The old flag is accepted with a warning but will be removed in 1.12.

…'--encryption-provider-config'. This change renames the '--experimental-encryption-provider-config' flag to '--encryption-provider-config'. The old flag is accepted but generates a warning. In 1.12, we will drop support for '--experimental-encryption-provider-config' entirely.

k8s-ci-robot · 2018-03-23T11:22:26Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: marrrvin
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers: deads2k, mwielgus

Assign the PR to them by writing /assign @deads2k @mwielgus in a comment when ready.

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

The pull request process is described here

Details

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

marrrvin · 2018-03-23T11:59:44Z

@php-coder FYI

php-coder · 2018-03-23T12:15:17Z

@marrrvin Thank you for your contribution!

Could you update PR title to make it shorter?

Also in the description, please, replace Fixes # by Fixes #61420 (in this case when PR will get merged, the related issue will be closed automatically).

And this issue deserves to be mentioned in the changelog, hence release note is needed. I'd suggest to something like:

Encryption at rest with static configs has been promoted to beta.
API server flag `--experimental-encryption-provider-config` has been renamed to `--encryption-provider-config`. The old flag is accepted with a warning but will be removed in 1.12

@ericchiang @liggitt PTAL. Do we need release note with required action in this case?

/sig auth
/ok-to-test

php-coder · 2018-03-23T12:29:42Z

@marrrvin Please, mention also that Encryption at rest with static configs has been promoted to beta. This is important message to users that it's not in Alpha anymore.

liggitt · 2018-03-23T12:53:53Z

before this can be merged, we have to fix the way the encryption config is parsed. currently it uses a non-standard parsing method that pays no attention to the version of the config:

kubernetes/staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go

Lines 67 to 79 in dce1b88

    
           var config EncryptionConfig 
        
           err = yaml.Unmarshal(configFileContents, &config) 
        
           if err != nil { 
        
           	return nil, fmt.Errorf("error while parsing file: %v", err) 
        
           } 
        
           if config.Kind == "" { 
        
           	return nil, fmt.Errorf("invalid configuration file, missing Kind") 
        
           } 
        
           if config.Kind != "EncryptionConfig" { 
        
           	return nil, fmt.Errorf("invalid configuration kind %q provided", config.Kind) 
        
           } 
        
           // TODO config.APIVersion is unchecked

the types must change to use standard methods for including kind/apiVersion (inlining metav1.TypeMeta like all other objects), define a versioned config (we can promote straight to v1beta1 if we don't require any structural changes), and parsing must require well-formed config files before this is promoted out of experimental mode.

marrrvin · 2018-03-23T14:17:33Z

@liggitt Thanks. Could you please put me an example of standard parsing method?

php-coder · 2018-03-23T14:27:51Z

Could you please put me an example of standard parsing method?

Looks like you may follow this example:

kubernetes/cmd/kube-proxy/app/server.go

Lines 292 to 314 in dce1b88

    
           // loadConfigFromFile loads the contents of file and decodes it as a 
        
           // KubeProxyConfiguration object. 
        
           func (o *Options) loadConfigFromFile(file string) (*kubeproxyconfig.KubeProxyConfiguration, error) { 
        
           	data, err := ioutil.ReadFile(file) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	} 
        
           	return o.loadConfig(data) 
        
           } 
        
           // loadConfig decodes data as a KubeProxyConfiguration object. 
        
           func (o *Options) loadConfig(data []byte) (*kubeproxyconfig.KubeProxyConfiguration, error) { 
        
           	configObj, gvk, err := o.codecs.UniversalDecoder().Decode(data, nil, nil) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	} 
        
           	config, ok := configObj.(*kubeproxyconfig.KubeProxyConfiguration) 
        
           	if !ok { 
        
           		return nil, fmt.Errorf("got unexpected config type: %v", gvk) 
        
           	} 
        
           	return config, nil 
        
           }

@liggitt Please, confirm.

liggitt · 2018-03-23T14:49:39Z

yes, roughly that.

fix type definition to actually be a runtime object, inline TypeMeta, etc
define v1beta1 API types, generate conversions/defaults
define scheme/codecs that have the external (v1beta1) and internal (existing) types registered
use that scheme/codec to load the config (handles decoding the v1beta1 version, conversion to internal version)

php-coder · 2018-03-23T15:25:29Z

@liggitt Thanks, Jordan! I've extracted this task to the separate issue: #61599

k8s-ci-robot · 2018-04-26T22:53:54Z

@marrrvin: PR needs rebase.

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.

dims

Looks like changes requested by @liggitt was done in another PR

php-coder · 2018-04-30T12:56:31Z

Looks like changes requested by @liggitt was done in another PR

Not yet, unfortunately (as now that PR requires another PR :) )

php-coder

Besides rebasing against the latest master, we need to also update another place that was introduced recently:

$ git grep 'experimental-encryption-provider-config'
...
cluster/gce/gci/apiserver_manifest_test.go:     return fmt.Sprintf("--experimental-encryption-provider-config=%s", path)
cluster/gce/gci/apiserver_manifest_test.go:// EncryptionProviderConfig file of type KMS and inject experimental-encryption-provider-config startup flag.

marrrvin · 2018-06-20T09:18:48Z

@php-coder ok. I'll try to do it on holidays.

dims · 2018-08-23T20:01:59Z

/milestone v1.12

Looks like this needs to be in 1.12 please drop the milestone if i am reading this wrong

dims · 2018-08-23T20:02:40Z

@marrrvin can you please rebase?

liggitt · 2018-08-29T15:41:31Z

/hold

while this is definitely desired, we shouldn't drop the experimental prefix until the config driving this graduates from alpha (in progress in #67383)

shubheksha · 2018-10-30T14:53:59Z

ping @marrrvin, @liggitt - is this on track for 1.13 since there hasn't been any update?

dims · 2018-11-16T03:02:22Z

@marrrvin this needs rebase ... code freeze is tomorrow! ( https://github.com/kubernetes/sig-release/tree/master/releases/release-1.13 )

stlaz · 2018-11-19T07:57:44Z

@dims should we just do the rebase ourselves? I've got it ready in https://github.com/stlaz/kubernetes/tree/enc_config_opt but won't be able to push it in this PR.
The hold could also be dropped since the alpha->stable PR was pushed already.

dims · 2018-11-19T11:51:43Z

@stlaz please open a fresh PR and we can discuss there.

stlaz · 2018-11-19T12:03:36Z

@dims Done in #71206

stlaz · 2018-11-27T11:42:55Z

This PR can now be closed as the above one was merged.

php-coder · 2018-11-27T11:50:35Z

/close

k8s-ci-robot · 2018-11-27T11:50:37Z

@php-coder: Closed this PR.

Details

In response to this:

/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 the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 23, 2018

k8s-ci-robot requested review from MaciekPytel, dims and smarterclayton March 23, 2018 11:22

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. 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 Mar 23, 2018

k8s-ci-robot added sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 23, 2018

This was referenced Mar 23, 2018

Promote encryption at rest with static configs to beta #61420

Closed

kubeapiserver: rename '--experimental-encryption-provider-config' to … kubernetes/website#7823

Closed

php-coder mentioned this pull request Mar 23, 2018

EncryptionConfig: use standard method for parsing config file #61599

Closed

4 tasks

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

dims approved these changes Apr 30, 2018

View reviewed changes

php-coder suggested changes May 10, 2018

View reviewed changes

k8s-ci-robot added this to the v1.12 milestone Aug 23, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 29, 2018

liggitt modified the milestones: v1.12, v1.13 Sep 5, 2018

dims mentioned this pull request Nov 8, 2018

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

Merged

liggitt self-assigned this Nov 15, 2018

mikedanese modified the milestones: v1.13, next-candidate Nov 17, 2018

stlaz mentioned this pull request Nov 19, 2018

Rename '--experimental-encryption-provider-config' to '--encryption-provider-config' #71206

Merged

k8s-ci-robot closed this Nov 27, 2018

Conversation

marrrvin commented Mar 23, 2018 • edited by ericchiang Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 23, 2018

Uh oh!

marrrvin commented Mar 23, 2018

Uh oh!

php-coder commented Mar 23, 2018

Uh oh!

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

Uh oh!

Uh oh!

liggitt commented Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marrrvin commented Mar 23, 2018

Uh oh!

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

Uh oh!

Uh oh!

liggitt commented Mar 23, 2018

Uh oh!

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

Uh oh!

Uh oh!

k8s-ci-robot commented Apr 26, 2018

Uh oh!

dims left a comment

Choose a reason for hiding this comment

Uh oh!

php-coder commented Apr 30, 2018

Uh oh!

php-coder left a comment

Choose a reason for hiding this comment

Uh oh!

marrrvin commented Jun 20, 2018

Uh oh!

dims commented Aug 23, 2018

Uh oh!

dims commented Aug 23, 2018

Uh oh!

liggitt commented Aug 29, 2018

Uh oh!

shubheksha commented Oct 30, 2018

Uh oh!

dims commented Nov 16, 2018

Uh oh!

stlaz commented Nov 19, 2018

Uh oh!

dims commented Nov 19, 2018

Uh oh!

stlaz commented Nov 19, 2018

Uh oh!

stlaz commented Nov 27, 2018

Uh oh!

php-coder commented Nov 27, 2018

Uh oh!

k8s-ci-robot commented Nov 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

marrrvin commented Mar 23, 2018 •

edited by ericchiang

Loading

php-coder commented Mar 23, 2018 •

edited

Loading

liggitt commented Mar 23, 2018 •

edited

Loading

php-coder commented Mar 23, 2018 •

edited

Loading

php-coder commented Mar 23, 2018 •

edited

Loading