Repository: godaddy/kubernetes-external-secrets (js)

Issue:
Operation cannot be fulfilled exception
Application is crashing if you try to change ExternalSecret resource at the same time as application is trying to update the secret in Kubernetes.

Probably the best solution - try one more time instead of crashing.

```

Error: Operation cannot be fulfilled on externalsecrets.kubernetes-client.io "test-secrets-syslogger": the object has been modified; please apply your changes to the latest version and try again
    at /Users/u/projects/tmp/kubernetes-external-secrets/node_modules/kubernetes-client/backends/request/client.js:214:25
    at Request._callback (/Users/u/projects/tmp/kubernetes-external-secrets/node_modules/kubernetes-client/backends/request/client.js:162:14)
    at Request.self.callback (/Users/u/projects/tmp/kubernetes-external-secrets/node_modules/request/request.js:185:22)
    at Request.emit (events.js:210:5)
    at Request.EventEmitter.emit (domain.js:476:20)
    at Request.<anonymous> (/Users/u/projects/tmp/kubernetes-external-secrets/node_modules/request/request.js:1161:10)
    at Request.emit (events.js:210:5)
    at Request.EventEmitter.emit (domain.js:476:20)
    at IncomingMessage.<anonymous> (/Users/u/projects/tmp/kubernetes-external-secrets/node_modules/request/request.js:1083:12)
    at Object.onceWrapper (events.js:299:28) {
  code: 409,
  statusCode: 409
}
[nodemon] app crashed - waiting for file changes before starting...

```

Pull request:
fix: status update conflicts should not cause crash, fixes #199
fixes #199 

Patch:
diff --git a/README.md b/README.md
index 9a46428..3081ae9 100644
--- a/README.md
+++ b/README.md
@@ -233,6 +233,11 @@ spec:
     - key: hello-service/credentials
       name: username
       property: username
