fix(plugins/plugin-kubectl): kubectl watch tables can be misparsed

Fixes #4139

Author: starpit

packages/core/src/core/jobs/watchable.ts

@@ -33,7 +33,9 @@ export interface Watchable {
 
 /** callbacks to indicate state changes */
 export interface WatchPusher {
-  update: (response: Row) => void
+  update: (response: Row, batch?: boolean) => void
+  batchUpdateDone: () => void
+
   offline: (rowKey: string) => void
 
   done: () => void

plugins/plugin-client-common/src/components/Table/LivePaginatedTable.tsx

@@ -18,7 +18,7 @@ import * as React from 'react'
 import { Table as KuiTable, Row as KuiRow, Watchable } from '@kui-shell/core'
 
 import PaginatedTable, { Props, State } from './PaginatedTable'
-import { kuiHeader2carbonHeader, kuiRow2carbonRow } from './kui2carbon'
+import { kuiHeader2carbonHeader, kuiRow2carbonRow, NamedDataTableRow } from './kui2carbon'
 
 type LiveProps = Props<KuiTable & Watchable>
 
@@ -27,6 +27,9 @@ interface LiveState extends State {
 }
 
 export default class LivePaginatedTable extends PaginatedTable<LiveProps, LiveState> {
+  /** To allow for batch updates, the setState can be deferred until a call to updateDone() */
+  private _deferredUpdate: NamedDataTableRow[]
+
   public constructor(props: LiveProps) {
     super(props)
     this.state = Object.assign(this.state, { isWatching: true })
@@ -60,11 +63,12 @@ export default class LivePaginatedTable extends PaginatedTable<LiveProps, LiveSt
 
     // initiate the pusher watch
     const update = this.update.bind(this)
+    const batchUpdateDone = this.batchUpdateDone.bind(this)
     const offline = this.offline.bind(this)
     const done = this.done.bind(this)
     const allOffline = this.allOffline.bind(this)
     const header = this.header.bind(this)
-    this.props.response.watch.init({ update, offline, done, allOffline, header })
+    this.props.response.watch.init({ update, batchUpdateDone, offline, done, allOffline, header })
   }
 
   /**
@@ -112,7 +116,7 @@ export default class LivePaginatedTable extends PaginatedTable<LiveProps, LiveSt
    * update consumes the update notification and apply it to the table view
    *
    */
-  private update(newKuiRow: KuiRow) {
+  private update(newKuiRow: KuiRow, batch = false) {
     const existingRows = this.state.rows
     const nRowsBefore = existingRows.length
 
@@ -139,7 +143,22 @@ export default class LivePaginatedTable extends PaginatedTable<LiveProps, LiveSt
       this.props.response.body[foundIndex] = newKuiRow
     }
 
-    this.setState({ rows: newRows })
+    if (!batch) {
+      this.setState({ rows: newRows })
+    } else if (this._deferredUpdate) {
+      this._deferredUpdate = this._deferredUpdate.concat(newRows)
+    } else {
+      this._deferredUpdate = newRows
+    }
+  }
+
+  /** End of a deferred batch of updates */
+  private batchUpdateDone() {
+    if (this._deferredUpdate) {
+      const rows = this._deferredUpdate
+      this._deferredUpdate = undefined
+      this.setState({ rows })
+    }
   }
 
   /**

plugins/plugin-kubectl/src/controller/kubectl/fqn.ts

@@ -66,7 +66,11 @@ export function kindAndNamespaceOf(resource: KubeResource) {
 }
 
 export function fqn(apiVersion: string, kind: string, name: string, namespace: string) {
-  return `${kindPart(apiVersion, kind)} ${namespace === '<none>' ? '' : `-n ${namespace}`} ${name}`
+  if (kind === 'Namespace' && apiVersion === 'v1') {
+    return `${kind} ${name}`
+  } else {
+    return `${kindPart(apiVersion, kind)} ${namespace === '<none>' ? '' : `-n ${namespace}`} ${name}`
+  }
 }
 
 export function fqnOf(resource: KubeResource) {

plugins/plugin-kubectl/src/controller/kubectl/watch/get-watch.ts

@@ -20,55 +20,50 @@ import { Table, Arguments, CodedError, Streamable, Abortable, Watchable, Watcher
 import fqn from '../fqn'
 import { formatOf, KubeOptions, KubeExecOptions } from '../options'
 
-import { preprocessTable, Pair } from '../../../lib/view/formatTable'
+import { Pair } from '../../../lib/view/formatTable'
 import { getCommandFromArgs } from '../../../lib/util/util'
 
 const debug = Debug('plugin-kubectl/controller/watch/watcher')
 
-/**
- * We might get partial rows back; they will have empty strings in the
- * value field.
- *
- */
-function isFullRow(row: Pair[], nCols: number): boolean {
-  return row.length === nCols && row.every(_ => _.value.length > 0)
-}
-
-/**
- * Return the pair of indices that define the extent of "full" rows
- * in the given table. Since we are streaming output from kubectl, we
- * get back partial rows in any one bundle of bits.
- *
- */
-function findFullRows(arr: Pair[][], nCols: number): { firstFullIdx: number; lastFullIdx: number } {
-  if (arr.length > 0) {
-    // value=NAME means that this is a header column; we know this
-    // because, below, we are in charge of the schema of the table!
-    // skip over that here, because we want to find the interval of
-    // "full" body rows, i.e. excluding the header row
-    const startIdx = arr.findIndex(row => row.length === nCols && row[0].value !== 'NAME')
+function preprocessTable(raw: string, nCols): { rows: Pair[][]; leftover: string } {
+  const rows = raw.split(/\n/).map(line => {
+    const cells = line.split(/\|/)
+    return cells.slice(0, cells.length - 1) // we have a trailing |
+  })
 
-    if (startIdx >= 0) {
-      for (let idx = startIdx; idx < arr.length; idx++) {
-        if (isFullRow(arr[idx], nCols)) {
-          // then we have found the \lower\ bound
-          for (let jdx = arr.length - 1; jdx >= idx; jdx--) {
-            if (isFullRow(arr[jdx], nCols)) {
-              // and now we have found the /upper/ bound
-              return { firstFullIdx: idx, lastFullIdx: jdx }
-            }
-          }
+  let lastFullRowIdx = rows.length
+  while (--lastFullRowIdx >= 0) {
+    if (rows[lastFullRowIdx].length === nCols) {
+      break
+    }
+  }
 
-          // hmm, we couldn't find an upper bound, but we did find a
-          // lower bound
-          return { firstFullIdx: idx, lastFullIdx: idx }
-        }
+  if (lastFullRowIdx < 0) {
+    return {
+      leftover: raw,
+      rows: []
+    }
+  } else if (lastFullRowIdx === rows.length - 1) {
+    return {
+      leftover: undefined,
+      rows: rows.map(line => line.map(value => ({ key: value, value }))).filter(_ => _.length > 0)
+    }
+  } else {
+    let lastNewlineIdx = raw.length
+    while (--lastNewlineIdx >= 0) {
+      if (raw.charAt(lastNewlineIdx) === '\n') {
+        break
       }
     }
-  }
 
-  // hmm, then we couldn't even find a lower bound
-  return { firstFullIdx: -1, lastFullIdx: -1 }
+    return {
+      leftover: raw.slice(lastNewlineIdx),
+      rows: rows
+        .slice(0, lastFullRowIdx + 1)
+        .map(line => line.map(value => ({ key: value, value })))
+        .filter(_ => _.length > 0)
+    }
+  }
 }
 
 class KubectlWatcher implements Abortable, Watcher {
@@ -129,7 +124,7 @@ class KubectlWatcher implements Abortable, Watcher {
     return async (_: Streamable) => {
       if (typeof _ === 'string') {
         // <-- strings flowing out of the PTY
-        debug('streaming pty output', _)
+        // debug('streaming pty output', _)
         if (/not found/.test(_)) {
           this.pusher.allOffline()
           return
@@ -146,26 +141,11 @@ class KubectlWatcher implements Abortable, Watcher {
         this.leftover = undefined
 
         // here is where we turn the raw data into tabular data
-        const allRows = preprocessTable([rawData])[0]
-
-        // find the interval of "full" rows; we may get back partial
-        // rows, due to the way output streams back to us from the
-        // underlying PTY
-        const { firstFullIdx, lastFullIdx } = findFullRows(allRows, this.nCols)
-
-        if (lastFullIdx < 0) {
-          // then we got no full rows
-          debug('no full rows', _)
-          this.leftover = _
-          return
-        } else if (lastFullIdx < allRows.length - 1) {
-          // the we got some trailing leftover bits
-          const lastNewlineIdx = _.lastIndexOf('\n')
-          this.leftover = _.slice(lastNewlineIdx)
-        }
+        const preprocessed = preprocessTable(rawData, this.nCols)
+        this.leftover = preprocessed.leftover === '\n' ? undefined : preprocessed.leftover
+        const { rows } = preprocessed
 
         // now process the full rows into table view updates
-        const rows = allRows.slice(firstFullIdx, lastFullIdx + 1)
         const tables = await Promise.all(
           rows.map(async row => {
             try {
@@ -208,8 +188,12 @@ class KubectlWatcher implements Abortable, Watcher {
           if (table) {
             table.body.forEach(row => {
               // push an update to the table model
-              this.pusher.update(row)
+              // true means we want to do a batch update
+              this.pusher.update(row, true)
             })
+
+            // batch update done!
+            this.pusher.batchUpdateDone()
           }
         })
       } else {
@@ -238,7 +222,7 @@ class KubectlWatcher implements Abortable, Watcher {
         .replace(/^k(\s)/, 'kubectl$1')
         .replace(/--watch=true|-w=true|--watch-only=true|--watch|-w|--watch-only/g, '--watch') // force --watch
         .replace(new RegExp(`(-o|--output)(\\s+|=)${this.output}`), '') +
-      ` -o custom-columns=NAME:.metadata.name,KIND:.kind,APIVERSION:.apiVersion,NAMESPACE:.metadata.namespace`
+      ` -o jsonpath='{.metadata.name}{"|"}{.kind}{"|"}{.apiVersion}{"|"}{.metadata.namespace}{"|\\n"}'`
     // ^^^^^ keep these in sync with nCols above !!
 
     this.args.REPL.qexec(`sendtopty ${command}`, this.args.block, undefined, {