Only substitute manifest image when there's a full match #10643

DJLemkes · 2019-06-12T21:33:58Z

While using this task, I encountered some strange behavior. Using a deployment file (snippet) like:

    spec:
      containers:
      - name: nginx
         image: nginx:1.7.9
      - name: edgecase
         image: nginx-image:1.7.9
      initContainers:
      - name: nginx-init-container
         image: nginx-init:0.1.0

And a containers element in my task definition like:

- task: KubernetesManifest@0
   displayName: Deploy application
   inputs:
     action: deploy
     manifests: kubernetes/deployment.yaml
     containers: |
       nginx:42
       nginx-image:42.1
       nginx-init:42.1.2

I got the following output which is incorrect and potentially dangerous because of unexpected containers being deployed

    spec:
      containers:
      - name: nginx
         image: nginx:42
      - name: edgecase
         image: nginx:42
      initContainers:
      - name: nginx-init-container
         image: nginx:42

This PR fixes that issue and only matches when there is a full match on the target image name. The solution is still doing raw string processing. Personally I would prefer using a Yaml lib (or even Kubernetes Yaml model) for parsing the incoming Yaml file but there are probably reasons for not doing this.

While writing test cases, I encountered runtime errors because of a circular import between utilities.ts and constants.ts. Because of that I moved the string equality utility to a separate file. This resolved the issue.

Note that I'm completely new to TypeScript (did some JS before) so please forgive possible style and convention mistakes.

msftclas · 2019-06-12T21:34:10Z

All CLA requirements met.

thesattiraju

@DJLemkes Thanks for the contribution.
It's almost there; you need to bump up the task patch version to allow these changes to be shipped.

thesattiraju · 2019-06-13T10:21:44Z

Tasks/KubernetesManifestV0/src/utils/CanaryDeploymentHelper.ts

@@ -8,7 +8,8 @@ import * as yaml from 'js-yaml';
 import * as TaskInputParameters from '../models/TaskInputParameters';
 import * as fileHelper from '../utils/FileHelper';
 import * as helper from './KubernetesObjectUtility';
-import { KubernetesWorkload } from '../models/constants';
+import {  KubernetesWorkload } from '../models/constants';
+import {StringComparer, isEqual} from '../utils/StringComparison';


NIT: spacing

thesattiraju · 2019-06-13T10:21:55Z

Tasks/KubernetesManifestV0/src/utils/KubernetesObjectUtility.ts

@@ -4,16 +4,16 @@ import * as tl from 'vsts-task-lib/task';
 import * as yaml from 'js-yaml';
 import { Resource } from 'kubernetes-common/kubectl-object-model';
 import { KubernetesWorkload, recognizedWorkloadTypes } from '../models/constants';
+import {StringComparer, isEqual} from '../utils/StringComparison';


NIT: spacing

thesattiraju · 2019-06-13T10:26:48Z

Tasks/KubernetesManifestV0/src/actions/createSecret.ts

@@ -3,13 +3,14 @@
 import { Kubectl } from 'kubernetes-common/kubectl-object-model';
 import * as utils from '../utils/utilities';
 import * as TaskInputParameters from '../models/TaskInputParameters';
+import {StringComparer, isEqual} from '../utils/StringComparison';


NIT: Spacing

This is missing

thesattiraju · 2019-06-13T10:27:00Z

Tasks/KubernetesManifestV0/src/models/constants.ts

@@ -1,7 +1,7 @@
 'use strict';

 import * as tl from 'vsts-task-lib/task';
-import * as utils from '../utils/utilities';
+import {StringComparer, isEqual} from '../utils/StringComparison';


NIT: Spacing

thesattiraju · 2019-06-13T10:33:10Z

Tasks/KubernetesManifestV0/package-lock.json

@@ -7,7 +7,7 @@
      "resolved": "https://registry.npmjs.org/@types/del/-/del-2.2.33.tgz",


Undo theses changes

thesattiraju · 2019-06-14T05:57:29Z

@DJLemkes If you build after the task version bump up, that would bump it up in task.loc.json file.
Also, there seem to be conflicts, you may have to rebase your upstream.

DJLemkes · 2019-06-14T06:04:27Z

Ok, I see. Sorry about that. Will do a build, add the task.loc.json and rebase.

When building, the package-lock.json of multiple tasks seem to be updated as well. Do you want me to commit them? I reverted the package-lock of the KubernetesManifest task but I added js-yaml for easier testing. That should be the reason why at least the package-lock of KubernetesManifest task is being updated, right?

thesattiraju · 2019-06-14T06:14:02Z

If you notice the changes correctly in the generated package-lock.json file, the majority of the changes would be a conversion of some static version to either * or ^<some-version>; which we don't want.

You can ignore this for now, as js-yaml is already being brought in by one of the dependencies, and it's already added in the package-lock.json file.

Previously, the line `image: nginx-init` would already match an 'nginx:42' container in the task definition resulting in `image: nginx:42`. This leads to unexpected images being deployed.

DJLemkes · 2019-06-14T06:31:32Z

Did a rebase and left the package-lock untouched when they were caused by my changes.

thesattiraju

LGTM 👍

thesattiraju · 2019-06-14T06:47:00Z

Tasks/KubernetesManifestV0/src/actions/createSecret.ts

@@ -3,13 +3,14 @@
 import { Kubectl } from 'kubernetes-common/kubectl-object-model';
 import * as utils from '../utils/utilities';
 import * as TaskInputParameters from '../models/TaskInputParameters';
+import {StringComparer, isEqual} from '../utils/StringComparison';


This is missing

sabbour · 2019-07-16T08:38:32Z

This introduced a regression bug. The use case below was working, and now it isn't.

Given the spec below with an untagged image, the task should be able to correctly apply the tag at deploy time.

spec:
      containers:
      - name: myapp
         image: myrepo.azurecr.io/myapp

- task: KubernetesManifest@0
   displayName: Deploy application
   inputs:
     action: deploy
     manifests: kubernetes/deployment.yaml
     containers: 'myrepo.azurecr.io/myapp:$(Build.BuildID)'

This is currently not working anymore, unless you do something like dummytag:

spec:
      containers:
      - name: myapp
         image: myrepo.azurecr.io/myapp:dummytag

sabbour · 2019-07-16T08:40:08Z

@bansalaseem @DS-MS PTAL.

DJLemkes requested a review from bansalaseem as a code owner June 12, 2019 21:33

DJLemkes mentioned this pull request Jun 13, 2019

KubernetesManifest: container image substitution not fully correct #10646

Closed

thesattiraju suggested changes Jun 13, 2019

View reviewed changes

DJLemkes added 3 commits June 14, 2019 08:19

Only substitute manifest image when there's a full match

7b71150

Previously, the line `image: nginx-init` would already match an 'nginx:42' container in the task definition resulting in `image: nginx:42`. This leads to unexpected images being deployed.

Cleaning up typos and sloppy leftover lines

621c550

Process review comments and bump task patch version

68e70ab

DJLemkes force-pushed the better-k8-manifest-image-substitution branch from c74e7b8 to 68e70ab Compare June 14, 2019 06:29

thesattiraju approved these changes Jun 14, 2019

View reviewed changes

Linting

f5db4be

bansalaseem approved these changes Jun 16, 2019

View reviewed changes

thesattiraju added 3 commits June 17, 2019 10:27

Merge branch 'master' into better-k8-manifest-image-substitution

abd0b06

Merge branch 'master' into better-k8-manifest-image-substitution

b8c4219

Merge branch 'master' into better-k8-manifest-image-substitution

257fc41

thesattiraju merged commit bfc9ddd into microsoft:master Jun 17, 2019

sabbour mentioned this pull request Jul 16, 2019

KubernetesManifest failing test for an image with inline comments #10901

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Only substitute manifest image when there's a full match #10643

Only substitute manifest image when there's a full match #10643

DJLemkes commented Jun 12, 2019

msftclas commented Jun 12, 2019 •

edited

Loading

thesattiraju left a comment

thesattiraju Jun 13, 2019

thesattiraju Jun 13, 2019

thesattiraju Jun 13, 2019

thesattiraju Jun 14, 2019

thesattiraju Jun 13, 2019

thesattiraju Jun 13, 2019

thesattiraju commented Jun 14, 2019

DJLemkes commented Jun 14, 2019

thesattiraju commented Jun 14, 2019

DJLemkes commented Jun 14, 2019

thesattiraju left a comment

thesattiraju Jun 14, 2019

sabbour commented Jul 16, 2019

sabbour commented Jul 16, 2019

		@@ -7,7 +7,7 @@
		"resolved": "https://registry.npmjs.org/@types/del/-/del-2.2.33.tgz",

Only substitute manifest image when there's a full match #10643

Only substitute manifest image when there's a full match #10643

Conversation

DJLemkes commented Jun 12, 2019

msftclas commented Jun 12, 2019 • edited Loading

thesattiraju left a comment

Choose a reason for hiding this comment

thesattiraju Jun 13, 2019

Choose a reason for hiding this comment

thesattiraju Jun 13, 2019

Choose a reason for hiding this comment

thesattiraju Jun 13, 2019

Choose a reason for hiding this comment

thesattiraju Jun 14, 2019

Choose a reason for hiding this comment

thesattiraju Jun 13, 2019

Choose a reason for hiding this comment

thesattiraju Jun 13, 2019

Choose a reason for hiding this comment

thesattiraju commented Jun 14, 2019

DJLemkes commented Jun 14, 2019

thesattiraju commented Jun 14, 2019

DJLemkes commented Jun 14, 2019

thesattiraju left a comment

Choose a reason for hiding this comment

thesattiraju Jun 14, 2019

Choose a reason for hiding this comment

sabbour commented Jul 16, 2019

sabbour commented Jul 16, 2019

msftclas commented Jun 12, 2019 •

edited

Loading