[Proposal] Generalize configuration of kaniko pod #1906

venkatk-25 · 2019-04-02T15:35:21Z

No description provided.

codecov-io · 2019-04-02T16:21:26Z

Codecov Report

Merging #1906 into master will increase coverage by 2.61%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1906      +/-   ##
==========================================
+ Coverage   49.46%   52.07%   +2.61%     
==========================================
  Files         174      179       +5     
  Lines        8036     7923     -113     
==========================================
+ Hits         3975     4126     +151     
+ Misses       3683     3415     -268     
- Partials      378      382       +4

Impacted Files	Coverage Δ
pkg/skaffold/test/test.go	`14.7% <0%> (-1.52%)`	⬇️
pkg/skaffold/build/plugin/plugin.go	`12.94% <0%> (-1.17%)`	⬇️
pkg/skaffold/plugin/builders/docker/builder.go	`21.73% <0%> (-1.07%)`	⬇️
pkg/skaffold/plugin/builders/bazel/builder.go	`14.28% <0%> (-0.72%)`	⬇️
pkg/skaffold/schema/profiles.go	`90.37% <0%> (-0.28%)`	⬇️
pkg/skaffold/jib/jib_gradle.go	`100% <0%> (ø)`	⬆️
pkg/skaffold/plugin/shared/client.go	`0% <0%> (ø)`	⬆️
pkg/skaffold/build/prebuilt.go	`72.72% <0%> (ø)`	⬆️
pkg/skaffold/deploy/kubectl.go	`68.05% <0%> (ø)`	⬆️
cmd/skaffold/app/cmd/runner.go	`0% <0%> (ø)`	⬆️
... and 23 more

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 1127984...df2db53. Read the comment docs.

balopat · 2019-04-02T22:27:27Z

hey @venkatk-25 - can you add me as design shepherd? I'll start to have a look tomorrow and give feedback!

venkatk-25 · 2019-04-03T03:13:57Z

@balopat updated

balopat

Thank you so much for putting this together, it looks great at first look.

I would still think about generating/helping the user with the env var somehow for GCR.
Integration testing for ECR - yeah we don't have the infrastructure for that just yet.

docs/design_proposals/generalizing_kaniko_config.md

balopat · 2019-04-04T18:02:19Z

docs/design_proposals/generalizing_kaniko_config.md

+    - secretName: e2esecret
+      mountPath:  "/secret"
+    env:
+       "GOOGLE_APPLICATION_CREDENTIALS" : "/secret/kaniko-secret"


this still feels like we could generate it - just because the user can make an error easily here - for example this example itself has inconsistency, the env should be: "GOOGLE_APPLICATION_CREDENTIALS" : "/secret/e2esecret"

I wasn't sure of this when I wrote it. From code this looked like a constant regardless of secret name. I guess the secret is expected to contain file kaniko-secret

skaffold/pkg/skaffold/build/kaniko/sources/sources.go

Lines 69 to 70 in 533378c

Name: "GOOGLE_APPLICATION_CREDENTIALS",

Value: "/secret/kaniko-secret",

I would still think about generating/helping the user with the env var somehow for GCR.

Regardless of above comment, I agree this is an additional and error prone step for GCR or maybe other secrets. I can think of following way to solve this:

Have one more optional field called type for secret which can be an enum of special secrets, so we can do some extra work on them when specified.

docs/design_proposals/generalizing_kaniko_config.md

Co-Authored-By: venkatk-25 <33486836+venkatk-25@users.noreply.github.com>

googlebot · 2019-04-09T10:33:25Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

docs/design_proposals/generalizing_kaniko_config.md

balopat

As I'm thinking about it, in general I think there is merit in having a little DSL on top of the kaniko configuration in the skaffold config that take in the least possible information and generates the rest.

However, with custom artifact builders introduced - scripts might require a different path. So I think this is good, but I would keep the "helper" configs and recommend them explicitly for a better developer experience. E.g.:

