Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
venkatk-25 opened this issue Mar 29, 2019 · 7 comments
Closed
Labels
area/build build/kaniko kind/design discussion kind/feature-request priority/p3 agreed that this would be good to have, but no one is available at the moment.

Comments

@venkatk-25
Copy link
Contributor

venkatk-25 commented Mar 29, 2019

This proposal is to generalize the Registry configuration for kaniko pod, so that in future registries supported by kaniko can be supported seamlessly.

Current Scenario:

Kaniko supports 2 types of registries GCR and Docker, with former being mandatory always.
The implementation itself is repository specific and not extensible as we need to keep adding more logic for every new type of repository and keep maintaining compatibilty with registries/credhelpers.

The current looks like below( I have kept only required pieces from original code snippet):

type ClusterDetails struct {
       // GCR creds
	PullSecret string `yaml:"pullSecret,omitempty"`
	PullSecretName string `yaml:"pullSecretName,omitempty"`

	DockerConfig *DockerConfig `yaml:"dockerConfig,omitempty"`
}

// DockerConfig contains information about the docker `config.json` to mount.
type DockerConfig struct {
	Path string `yaml:"path,omitempty"`
	SecretName string `yaml:"secretName,omitempty"`
}

Breaking down the requirements and generalizing:

If you observe the above piece of configuration, you will see both creds have same 2 fields, SecretName/SecretPath.
When we look into how they behave differently, we can observe the differences between them as follows:

  1. The path where to mount the config varies based on secret
  2. Some of them(like GCR) might require env variables to be set.

If we can generalize the above points, then any configuration(GCR/ER/private registry/S3/GCS, etc..) breaks down to following steps:

  1. Create a secret with the configuration
  2. Mount the config in specific location in pod
  3. In some cases set some env variables, for the same.

New Proposal:

The same above can be re-configured in a more generic way as follows:

type ClusterDetails struct {
	SecretConfigs []*SecretConfig `yaml:"secrets,omitempty"`
        env map[string]string      `yaml:"env,omitempty"`
}

// DockerConfig contains information about the docker `config.json` to mount.
type SecretConfig struct {
	LocalPath string `yaml:"localPath"`
	SecretName string `yaml:"secretName,omitempty"`
        MountPath   string `yaml:"mountPath,omitempty"`
}

How different configurations fit into this scheme:

GCR/GCS

current scheme:

build:
  cluster:
    pullSecretName: e2esecret

new scheme:

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

Private Registry:

current scheme:

build:
  cluster:
    dockerConfig:
      secretName: e2esecret

new scheme:

build:
  cluster:
    secrets: 
    - secretName: docker-secret
      mountPath:  "/kaniko/.docker"

ECR:

current scheme: None
new scheme( pushing-to-amazon-ecr) :

build:
  cluster:
    secrets: 
    - secretName: aws-secret
      mountPath:  "/root/.aws/"
    - secretName: docker-config
      mountPath:  "/kaniko/.docker/"

Pros :

  1. more extensible and future proof.
  2. Can support varies use cases, like setting http_proxy, source from github, etc. to name a few
  3. Not limited to kaniko, can be extended to any cluster builder, keeping in spirit of recent changes.

Cons:

  1. Slightly more config for users, specifically for GCR
@venkatk-25 venkatk-25 changed the title Generalizing Container Registry config for Kaniko Generalizing Container Registry config for Cluster builds( Kaniko ) Mar 29, 2019
@venkatk-25
Copy link
Contributor Author

venkatk-25 commented Mar 29, 2019

@balopat @tejal29 @priyawadhwa
Taking a stab at combining 2 existing PRs and my other requirement of having to configure proxy support in kaniko pod.
#1728 by me
#1700 by @azaiter
#731 saw @priyawadhwa also gave similar suggestion

@azaiter
Copy link

azaiter commented Mar 29, 2019

Thank you @venkatk-25 for opening this proposal up. I think this scheme would be sufficient to cover all major registries that I can think of. It looks like for the sake of simplicity, you've used a string map for environment variables declaration. What about using an array of v1 EnvVar type? This will allow defining environment variables from existing secrets/configmaps/etc. What do you think?

@venkatk-25
Copy link
Contributor Author

@azaiter, thought of it but since the schema didn't use the kube types anywhere, so wasn't sure but yes it makes perfect sense

@tejal29
Copy link
Member

tejal29 commented Apr 1, 2019

@venkatk-25 this is great and good candidate to put it in docs/design_proposals

@venkatk-25
Copy link
Contributor Author

@tejal29 its not clear whether the proposal should be file in docs/design_proposals or description in PR from README. Can you clarify it, so I can create it accordingly.

@venkatk-25
Copy link
Contributor Author

raised PR #1906

@tejal29 tejal29 added the priority/p3 agreed that this would be good to have, but no one is available at the moment. label Sep 24, 2019
@tstromberg
Copy link
Contributor

Thank you for the idea! I'm closing this issue as it's been open a while, and no progress seems to be being made on it. If someone feels strongly about this, feel free to comment here or re-raise an issue referencing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build build/kaniko kind/design discussion kind/feature-request priority/p3 agreed that this would be good to have, but no one is available at the moment.
Projects
None yet
Development

No branches or pull requests

5 participants