OKE-1853: Refactor lookup node id from node.spec.ProviderID #149

simonlord · 2018-08-17T14:45:30Z

OKE-1853: Refactor lookup node id from node.spec.ProviderID …
Completely removed cache and vcn/subnet based lookup. Requires nodes have
node.spec.ProviderID set correctly.

deploy.sh now drops enough info to disk for the flexvolume driver binary to
find, auth and use the kube master api server to query for a list of nodes.

DISCUSS: do we need a non rbac manifest now that we're using a service account?:

DISCUSS: should the rbac roles be rolled into the main driver manifest file?

prydie

Few minor nitpicks but otherwise awesome work 👍 . Will be so pleased to see this land 😄

prydie · 2018-08-17T14:52:28Z

manifests/rbac.yaml

+subjects:
+- kind: ServiceAccount
+  name: oci-fvd
+  namespace: kube-system


Trailing newline (you probably need to set "files.insertFinalNewline": true in VSCode)

prydie · 2018-08-17T14:52:38Z

pkg/oci/client/cache/ocicache.go

@@ -1,92 +0,0 @@
-// Copyright 2017 Oracle and/or its affiliates. All rights reserved.


prydie · 2018-08-17T14:54:01Z

pkg/oci/driver/driver.go

+		}
+	}
+	if id == "" {
+		return id, errors.New("Failed to find node's OCID in returned node list")


Error string capitalisation

prydie · 2018-08-17T14:54:30Z

pkg/oci/driver/driver.go

 // Attach initiates the attachment of the given OCI volume to the k8s worker
 // node.
 func (d OCIFlexvolumeDriver) Attach(opts flexvolume.Options, nodeName string) flexvolume.DriverStatus {
+


prydie · 2018-08-17T14:55:26Z

pkg/oci/driver/driver.go

+
+	nodeList, err := k.CoreV1().Nodes().List(metav1.ListOptions{})
+	if err != nil {
+		return "", err


errors.Wrap(err, "listing nodes")

prydie · 2018-08-17T15:03:01Z

pkg/oci/driver/driver.go

+	return kubeClient, err
+}
+
+// findNodeID returns the OCID for the given nodeName


nit: trailing full stop 😜

prydie · 2018-08-17T15:04:31Z

manifests/oci-flexvolume-driver.yaml

@@ -10,6 +10,7 @@ spec:
      labels:
        app: oci-flexvolume-driver
    spec:
+      serviceAccountName: oci-fvd


s/oci-fvd/oci-flexvolume-driver/

prydie · 2018-08-17T15:04:44Z

manifests/rbac.yaml

+apiVersion: v1
+kind: ServiceAccount
+metadata:
+  name: oci-fvd


s/oci-fvd/oci-flexvolume-driver/

prydie · 2018-08-17T15:07:06Z

deploy.sh

@@ -40,6 +40,10 @@ if [ -f "$CONFIG_FILE" ]; then
  cp  "$CONFIG_FILE"  "$driver_dir/$config_file_name"
 fi

+echo https://$KUBERNETES_SERVICE_HOST:$KUBERNETES_SERVICE_PORT > $driver_dir/master_url


Is this script shared between both the master and worker DaemonSets? If so we'll want to make sure we don't output this stuff in the worker DaemonSet.

prydie · 2018-08-17T15:11:28Z

pkg/oci/driver/driver.go

+
+// findNodeID returns the OCID for the given nodeName
+func findNodeID(k kubernetes.Interface, nodeName string) (string, error) {
+	var id string


nit: Would declare adjacent to the for loop personally

prydie · 2018-08-17T15:33:57Z

pkg/oci/driver/driver.go

@@ -94,15 +109,70 @@ func deriveVolumeOCID(regionKey string, volumeName string) string {
 	return volumeOCID
 }

+func constructKubeClient() (*kubernetes.Clientset, error) {


We need to be able to construct the client from the env for the managed use case. Let's discuss offline on Monday.

We should be using the same patterns as normal kube services to construct clients. So not copying in cluster access to new directories, using kube config files, etc.

jhorwit2 · 2018-08-19T14:21:56Z

manifests/rbac.yaml

+apiVersion: rbac.authorization.k8s.io/v1beta1
+kind: ClusterRoleBinding
+metadata:
+  name: oci-fvd


We should use more descriptive names

jhorwit2 · 2018-08-19T14:24:01Z

pkg/oci/driver/driver.go

@@ -94,15 +109,70 @@ func deriveVolumeOCID(regionKey string, volumeName string) string {
 	return volumeOCID
 }

+func constructKubeClient() (*kubernetes.Clientset, error) {


We should be using the same patterns as normal kube services to construct clients. So not copying in cluster access to new directories, using kube config files, etc.

jhorwit2 · 2018-08-19T15:30:13Z

pkg/oci/driver/driver.go

+func findNodeID(k kubernetes.Interface, nodeName string) (string, error) {
+	var id string
+
+	nodeList, err := k.CoreV1().Nodes().List(metav1.ListOptions{})


If we have the name name why not use a get?

jhorwit2 · 2018-08-19T15:35:12Z

pkg/oci/driver/driver.go

+	}
+	for _, node := range nodeList.Items {
+		if node.Name == nodeName {
+			id = node.Spec.ProviderID


Should we strip the oci prefix that can be on id's here?

jhorwit2 · 2018-08-19T18:32:27Z

@simonlord We should avoid trying to do fancy tactics to provide authentication in the driver itself. This ultimately will depend on the users setup. The recommended way in Kubernetes to do this type of authentication is via kubeconfigs; not copying service accounts.

An example of when this pattern breaks down is when the flex volume driver container is used as part of self-hosted deployment where it's a sidecar in the KCM deployment. Since it's a sidecar t'll already have access to the service account of the KCM. That pattern is used today by ODC.

Also, what happens when SA's can be rotated which is a feature under fairly active discussion. The goal of this driver should be to provide a generic solution to authentication just as every core component does which helps support multiple paradigms of setting up k8s.

jhorwit2 · 2018-08-19T18:35:26Z

Nit: Upgrading client-go doesn't need to be part of this PR.

owainlewis · 2018-09-17T13:36:11Z

manifests/oci-flexvolume-driver.yaml

@@ -31,6 +32,9 @@ spec:
            - mountPath: /tmp
              name: config
              readOnly: true
+            - mountPath: /tmp2


An aside (to this PR) but do we really want to mount in /tmp vs something else? (above).

honestly i don't know, i was just following suit

Separate discussion maybe but these should be written to more sensible locations IMO.

prydie

Couple of minor niggles but nice one 👍. Super excited to say good by to the caching / search code 🎉

prydie · 2018-09-17T15:20:48Z

README.md

+    --from-file=kubeconfig=kubeconfig
+```
+
+Once the Secret is set and the daemonsets deployed, the kubeconfig file will be placed onto the master nodes.


Once the Secret is set and the DaemonSet is deployed, the kubeconfig file will be placed onto the master nodes.

prydie · 2018-09-17T15:24:20Z

test/integration/framework/framework.go

@@ -17,12 +17,17 @@ package framework
 import (
 	"errors"
 	"os"
+
+	"github.com/oracle/oci-flexvolume-driver/pkg/oci/driver"


Import grouping

Sorry I don't understand

Oops my bad. Yeah, supper nit picky but we normally group imports like so:

import ( <stdlib> <k8s.io/...> <third party> <internal> )

prydie · 2018-09-17T15:27:30Z

manifests/rbac.yaml

@@ -0,0 +1,30 @@
+---


I like the separation of rbac as a separate manifest 👍

prydie · 2018-09-17T15:29:48Z

README.md

-
-NOTE: If running kube-controller-managers in a container you _must_ ensure that
-the plugin directory is mounted into the container.
+You'll still need to add the config file as a kubernetes secret.


s/secret/Secret/

owainlewis · 2018-09-18T13:46:46Z

pkg/oci/client/oci.go

-	// GetInstanceByNodeName retrieves the oci.Instance corresponding or
-	// a SearchError if no instance matching the node name is found.
-	GetInstanceByNodeName(name string) (*core.Instance, error)
+	// GetInstance retrieves the oci.Instance for a given ocid.


Trivial: for a given OCID

owainlewis · 2018-09-18T13:55:25Z

Looking good. Some follow on work needed to coordinate changes with consumers of the driver.

owainlewis · 2018-09-18T13:57:38Z

Gopkg.toml

 [[constraint]]
  name = "gopkg.in/yaml.v2"
  branch = "v2"

 [[constraint]]
  name = "github.com/oracle/oci-go-sdk"
  version = "v1.0.0"
+
+[[constraint]]
+  branch = "release-1.11"


Approval needed.

Do i need to do anything for this?

We've generally moved from using the release branch to specifying specific versions of kube deps. We should already have pre-approvals of specific 1.11.x versions of apimachinery I believe too 🙂

Approved for kubernetes-1.11.1

owainlewis

We should ensure we're following up on this and communicating the changes with consumers of this plugin.

jhorwit2 · 2018-09-19T13:29:52Z

manifests/rbac.yaml

+---
+apiVersion: v1
+kind: ServiceAccount
+metadata:


nit: The service account should be above / in the deployment file. Otherwise there is a race condition on the pods being created and the service account being created which will cause failure events for a short period after deployment.

jhorwit2 · 2018-09-19T13:34:13Z

pkg/oci/driver/driver.go

+		return "", err
+	}
+	if n.Spec.ProviderID == "" {
+		return "", errors.New("failed to find node's OCID in returned node spec")


nit: this error should be something like node is missing provider id so it's clear where it couldn't find the ocid.

jhorwit2 · 2018-09-19T13:36:31Z

pkg/oci/driver/driver.go

+	path := GetConfigPath()
+	if _, err := os.Stat(path); !os.IsNotExist(err) {
+		k, err := constructKubeClient()
+		return &OCIFlexvolumeDriver{K: k, master: true}, err


nit: this should be

if err != nil { return nil, err } return &OCIFlexvolumeDriver{K: k, master: true}, nil

…esolution Completely removed cache and vcn/subnet based lookup. Requires nodes have node.spec.ProviderID set correctly. deploy.sh now drops enough info to disk for the flexvolume driver binary to find, auth and use the kube master api server to query for a list of nodes.

simonlord requested review from prydie and owainlewis August 17, 2018 14:45

prydie suggested changes Aug 17, 2018

View reviewed changes

prydie requested a review from jhorwit2 August 17, 2018 15:30

prydie reviewed Aug 17, 2018

View reviewed changes

jhorwit2 suggested changes Aug 19, 2018

View reviewed changes

simonlord force-pushed the sl/with-apiserver branch from 9c33bdf to ee71309 Compare September 10, 2018 13:35

owainlewis reviewed Sep 17, 2018

View reviewed changes

simonlord force-pushed the sl/with-apiserver branch from ee71309 to fe8eb2a Compare September 17, 2018 15:00

prydie suggested changes Sep 17, 2018

View reviewed changes

simonlord force-pushed the sl/with-apiserver branch 2 times, most recently from 8f2df6b to 4132852 Compare September 18, 2018 13:02

owainlewis reviewed Sep 18, 2018

View reviewed changes

owainlewis added the enhancement label Sep 18, 2018

owainlewis reviewed Sep 18, 2018

View reviewed changes

owainlewis added the needs-approval label Sep 18, 2018

simonlord force-pushed the sl/with-apiserver branch from 4132852 to 036a225 Compare September 18, 2018 14:52

Deps for client-go, k8s fake clientset and clientcmd

df705b2

simonlord force-pushed the sl/with-apiserver branch from 036a225 to 2d218d8 Compare September 19, 2018 10:46

prydie approved these changes Sep 19, 2018

View reviewed changes

simonlord force-pushed the sl/with-apiserver branch from 2d218d8 to 6d3793d Compare September 19, 2018 12:53

owainlewis removed the needs-approval label Sep 19, 2018

owainlewis approved these changes Sep 19, 2018

View reviewed changes

owainlewis self-assigned this Sep 19, 2018

jhorwit2 reviewed Sep 19, 2018

View reviewed changes

simonlord force-pushed the sl/with-apiserver branch from 6d3793d to 25f8e03 Compare September 20, 2018 09:03

jhorwit2 approved these changes Sep 20, 2018

View reviewed changes

owainlewis merged commit 5e34244 into master Sep 21, 2018

owainlewis deleted the sl/with-apiserver branch September 21, 2018 09:08

This was referenced Sep 26, 2018

Don't use VCN for Finding Nodes #137

Closed

Support multiple custers in the same VCN #64

Closed

		@@ -1,92 +0,0 @@
		// Copyright 2017 Oracle and/or its affiliates. All rights reserved.

OKE-1853: Refactor lookup node id from node.spec.ProviderID #149

OKE-1853: Refactor lookup node id from node.spec.ProviderID #149

Conversation

simonlord commented Aug 17, 2018

prydie 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

jhorwit2 Aug 19, 2018 • edited Loading

Choose a reason for hiding this comment

jhorwit2 commented Aug 19, 2018 • edited Loading

jhorwit2 commented Aug 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owainlewis Sep 18, 2018 • edited Loading

Choose a reason for hiding this comment

prydie 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

owainlewis commented Sep 18, 2018

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

owainlewis 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

jhorwit2 Aug 19, 2018 •

edited

Loading

jhorwit2 commented Aug 19, 2018 •

edited

Loading

owainlewis Sep 18, 2018 •

edited

Loading