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

federation: Adding support for namespace admission controls in federation-apiserver #31139

Merged
merged 2 commits into from
Aug 23, 2016
Merged

federation: Adding support for namespace admission controls in federation-apiserver #31139

merged 2 commits into from
Aug 23, 2016

Conversation

nikhiljindal
Copy link
Contributor

@nikhiljindal nikhiljindal commented Aug 22, 2016 •
edited

Now that we have namespaces in federation apiserver, we can support namespace admission controls.

There are 3 of these:
namespace/autoprovision, namespace/exists and namespace/lifecycle.
namespace/autoprovision, namespace/exists should be deprecated in kubernetes(#31195). Adding support for namespace/lifecycle to federation-apiserver.
As in kube-apiserver, enabling namespace/lifecycle by default.

Action required: If you have a running federation control plane, you will have to ensure that for all federation resources, the corresponding namespace exists in federation control plane.

federation-apiserver now supports NamespaceLifecycle admission control, which is enabled by default. Set the --admission-control flag on the server to change that.

cc @kubernetes/sig-cluster-federation @quinton-hoole

This change is Reviewable

@nikhiljindal
Copy link
Contributor Author

Am working on fixing the federation e2es.

@nikhiljindal nikhiljindal added area/cluster-federation release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Aug 22, 2016
@nikhiljindal nikhiljindal added this to the v1.4 milestone Aug 22, 2016
@nikhiljindal
Copy link
Contributor Author

cc @derekwaynecarr

@nikhiljindal nikhiljindal mentioned this pull request Aug 22, 2016
Closed
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 22, 2016
@ghost ghost added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Aug 22, 2016
@ghost ghost assigned ghost and derekwaynecarr and unassigned spxtr Aug 22, 2016
@nikhiljindal nikhiljindal force-pushed the namespaceAdmissionControl branch 4 times, most recently from c773f64 to 7cd8664 Compare August 22, 2016 21:43
@nikhiljindal
Copy link
Contributor Author

This is now ready for review. Have fixed all federation e2es and have added a test to ensure that a request to create a namespaced resource fails if the namespace does not exist.

I have a TODO to clean up the federation namespace after each test, which I will send another PR for (need to fix #31077 before that).

ptal.

This was referenced Aug 22, 2016
Closed
Closed
@derekwaynecarr
Copy link
Member

I think you can only add NamespaceLifecycle. The others should be marked
deprecated.

On Monday, August 22, 2016, Kubernetes Bot notifications@github.com wrote:

GCE e2e build/test passed for commit 7cd8664
7cd8664
.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#31139 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AF8dbOdbxo5HU1M50bX_onw8GuCKOJ1Vks5qii58gaJpZM4JqIrn
.

@nikhiljindal nikhiljindal mentioned this pull request Aug 23, 2016
Closed
@nikhiljindal
Copy link
Contributor Author

nikhiljindal commented Aug 23, 2016 •
edited

Removed the other 2 namespace admission controls and filed #31195 to keep track to deprecate it in kube-apiserver.

Updated the PR description and release-notes

@ghost ghost added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Aug 23, 2016
ghost
ghost reviewed Aug 23, 2016
View reviewed changes
federation/cluster/common.sh
@@ -94,7 +94,7 @@ function create-federation-api-objects {
export FEDERATION_API_NODEPORT=32111
export FEDERATION_NAMESPACE
export FEDERATION_NAME="${FEDERATION_NAME:-federation}"
export DNS_ZONE_NAME="${DNS_ZONE_NAME:-federation.example}" # See https://tools.ietf.org/html/rfc2606
export DNS_ZONE_NAME="${DNS_ZONE_NAME:-federation.example.}" # See https://tools.ietf.org/html/rfc2606
Copy link

Choose a reason for hiding this comment

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

Doesn't seem to belong in this PR?

@k8s-bot
Copy link

k8s-bot commented Aug 23, 2016

GCE e2e build/test passed for commit db7af6c.

ghost
ghost reviewed Aug 23, 2016
View reviewed changes
test/e2e/federation-apiserver.go
@@ -84,8 +84,44 @@ var _ = framework.KubeDescribe("Federation apiserver [Feature:Federation]", func
framework.Logf("Verified that zero clusters remain")
})
})
Describe("Admission control", func() {
AfterEach(func() {
framework.SkipUnlessFederated(f.Client)
Copy link

Choose a reason for hiding this comment

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

Surely this belongs in BeforeEach(), not AfterEach()?

@ghost
Copy link

ghost commented Aug 23, 2016

Thanks @nikhiljindal. Comments can be addressed in a followup PR. LGTM.

@ghost ghost added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 23, 2016
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 23, 2016

GCE e2e build/test passed for commit db7af6c.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 13305ef into kubernetes:master Aug 23, 2016
@nikhiljindal nikhiljindal added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 25, 2016
@ghost
Copy link

ghost commented Aug 29, 2016

@nikhiljindal One of these tests is ~perma-failing since 8/23. Could you please take a look?

https://k8s-testgrid.appspot.com/google-gce#gce-federation

@ghost ghost mentioned this pull request Aug 29, 2016
Closed
@nikhiljindal
Copy link
Contributor Author

nikhiljindal commented Aug 29, 2016 •
edited

Sure. Am on it: #31624

@nikhiljindal nikhiljindal mentioned this pull request Sep 6, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@derekwaynecarr derekwaynecarr

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@nikhiljindal @derekwaynecarr @k8s-bot @k8s-github-robot @googlebot @spxtr