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

Disable rbac/v1alpha1, settings/v1alpha1, and scheduling/v1alpha1 by default #51839

Merged
merged 1 commit into from
Sep 6, 2017
Merged

Disable rbac/v1alpha1, settings/v1alpha1, and scheduling/v1alpha1 by default #51839

merged 1 commit into from
Sep 6, 2017

Conversation

jennybuckley
Copy link

@jennybuckley jennybuckley commented Sep 1, 2017 •
edited
Loading

What this PR does / why we need it: Disables alpha features which were previously enabled by default. Also changes tests which relied on these alpha features being enabled by default.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #47691

Special notes for your reviewer:

Release note:

Fixed a bug where some alpha features were enabled by default.

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 1, 2017
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

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.

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 1, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @jennybuckley. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

@jennybuckley
Copy link
Author

/cc @caesarxuchao

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 1, 2017
caesarxuchao
caesarxuchao reviewed Sep 1, 2017
View reviewed changes
test/e2e/servicecatalog/podpreset.go Outdated
@@ -22,7 +22,7 @@ import (
"time"

"k8s.io/api/core/v1"
settings "k8s.io/api/settings/v1alpha1"
settings "pkg/apis/settings"
Copy link
Member

Choose a reason for hiding this comment

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

Why is the import path changed?

Copy link
Member

Choose a reason for hiding this comment

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

In this PR, we'll need to make this test a feature test, by adding [Feature:PodPreset] in the test name, then the test won't run in pull-kubernetes-e2e-gce-etcd3.

In another PR, we need to add the test to the alpha suite, https://github.com/kubernetes/test-infra/blob/52b103fadb98e290d67fefeba6051eea8110fc46/jobs/config.json#L639.

cc @droot

Copy link
Author

Choose a reason for hiding this comment

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

@caesarxuchao I think I have fixed the issue now, when you have time, could you look it over again?

Thanks

@k8s-github-robot k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 1, 2017
@krmayankk
Copy link

@jennybuckley with this change would enabling RBAC require feature flags for e.g. in 1.8 ?

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 5, 2017
@roycaihw
Copy link
Member

roycaihw commented Sep 5, 2017

cc @roycaihw

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 5, 2017
@k8s-github-robot k8s-github-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 Sep 5, 2017
@jennybuckley jennybuckley mentioned this pull request Sep 5, 2017
Merged
@jennybuckley
Copy link
Author

Created kubernetes/test-infra#4392 to add the podpreset tests to the alpha suite

@jennybuckley
Copy link
Author

@krmayankk yes, but only rbac/v1alpha1 will need that as rbac/v1beta1 and rbac/v1 are still enabled by default
rbac/v1alpha1 being enabled by default up until now is a bug (#47687)

@caesarxuchao
Copy link
Member

lgtm. Could you squash?

@liggitt could you approve the change? It's addressing our previous discussion here. Thanks.

@jennybuckley
Copy link
Author

@caesarxuchao squashed

@liggitt
Copy link
Member

liggitt commented Sep 5, 2017

this just affects surfaced APIs, right, not the ability to decode in etcd?

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Sep 5, 2017
caesarxuchao
caesarxuchao reviewed Sep 5, 2017
View reviewed changes
pkg/master/master.go Outdated
// See https://github.com/kubernetes/kubernetes/pull/47690.
// TODO: disable rbac/v1alpha1 and settings/v1alpha1 by default in 1.8
rbacv1alpha1.SchemeGroupVersion,
settingv1alpha1.SchemeGroupVersion,
schedulingapiv1alpha1.SchemeGroupVersion,
Copy link
Member

Choose a reason for hiding this comment

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

Eh, the schduelingv1alpha1 is enabled by default. It's introduced in 1.8, so we can just remove it from the list. @jennybuckley could you remove this line as well?

cc @bsalamat the original author.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I removed it as well as from the build file and the imports.
I was going to bring it up because I noticed that there was another alpha feature enabled by default, but I wasn't sure if that was intentional.

@jennybuckley jennybuckley changed the title Disable rbac/v1alpha1 settings/v1alpha1 by default Disable rbac/v1alpha1 settings/v1alpha1 and scheduling/v1alpha1 by default Sep 5, 2017
@jennybuckley jennybuckley changed the title Disable rbac/v1alpha1 settings/v1alpha1 and scheduling/v1alpha1 by default Disable rbac/v1alpha1, settings/v1alpha1, and scheduling/v1alpha1 by default Sep 5, 2017
@jennybuckley
Copy link
Author

/assign @sttts

@liggitt liggitt added this to the v1.8 milestone Sep 6, 2017
@liggitt liggitt added sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. labels Sep 6, 2017
@liggitt
Copy link
Member

liggitt commented Sep 6, 2017

tagging owning sigs - @kubernetes/sig-auth-api-reviews @kubernetes/sig-scheduling-api-reviews @kubernetes/sig-apps-api-reviews

lgtm for sig-apps

We do not enable alpha APIs by default. rbac and podpresents were accidentally enabled by default in previous releases. We announced in 1.7 they would be disabled by default in 1.8 (and how to re-enable with --runtime-config). This should merge to complete that work and ensure we do not enable a new alpha API by default in 1.8

@liggitt
Copy link
Member

liggitt commented Sep 6, 2017

a test like liggitt@746a119 would prevent alpha versions from getting re-added accidentally

@liggitt liggitt assigned liggitt and unassigned madhusudancs and pmorie Sep 6, 2017
@liggitt
Copy link
Member

liggitt commented Sep 6, 2017

/retest

@bsalamat
Copy link
Member

bsalamat commented Sep 6, 2017

lgtm for disabling alpha scheduling APIs by default. It was not my intention to enable them by default.

@dims
Copy link
Member

dims commented Sep 6, 2017

@jennybuckley @bsalamat : looks like this is stuck on a new test to be written. right?

@liggitt
Copy link
Member

liggitt commented Sep 6, 2017

Can just pick liggitt@746a119 if you like

@jennybuckley
Copy link
Author

@liggitt should I add this test to this PR, or in a separate one?

@liggitt
Copy link
Member

liggitt commented Sep 6, 2017

I can open separately
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2017
@sttts
Copy link
Contributor

sttts commented Sep 6, 2017

/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jennybuckley, liggitt, sttts

Associated issue: 47691

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 Sep 6, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 51839, 51987)

@k8s-github-robot k8s-github-robot merged commit dc98597 into kubernetes:master Sep 6, 2017
@liggitt liggitt mentioned this pull request Sep 12, 2017
Merged
k8s-github-robot pushed a commit that referenced this pull request Sep 13, 2017
Merge pull request #52339 from liggitt/alpha-test
5bc9d7b 
Automatic merge from submit-queue (batch tested with PRs 52339, 52343, 52125, 52360, 52301)

Prevent enabling alpha APIs by default

related to #47691
This is a follow up to #51839 to add a check that we do not enable alpha APIs by default
@ericchiang ericchiang mentioned this pull request Sep 15, 2017
Closed
1 task
@jennybuckley jennybuckley mentioned this pull request Sep 26, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@caesarxuchao caesarxuchao caesarxuchao left review comments

Assignees

@sttts sttts

@liggitt liggitt

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 Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.

Disable rbac/v1alpha1 and settings/v1alpha1 by default in 1.8