wire in aggregator #14285

deads2k · 2017-05-22T19:02:16Z

I think there are more picks that are going to be required and I'm sorting out some bad behavior around hangs, but this is the direction I'm going in.

@openshift/api-review for the the aggregator config commit.

Right now, I've only tested this manually (no test image is yet pushed and it requires custom master-config.yaml and active nodes).

Generate front-proxy-ca cert/key and aggregator-front-proxy-client cert/key. I did this with

oadm ca create-signer-cert --cert=openshift.local.config/master/front-proxy-ca.crt --key=openshift.local.config/master/front-proxy-ca.key
oadm create-api-client-config --certificate-authority=openshift.local.config/master/front-proxy-ca.crt --signer-cert=openshift.local.config/master/front-proxy-ca.crt --signer-key=openshift.local.config/master/front-proxy-ca.key --user aggregator-front-proxy --client-dir=openshift.local.config/master

and then deleting everything but the files I wanted.
2. Generated a fresh master-config.yaml.
3. Replace stanzas in master-config.yaml with

aggregatorConfig:
  proxyClientInfo:
    certFile: aggregator-front-proxy.crt
    keyFile: aggregator-front-proxy.key
authConfig:
  requestHeader:
    clientCA: front-proxy-ca.crt
    clientCommonNames: 
    - aggregator-front-proxy
    usernameHeaders:
    - X-Remote-User
    groupHeaders:
    - X-Remote-Group
    extraHeaderPrefixes:
    - X-Remote-Extra-

sudo $(which openshift) start --master-config openshift.local.config/master/master-config.yaml --node-config openshift.local.config/node-deads-dev-01/node-config.yaml
Create the image to use by cloning kube and building: nice make WHAT=vendor/k8s.io/sample-apiserver/ && vendor/k8s.io/sample-apiserver/hack/build-image.sh
oc new-project wardle - new project to start with
oadm policy add-scc-to-user privileged -z apiserver - etcd doesn't like running as non-root and I didn't feel like trying to deal with it
oc create policybinding kube-system -n kube-system
oc create -f test/extended/testdata/aggregator/ - create the resources
oc get flunders - proof!

deads2k · 2017-05-22T19:02:27Z

[test]

deads2k · 2017-05-22T20:22:27Z

Error from server (InternalError): an error on the server ("Error: 'net/http: invalid header field name \"X-Remote-Extra-authorization.openshift.io/scopes\"'\nTrying to reach: 'https://172.30.134.150/apis/wardle.k8s.io/v1alpha1'") has prevented the request from succeeding

Well, that's a problem. @liggitt got suggestions?

deads2k · 2017-05-22T20:27:13Z

Well, it can proxy certificate based users at the moment, so its a strict improvement. Let's get some reviews on this much.

deads2k · 2017-05-22T20:29:05Z

@bparees you were asking.

liggitt · 2017-05-23T00:33:37Z

Error from server (InternalError): an error on the server ("Error: 'net/http: invalid header field name "X-Remote-Extra-authorization.openshift.io/scopes"'\nTrying to reach: 'https://172.30.134.150/apis/wardle.k8s.io/v1alpha1'") has prevented the request from succeeding

Well, that's a problem. @liggitt got suggestions?

...

deads2k · 2017-05-23T11:38:35Z

Well, that's a problem. @liggitt got suggestions?

Yeah. Technically its never worked, so changing the name can work. scopes.authorization.openshift.io I guess.

deads2k · 2017-05-23T18:39:01Z

I won't claim its turnkey, but I've added directions @bparees or @pmorie want to try it.

bparees · 2017-05-23T18:40:37Z

jessica actually gave me a path to console SC usage that doesn't require the aggregator after all, so i'm going to try that first. I'm still bleeding from my experience as an api server guinea pig.

deads2k · 2017-05-23T18:51:46Z

@sdodson I'm guessing that @pmorie and @derekwaynecarr have already engaged you regarding how to wire up the service catalog for its beta installation, but the instructions I've included in the description show the particular fields used for wiring the API server to enable the aggregator to route traffic to the service catalog.

sdodson · 2017-05-23T18:56:31Z