+    - key: hello-service/credentials
+      name: password
+      # Version Stage in Secrets Manager
+      versionStage: AWSPREVIOUS
+      property: password_previous
 ```
 
 alternatively you can use `dataFrom` and get all the values from hello-service/credentials:
diff --git a/lib/backends/secrets-manager-backend.js b/lib/backends/secrets-manager-backend.js
index 0cc86c1..ecdae80 100644
--- a/lib/backends/secrets-manager-backend.js
+++ b/lib/backends/secrets-manager-backend.js
@@ -20,11 +20,12 @@ class SecretsManagerBackend extends KVBackend {
    * Get secret property value from Secrets Manager.
    * @param {string} key - Key used to store secret property value in Secrets Manager.
    * @param {object} keyOptions - Options for this specific key, eg version etc.
+   * @param {string} keyOptions.versionStage - Version stage
    * @param {object} specOptions - Options for this external secret, eg role
    * @param {string} specOptions.roleArn - IAM role arn to assume
    * @returns {Promise} Promise object representing secret property value.
    */
-  async _get ({ key, specOptions: { roleArn } }) {
+  async _get ({ key, specOptions: { roleArn }, keyOptions: { versionStage = 'AWSCURRENT' } }) {
     this._logger.info(`fetching secret property ${key} with role: ${roleArn || 'pods role'}`)
 
     let client = this._client
@@ -41,7 +42,7 @@ class SecretsManagerBackend extends KVBackend {
     }
 
     const data = await client
-      .getSecretValue({ SecretId: key })
+      .getSecretValue({ SecretId: key, VersionStage: versionStage })
       .promise()
 
     if ('SecretBinary' in data) {
diff --git a/lib/poller.js b/lib/poller.js
index d64819b..ebbf7db 100644
--- a/lib/poller.js
+++ b/lib/poller.js
@@ -13,6 +13,7 @@ const merge = require('lodash.merge')
  * @param {string} properties[].name - Kubernetes Secret property name.
  * @param {string} properties[].property - If the backend secret is an
  *   object, this is the property name of the value to use.
+ * @param {string} properties[].versionStage - If the backend supports versioning, eg secretsManager backend
  */
 
 /** Poller class. */
@@ -132,7 +133,7 @@ class Poller {
   }
 
   /**
-   * Create or update Kubernets secret in the cluster.
+   * Create or update Kubernetes secret in the cluster.
    * @returns {Promise} Promise object representing operation result.
    */
   async _upsertKubernetesSecret () {
@@ -158,17 +159,25 @@ class Poller {
   }
 
   async _updateStatus (status) {
-    this._logger.debug(`updating status for ${this._namespace}/${this._name} to: ${status}`)
-    await this._status.put({
-      body: {
-        ...this._externalSecret,
-        status: {
-          lastSync: `${new Date().toISOString()}`,
-          observedGeneration: this._externalSecret.metadata.generation,
-          status
+    try {
+      this._logger.debug(`updating status for ${this._namespace}/${this._name} to: ${status}`)
+      await this._status.put({
+        body: {
+          ...this._externalSecret,
+          status: {
+            lastSync: `${new Date().toISOString()}`,
+            observedGeneration: this._externalSecret.metadata.generation,
+            status
+          }
         }
+      })
+    } catch (err) {
+      if (err.statusCode !== 409) {
+        this._logger.error(err, `failure while updating status for externalsecret ${this._namespace}/${this._name}`)
+        throw err
       }
-    })
+      this._logger.info(`status update failed for externalsecret ${this._namespace}/${this._name}, due to modification, new poller should start`)
+    }
   }
 
   /**


Test patch:
diff --git a/lib/backends/kv-backend.test.js b/lib/backends/kv-backend.test.js
index 200ac7b..9f4b0eb 100644
--- a/lib/backends/kv-backend.test.js
+++ b/lib/backends/kv-backend.test.js
@@ -130,6 +130,35 @@ describe('kv-backend', () => {
       })
       expect(secretPropertyValues).deep.equals([{ fakePropertyName1: 'fakePropertyValue1' }, { fakePropertyName2: 'fakePropertyValue2' }])
     })
+
+    it('fetches secret property with version', async () => {
+      kvBackend._get.onFirstCall().resolves('fakePropertyValue1')
+      kvBackend._get.onSecondCall().resolves('fakePropertyValue2')
+
+      const secretPropertyValues = await kvBackend._fetchDataValues({
+        data: [{
+          key: 'fakePropertyKey1',
+          name: 'fakePropertyName1'
+        }, {
+          key: 'fakePropertyKey2',
+          versionStage: 'AWSPREVIOUS',
+          name: 'fakePropertyName2'
+        }],
+        specOptions: {}
+      })
+
+      expect(kvBackend._get.getCall(0).args[0]).to.deep.equal({
+        key: 'fakePropertyKey1',
+        keyOptions: {},
+        specOptions: {}
+      })
+      expect(kvBackend._get.getCall(1).args[0]).to.deep.equal({
+        key: 'fakePropertyKey2',
+        keyOptions: { versionStage: 'AWSPREVIOUS' },
+        specOptions: {}
+      })
+      expect(secretPropertyValues).deep.equals([{ fakePropertyName1: 'fakePropertyValue1' }, { fakePropertyName2: 'fakePropertyValue2' }])
+    })
   })
 
   describe('_fetchDataFromValues', () => {
diff --git a/lib/backends/secrets-manager-backend.test.js b/lib/backends/secrets-manager-backend.test.js
index 2e51792..18fdd87 100644
--- a/lib/backends/secrets-manager-backend.test.js
+++ b/lib/backends/secrets-manager-backend.test.js
@@ -13,6 +13,7 @@ describe('SecretsManagerBackend', () => {
   let assumeRoleMock
   let secretsManagerBackend
   const specOptions = {}
+  const keyOptions = {}
 
   const assumeRoleCredentials = {
     Credentials: {
@@ -51,11 +52,13 @@ describe('SecretsManagerBackend', () => {
 
       const secretPropertyValue = await secretsManagerBackend._get({
         key: 'fakeSecretKey',
-        specOptions
+        specOptions,
+        keyOptions
       })
 
       expect(clientMock.getSecretValue.calledWith({
-        SecretId: 'fakeSecretKey'
+        SecretId: 'fakeSecretKey',
+        VersionStage: 'AWSCURRENT'
       })).to.equal(true)
       expect(clientFactoryMock.getCall(0).args).deep.equals([])
       expect(assumeRoleMock.callCount).equals(0)
@@ -69,11 +72,13 @@ describe('SecretsManagerBackend', () => {
 
       const secretPropertyValue = await secretsManagerBackend._get({
         key: 'fakeSecretKey',
-        specOptions
+        specOptions,
+        keyOptions
       })
 
       expect(clientMock.getSecretValue.calledWith({
-        SecretId: 'fakeSecretKey'
+        SecretId: 'fakeSecretKey',
+        VersionStage: 'AWSCURRENT'
       })).to.equal(true)
       expect(clientFactoryMock.getCall(0).args).deep.equals([])
       expect(assumeRoleMock.callCount).equals(0)
@@ -87,7 +92,8 @@ describe('SecretsManagerBackend', () => {
 
       const secretPropertyValue = await secretsManagerBackend._get({
         key: 'fakeSecretKey',
-        specOptions: { roleArn: 'my-role' }
+        specOptions: { roleArn: 'my-role' },
+        keyOptions
       })
 
       expect(clientFactoryMock.lastArg).deep.equals({
@@ -96,7 +102,8 @@ describe('SecretsManagerBackend', () => {
         sessionToken: assumeRoleCredentials.Credentials.SessionToken
       })
       expect(clientMock.getSecretValue.calledWith({
-        SecretId: 'fakeSecretKey'
+        SecretId: 'fakeSecretKey',
+        VersionStage: 'AWSCURRENT'
       })).to.equal(true)
       expect(clientFactoryMock.getCall(0).args).deep.equals([])
       expect(clientFactoryMock.getCall(1).args).deep.equals([{
@@ -107,5 +114,27 @@ describe('SecretsManagerBackend', () => {
       expect(assumeRoleMock.callCount).equals(1)
       expect(secretPropertyValue).equals('fakeAssumeRoleSecretValue')
     })
+
+    it('returns secret property value with versionStage', async () => {
+      getSecretValuePromise.promise.resolves({
+        SecretString: 'fakeSecretPropertyValuePreviousVersion'
+      })
+
+      const secretPropertyValue = await secretsManagerBackend._get({
+        key: 'fakeSecretKey',
+        specOptions,
+        keyOptions: {
+          versionStage: 'AWSPREVIOUS'
+        }
+      })
+
+      expect(clientMock.getSecretValue.calledWith({
+        SecretId: 'fakeSecretKey',
+        VersionStage: 'AWSPREVIOUS'
+      })).to.equal(true)
+      expect(clientFactoryMock.getCall(0).args).deep.equals([])
+      expect(assumeRoleMock.callCount).equals(0)
+      expect(secretPropertyValue).equals('fakeSecretPropertyValuePreviousVersion')
+    })
   })
 })
diff --git a/lib/poller.test.js b/lib/poller.test.js
index d9abd73..05132c5 100644
--- a/lib/poller.test.js
+++ b/lib/poller.test.js
@@ -371,6 +371,38 @@ describe('Poller', () => {
     })
   })
 
+  describe('_updateStatus', () => {
+    it('handles 409 - Conflict', async () => {
+      const conflictError = new Error('Conflict')
+      conflictError.statusCode = 409
+      externalSecretsApiMock.status.put.throws(conflictError)
+
+      const poller = pollerFactory()
+
+      await poller._updateStatus('SUCCESS')
+
+      expect(loggerMock.info.calledWith(`status update failed for externalsecret ${poller._namespace}/${poller._name}, due to modification, new poller should start`)).to.equal(true)
+    })
+
+    it('rethrows other errors', async () => {
+      const notFoundError = new Error('Not Found')
+      notFoundError.statusCode = 404
+      externalSecretsApiMock.status.put.throws(notFoundError)
+
+      const poller = pollerFactory()
+      let error
+
+      try {
+        await poller._updateStatus('SUCCESS')
+      } catch (err) {
+        error = err
+      }
+
+      expect(error).to.not.equal(undefined)
+      expect(error.message).equals('Not Found')
+    })
+  })
+
   describe('_scheduleNextPoll', () => {
     let poller
     let clock