Labelling this PR as size/M

GCE e2e test build/test passed for commit 5e56389b9160a4b89aff80811210e8cdacf3594d.

• Build Log
• Test Artifacts
• Internal Jenkins Results

GCE e2e test build/test passed for commit 974d2eead2b383564566c02125f3a0b54c380393.

• Build Log
• Test Artifacts
• Internal Jenkins Results

If I want to run a third party API server, do I want to install /apis? I would think I don't. Probably should put this behind a boolean and default it to "not".

Why not?

/apis is where discovery information about all the groups supported by that server will be listed.
Discovery summarizer will use that to summarize all groups in the cluster (#20358) and dynamic client will use it to talk to the server directly.

Why is the summarizer talking to /apis on other servers and not talking directly to the individual group descriptions?

Sorry, read group descriptions from where?

The config file for discovery summarizer only contains server URLs and the summarizer is able to gather all the information from there.

Whats wrong with getting groups information from /apis? Thats what I think the dynamic client does as well. cc @krousey

I don't like a model that assumes that every third party API group exposes /apis. I don't think it's required (it's a nice property, but not fundamental), I don't think every API server will be exposed at the root, and I don't think the summarizer is aggregating "servers that look exactly like a Kube server". I might be ok with the summarizer taking a URL + optional path and based on the returned object type summarizing them - but I don't think that summarizer should assume /apis/ at the root.

I would expect the summarizer to fetch groups, not API servers, identified at paths (APIGroup).

• In current implementation, Summarizer reads a config file with list of servers and then fetches groups from those servers.
• Yes, we can remove the /apis assumption from the summarizer. I can just add another field groupsDiscoveryPath to the FederatedServers struct. If present, the summarizer will fetch that path instead of /apis.
• But, it is not clear to me how a dynamic client will interact with such a server. One way is to fetch that path using discovery API, but then we will need a fixed path for that :)

FWIW, the proposal specified it as one of the constraints that all federated servers need to list the supported groupVersions at /apis: https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/federated-api-servers.md#constraints, since that is what clients today use to discover all the groups supported by a server.

In the proposal I read should as "It's good that they do" - I guess I
want to pin this down to the minimum possible thing.

A dynamic client would call the summarizer, then walk to the APIGroup
endpoint. It shouldn't be walking from the summarizer /apis, to the third
party /apis, to the group api. One of the key design goals discussed (i
don't see it in the proposal, but i think it's just something we forgot to
add) is that a client should be able to hit /apis on the summarizer, and
then if any group is unrecognized, jump directly to that group and get the
list of resources, versions, and the swagger link. So we do 1+N calls at
most where N is number of APIGroups, and each APIGroup is something
cacheable.

On Wed, Feb 3, 2016 at 9:45 PM, Nikhil Jindal notifications@github.com
wrote:

• In current implementation, Summarizer reads a config file with list
  of servers and then fetches groups from those servers.
• Yes, we can remove the /apis assumption from the summarizer. I can
  just add another field groupsDiscoveryPath to the FederatedServers
  struct. If present, the summarizer will fetch that path instead of
  /apis.
• But, it is not clear to me how a dynamic client will interact with
  such a server. One way is to fetch that path using discovery API, but then
  we will need a fixed path for that :)

FWIW, the proposal specified it as one of the constraints that all
federated servers need to list the supported groupVersions at /apis:
https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/federated-api-servers.md#constraints,
since that is what clients today use to discover all the groups supported
by a server.

—
Reply to this email directly or view it on GitHub
#20532 (comment)
.

Yes.
When there is a summarizer, clients get groups discovery from summarizer's /apis and then jump directly to the group endpoint.

But we also want to support clients talking directly to a particular server, without talking to the summarizer first.
In that case, they talk to federated server's /apis to get groups discovery and then jump directly to the group endpoint.

Basically, the client should work same - when its talking directly to a single server and when its talking to a federated server via summarizer and proxy.

Have updated the code to register the handler at APIGroupPrefix which is passed as part of config, instead of hardcoding to "/apis" (which is what I should have done at the first place).

So while we should continue the important discussion we are having, I believe, this PR should be fine irrespective.

GCE e2e test build/test passed for commit 173f1cafee0569e30327fd7250e56b928c18b02a.

• Build Log
• Test Artifacts
• Internal Jenkins Results