build: 
  cluster: 
     secrets: 
        - dockerConfig: docker-config 
        - ecrCreds: aws-secret 
        - gcrCreds: gcr-secret
        - secretName: my-special-builder-secret
          mountPath: "/app/mysecret"
        - secretName: my-special-builder-secret2
          mountPath: "/app/mysecret2"
        env: 
           "MY_SPECIAL_BUILDER_VAR" : "1234"

What do you think?

balopat · 2019-05-23T21:31:13Z

Friendly ping @venkatk-25

corneliusweig · 2019-05-27T21:54:12Z

@venkatk-25 can you also add a reminder to update the docs at docs/content/en/docs/how-tos/builders/_index.md?

Co-Authored-By: Balint Pato <balopat@users.noreply.github.com>

venkatk-25 · 2019-05-29T12:55:33Z

@balopat sorry was travelling and couldn't respond earlier.

I suggest the following changes instead to be more consistent by introducing type field to the secret.
User would need to provide one of mountPath or type( enum of GCR, ECR, Docker, ... )

type SecretConfig struct {
	LocalPath  string `yaml:"localPath"`
	SecretName string `yaml:"secretName,omitempty"`
	MountPath  string `yaml:"mountPath,omitempty"`
        Type: string `yaml:"type,omitempty"`
}

and yaml will look like.

build:
  cluster:
    secrets: 
    - secretName: e2esecret
      type: "GCR"
    - secretName: e2esecret
      mountPath:  "/secret"
    env:
       "GOOGLE_APPLICATION_CREDENTIALS" : "/secret/e2esecret"

venkatk-25 · 2019-05-29T12:57:20Z

@venkatk-25 can you also add a reminder to update the docs at docs/content/en/docs/how-tos/builders/_index.md?

@corneliusweig I did not understand this. Is this related to this design or you are asking me to work on docs for existing features

corneliusweig · 2019-06-01T10:38:05Z

@venkatk-25 Sorry for being so unclear about this. What I mean is to add a reminder in the implementation plan that the docs at this location need to be updated. Doc updates are so easily forgotten...

balopat · 2019-06-22T20:19:09Z

@balopat sorry was travelling and couldn't respond earlier.

No worries, I've been super busy too :)

I suggest the following changes instead to be more consistent by introducing type field to the secret.
User would need to provide one of mountPath or type( enum of GCR, ECR, Docker, ... )

