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

federation: Adding namespaces API #26298

Merged
merged 3 commits into from
Jul 19, 2016

Conversation

nikhiljindal
Copy link
Contributor

@nikhiljindal nikhiljindal commented May 25, 2016

Adding namespaces API to federation-apiserver and updating the federation client to include namespaces


Original description:

This adds the namespaces API to federation-apiserver.

The first commit is #26142.

@nikhiljindal nikhiljindal added the release-note-none Denotes a PR that doesn't merit a release note. label May 25, 2016
@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 25, 2016
@nikhiljindal
Copy link
Contributor Author

This is required if we want to support NamespaceLifecycle AdmissionControl.
Note that the default value of AdmissionControl is AlwaysAdmit and hence this is not required for the default value.

But at almost all the places, we set --admission-control=NamespaceLifecycle,LimitRanger,SecurityContextDeny,ServiceAccount,ResourceQuota.

To support NamespaceLifecycle we need this change.
Similarly, to support ResourceQuota we will need to add resource quota API to federation-apiserver and similarly for others.

So I think for 1.3, we can just say that we do not support these Admission Controls. We will support them in 1.4. In that case we can just hold this PR till 1.4. Else, we can merge this PR and I can send a similar one for ResourceQuota and others.

@kubernetes/sig-cluster-federation @quinton-hoole thoughts?

@nikhiljindal nikhiljindal added area/cluster-federation sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels May 25, 2016
@nikhiljindal
Copy link
Contributor Author

Discussed this with @quinton-hoole and decided its fine to hold this till 1.4

@nikhiljindal nikhiljindal added this to the next-candidate milestone May 25, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 27, 2016
@madhusudancs
Copy link
Contributor

@nikhiljindal Now that #26142 is merged can we get this one merged as well for 1.3? I think we will need this to create federated services, at least if we want them namespaced.

@nikhiljindal
Copy link
Contributor Author

@madhusudancs As described in #26298 (comment), this is not strictly required for creating namespaced services. This change is required only if the --admission-control flag on apiserver includes NamespaceLifecycle (default value is AlwaysAdmit).

We dont need this change if we are not supporting any other admission control than AlwaysAdmit in 1.3.

@madhusudancs
Copy link
Contributor

@nikhiljindal Thanks for that clarification again and the patience. I didn't understand your previous comment correctly. Tried without --admission-control=NamespaceLifecycle and I am able to create services.

jessfraz pushed a commit to jessfraz/kubernetes that referenced this pull request Jun 2, 2016
Automatic merge from submit-queue

federation: Update the list of supported admission controls

Ref kubernetes#26298 (comment)

In 1.3, we are going to support only AlwaysAdmit and AlwaysDeny admission controls.
Updating the documentation accordingly.

@kubernetes/sig-cluster-federation
@lavalamp
Copy link
Member

lavalamp commented Jun 2, 2016

I don't understand how you're making services without namespaces...

On Tue, May 31, 2016 at 6:42 AM, Madhusudan.C.S [email protected]
wrote:

@nikhiljindal https://github.com/nikhiljindal Thanks for that
clarification again and the patience. I didn't understand your previous
comment correctly. Tried without --admission-control=NamespaceLifecycle and
I am able to create services.


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

@madhusudancs
Copy link
Contributor

@lavalamp We are creating the federated service shards with namespaces, i.e. the shards of the federated service that gets created in the Kubernetes clusters have a namespace. What @nikhiljindal and I are referring to here is the namespace in Ubernetes. To make it more concrete here is what is happening right now:

Host: federation-apiserver
POST /api/v1/namespaces/myns/services/mysvc

federation-apiserver accepts this, service controller watches this service (and the namespace is still associated with the service object) and does:

Host: kube-apiserver
POST /api/v1/namespaces/myns/services/mysvc

@lavalamp
Copy link
Member

lavalamp commented Jun 3, 2016

Yeah, I understand what you're doing. I'm saying you must have performed
some black magic on the federation-apiserver to be able to use that path
without actually having it be aware of namespace objects. This makes me
concerned about namespace handling in general in federation-apiserver; we
had a lot of bugs in that area in kube-apiserver and they are typically
critical security bugs.

On Thu, Jun 2, 2016 at 10:02 PM, Madhusudan.C.S [email protected]
wrote:

@lavalamp https://github.com/lavalamp We are creating the federated
service shards with namespaces, i.e. the shards of the federated service
that gets created in the Kubernetes clusters have a namespace. What
@nikhiljindal https://github.com/nikhiljindal and I are referring to
here is the namespace in Ubernetes. To make it more concrete here is what
is happening right now:

