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

Turn off the alpha features by default #47690

Merged
merged 1 commit into from
Jun 17, 2017
Merged

Turn off the alpha features by default #47690

merged 1 commit into from
Jun 17, 2017

Conversation

caesarxuchao
Copy link
Member

@caesarxuchao caesarxuchao commented Jun 17, 2017 •
edited by liggitt
Loading

Fix #47687.

@liggitt @sttts do you know if it's intentional to turn on rbac v1alpha1?

The following alpha API groups were unintentionally enabled by default in previous releases, and will no longer be enabled by default in v1.8:
rbac.authorization.k8s.io/v1alpha1
settings.k8s.io/v1alpha1
If you wish to continue using them in v1.8, please enable them explicitly using the `--runtime-config` flag of the apiserver (for example, `--runtime-config="rbac.authorization.k8s.io/v1alpha1,settings.k8s.io/v1alpha1"`)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 17, 2017
liggitt
liggitt reviewed Jun 17, 2017
View reviewed changes
pkg/master/master.go Outdated
@@ -47,9 +46,7 @@ import (
extensionsapiv1beta1 "k8s.io/kubernetes/pkg/apis/extensions/v1beta1"
networkingapiv1 "k8s.io/kubernetes/pkg/apis/networking/v1"
policyapiv1beta1 "k8s.io/kubernetes/pkg/apis/policy/v1beta1"
rbacv1alpha1 "k8s.io/kubernetes/pkg/apis/rbac/v1alpha1"
Copy link
Member

Choose a reason for hiding this comment

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

don't turn this off... it's been on since 1.5 and we need to ensure we can read existing data. new data is written in v1beta1 as of 1.6

Copy link
Member

@liggitt liggitt Jun 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

actually, I just tried launching with v1alpha1 data in etcd with this PR (which leaves v1alpha1 registered, but disabled by default), and the data was able to be read out of etcd just fine via the v1beta1 endpoint. I don't really feel strongly about disabling the group, since new data is actually being stored in v1beta1 format as of 1.6

Copy link
Member Author

@caesarxuchao caesarxuchao Jun 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I think it's because when stored to etcd, the path doesn't include the version, so as long as the codec is able to decode (i.e., v1lapha1 is registered to api.Scheme), it's readable.

Copy link
Member

@liggitt liggitt Jun 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I knew the path was stable, but I thought we had already started cleaning up the shared global api.Scheme and you had to opt into the groups registered into your storage scheme, but apparently not

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jun 17, 2017
@caesarxuchao
Copy link
Member Author

Some e2e tests rely on settings/v1alpha1 to be enabled by default:
https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/47690/pull-kubernetes-e2e-gce-etcd3/36522/

PodPreset should create a pod preset 39s

@caesarxuchao
Copy link
Member Author

@liggitt i think we can disable rbac and settings v1alpha1, reading v1alpha1 data is not an issue, but i need to fix the e2e tests depends on settings/v1alpha1.

@caesarxuchao
Copy link
Member Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@caesarxuchao caesarxuchao added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-label-needed labels Jun 17, 2017
@caesarxuchao
Copy link
Member Author

Discussed with @pwittrock @foxish @dchen1107 offline, because there could be OSS users depending on rbac/v1alpha1 and setttings/v1alpha1 being enabled by default, in 1.7, we'll make an action-required release-note to inform people to enable these alpha features explicitly if needed, and will disable them by default in 1.8.

@liggitt
Copy link
Member

liggitt commented Jun 17, 2017 •
edited
Loading

Discussed with @pwittrock @foxish @dchen1107 offline, because there could be OSS users depending on rbac/v1alpha1 and setttings/v1alpha1 being enabled by default, in 1.7, we'll make an action-required release-note to inform people to enable these alpha features explicitly if needed, and will disable them by default in 1.8.

That seems more than reasonable. Can you add comments to the code as breadcrumbs to remind us and queue up the PR to do that in 1.8 so we don't forget?

@caesarxuchao caesarxuchao mentioned this pull request Jun 17, 2017
Closed
@caesarxuchao
Copy link
Member Author

Created #47691 as a reminder.

@caesarxuchao
Copy link
Member Author

@liggitt could you also take a look at the release note? Thanks.

@dchen1107
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2017
@dchen1107 dchen1107 added this to the v1.7 milestone Jun 17, 2017
@caesarxuchao
Copy link
Member Author

updated bazel.

@caesarxuchao caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Jun 17, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, dchen1107

Associated issue: 47687

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

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

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 17, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit a57c33b into kubernetes:master Jun 17, 2017
@caesarxuchao caesarxuchao mentioned this pull request Sep 5, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt liggitt left review comments

Assignees

@sttts sttts

@liggitt liggitt

@dchen1107 dchen1107

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-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.
6 participants
@caesarxuchao @liggitt @dchen1107 @k8s-github-robot @sttts @k8s-ci-robot