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

Turn off the alpha features by default #47690

Merged

Conversation

caesarxuchao
Copy link
Member

@caesarxuchao caesarxuchao commented Jun 17, 2017

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
@@ -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

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

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

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

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
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

6 participants