I don't think the summarizer should include all groups automatically

• as a concrete example, having two summarizers (one is prod, one is
  preproduction / testing of new features), or organizations choosing
  not to expose group X, or forcing group X to go through a proxy /
  firewall (this is actually something we have people doing today who
  require higher level of auth / security for certain APIs).

So summarizer pointing to the API group endpoint vs API groups makes
it more granular.

To clarify, are you saying that instead of reading a list of server URLs from a config file and fetching the api groups automatically, summarizer should directly get a list of api groups in the config file?

I discussed this with @lavalamp and he pointed out that this will not work for dynamic groups, for ex: the third party groups.

GCE e2e build/test failed for commit d267ab5.

• Build Log
• Test Artifacts
• Internal Jenkins Results

I that it's more common for folks to want to expose some of these APIs, not
all of them. I doubt most people are going to want to summarize
extensions, third party, etc without them being whitelisted. I'd prefer
not to have the dynamic path be the only story, since it either requires
you run your groups on individual API servers or have only way of exposing
them.

On Thu, Feb 4, 2016 at 9:40 PM, Kubernetes Bot notifications@github.com
wrote:

GCE e2e build/test failed for commit d267ab5
d267ab5
.

• Build Log
  https://storage.cloud.google.com/kubernetes-jenkins/pr-logs/pull/20532/kubernetes-pull-build-test-e2e-gce/26883/build-log.txt
• Test Artifacts
  https://console.developers.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/20532/kubernetes-pull-build-test-e2e-gce/26883/artifacts/
• Internal Jenkins Results
  http://goto.google.com/prkubekins/job/kubernetes-pull-build-test-e2e-gce//26883

—
Reply to this email directly or view it on GitHub
#20532 (comment)
.

@nikhiljindal
You must link to the test flake issue which caused you to request this manual re-test.
Re-test requests should be in the form of: k8s-bot test this issue: #<number>
Here is the list of open test flakes.

GCE e2e build/test failed for commit d267ab5.

• Build Log
• Test Artifacts
• Internal Jenkins Results

@k8s-bot: test this github issue: #20646

@k8s-bot: test this issue: #20646

@k8s-bot: test this issue #20646

@k8s-bot test this issue: #20646

@smarterclayton Are you OK with a world where we make it easiest to put all your groups in /apis, but also offer exceptions in the discovery summarizer?

I think if we only offer a per-group whitelist, that's very inconvenient because it means you always have to restart two components to turn on a new api. I can definitely see why some people would want that control. Maybe we can hash this out in the PR that adds the config format for the discovery summarizer.

It looks like this PR is just about making /apis easier--I think your comments are actually about the discovery summarizer, not this PR, is that correct? If so, this LGTM

Yes. In my discovery summarizer PR (#20358), the summarizer is reading a list of FederatedServers from the config file. The FederatedServer struct just has a ServerAddress for now. In future, we can easily add filtering options (whitelist or blacklist) for groups.

GCE e2e test build/test passed for commit d267ab5.

• Build Log
• Test Artifacts
• Internal Jenkins Results

We can move the discussion to summarizer. I think the assumption that
/apis is available on every kubernetes compatible server is a bad one, and
assuming anything about server paths in real world environments is also
fraught. So my problem isn't with the existence of /apis, it's assuming
(and requiring) /apis to make sense.

On Fri, Feb 5, 2016 at 1:58 PM, Kubernetes Bot notifications@github.com
wrote:

GCE e2e test build/test passed for commit d267ab5
d267ab5
.

• Build Log
  https://storage.cloud.google.com/kubernetes-jenkins/pr-logs/pull/20532/kubernetes-pull-build-test-e2e-gce/27072/build-log.txt
• Test Artifacts
  https://console.developers.google.com/storage/browser/kubernetes-jenkins/pr-logs/pull/20532/kubernetes-pull-build-test-e2e-gce/27072/artifacts/
• Internal Jenkins Results
  http://goto.google.com/prkubekins/job/kubernetes-pull-build-test-e2e-gce//27072

—
Reply to this email directly or view it on GitHub
#20532 (comment)
.

Yes. I can make the /apis path configurable in the summarizer config.
Will update that PR once #20626 is merged.

Adding the LGTM tag as per comments above.

Thanks! :)