I like the idea to standardize on secretName - maybe we can just call it name as we are under secrets anyway?
You still mentioned in your example GOOGLE_APPLICATION_CREDENTIALS env var - that shouldn't need to be there if there is a GCR type secret! We should mention this in the doc. Also - as I'm thinking about this - we should mention that with the cluster details are extracted we are working with the key assumption that a single GCR/ECR type secret covers most use cases (i.e. users typically don't want to push to two different GCR registries from a single multi-artifact skaffold project).
type field could work, but I'd prefer to do something similar to how we handle artifact types in the build section. That enables to give more structure to these options, and helps saving extra lines:

build: 
  cluster: 
     secrets: 
        - docker-config:  // this should be equivalent to the one below
            localPath: "~/docker.json"                       
        - name: "docker-cfg"
            mountPath: "/kaniko/.docker"
            localPath: "~/docker.json"

tejal29 · 2019-08-21T23:56:25Z

@venkatk-25 friendly ping

balopat · 2019-09-17T22:35:32Z

Hi @venkatk-25, I'm going to close this now as it is a bit stalled. Please resubmit if you want to keep working on it! Thank you very much!

Proposal for new way to configure kaniko pod

92b0062

venkatk-25 requested review from balopat, dgageot, nkubala, priyawadhwa and r2d4 as code owners April 2, 2019 15:35

googlebot added the cla: yes label Apr 2, 2019

venkatk-25 mentioned this pull request Apr 2, 2019

Generalizing Container Registry config for Cluster builds( Kaniko ) #1892

Closed

tejal29 self-assigned this Apr 2, 2019

tejal29 added the kind/design discussion label Apr 2, 2019

balopat self-assigned this Apr 2, 2019

Updating the doc with design shepherd

abd07f1

balopat changed the title ~~[Proposal] Gerneralize configuration of kaniko pod~~ [Proposal] Generalize configuration of kaniko pod Apr 4, 2019

balopat suggested changes Apr 4, 2019

View reviewed changes

tejal29 mentioned this pull request Apr 8, 2019

Skaffold/Kaniko with ECR #731

Closed

tejal29 reviewed Apr 8, 2019

View reviewed changes

docs/design_proposals/generalizing_kaniko_config.md Show resolved Hide resolved

Apply suggestions from code review

df2db53

Co-Authored-By: venkatk-25 <33486836+venkatk-25@users.noreply.github.com>

googlebot added cla: no and removed cla: yes labels Apr 9, 2019

Updating related issues section

6c15765

balopat reviewed May 10, 2019

View reviewed changes

docs/design_proposals/generalizing_kaniko_config.md Outdated Show resolved Hide resolved

balopat reviewed May 10, 2019

View reviewed changes

docs/design_proposals/generalizing_kaniko_config.md Outdated Show resolved Hide resolved

balopat suggested changes May 10, 2019

View reviewed changes

Apply suggestions from code review

e271750

Co-Authored-By: Balint Pato <balopat@users.noreply.github.com>

azaiter mentioned this pull request Jul 8, 2019

Add support for ECR pod specs for kaniko #1700

Closed

balopat closed this Sep 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Proposal] Generalize configuration of kaniko pod #1906

[Proposal] Generalize configuration of kaniko pod #1906

venkatk-25 commented Apr 2, 2019

codecov-io commented Apr 2, 2019 •

edited

Loading

balopat commented Apr 2, 2019

venkatk-25 commented Apr 3, 2019

balopat left a comment

balopat Apr 4, 2019

venkatk-25 Apr 9, 2019 •

edited

Loading

venkatk-25 Apr 9, 2019 •

edited

Loading

googlebot commented Apr 9, 2019

balopat left a comment

balopat commented May 23, 2019

corneliusweig commented May 27, 2019

venkatk-25 commented May 29, 2019 •

edited

Loading

venkatk-25 commented May 29, 2019

corneliusweig commented Jun 1, 2019

balopat commented Jun 22, 2019 •

edited

Loading

tejal29 commented Aug 21, 2019

balopat commented Sep 17, 2019

	Name: "GOOGLE_APPLICATION_CREDENTIALS",
	Value: "/secret/kaniko-secret",

[Proposal] Generalize configuration of kaniko pod #1906

[Proposal] Generalize configuration of kaniko pod #1906

Conversation

venkatk-25 commented Apr 2, 2019

codecov-io commented Apr 2, 2019 • edited Loading

Codecov Report

balopat commented Apr 2, 2019

venkatk-25 commented Apr 3, 2019

balopat left a comment

Choose a reason for hiding this comment

balopat Apr 4, 2019

Choose a reason for hiding this comment

venkatk-25 Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

venkatk-25 Apr 9, 2019 • edited Loading

Choose a reason for hiding this comment

googlebot commented Apr 9, 2019

balopat left a comment

Choose a reason for hiding this comment

balopat commented May 23, 2019

corneliusweig commented May 27, 2019

venkatk-25 commented May 29, 2019 • edited Loading

venkatk-25 commented May 29, 2019

corneliusweig commented Jun 1, 2019

balopat commented Jun 22, 2019 • edited Loading

tejal29 commented Aug 21, 2019

balopat commented Sep 17, 2019

codecov-io commented Apr 2, 2019 •

edited

Loading

venkatk-25 Apr 9, 2019 •

edited

Loading

venkatk-25 Apr 9, 2019 •

edited

Loading

venkatk-25 commented May 29, 2019 •

edited

Loading

balopat commented Jun 22, 2019 •

edited

Loading