Use auto-generated client code to communicate with API server #2007

aanm · 2017-11-09T23:04:42Z

This PR moves the TPR code to the v1 package, allowing us to have a package for each type of cilium network policy.

Use auto-generated client to communicate with kube-apiserver

Note to reviewers: the code under pkg/k8s/client was autogenerated.

nebril

Overall looks good, 2 comments inline.

nebril · 2017-11-13T14:15:54Z

pkg/k8s/apis/cilium.io/utils.go

+
+// ParseToCiliumRule returns an api.Rule with all the labels parsed into cilium
+// labels.
+func ParseToCiliumRule(namespace, name string, r *api.Rule) *api.Rule {


Is this function tested anywhere? There is a lot of logic there, would be nice if at least some paths were tested.

nebril · 2017-11-13T14:20:10Z

pkg/k8s/client/clientset/versioned/typed/cilium/v1/cilium_client.go

+
+// NewForConfigOrDie creates a new CiliumV1Client for the given config and
+// panics if there is an error in the config.
+func NewForConfigOrDie(c *rest.Config) *CiliumV1Client {


Is this function used anywhere?

Note to reviewers: the code under pkg/k8s/client was autogenerated.

:-)

tgraf · 2017-11-14T00:58:48Z

pkg/k8s/apis/cilium.io/utils.go

+
+var (
+	// log is the k8s package logger object.
+	log = logrus.WithField(logfields.LogSubsys, subsysK8s)


This seems to be the only usage of this, there is not much point for a const. I would just encode it here. The comment is not even needed then.

I've noticed at the end of my changes that I'm using subsysK8s = "k8s" for both version. What if I would create a log package in k8s/log?

Are we gaining anything from a separate package? This string is never going to change again.

tgraf · 2017-11-14T01:01:08Z

pkg/k8s/apis/cilium.io/v1/register.go

+}
+
+var (
+	SchemeBuilder runtime.SchemeBuilder


Please add comments to variables needed by the auto generated code

tgraf · 2017-11-14T01:02:44Z

pkg/k8s/apis/cilium.io/v1/register.go

+
+	log.Info("Creating v1.CiliumNetworkPolicy ThirdPartyResource")
+	// wait for TPR being established
+	err = wait.Poll(500*time.Millisecond, 60*time.Second, func() (bool, error) {


Is 1 minute enough? Why?

It should be, in here, we have the k8s client already set up so if we take more than 1 minute to install the TPR rule something is wrong. Don't you agree?

Is the Get subject to a different timeout than the Create? I'm assuming both are subject to communication with the API server. Why does the get require a separate timeout?

tgraf · 2017-11-14T01:04:06Z

pkg/k8s/apis/cilium.io/v1/register.go

+		// is to check if the return error was or not nil
+		return true, nil
+	})
+	if err != nil {


When is err != nil? Do we need to inform the user or stop the agent?

The error comes from line 102. And is returned to the caller of this function which is currently stopping the agent because we can't run cilium without installing the TPR to listen for CNP on k8s < 1.7

tgraf · 2017-11-14T01:04:20Z

pkg/k8s/apis/cilium.io/v1/types.go

+
+var (
+	// log is the k8s package logger object.
+	log = logrus.WithField(logfields.LogSubsys, subsysK8s)


tgraf · 2017-11-14T01:06:20Z

daemon/k8s_watcher.go

-
-	stopCiliumRulesController := make(chan struct{})
-	go ciliumRulesController.Run(stopCiliumRulesController)
+	si := informer.NewSharedInformerFactory(ciliumNPClient, reSyncPeriod)


tgraf · 2017-11-14T01:11:15Z

daemon/k8s_watcher.go

-	go ciliumRulesController.Run(stopCiliumRulesController)
+	si := informer.NewSharedInformerFactory(ciliumNPClient, reSyncPeriod)
+
+	createHandlerStore := func(si informer.SharedInformerFactory, version int) (cnpStore cache.Store) {


Why is this an inline func?

I had restructured the code differently and before I needed the inline func. I've remove it.

tgraf · 2017-11-14T01:14:24Z

daemon/k8s_watcher.go

-	if err != nil {
+	if err == nil {
+		if len(rules) > 0 {
+			// All rules were stored with the same labels so there's no need


I don't understand this comment

// On a CNP, the transformed rule is stored in the local repository // with a set of labels. On a CNP with multiple rules all rules are // stored in the local repository with the same set of labels. // Therefore the deletion on the local repository can be done with // the set of labels of the first rule.

better?

tgraf · 2017-11-14T01:18:02Z

daemon/k8s_watcher.go

+		return
+	}
+
+	// Since we are updating the status map from all nodes we need to prevent


I assume this comment means that we want to ignore updates if no spec change has occured

// Ignore updates of the spec remains unchanged.

- Moved common code to utils.go Signed-off-by: André Martins <andre@cilium.io>

Signed-off-by: André Martins <andre@cilium.io>

After creating DeepCopy methods, it is necessary to create a event handler for each cilium version type in kubernetes watcher. That lead to use the auto-generated clientset in order to talk with the kube-apiserver. Since kubernetes client-go was updated, the watcher doesn't stop receiving events after receiving a bad policy, but still prints the error messages AND the watcher only continues to work after the bad policy is removed from the kube-apiserver. Unfortunately the splitage of cilium network policies into 2 different and dedicated version lead to some code duplication. Signed-off-by: André Martins <andre@cilium.io>

Signed-off-by: André Martins <andre@cilium.io>

houndci-bot · 2017-11-14T14:05:21Z

pkg/k8s/client/clientset/versioned/typed/cilium/v2/fake/fake_ciliumnetworkpolicy.go

+)
+
+// FakeCiliumNetworkPolicies implements CiliumNetworkPolicyInterface
+type FakeCiliumNetworkPolicies struct {


type name will be used as fake.FakeCiliumNetworkPolicies by other packages, and that stutters; consider calling this CiliumNetworkPolicies

houndci-bot · 2017-11-14T14:05:22Z

pkg/k8s/client/clientset/versioned/typed/cilium/v2/generated_expansion.go

+
+package v2
+
+type CiliumNetworkPolicyExpansion interface{}


exported type CiliumNetworkPolicyExpansion should have comment or be unexported

houndci-bot · 2017-11-14T14:05:22Z

pkg/k8s/apis/cilium.io/v1/register.go

+	// ThirdPartyResourceGroup is the name of the third party resource group
+	ThirdPartyResourceGroup = k8sconst.GroupName
+
+	// CustomResourceDefinitionVersion is the current version of the resource


comment on exported const ThirdPartyResourceVersion should be of the form "ThirdPartyResourceVersion ..."

houndci-bot · 2017-11-14T14:05:22Z

pkg/k8s/apis/cilium.io/v1/zz_generated.deepcopy.go

+func (in *CiliumNetworkPolicyList) DeepCopyObject() runtime.Object {
+	if c := in.DeepCopy(); c != nil {
+		return c
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

houndci-bot · 2017-11-14T14:05:22Z

pkg/k8s/apis/cilium.io/v1/zz_generated.deepcopy.go

+func (in *CiliumNetworkPolicy) DeepCopyObject() runtime.Object {
+	if c := in.DeepCopy(); c != nil {
+		return c
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

houndci-bot · 2017-11-14T14:05:23Z

pkg/k8s/client/clientset/versioned/scheme/register.go

+)
+
+var Scheme = runtime.NewScheme()
+var Codecs = serializer.NewCodecFactory(Scheme)


exported var Codecs should have comment or be unexported

houndci-bot · 2017-11-14T14:05:23Z

pkg/k8s/client/clientset/versioned/scheme/register.go

+	serializer "k8s.io/apimachinery/pkg/runtime/serializer"
+)
+
+var Scheme = runtime.NewScheme()


exported var Scheme should have comment or be unexported

houndci-bot · 2017-11-14T14:05:23Z

pkg/k8s/client/clientset/versioned/typed/cilium/v1/doc.go

+
+// This package is generated by client-gen with custom arguments.
+
+// This package has the automatically generated typed clients.


package comment should be of the form "Package v1 ..."

houndci-bot · 2017-11-14T14:05:23Z

pkg/k8s/client/clientset/versioned/typed/cilium/v1/fake/fake_ciliumnetworkpolicy.go

+)
+
+// FakeCiliumNetworkPolicies implements CiliumNetworkPolicyInterface
+type FakeCiliumNetworkPolicies struct {


type name will be used as fake.FakeCiliumNetworkPolicies by other packages, and that stutters; consider calling this CiliumNetworkPolicies

houndci-bot · 2017-11-14T14:05:23Z

pkg/k8s/client/clientset/versioned/typed/cilium/v1/fake/fake_cilium_client.go

+	*testing.Fake
+}
+
+func (c *FakeCiliumV1) CiliumNetworkPolicies(namespace string) v1.CiliumNetworkPolicyInterface {


exported method FakeCiliumV1.CiliumNetworkPolicies should have comment or be unexported

houndci-bot · 2017-11-14T14:05:24Z

pkg/k8s/client/clientset/versioned/typed/cilium/v2/fake/fake_ciliumnetworkpolicy.go

+)
+
+// FakeCiliumNetworkPolicies implements CiliumNetworkPolicyInterface
+type FakeCiliumNetworkPolicies struct {


type name will be used as fake.FakeCiliumNetworkPolicies by other packages, and that stutters; consider calling this CiliumNetworkPolicies

houndci-bot · 2017-11-14T14:05:24Z

pkg/k8s/client/clientset/versioned/typed/cilium/v2/generated_expansion.go

+
+package v2
+
+type CiliumNetworkPolicyExpansion interface{}


exported type CiliumNetworkPolicyExpansion should have comment or be unexported

houndci-bot · 2017-11-14T14:05:24Z

pkg/k8s/apis/cilium.io/v1/register.go

+	// ThirdPartyResourceGroup is the name of the third party resource group
+	ThirdPartyResourceGroup = k8sconst.GroupName
+
+	// CustomResourceDefinitionVersion is the current version of the resource


comment on exported const ThirdPartyResourceVersion should be of the form "ThirdPartyResourceVersion ..."

houndci-bot · 2017-11-14T14:05:24Z

pkg/k8s/apis/cilium.io/v1/zz_generated.deepcopy.go

+func (in *CiliumNetworkPolicyList) DeepCopyObject() runtime.Object {
+	if c := in.DeepCopy(); c != nil {
+		return c
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

houndci-bot · 2017-11-14T14:05:24Z

pkg/k8s/apis/cilium.io/v1/zz_generated.deepcopy.go

+func (in *CiliumNetworkPolicy) DeepCopyObject() runtime.Object {
+	if c := in.DeepCopy(); c != nil {
+		return c
+	} else {


if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)

houndci-bot · 2017-11-14T14:05:25Z

pkg/k8s/client/clientset/versioned/scheme/register.go

+)
+
+var Scheme = runtime.NewScheme()
+var Codecs = serializer.NewCodecFactory(Scheme)


exported var Codecs should have comment or be unexported

houndci-bot · 2017-11-14T14:05:25Z

pkg/k8s/client/clientset/versioned/scheme/register.go

+	serializer "k8s.io/apimachinery/pkg/runtime/serializer"
+)
+
+var Scheme = runtime.NewScheme()


exported var Scheme should have comment or be unexported

houndci-bot · 2017-11-14T14:05:25Z

pkg/k8s/client/clientset/versioned/typed/cilium/v1/doc.go

+
+// This package is generated by client-gen with custom arguments.
+
+// This package has the automatically generated typed clients.


package comment should be of the form "Package v1 ..."

houndci-bot · 2017-11-14T14:05:25Z

pkg/k8s/client/clientset/versioned/typed/cilium/v1/fake/fake_ciliumnetworkpolicy.go

+)
+
+// FakeCiliumNetworkPolicies implements CiliumNetworkPolicyInterface
+type FakeCiliumNetworkPolicies struct {


type name will be used as fake.FakeCiliumNetworkPolicies by other packages, and that stutters; consider calling this CiliumNetworkPolicies

houndci-bot · 2017-11-14T14:05:25Z

pkg/k8s/client/clientset/versioned/typed/cilium/v1/fake/fake_cilium_client.go

+	*testing.Fake
+}
+
+func (c *FakeCiliumV1) CiliumNetworkPolicies(namespace string) v1.CiliumNetworkPolicyInterface {


exported method FakeCiliumV1.CiliumNetworkPolicies should have comment or be unexported

aanm added the wip label Nov 9, 2017

aanm requested a review from a team November 9, 2017 23:04

aanm requested review from a team as code owners November 9, 2017 23:04

aanm requested review from a team November 9, 2017 23:04

aanm requested a review from a team as a code owner November 9, 2017 23:04

aanm force-pushed the moving-tpr-to-v1 branch from a677fb7 to 62cf3a2 Compare November 10, 2017 09:00

cilium deleted a comment from houndci-bot Nov 10, 2017

aanm force-pushed the moving-tpr-to-v1 branch 4 times, most recently from 217fcb3 to 0bdbe96 Compare November 10, 2017 15:34

aanm changed the title ~~WIP Moving tpr to v1~~ Moving TPR to v1 Nov 10, 2017

aanm added sig/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. pending-review release-note/minor This PR changes functionality that users may find relevant to operating Cilium. and removed wip labels Nov 10, 2017

cilium deleted a comment from houndci-bot Nov 10, 2017

nebril reviewed Nov 13, 2017

View reviewed changes

tgraf requested changes Nov 14, 2017

View reviewed changes

tgraf changed the title ~~Moving TPR to v1~~ Use auto-generated client code to communicate with API server Nov 14, 2017

aanm added 4 commits November 14, 2017 13:17

k8s: Moved CRD code to v2 pkg

ce869e2

- Moved common code to utils.go Signed-off-by: André Martins <andre@cilium.io>

k8s: moved TPR to v1 pkg

d18428f

Signed-off-by: André Martins <andre@cilium.io>

vendor: add missing deps for generated client code

df4601b

Signed-off-by: André Martins <andre@cilium.io>

aanm force-pushed the moving-tpr-to-v1 branch from 0bdbe96 to df4601b Compare November 14, 2017 14:05

aanm requested a review from tgraf November 14, 2017 14:05

houndci-bot reviewed Nov 14, 2017

View reviewed changes

tgraf approved these changes Nov 14, 2017

View reviewed changes

tgraf removed the stable/needs-backport label Nov 14, 2017

tgraf merged commit 94e68c8 into master Nov 14, 2017

tgraf deleted the moving-tpr-to-v1 branch November 14, 2017 15:16


		// This package is generated by client-gen with custom arguments.

		// This package has the automatically generated typed clients.

Use auto-generated client code to communicate with API server #2007

Use auto-generated client code to communicate with API server #2007

Conversation

aanm commented Nov 9, 2017 • edited

nebril 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

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

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

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

aanm commented Nov 9, 2017 •

edited