Rebase on 1.7 API machinery #944

ash2k · 2017-06-15T06:31:13Z

There was a bug in Kubernetes I found (kubernetes/kubernetes#46817) and fixed (kubernetes/kubernetes#46683) recently and I want one of the projects I'm working on (Smith) to pick it up. So I need to update dependency on apimachinery repo. Smith integrates with Service Catalog and hence I need to satisfy the Service Catalog dependency as well. Currently if I just update client-go and apimachinery to the latest versions some stuff from Service Catalog that I use in Smith will not compile. Hence the attempt to update the dependencies.
There are some places that still do not compile but I don't know how to fix them :(

/cc @arschles

nilebox · 2017-06-15T06:37:04Z

One of the major things broken after migrating to the latest apiserver is that they got rid of insecure HTTP server, https is the only option now.

IIRC the insecure server was used at least for the walkthrough and integration tests.

nilebox · 2017-06-15T06:42:38Z

looks like I managed to fix most of the issues with API server and Admission control, and Service Catalog compiles now, but the tests are still failing

nilebox · 2017-06-15T06:49:11Z

The examples for the changes were mostly taken from https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/sample-apiserver (and some others are based on the history of commits for the particular types)

nilebox · 2017-06-15T11:45:31Z

pkg/storage/tpr/storage_interface_test.go

@@ -141,6 +141,9 @@ func TestCreateWithNoNamespace(t *testing.T) {
 		Spec: sc.BrokerSpec{
 			URL: "http://my-awesome-broker.io",
 		},
+		Status: sc.BrokerStatus{
+			Conditions: []sc.BrokerCondition{},


It seems there was some change which affected treating nil arrays in JSON encoding/decoding.
Without this change (and other changes in this file), deepCompare fails with error:

input and output are different: object.Status.Conditions: a: []servicecatalog.BrokerCondition(nil) b: []servicecatalog.BrokerCondition{}

The change you're referring to is using protobuf serialization in etcd.

Replacing reflect.DeepEqual with semantic.DeepEqual worked

nilebox · 2017-06-15T11:47:45Z

cmd/apiserver/app/server/options.go

@@ -31,16 +31,16 @@ type ServiceCatalogServerOptions struct {
 	StorageTypeString string
 	// the runtime configuration of our server
 	GenericServerRunOptions *genericserveroptions.ServerRunOptions
+	// the admission options
+	AdmissionOptions *genericserveroptions.AdmissionOptions


AdmissionOptions were moved out of genericserveroptions.ServerRunOptions

In the apimachinery repo, you mean?

@pmorie Yes, now they need to be explicitly declared separately from ServerRunOptions

nilebox · 2017-06-15T11:48:34Z

cmd/apiserver/app/server/options.go

 	// the https configuration. certs, etc
 	SecureServingOptions *genericserveroptions.SecureServingOptions
 	// authn for the API
 	AuthenticationOptions *genericserveroptions.DelegatingAuthenticationOptions
 	// authz for the API
 	AuthorizationOptions *genericserveroptions.DelegatingAuthorizationOptions
-	// InsecureOptions are options for serving insecurely.
-	InsecureServingOptions *genericserveroptions.ServingOptions


Insecure (HTTP) server is not supported anymore, only HTTPS is supported by API server now

nilebox · 2017-06-15T11:49:38Z

cmd/apiserver/app/server/server.go

+
+// registerAllAdmissionPlugins registers all admission plugins
+func registerAllAdmissionPlugins(plugins *admission.Plugins) {
+	lifecycle.Register(plugins)


Admission plugins registration is not global anymore, requires *admission.Plugins instance

nilebox · 2017-06-15T11:54:07Z

Unit tests succeed now, but integration tests fail because of removed insecure (HTTP) server support:

Starting the API Server
Waiting for API Server to be available...
Timed-out waiting for API Server
+curl http://localhost:32768
curl: (56) Recv failure: Connection reset by peer

Need to refactor all the places where we depend on insecure server:

integration tests
walkthrough
something else?

pmorie · 2017-06-15T12:58:34Z

While I applaud the initiative taken here, as you can tell, it's a very large change and has a lot of implications. So, before we merge this change, we really need to:

Ensure that we're rebasing onto the final release versions of the 1.7 machinery
Discuss the implications (such as no longer supporting insecure) as a group and determine a plan to deal with them
Get aggregator setup instructions merged
Determine exactly what implications the move to protobuf serialization has and see whether there is a plan to deal with them

The protobuf serialization and losing support for insecure are the two pieces that worry me the most.

In the interest of full disclosure, our last rebase onto newer machinery took a couple weeks to land. I think you should expect that there will be a similar time-frame for this one.

nilebox · 2017-06-15T13:42:19Z

@pmorie Fair enough, let's see how it goes after Kubernetes 1.7 is released

MHBauer · 2017-06-15T15:54:21Z

I wanted to do something like this as well, but only to move from 1.6-rc1 to 1.6.4. I think we need to write down a policy & process for what we want to do.

I'm someone surprised the integration tests are breaking. They used to all be https enabled, but I guess the insecure server snuck in somehow.

nilebox · 2017-06-15T22:08:17Z

@MHBauer that was just my guess, could be some other issue with secure server then

MHBauer · 2017-06-27T00:25:01Z

plugin/pkg/admission/namespace/lifecycle/admission.go

-	admission.RegisterPlugin(PluginName, func(io.Reader) (admission.Interface, error) {
+// Register registers a plugin
+func Register(plugins *admission.Plugins) {
+	plugins.Register(PluginName, func(io.Reader) (admission.Interface, error) {


yay, we'll get rid of an init. This was being done upstream and we're following? If so, that's very good and makes me happy.

@MHBauer yes, see https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/apiserver/pkg/admission/plugin/namespace/lifecycle/admission.go#L54 for example

MHBauer · 2017-06-27T05:16:22Z

I think I can help with the secure/insecure server stuff. I'm not sure if we want to do it now or right around 1.7.

I see there were some concerns about timeouts or something which prompted the move to insecure. I wish the servers took a 0 port to mean "bind me something unbound" like every other server instead of overloading a port number to mean "I don't want a server".

MHBauer · 2017-06-27T05:18:13Z

Further the check shouldn't be using http to see if the apiserver is up, as the actual test uses https.

nilebox · 2017-06-27T05:20:42Z

@MHBauer Your help would be greatly appreciated.

I don't think we have to wait until 1.7 is released to continue working on this pull request, as 1.7-rc.1 is out already and there should be no API changes breaking our code (it would be nice to update the dependency in glide.yaml to release-1.7 branch though).

MHBauer · 2017-06-27T21:30:53Z

I was wrong. There was a hidden manual override in the kubectl script so it always used http.

Can try and see if #991 makes it get farther in the tests. Doesn't solve the golang integration tests. Only the kubectl<->apiserver communications.

ash2k · 2017-07-01T04:01:01Z

I will rebase this PR once kubernetes/client-go#231 is addressed.

ash2k · 2017-07-08T11:00:57Z

Rebased. Integration tests fail for some reason. I'm sorry, don't have time to debug them at the moment. At least everything seem to compile.

I used apiserver hash that is from 23rd of June - same day last client-go/apimachinery resync for 1.7 happened. Filled kubernetes/apiserver#16 to get proper version.

nilebox · 2017-07-10T04:15:56Z

@ash2k it's the same failure as before re-rebasing (see it mentioned above):

Starting the API Server
55fe2295f01c810bf056862ba889fc1dfe36fdb4cb5f4d7210edd43674bda819
Waiting for API Server to be available...
Timed-out waiting for API Server
+curl http://localhost:32768
curl: (56) Recv failure: Connection reset by peer

I believe it's failing due to waiting for insecure server to start up, which is not available anymore.
@MHBauer are you still keen to help with this?

nilebox · 2017-07-11T07:01:24Z

After applying the fix #1015 I get:

Waiting for API Server to be available...
Timed-out waiting for API Server
+ curl --cacert /Users/nislamov/go/src/github.com/kubernetes-incubator/service-catalog/.var/run/kubernetes-service-catalog/apiserver.crt https://localhost:32777
curl: (35) gnutls_handshake() failed: The TLS connection was non-properly terminated.

TLS handshake fails for some reason.
From the apiserver logs I see:

I0711 06:50:48.924636       1 etcd_config.go:102] Installing API group servicecatalog.k8s.io
F0711 06:50:48.925326       1 etcd_config.go:104] Error installing API group servicecatalog.k8s.io:
...
error in registering resource: bindings, no kind "ExportOptions" is registered for version "v1",
error in registering resource: bindings/status, no kind "ExportOptions" is registered for version "v1",
error in registering resource: brokers, no kind "ExportOptions" is registered for version "v1",
error in registering resource: brokers/status, no kind "ExportOptions" is registered for version "v1",
error in registering resource: instances, no kind "ExportOptions" is registered for version "v1",
error in registering resource: instances/status, no kind "ExportOptions" is registered for version "v1",
error in registering resource: serviceclasses, no kind "ExportOptions" is registered for version "v1"

@MHBauer @ash2k any ideas why this is happening?

MHBauer · 2017-07-12T05:37:05Z

Yes, I can still help. I want this done. Do we have an acceptable and known compile compatible levels of all the libraries picked out?

I'll take a look at the tests in the morning.

MHBauer · 2017-07-12T05:42:07Z

pkg/apiserver/config.go

+	Codecs = serializer.NewCodecFactory(Scheme)
+)
+
+func init() {


Is this truly necessary? My hate for inits knows no bounds.

ask @deads2k :) https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/sample-apiserver/pkg/apiserver/apiserver.go#L44

I don't remember why I copied this, presumably something didn't work without that but I'll double check.

Actually this method invokes apimachinery AddToGroupVersion (https://github.com/kubernetes/apimachinery/blob/master/pkg/apis/meta/v1/register.go#L43) which should register the ExportOptions type, so I'm not sure why I see the error in the log mentioned above.

Moved this code to a separate file

It's a constant/global for the binary. Not having an init block just means that you can have a uninitialized value that creates unusual behavior until you wrap a method in a sync.Once and then have every caller to your package call it. That's nuts.

Alternatively, you could plumb it as an arg everywhere that needs it, which is a valid solution, but a level of purity we hadn't reached in the apiserver refactors yet.

nilebox · 2017-07-12T07:53:06Z

@MHBauer @ash2k see my latest commit. I managed to start a server, the integration test build is failing now:

# github.com/kubernetes-incubator/service-catalog/test/integration
test/integration/framework.go:97: options.InsecureServingOptions undefined (type *"github.com/kubernetes-incubator/service-catalog/cmd/apiserver/app/server".ServiceCatalogServerOptions has no field or method InsecureServingOptions)
test/integration/framework.go:98: options.SecureServingOptions.ServingOptions undefined (type *options.SecureServingOptions has no field or method ServingOptions)
FAIL	github.com/kubernetes-incubator/service-catalog/test/integration [build failed]

Happy to look at it later.

nilebox · 2017-07-12T10:50:18Z

Now the server starts up, but there is a TLS handshake error in test/integration/framework.go#L105:

2017-07-12 20:47:23.474246 I | server.go:2753: http: TLS handshake error from [::1]:53465: remote error: tls: bad certificate
	framework.go:69: Starting server on port: 31786
	framework.go:106: timed out waiting for the condition

We probably need some client-side configuration for accepting the self-signed certificate.

MHBauer · 2017-07-12T23:17:10Z

for the 'check if it's up yet' part in waitForApiserverUp()

	tr := &http.Transport{
		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
	}
	http := &http.Client{Transport: tr}

MHBauer · 2017-07-12T23:18:00Z

This should help with the svc-cat client configuration.

	config.Host = secureAddr
	config.CertFile = secureServingOptions.ServerCert.CertKey.CertFile
	config.KeyFile = secureServingOptions.ServerCert.CertKey.KeyFile

pmorie

Looking great; a couple small asks and comments below. I have not yet had a chance to pull this branch and test it out; that is one of my priorities tomorrow morning.

Welcome to the small club of those who have rebased on the k8s API machinery. The question I ask everyone who has done this is: what did you have to explore in the machinery to understand the necessary changes? Can any comments be improved in the API machinery? If any fixes came immediately to mind, it would be fantastic to get PRs for them into k8s if you have some time, and issues at least, if you don't.

A couple observations I made:

The actual commit for the code changes aside from bumps and regen is really small -- 238 total diff stat lines (!)
Things seem to be moving in the right direction in terms of removing global registrations (example: admission controller plugins)
Dependency bump is pretty enormous :)

pmorie · 2017-07-14T05:56:58Z

cmd/apiserver/app/server/util.go

 	// server configuration options
-	if err := s.SecureServingOptions.MaybeDefaultWithSelfSignedCerts(s.GenericServerRunOptions.AdvertiseAddress.String()); err != nil {
+	if err := s.SecureServingOptions.MaybeDefaultWithSelfSignedCerts(s.GenericServerRunOptions.AdvertiseAddress.String(), nil, []net.IP{net.ParseIP("127.0.0.1")}); err != nil {


nit: can you doc the nil param here with a /* ... */ inline comment? Helps to make nil parameters much more readable.

pmorie · 2017-07-14T05:58:53Z

cmd/controller-manager/app/controller_manager.go

@@ -143,6 +143,8 @@ func Run(controllerManagerOptions *options.ControllerManagerServer) error {
 		// TODO: disambiguate API errors
 		return fmt.Errorf("failed to get Service Catalog client configuration: %v", err)
 	}
+	// TODO (nilebox): Dirty temporary hack. Should whether accept as flag for application, or handle TLS correctly
+	serviceCatalogKubeconfig.Insecure = true


I'd be fine running a flag for TLS verification to the SC API server in this PR; should be a fairly simple change.

pmorie · 2017-07-14T06:01:15Z

pkg/apis/servicecatalog/serialization_test.go

@@ -156,7 +157,7 @@ func roundTrip(t *testing.T, codec runtime.Codec, item runtime.Object) {
 		return
 	}

-	if !api.Semantic.DeepEqual(original, item) {
+	if !equality.Semantic.DeepEqual(original, item) {


I believe the package change here is due to the new version of demantic dep equal that isn't broken by protobuf changes, right?

pmorie · 2017-07-14T06:02:08Z

pkg/apiserver/util.go

@@ -56,7 +56,7 @@ func createSkeletonServer(genericCfg *genericapiserver.Config) (*ServiceCatalogA
 	// only way to get here from there is by Complete()'ing. Thus
 	// we skip the complete on the underlying config and go
 	// straight to running it's New() method.
-	genericServer, err := genericCfg.SkipComplete().New()
+	genericServer, err := genericCfg.SkipComplete().New("service-catalog-apiserver", genericapiserver.EmptyDelegate)


Make this a const?

pmorie · 2017-07-14T06:03:21Z

pkg/registry/servicecatalog/binding/storage.go

@@ -94,12 +94,12 @@ func toSelectableFields(binding *servicecatalog.Binding) fields.Set {
 }

 // GetAttrs returns labels and fields of a given object for filtering purposes.
-func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, error) {
+func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, bool, error) {


Will you godoc the parameters or use names? It's hard to infer what the bool represents.

@pmorie GetAttrs is not documented upstream. I do not expect anyone to know what these args are. @cheftako

https://github.com/kubernetes/apiserver/blob/master/pkg/storage/selection_predicate.go#L25-L27

// AttrFunc returns label and field sets and the uninitialized flag for List or Watch to match. // In any failure to parse given object, it returns error. type AttrFunc func(obj runtime.Object) (labels.Set, fields.Set, bool, error)

https://github.com/kubernetes/apiserver/blob/master/pkg/storage/selection_predicate.go#L25-L27

Looking through git history the bool is "the uninitialized flag". I do not know what that means. My guess from looking at the commit is whether the thing that we're getting has been set?

kubernetes/apiserver@5fa08b8

@pmorie as @MHBauer pointed, the change just follows the breaking change in AttrFunc method https://github.com/kubernetes/apiserver/blob/master/pkg/storage/selection_predicate.go#L25

And the method doesn't have any parameters description in many implementations: podtemplate/strategy.go#L92, replicationcontroller/strategy.go#L148, daemonset/strategy.go#L134.

I agree that it is not well documented, but for Service Catalog just being compliant with Kubernetes core code should be enough. It makes it easier to sync up all the breaking changes as needed.

Looking through git history the bool is "the uninitialized flag".

@MHBauer it is the "uninitialized flag", as mentioned in the comment to the AttrFunc you quoted above. I would prefer to keep it as it is to be in sync with upstream.

I do not know what that means. My guess from looking at the commit is whether the thing that we're getting has been set?

it checks whether there is still at least one initializer set (see comment to the field meta/v1/types.go#L216):

// This field is a list of initializers that have not yet acted on this object. If nil or empty, // this object has been completely initialized. Otherwise, the object is considered uninitialized // and is hidden (in list/watch and get calls) from clients that haven't explicitly asked to // observe uninitialized objects.

if there is at least one initializer set, it means that object is not fully initialized yet. Or, in other words, it is "uninitialized". The uninitialized objects are ignored for list/watch requests by default, as the comment above describes - that's what "unitialized flag" is for.

"initializers" approach seem to be similar to "finalizers" (but for the opposite purpose), if that helps to understand the mechanics.

@nilebox thanks for the explanation. I think you could get a nice PR upstream with some of this as documentation.

pmorie · 2017-07-14T06:04:00Z

pkg/registry/servicecatalog/broker/storage.go

@@ -94,12 +94,12 @@ func toSelectableFields(broker *servicecatalog.Broker) fields.Set {
 }

 // GetAttrs returns labels and fields of a given object for filtering purposes.
-func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, error) {
+func GetAttrs(obj runtime.Object) (labels.Set, fields.Set, bool, error) {


Same thing on the other GetAttrs funcs

pmorie · 2017-07-14T06:05:46Z

pkg/registry/servicecatalog/server/server.go

+	"github.com/kubernetes-incubator/service-catalog/pkg/apis/servicecatalog/install"
+)
+
+var (


@cheftako This new file would have been a good thing to explain in API machinery release notes.

nilebox · 2017-07-17T03:48:15Z

pkg/apis/componentconfig/types.go

@@ -54,6 +54,13 @@ type ControllerManagerConfiguration struct {
 	// ServiceCatalogKubeconfigPath is the path to the kubeconfig file with
 	// information about the service catalog API server.
 	ServiceCatalogKubeconfigPath string
+	// InsecureSkipVerify controls whether a client verifies the


This comment was shamelessly stolen from crypto/tls

nilebox · 2017-07-17T03:49:23Z

@pmorie minor comments addressed and squashed in the same commit.
The addition of --service-catalog-insecure-skip-verify flag is in separate commit for easier review, please take a look at diff 4925c76

MHBauer · 2017-07-17T05:38:06Z

charts/catalog/templates/controller-manager-deployment.yaml

@@ -54,6 +54,7 @@ spec:
        - --service-catalog-api-server-url
        - https://{{ template "fullname" . }}-apiserver
        {{- end }}
+        - "--service-catalog-insecure-skip-verify=true"


Why on by default? I feel like that should be mentioned in the readme/walkthrough somewhere. I'd prefer if we could have everything be secure by default. And if not, I'd like to fully understand why.

I might be wrong, but if we want to disable this flag, we also need to pass some extra information in the controller-manager (information about cerificate and authority we trust to). Without it the TLS verification will still fail.
In other words, this flag doesn't fix the original problem, just removes the hack from the code.

I haven't fully analyzed the part of interaction between controller-manager and service-catalog, and would prefer to ask someone more familiar with it (you? 😉) to test it with disabled flag and raise a separate PR adressing this.

I'm fine with looking at it separately.

MHBauer · 2017-07-19T22:21:42Z

Rebase makes me sad.

I still want this. A lot.

nilebox · 2017-07-19T22:40:02Z

@MHBauer I'll rebase once I get one more LGTM

nilebox · 2017-07-20T12:24:38Z

sigh... I have some issue with git rebase:

❯ git rebase upstream/master
First, rewinding head to replay your work on top of it...
Applying: Update vendor
Using index info to reconstruct a base tree...
M	glide.lock
M	glide.yaml
.git/rebase-apply/patch:4608: trailing whitespace.
			"details": [{"code": "conflict1", "message":"error message1"},
.git/rebase-apply/patch:5806: trailing whitespace.

.git/rebase-apply/patch:5863: trailing whitespace.
2015/11/08 18:28:39 loading certificate...
.git/rebase-apply/patch:5864: trailing whitespace.
2015/11/08 18:28:39 retrieve oauth token...
.git/rebase-apply/patch:5865: trailing whitespace.
2015/11/08 18:28:39 querying the list of resource groups...
/usr/local/Cellar/git/2.13.3/libexec/git-core/git-rebase--am: line 16: 70448 Segmentation fault: 11  git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" ${gpg_sign_opt:+"$gpg_sign_opt"} < "$GIT_DIR/rebased-patches"

@MHBauer can you try if it works for you?

MHBauer · 2017-07-20T15:39:27Z

holy crap, we segfaulted git?

First, rewinding head to replay your work on top of it...
Applying: Update vendor
Using index info to reconstruct a base tree...
M	glide.lock
M	glide.yaml
.git/rebase-apply/patch:4608: trailing whitespace.
			"details": [{"code": "conflict1", "message":"error message1"},
.git/rebase-apply/patch:5806: trailing whitespace.

.git/rebase-apply/patch:5863: trailing whitespace.
2015/11/08 18:28:39 loading certificate...
.git/rebase-apply/patch:5864: trailing whitespace.
2015/11/08 18:28:39 retrieve oauth token...
.git/rebase-apply/patch:5865: trailing whitespace.
2015/11/08 18:28:39 querying the list of resource groups...
/usr/local/Cellar/git/2.13.0/libexec/git-core/git-rebase--am: line 16: 80355 Segmentation fault: 11  git am $git_am_opt --rebasing --resolvemsg="$resolvemsg" ${gpg_sign_opt:+"$gpg_sign_opt"} < "$GIT_DIR/rebased-patches"

I've never seen messages like this.

Tried optimizing the git repo.
Tried disabling signing.
use a new version of git
use an older version of git

use a different strategy finally worked. git rebase -m master. Not sure if that was it or something else.

Give that glide.lock is one of the files, it's probably better to regenerate the first two commits manually and then cherry pick the second two of the set.

MHBauer · 2017-07-21T00:08:52Z

I see

# github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/internalclientset/typed/servicecatalog/internalversion/fake
pkg/client/clientset_generated/internalclientset/typed/servicecatalog/internalversion/fake/fake_binding.go:93: not enough arguments in call to testing.NewListAction
        have (schema.GroupVersionResource, string, "github.com/kubernetes-incubator/service-catalog/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
        want (schema.GroupVersionResource, schema.GroupVersionKind, string, interface {})
pkg/client/clientset_generated/internalclientset/typed/servicecatalog/internalversion/fake/fake_broker.go:87: not enough arguments in call to testing.NewRootListAction
        have (schema.GroupVersionResource, "github.com/kubernetes-incubator/service-catalog/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
        want (schema.GroupVersionResource, schema.GroupVersionKind, interface {})
pkg/client/clientset_generated/internalclientset/typed/servicecatalog/internalversion/fake/fake_instance.go:93: not enough arguments in call to testing.NewListAction
        have (schema.GroupVersionResource, string, "github.com/kubernetes-incubator/service-catalog/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
        want (schema.GroupVersionResource, schema.GroupVersionKind, string, interface {})
pkg/client/clientset_generated/internalclientset/typed/servicecatalog/internalversion/fake/fake_serviceclass.go:78: not enough arguments in call to testing.NewRootListAction
        have (schema.GroupVersionResource, "github.com/kubernetes-incubator/service-catalog/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
        want (schema.GroupVersionResource, schema.GroupVersionKind, interface {})
# github.com/kubernetes-incubator/service-catalog/pkg/client/clientset_generated/clientset/typed/servicecatalog/v1alpha1/fake
pkg/client/clientset_generated/clientset/typed/servicecatalog/v1alpha1/fake/fake_binding.go:93: not enough arguments in call to testing.NewListAction
        have (schema.GroupVersionResource, string, "github.com/kubernetes-incubator/service-catalog/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
        want (schema.GroupVersionResource, schema.GroupVersionKind, string, interface {})
pkg/client/clientset_generated/clientset/typed/servicecatalog/v1alpha1/fake/fake_broker.go:87: not enough arguments in call to testing.NewRootListAction
        have (schema.GroupVersionResource, "github.com/kubernetes-incubator/service-catalog/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
        want (schema.GroupVersionResource, schema.GroupVersionKind, interface {})
pkg/client/clientset_generated/clientset/typed/servicecatalog/v1alpha1/fake/fake_instance.go:93: not enough arguments in call to testing.NewListAction
        have (schema.GroupVersionResource, string, "github.com/kubernetes-incubator/service-catalog/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
        want (schema.GroupVersionResource, schema.GroupVersionKind, string, interface {})
pkg/client/clientset_generated/clientset/typed/servicecatalog/v1alpha1/fake/fake_serviceclass.go:78: not enough arguments in call to testing.NewRootListAction
        have (schema.GroupVersionResource, "github.com/kubernetes-incubator/service-catalog/vendor/k8s.io/apimachinery/pkg/apis/meta/v1".ListOptions)
        want (schema.GroupVersionResource, schema.GroupVersionKind, interface {})

Which is all generated code...

nilebox · 2017-07-21T00:37:52Z

@MHBauer pushed updated vendor and auto-generated files.
Test is still failing for me:

I0721 00:26:12.197526    2812 request.go:991] Response Body: {"kind":"Status","apiVersion":"v1","metadata":{},"status":"Failure","message":"brokers \"test-broker\" not found","reason":"NotFound","details":{"name":"test-broker","kind":"brokers"},"code":404}
--- PASS: TestBasicFlows (9.24s)
	framework.go:83: Starting server on port: 32417
	controller_test.go:309: controller start
	controller_test.go:317: informers start
	framework.go:79: Shutting down server on port: 32417
FAIL

nilebox · 2017-07-21T03:06:51Z

Fixed gengo dependency version by looking at https://github.com/kubernetes/kubernetes/blob/release-1.7/Godeps/Godeps.json

Still have some failure in integration test:

FAIL
exit status 1
FAIL	github.com/kubernetes-incubator/service-catalog/test/integration	63.474s
!!! [0721 02:00:31] Call tree:
!!! [0721 02:00:31]  1: test/integration.sh:34 runTests(...)
!!! Error in test/integration.sh:28
  Error in test/integration.sh:28. '((i<3-1))' exited with status 1

Tried to manually deploy Service Catalog to minikube to test it, have some issue with RBAC:

1. Executing helm install /Users/nislamov/go/src/github.com/kubernetes-incubator/service-catalog/charts/catalog --name catalog --namespace catalog --set debug=true,insecure=true,controllerManager.image=nilebox/controller-manager:canary,apiserver.image=nilebox/apiserver:canary,apiserver.service.type=LoadBalancer
Error: release catalog failed: clusterroles.rbac.authorization.k8s.io "servicecatalog.k8s.io:apiserver" is forbidden: attempt to grant extra privileges: [{[get] [] [namespaces] [] []} {[list] [] [namespaces] [] []} {[watch] [] [namespaces] [] []}] user=&{system:serviceaccount:kube-system:default beada736-6dbe-11e7-9960-080027321847 [system:serviceaccounts system:serviceaccounts:kube-system system:authenticated] map[]} ownerrules=[] ruleResolutionErrors=[]
Command failed. Will retry in 5 seconds.

It worked for me before:

# run a clean cluster with helm installed
minikube delete
minikube start
helm init
# run walkthough
/contrib/hack/test_walkthrough.sh

Looks like whether RBAC PR broke something, or I am missing something.
@MHBauer can you take a look?

MHBauer · 2017-07-21T17:47:05Z

looks like the healthchecker test is failing. maybe because no insecure? I will continue to debug.

--- FAIL: TestEtcdHealthCheckerSuccess (3.18s)
        framework.go:83: Starting server on port: 21416
        clientset_test.go:111: health check endpoint should not have failed
2017-07-21 13:44:20.283006 I | server.go:2753: http: TLS handshake error from [::1]:58858: remote error: tls: bad certificate
2017-07-21 13:44:22.349075 I | server.go:2753: http: TLS handshake error from [::1]:33104: remote error: tls: bad certificate
--- FAIL: TestEtcdHealthCheckerFail (2.07s)
        framework.go:83: Starting server on port: 4155
        clientset_test.go:134: health check endpoint should have failed and did not

MHBauer · 2017-07-21T18:01:18Z

lawlz, it's the exact same solution as the other thing, at least for the test.

	tr := &http.Transport{
		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
	}
	c := &http.Client{Transport: tr}
	resp, err := c.Get(clientconfig.Host + "/healthz")

MHBauer · 2017-07-21T18:38:01Z

https://travis-ci.org/MHBauer/service-catalog/builds/256160166?utm_source=email&utm_medium=notification shows travis working.

… and temporarily disable TLS verification on client side

…code

Disable TLS verification for healthcheck client

nilebox · 2017-07-22T11:14:46Z

@MHBauer Thanks, added fix for healthcheck integration test (disabled TLS verification).
Also rebased again, had another conflict.

duglin · 2017-07-22T15:23:37Z

cmd/controller-manager/app/controller_manager.go

@@ -143,6 +143,8 @@ func Run(controllerManagerOptions *options.ControllerManagerServer) error {
 		// TODO: disambiguate API errors
 		return fmt.Errorf("failed to get Service Catalog client configuration: %v", err)
 	}
+	// TODO (nilebox): Dirty temporary hack. Should whether accept as flag for application, or handle TLS correctly


s/whether/we/ ?? not sure what its trying to say

"accept as --skip-verify flag" (instead of hardcode) OR "handle TLS correctly" (by passing certificate authority and other stuff into client configuration). sorry for my poor English if it's confusing you.

This comment is obsolete anyway, I went with the flag for now (see newer commits), as suggested by @pmorie.
We will still need to fix all the places where we pass --insecure-skip-verify flags by adding TLS configuration on the client side, but I see this to be a separate PR, and I think we have agreement on this.

If the comment is obsolete should we remove it?

@duglin it is removed in the next commit: 213f946

ah ok thanks

MHBauer · 2017-07-24T14:45:53Z

Everything appears as it should be after some quick tests this morning.

Still LGTM.

duglin · 2017-07-24T14:47:51Z

LGTM

k8s-ci-robot requested a review from arschles June 15, 2017 06:31

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 15, 2017

nilebox reviewed Jun 15, 2017

View reviewed changes

pmorie added the do-not-merge label Jun 15, 2017

nilebox mentioned this pull request Jun 19, 2017

Rebasing Service Catalog onto API machinery 1.7 #949

Closed

pmorie changed the title ~~Update dependencies~~ Rebase on 1.7 API machinery Jun 20, 2017

MHBauer reviewed Jun 27, 2017

View reviewed changes

MHBauer reviewed Jul 12, 2017

View reviewed changes

pmorie reviewed Jul 14, 2017

View reviewed changes

pmorie added this to the 0.0.14 milestone Jul 14, 2017

nilebox reviewed Jul 17, 2017

View reviewed changes

MHBauer reviewed Jul 17, 2017

View reviewed changes

MHBauer added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2017

MHBauer removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2017

ash2k and others added 5 commits July 22, 2017 20:37

Update vendor

7389158

Re-generate code

31c4342

Fix compilation, API server startup, remove insecure server leftovers…

33b9407

… and temporarily disable TLS verification on client side

Added --service-catalog-insecure-skip-verify flag to replace the hard…

213f946

…code

Fix build after rebasing on upstream/master

65ffd10

Disable TLS verification for healthcheck client

duglin reviewed Jul 22, 2017

View reviewed changes

duglin added LGTM2 and removed do-not-merge labels Jul 24, 2017

duglin merged commit 2bb334a into kubernetes-retired:master Jul 24, 2017

nilebox deleted the update-dependencies branch July 25, 2017 02:19

Rebase on 1.7 API machinery #944

Rebase on 1.7 API machinery #944

Conversation

ash2k commented Jun 15, 2017

nilebox commented Jun 15, 2017

nilebox commented Jun 15, 2017

nilebox commented Jun 15, 2017

nilebox Jun 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilebox commented Jun 15, 2017

pmorie commented Jun 15, 2017

nilebox commented Jun 15, 2017

MHBauer commented Jun 15, 2017

nilebox commented Jun 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MHBauer commented Jun 27, 2017

MHBauer commented Jun 27, 2017

nilebox commented Jun 27, 2017 • edited Loading

MHBauer commented Jun 27, 2017

ash2k commented Jul 1, 2017

ash2k commented Jul 8, 2017

nilebox commented Jul 10, 2017

nilebox commented Jul 11, 2017 • edited Loading

MHBauer commented Jul 12, 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

nilebox commented Jul 12, 2017

nilebox commented Jul 12, 2017 • edited Loading

MHBauer commented Jul 12, 2017

MHBauer commented Jul 12, 2017

pmorie left a 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

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

nilebox Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

nilebox Jul 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmorie Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nilebox commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MHBauer commented Jul 19, 2017

nilebox commented Jul 19, 2017

nilebox commented Jul 20, 2017

MHBauer commented Jul 20, 2017 • edited Loading

MHBauer commented Jul 21, 2017

nilebox commented Jul 21, 2017

nilebox commented Jul 21, 2017

MHBauer commented Jul 21, 2017

MHBauer commented Jul 21, 2017

MHBauer commented Jul 21, 2017

nilebox commented Jul 22, 2017

Choose a reason for hiding this comment

nilebox Jul 24, 2017 • edited Loading

Choose a reason for hiding this comment

nilebox Jun 15, 2017 •

edited

Loading

nilebox commented Jun 27, 2017 •

edited

Loading

nilebox commented Jul 11, 2017 •

edited

Loading

nilebox commented Jul 12, 2017 •

edited

Loading

nilebox Jul 17, 2017 •

edited

Loading

nilebox Jul 17, 2017 •

edited

Loading

pmorie Jul 14, 2017 •

edited

Loading

MHBauer commented Jul 20, 2017 •

edited

Loading

nilebox Jul 24, 2017 •

edited

Loading