@ewolinetz and @jpeeler are collaborating (or about to) on that work.

liggitt · 2017-05-24T02:12:30Z

add aggregator config commit LGTM

mfojtik · 2017-05-24T13:19:25Z

pkg/cmd/server/origin/aggregator.go

+		// TODO this is probably an indication that we need explicit and precise control over the discovery chain
+		// but for now its a special case
+		// apps has to come last for compatibility with 1.5 kubectl clients
+		if apiService.Spec.Group == "apps" {


just to confirm, this won't conflict with apps.openshift.io?

mfojtik · 2017-05-24T13:19:59Z

pkg/cmd/server/origin/master.go

+
+	aggregatorConfig, err := c.createAggregatorConfig(*kc.Master.GenericConfig)
+	if err != nil {
+		glog.Fatalf("Failed to launch master: %v", err)


Failed to create aggregator config

mfojtik · 2017-05-24T13:21:48Z

pkg/authorization/api/types.go

@@ -22,7 +22,7 @@ const (
 	VerbAll        = "*"
 	NonResourceAll = "*"

-	ScopesKey           = "authorization.openshift.io/scopes"
+	ScopesKey           = "scopes.authorization.openshift.io"


do we need to update/migrate something after this change?

do we need to update/migrate something after this change?

No. I reasoned through this with jordan. It's merging in a separate pull.

mfojtik · 2017-05-24T13:26:26Z

LGTM, but will prefer @enj to have second look before merging.

deads2k · 2017-05-24T17:56:09Z

@smarterclayton I seem to be failing on

===== Verifying Generated Bindata =====
Generating bindata...
FAILURE: Generation of fresh bindata failed:
error: extended bindata is 656537 bytes, reduce the size of the import

Any idea what's up?

deads2k · 2017-05-24T19:02:20Z

comments addressed [merge]

enj

Minor comments.

enj · 2017-05-23T18:57:45Z

test/integration/etcd_storage_path_test.go

 	if err != nil {
 		t.Fatalf("error starting server: %#v", err)
 	}
+	kubeConfigFile := masterConfig.MasterClients.OpenShiftLoopbackKubeConfig


I assume you will drop this commit so the comment gets added.

enj · 2017-05-23T19:17:07Z

pkg/authorization/api/types.go

@@ -22,7 +22,7 @@ const (
 	VerbAll        = "*"
 	NonResourceAll = "*"

-	ScopesKey           = "authorization.openshift.io/scopes"
+	ScopesKey           = "scopes.authorization.openshift.io"


er... so based on comments this never worked? How did we not notice?

enj · 2017-05-24T18:51:14Z

pkg/cmd/server/origin/master.go

+		go apiserver.Run(utilwait.NeverStop)
+
+		// Attempt to verify the server came up for 20 seconds (100 tries * 100ms, 100ms timeout per try)
+		cmdutil.WaitForSuccessfulDial(c.TLS, c.Options.ServingInfo.BindNetwork, c.Options.ServingInfo.BindAddress, 100*time.Millisecond, 100*time.Millisecond, 100)


Do we not care if we fail here?

Do we not care if we fail here?

pre-existing. Not really sure what it does

pre-existing. Not really sure what it does

Tries a bunch of times and returns and error on failure (which we ignore and just keep going).

enj · 2017-05-24T18:54:06Z

pkg/cmd/server/origin/master.go

+	if err != nil {
+		glog.Fatalf("Failed to create aggregator config: %v", err)
+	}
+	aggregatorServer, err := createAggregatorServer(aggregatorConfig, apiserver.GenericAPIServer, kc.Informers.InternalKubernetesInformers(), stopCh)


Do you plan to use kc.Informers.InternalKubernetesInformers() later?

Do you plan to use kc.Informers.InternalKubernetesInformers() later?

it gets started elsewhere to drive this.

it gets started elsewhere to drive this.

I do not understand what you mean. All I was saying is that createAggregatorServer does not use the sharedInformers parameter, so why bother passing it?

deads2k · 2017-05-26T14:53:05Z

Looks like I have some verifies to sort through.

smarterclayton · 2017-05-26T15:13:15Z

This is a blocker for the 3.6 release, I'm putting this at [severity:blocker]. Will review

deads2k · 2017-05-26T16:11:22Z

rebased and fixed up verifies I think

deads2k · 2017-05-26T19:04:31Z

@smarterclayton I seem to be unable to make protobuf verification happy. Can you try and see?

openshift-bot · 2017-05-26T19:30:02Z

Evaluated for origin test up to c57c9c8

openshift-bot · 2017-05-26T21:21:45Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/1808/) (Base Commit: 69de7d3)

smarterclayton · 2017-05-26T22:28:54Z

Flake from journalctl output.

openshift-bot · 2017-05-28T04:37:31Z

Evaluated for origin merge up to c57c9c8

openshift-bot · 2017-05-28T04:37:42Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin/832/) (Base Commit: 63fe34a) (Extended Tests: blocker) (Image: devenv-rhel7_6280)

deads2k added the needs-api-review label May 22, 2017

deads2k force-pushed the agg-03-wire branch from c35fccb to 71c77aa Compare May 22, 2017 19:12

deads2k changed the title ~~[wip] wire in aggregator~~ wire in aggregator May 22, 2017

deads2k force-pushed the agg-03-wire branch from 9c62263 to 1f4a386 Compare May 23, 2017 14:54

mfojtik requested a review from enj May 23, 2017 15:01

deads2k force-pushed the agg-03-wire branch from 1f4a386 to 9d6404f Compare May 23, 2017 17:24

deads2k assigned enj May 23, 2017

deads2k added api-approved and removed needs-api-review labels May 24, 2017

mfojtik reviewed May 24, 2017

View reviewed changes

deads2k force-pushed the agg-03-wire branch from ff44616 to 89e2c93 Compare May 24, 2017 17:55

enj reviewed May 24, 2017

View reviewed changes

deads2k force-pushed the agg-03-wire branch from 89e2c93 to 6b86321 Compare May 24, 2017 19:27

deads2k added 6 commits May 26, 2017 12:08

UPSTREAM: 43301: add APIService conditions

dc2a2aa

UPSTREAM: <drop>: regenerate proto

14338b7

add aggregator config

48077e9

wire in aggregator

702e4b7

UPSTREAM: 44837: Fix Content-Type error of apis

1796e7d

add artifacts for aggregation testing

236ecbc

deads2k force-pushed the agg-03-wire branch from 6b86321 to b64b5e0 Compare May 26, 2017 16:08

generated

c57c9c8

deads2k force-pushed the agg-03-wire branch from b64b5e0 to c57c9c8 Compare May 26, 2017 19:24

openshift-bot merged commit fce1a37 into openshift:master May 28, 2017

ncdc mentioned this pull request May 30, 2017

Various build/test fixes #14412

Merged

This was referenced Jun 1, 2017

ServiceCatalog needs to enable API aggregation openshift/openshift-ansible#4350

Closed

[DO NOT MERGE] demonstrate commands in a bash script to help #14459

Closed

deads2k deleted the agg-03-wire branch August 3, 2017 19:25

wire in aggregator #14285

wire in aggregator #14285

Conversation

deads2k commented May 22, 2017 • edited by jpeeler Loading

deads2k commented May 22, 2017

deads2k commented May 22, 2017

deads2k commented May 22, 2017

deads2k commented May 22, 2017

liggitt commented May 23, 2017

deads2k commented May 23, 2017

deads2k commented May 23, 2017

bparees commented May 23, 2017

deads2k commented May 23, 2017

sdodson commented May 23, 2017

liggitt commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented May 24, 2017

deads2k commented May 24, 2017

deads2k commented May 24, 2017

enj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented May 26, 2017

smarterclayton commented May 26, 2017

deads2k commented May 26, 2017

deads2k commented May 26, 2017

openshift-bot commented May 26, 2017

openshift-bot commented May 26, 2017

smarterclayton commented May 26, 2017

openshift-bot commented May 28, 2017

openshift-bot commented May 28, 2017 • edited Loading

deads2k commented May 22, 2017 •

edited by jpeeler

Loading

enj May 24, 2017 •

edited

Loading

openshift-bot commented May 28, 2017 •

edited

Loading