Host: federation-apiserver
POST /api/v1/namespaces/myns/services/mysvc

federation-apiserver accepts this, service controller watches this service
(and the namespace is still associated with the service object) and does:

Host: kube-apiserver
POST /api/v1/namespaces/myns/services/mysvc


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#26298 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAnglpC-3HxJyJjV4x6WoB_FEcxq8s4Aks5qH7V6gaJpZM4Im7pn
.

@derekwaynecarr
Copy link
Member

@lavalamp - if admission control is not running on that server for NamespaceLifecycle then they could have been doing anything they wanted ;-)

@nikhiljindal
Copy link
Contributor Author

@derekwaynecarr Sorry I did not understand you. Who are "they"?

I discussed this with @lavalamp. We are not doing any black magic in federation-apiserver to be able to use that path. /api/v1/namespaces/myns/services/mysvc just works. /api/v1/namespaces/myns/ returns 404.

Also note that while federation-apiserver will only support AlwaysAdmit admission control, the underlying k8s clusters still have the standard admission control.

@derekwaynecarr
Copy link
Member

I have not tracked federation-server closely as of late, but I thought we were planning on supporting a unified namespace? If so, would you not need the namespace lifecycle handler? I suspect I need to get up to spped more on the federation-server, been too heads down looking at other issues.

@colhom
Copy link

colhom commented Jun 15, 2016

@nikhiljindal when this lands, we should modify the e2e framework:

  • before hook to ensure existence of a pre-defined namespace in federation apiserver
  • change existing federation cleanup hook to delete everything in a pre-defined namespace, instead of just nuking the entire clusters collection as it does now.

@nikhiljindal nikhiljindal added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 29, 2016
@nikhiljindal nikhiljindal removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2016
@nikhiljindal
Copy link
Contributor Author

Rebased

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 18, 2016
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 18, 2016
@nikhiljindal
Copy link
Contributor Author

Fixed boilerplate and moved client changes to 1.4 clientset from 1.3 clientset.

@nikhiljindal nikhiljindal modified the milestones: v1.4, next-candidate Jul 18, 2016
@nikhiljindal
Copy link
Contributor Author

All tests pass!
This is now ready to be merged, once LGTM'd

@ghost
Copy link

ghost commented Jul 18, 2016

@nikhiljindal Have you confirmed that the federation e2e tests still pass? See comment from @colhom above. I don't see any e2e test updates in this PR?

"services/status": serviceStatusStorage,
"services": serviceStore,
"services/status": serviceStatusStorage,
"namespaces": namespaceStorage,
Copy link

Choose a reason for hiding this comment

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

nit: can we make namespaceStorage consistent with serviceStore here (or vice verse). i.e. either Store or Storage, not both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@ghost
Copy link

ghost commented Jul 18, 2016

Other than the above comment LGTM. The non-autogenerated portion of this PR is extremely small, as far as I can tell, and LGTM.

@ghost
Copy link

ghost commented Jul 18, 2016

@nikhiljindal feel free to apply the label once you have addressed the nit and the e2e comments.

@nikhiljindal
Copy link
Contributor Author

Thanks @quinton-hoole
Fixed the comment and verified that e2e tests are passing. Adding LGTM tag.

Can update the framework as per @colhom's suggestion in a separate PR.
Will be good to do that once namespace controller is in as well, which will ensure that namespaces are created and deleted in underlying clusters.

@nikhiljindal nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 19, 2016
@k8s-bot
Copy link

k8s-bot commented Jul 19, 2016

GCE e2e build/test passed for commit 48658ad.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot
Copy link

@nikhiljindal PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2016
smarterclayton pushed a commit to smarterclayton/kubernetes that referenced this pull request Jul 19, 2016
Automatic merge from submit-queue

federation: Adding namespaces API

Adding namespaces API to federation-apiserver and updating the federation client to include namespaces


--------------------------

Original description:

This adds the namespaces API to federation-apiserver.

The first commit is kubernetes#26142.
@k8s-github-robot k8s-github-robot merged commit 48658ad into kubernetes:master Jul 19, 2016
perotinus pushed a commit to kubernetes-retired/cluster-registry that referenced this pull request Sep 2, 2017
Automatic merge from submit-queue

federation: Update the list of supported admission controls

Ref kubernetes/kubernetes#26298 (comment)

In 1.3, we are going to support only AlwaysAdmit and AlwaysDeny admission controls.
Updating the documentation accordingly.

@kubernetes/sig-cluster-federation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants