Use `pullSecretPath` to set GOOGLE_APPLICATION_CREDENTIALS #4147

tejal29 · 2020-05-11T20:42:42Z

In this PR,

rename pullSecret to pullSecretPath
Use pullSecretPath specified in skaffold config to construct the GAC env variable.
Display a warning message when default kaniko secret path is used.
Added an integration test to specify kaniko secret name and secret path.

Output Changes
yes.

A warning is shown on user terminal whenever default pull secret path is applied.
This could add to noise. We should follow it up when we add pod health check and determine kaniko pod failure.

Cases where, kaniko secret name pullSecretName is specified, pullSecretPath which points to path with in the volume mounted secret or local path to create secret from is not specifief

kaniko secret pullSecretName exists.

tejaldesai@tejaldesai-macbookpro2 kaniko (fix_kaniko_secret)../../out/skaffold dev -d gcr.io/tejal-test
Listing files to watch...
 - skaffold-example
Generating tags...
 - skaffold-example -> gcr.io/tejal-test/skaffold-example:v1.9.0-21-g897b49bd2
Checking cache...
 - skaffold-example: Not found. Building
Checking for kaniko secret [default/e2esecret]...
WARN[0000] Setting secret keyfile path to kaniko-secret. If this is incorrect, please specify using config key `pullSecret`.
See https://skaffold.dev/docs/references/yaml/#build-cluster-pullSecret

kaniko secret pullSecretName does not exist and no pullSecretPath specified to create it from

Listing files to watch...
 - skaffold-example
Generating tags...
 - skaffold-example -> gcr.io/tejal-test/skaffold-example:v1.9.0-21-g897b49bd2-dirty
Checking cache...
 - skaffold-example: Not found. Building
Checking for kaniko secret [default/e2esecret-not-exists]...
Creating kaniko secret [default/e2esecret-not-exists]...
exiting dev mode because first build failed: setting up pull secret: secret e2esecret-not-exists does not exisit. No path specified to create it

Kaniko secret specified using pullSecretName. pullSecretPath points to the path within the secret volume.
skaffold.yaml

tejaldesai@tejaldesai-macbookpro2 kaniko (fix_kaniko_secret)head skaffold.yaml 
apiVersion: skaffold/v2beta3
kind: Config
build:
  artifacts:
    - image: skaffold-example
      kaniko:
        cache: {}
  cluster:
    pullSecretName: e2esecret-with-path
    pullSecretPath: kaniko-secret.json

Skaffold logs

../../out/skaffold dev -d gcr.io/k8s-skaffold
Listing files to watch...
 - skaffold-example
Generating tags...
 - skaffold-example -> gcr.io/k8s-skaffold/skaffold-example:v1.9.0-21-g897b49bd2-dirty
Checking cache...
 - skaffold-example: Not found. Building
Checking for kaniko secret [default/e2esecret-with-path]...
Building [skaffold-example]...

Kaniko Pod description

kubectl describe po/kaniko-8t24q
Name:         kaniko-8t24q
Namespace:    default
Priority:     0
Node:         gke-integration-tests-default-pool-b6e32cfb-hqj3/10.128.15.201
Start Time:   Mon, 11 May 2020 13:36:07 -0700
Labels:       skaffold-kaniko=skaffold-kaniko
Annotations:  kubernetes.io/limit-ranger: LimitRanger plugin set: cpu request for container kaniko; cpu request for init container kaniko-init-container
Status:       Running
IP:           10.32.6.48
Init Containers:
  ....
Containers:
  kaniko:
    Container ID:  d...
    Host Port:     <none>
    Args:
      --dockerfile
      Dockerfile
      --context
      dir:///kaniko/buildcontext
      --destination
      gcr.io/k8s-skaffold/skaffold-example:v1.9.0-21-g897b49bd2-dirty
      -v
      info
      --cache=true
    State:          Running
      Started:      Mon, 11 May 2020 13:36:12 -0700
    Ready:          True
    Restart Count:  0
    Requests:
      cpu:  100m
    Environment:
      GOOGLE_APPLICATION_CREDENTIALS:  /secret/kaniko-secret.json

…tegration test to test it. Add a warning message when default are used

tstromberg · 2020-05-11T20:46:39Z

/cc @yuwenma

tejal29 · 2020-05-11T21:01:06Z

/cc @prary Would be able to try this branch? I want to make sure we don't break your existing workflow with this change.

tejal29 · 2020-05-11T21:12:00Z

Hold off merging until @prary can verify.

codecov · 2020-05-11T21:22:24Z

Codecov Report

Merging #4147 into master will increase coverage by 0.02%.
The diff coverage is 90.32%.

@@            Coverage Diff             @@
##           master    #4147      +/-   ##
==========================================
+ Coverage   71.93%   71.95%   +0.02%     
==========================================
  Files         322      322              
  Lines       12313    12328      +15     
==========================================
+ Hits         8857     8871      +14     
- Misses       2896     2897       +1     
  Partials      560      560

Impacted Files	Coverage Δ
pkg/skaffold/schema/latest/config.go	`100.00% <ø> (ø)`
pkg/skaffold/schema/defaults/defaults.go	`90.72% <75.00%> (ø)`
pkg/skaffold/build/cluster/secret.go	`41.53% <86.66%> (+5.33%)`	⬆️
pkg/skaffold/build/cluster/pod.go	`87.84% <100.00%> (+0.27%)`	⬆️
pkg/skaffold/schema/v2beta4/upgrade.go	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7731b62...f5db1db. Read the comment docs.

balopat · 2020-05-11T21:25:02Z

pkg/skaffold/build/cluster/secret.go

-			return nil, fmt.Errorf("checking for existing kaniko secret: %w", err)
+	if _, err := secrets.Get(b.PullSecretName, metav1.GetOptions{}); err != nil {
+		color.Default.Fprintf(out, "Creating kaniko secret [%s/%s]...\n", b.Namespace, b.PullSecretName)
+		if b.PullSecret == "" {


This looks like a bug to me - why don't we fall back to default pullsecret name?

pullSecretName and pullSecret are two different configs

pullSecretName is used to indicate the secret name. The default is kaniko-secret and is only set if pullSecret is specified.

pullSecret refers to secret file path.

With this PR, pullSecret is used for a dual purpose.

To create a secret if it does not exist from the file specified in this value

To set GOOGLE_APPLICATION_CREDENTAILS env variable to this path when secret exists.

A bunch of tekton users, create secrets one time using a command like below.
See Additional Information in linked issue
A way to specify which path the secret lies at when using a secret is created using command if secret already exists.

kubectl create secret generic kaniko-secret --from-file=/Users/tejaldesai/workspace/keys/my-key.json

If a secret already exists, we should set the GOOGLE_APPLICATION_CREDENTAILS to path within the volume.

It sounds like pullSecret should be renamed to pullSecretPath.

done! Renamed pullSecret to pullSecretPath

balopat · 2020-05-11T21:25:18Z

pkg/skaffold/build/cluster/secret.go

+	}
+	if b.PullSecret == "" {
+		// TODO: Remove the warning when pod health check can display pod failure errors.
+		logrus.Warnf("Setting secret keyfile path to %s. If this is incorrect, please specify using config key `pullSecret`.\nSee https://skaffold.dev/docs/references/yaml/#build-cluster-pullSecret", defaultKanikoSecretPath)


do we really want a warning here? I think a debug level is sufficient, as the user should only see this when there is something wrong. For those users who have 100% right with the default value, this just adds extra noise.

it does add to extra noise. However, it is not documented very well that skaffold expects the secret to be at path kaniko-secret when creating a secret.
Untill, we can provide an actionable error, i feel the noise is fine.

Should be pullSecretPath (not pullSecret) in the message, right?

integration/examples/kaniko/skaffold.yaml

pkg/skaffold/build/cluster/pod.go

chanseokoh · 2020-05-11T21:41:31Z

Just a thought: I presume there's a history for the name pullSecret. Although the skaffold.yaml reference documents that it's "for pulling base images and pushing the final image", but I've been thinking this can be confusing to some people. For example, #3828 was about pushing a final image to GCR.

prary · 2020-05-12T11:01:19Z

Hold off merging until @prary can verify.

will verify and update you guys asap.

tejal29 · 2020-05-14T20:29:36Z

@prary ping! Can you please try this out? Let me know if i can help verify.

Thanks
Tejal

prary · 2020-05-16T03:02:46Z

Hi @tejal29
I did try it but it failed, my workspace broke in middle hence reason for failure is unknown. I will get back to you with proper analysis once workspace issue is resolved, apologies for the delay.

Thanks for informing about this PR.

pkg/skaffold/build/cluster/pod.go

tstromberg · 2020-05-18T20:26:05Z

pkg/skaffold/build/cluster/secret.go

-			return nil, fmt.Errorf("checking for existing kaniko secret: %w", err)
+	if _, err := secrets.Get(b.PullSecretName, metav1.GetOptions{}); err != nil {
+		color.Default.Fprintf(out, "Creating kaniko secret [%s/%s]...\n", b.Namespace, b.PullSecretName)
+		if b.PullSecret == "" {


It sounds like pullSecret should be renamed to pullSecretPath.

pkg/skaffold/build/cluster/secret_test.go

kiran0707 · 2020-05-21T16:40:59Z

Hi,
Tested the change on behalf of @prary . It worked fine... the usual . it didnt break anything.

There is one thing that we have been seeing for past few skaffold versions.
At end of skaffold build, it tries to get the image ( i think for verifying whether image was pushed ), this thing results in error if this file $USERS_HOME/.docker/config.json is missing.

tejal29 · 2020-05-27T22:39:49Z

Thanks @kiran0707 Will look into this.

tstromberg · 2020-05-29T17:35:03Z

Code looks good. Please check the PR title and description for accuracy.

tejal29 · 2020-06-01T20:08:47Z

Code looks good. Please check the PR title and description for accuracy.

done. Can you take a look again.
Thanks

addressed comments

Use pullSecret to add GOOGLE_APPLICATION_CREDENTIALS env. Add an in…

0373a14

…tegration test to test it. Add a warning message when default are used

tejal29 added the fixit label May 11, 2020

tejal29 requested a review from nkubala May 11, 2020 20:42

tejal29 requested review from balopat, briandealwis and dgageot as code owners May 11, 2020 20:42

googlebot added the cla: yes label May 11, 2020

pull-request-size bot added the size/M label May 11, 2020

tejal29 requested review from nkubala and removed request for nkubala May 11, 2020 20:43

fix tests which relied on old error message

6d93f37

tejal29 added the !! do-not-merge !! label May 11, 2020

balopat previously requested changes May 11, 2020

View reviewed changes

balopat reviewed May 11, 2020

View reviewed changes

integration/examples/kaniko/skaffold.yaml Outdated Show resolved Hide resolved

balopat reviewed May 11, 2020

View reviewed changes

pkg/skaffold/build/cluster/pod.go Outdated Show resolved Hide resolved

replace the secret name

51d02c1

tejal29 force-pushed the fix_kaniko_secret branch from fc23a32 to 51d02c1 Compare May 11, 2020 21:51

use linux filepath separator

65ae5db

tstromberg suggested changes May 18, 2020

View reviewed changes

tejal29 removed the !! do-not-merge !! label May 27, 2020

tejal29 added 5 commits May 28, 2020 12:15

code review comments

5d2cd78

Merge branch 'master' into fix_kaniko_secret

e0a4e7a

rename pullsecret to pullsecretPath

0c5c660

change name to pullSecretPath

8d290fa

make generate schemas

3445a03

pull-request-size bot added size/L and removed size/M labels May 29, 2020

fix tests

f559577

tejal29 changed the title ~~Use pullSecret to set GOOGLE_APPLICATION_CREDENTIALS or Warn with default set~~ Use pullSecretPath to set GOOGLE_APPLICATION_CREDENTIALS Jun 1, 2020

Merge branch 'master' into fix_kaniko_secret

f5db1db

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Jun 10, 2020

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jun 10, 2020

tstromberg approved these changes Jun 10, 2020

View reviewed changes

tejal29 merged commit d97c88f into GoogleContainerTools:master Jun 10, 2020

fmotrifork mentioned this pull request Jun 12, 2020

skaffold 1.11.0 fishworks/fish-food#783

Merged

chanseokoh mentioned this pull request Jun 23, 2020

Fix docs and error message about pullSecretPath #4381

Merged

tejal29 deleted the fix_kaniko_secret branch April 15, 2021 07:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `pullSecretPath` to set GOOGLE_APPLICATION_CREDENTIALS #4147

Use `pullSecretPath` to set GOOGLE_APPLICATION_CREDENTIALS #4147

tejal29 commented May 11, 2020 •

edited by chanseokoh

Loading

tstromberg commented May 11, 2020

tejal29 commented May 11, 2020

tejal29 commented May 11, 2020

codecov bot commented May 11, 2020 •

edited

Loading

balopat May 11, 2020

tejal29 May 11, 2020

tstromberg May 18, 2020

tejal29 May 29, 2020

balopat May 11, 2020

tejal29 May 11, 2020

chanseokoh Jun 10, 2020

chanseokoh commented May 11, 2020

prary commented May 12, 2020

tejal29 commented May 14, 2020

prary commented May 16, 2020

tstromberg May 18, 2020

kiran0707 commented May 21, 2020

tejal29 commented May 27, 2020

tstromberg commented May 29, 2020

tejal29 commented Jun 1, 2020

Use pullSecretPath to set GOOGLE_APPLICATION_CREDENTIALS #4147

Use pullSecretPath to set GOOGLE_APPLICATION_CREDENTIALS #4147

Conversation

tejal29 commented May 11, 2020 • edited by chanseokoh Loading

tstromberg commented May 11, 2020

tejal29 commented May 11, 2020

tejal29 commented May 11, 2020

codecov bot commented May 11, 2020 • edited Loading

Codecov Report

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

chanseokoh commented May 11, 2020

prary commented May 12, 2020

tejal29 commented May 14, 2020

prary commented May 16, 2020

Choose a reason for hiding this comment

kiran0707 commented May 21, 2020

tejal29 commented May 27, 2020

tstromberg commented May 29, 2020

tejal29 commented Jun 1, 2020

Use `pullSecretPath` to set GOOGLE_APPLICATION_CREDENTIALS #4147

Use `pullSecretPath` to set GOOGLE_APPLICATION_CREDENTIALS #4147

tejal29 commented May 11, 2020 •

edited by chanseokoh

Loading

codecov bot commented May 11, 2020 •

edited

Loading