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

Use pullSecretPath to set GOOGLE_APPLICATION_CREDENTIALS #4147

Merged
merged 11 commits into from
Jun 10, 2020
12 changes: 6 additions & 6 deletions docs/content/en/schemas/v2beta5.json
Original file line number Diff line number Diff line change
Expand Up @@ -621,11 +621,6 @@
"description": "Kubernetes namespace. Defaults to current namespace in Kubernetes configuration.",
"x-intellij-html-description": "Kubernetes namespace. Defaults to current namespace in Kubernetes configuration."
},
"pullSecret": {
"type": "string",
"description": "path to the Google Cloud service account secret key file.",
"x-intellij-html-description": "path to the Google Cloud service account secret key file."
},
"pullSecretMountPath": {
"type": "string",
"description": "path the pull secret will be mounted at within the running container.",
Expand All @@ -637,6 +632,11 @@
"x-intellij-html-description": "name of the Kubernetes secret for pulling base images and pushing the final image. If given, the secret needs to contain the Google Cloud service account secret key under the key <code>kaniko-secret</code>.",
"default": "kaniko-secret"
},
"pullSecretPath": {
"type": "string",
"description": "path to the Google Cloud service account secret key file.",
"x-intellij-html-description": "path to the Google Cloud service account secret key file."
},
"randomDockerConfigSecret": {
"type": "boolean",
"description": "adds a random UUID postfix to the default name of the docker secret to facilitate parallel builds, e.g. docker-cfgfd154022-c761-416f-8eb3-cf8258450b85.",
Expand Down Expand Up @@ -687,7 +687,7 @@
"preferredOrder": [
"HTTP_PROXY",
"HTTPS_PROXY",
"pullSecret",
"pullSecretPath",
"pullSecretName",
"pullSecretMountPath",
"namespace",
Expand Down
3 changes: 2 additions & 1 deletion integration/examples/kaniko/skaffold.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ build:
kaniko:
cache: {}
cluster:
pullSecretName: e2esecret
pullSecretName: e2esecret-with-path
pullSecretPath: kaniko-secret.json
namespace: default
deploy:
kubectl:
Expand Down
11 changes: 8 additions & 3 deletions pkg/skaffold/build/cluster/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package cluster
import (
"fmt"
"sort"
"strings"

"github.com/google/go-containerregistry/pkg/name"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -64,7 +65,7 @@ func (b *Builder) kanikoPodSpec(artifact *latest.KanikoArtifact, tag string) (*v
Image: artifact.Image,
ImagePullPolicy: v1.PullIfNotPresent,
Args: args,
Env: env(artifact, b.ClusterDetails.HTTPProxy, b.ClusterDetails.HTTPSProxy),
Env: b.env(artifact, b.ClusterDetails.HTTPProxy, b.ClusterDetails.HTTPSProxy),
VolumeMounts: []v1.VolumeMount{vm},
Resources: resourceRequirements(b.ClusterDetails.Resources),
}},
Expand Down Expand Up @@ -123,10 +124,14 @@ func (b *Builder) kanikoPodSpec(artifact *latest.KanikoArtifact, tag string) (*v
return pod, nil
}

func env(artifact *latest.KanikoArtifact, httpProxy, httpsProxy string) []v1.EnvVar {
func (b *Builder) env(artifact *latest.KanikoArtifact, httpProxy, httpsProxy string) []v1.EnvVar {
pullSecretPath := strings.Join(
tejal29 marked this conversation as resolved.
Show resolved Hide resolved
[]string{b.ClusterDetails.PullSecretMountPath, b.ClusterDetails.PullSecretPath},
"/", // linux filepath separator.
)
env := []v1.EnvVar{{
Name: "GOOGLE_APPLICATION_CREDENTIALS",
Value: "/secret/kaniko-secret",
Value: pullSecretPath,
}, {
// This should be same https://github.com/GoogleContainerTools/kaniko/blob/77cfb912f3483c204bfd09e1ada44fd200b15a78/pkg/executor/push.go#L49
Name: "UPSTREAM_CLIENT_TYPE",
Expand Down
3 changes: 2 additions & 1 deletion pkg/skaffold/build/cluster/pod_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ func TestKanikoPodSpec(t *testing.T) {
ClusterDetails: &latest.ClusterDetails{
Namespace: "ns",
PullSecretName: "secret",
PullSecretPath: "kaniko-secret.json",
PullSecretMountPath: "/secret",
HTTPProxy: "http://proxy",
HTTPSProxy: "https://proxy",
Expand Down Expand Up @@ -273,7 +274,7 @@ func TestKanikoPodSpec(t *testing.T) {
ImagePullPolicy: v1.PullIfNotPresent,
Env: []v1.EnvVar{{
Name: "GOOGLE_APPLICATION_CREDENTIALS",
Value: "/secret/kaniko-secret",
Value: "/secret/kaniko-secret.json",
}, {
Name: "UPSTREAM_CLIENT_TYPE",
Value: "UpstreamClient(skaffold-)",
Expand Down
37 changes: 23 additions & 14 deletions pkg/skaffold/build/cluster/secret.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,41 +24,50 @@ import (
"github.com/sirupsen/logrus"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
typedV1 "k8s.io/client-go/kubernetes/typed/core/v1"

"github.com/GoogleContainerTools/skaffold/pkg/skaffold/color"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/constants"
"github.com/GoogleContainerTools/skaffold/pkg/skaffold/kubernetes"
)

const (
defaultKanikoSecretPath = "kaniko-secret"
)

func (b *Builder) setupPullSecret(out io.Writer) (func(), error) {
if b.PullSecret == "" && b.PullSecretName == "" {
if b.PullSecretPath == "" && b.PullSecretName == "" {
return func() {}, nil
}

color.Default.Fprintf(out, "Creating kaniko secret [%s/%s]...\n", b.Namespace, b.PullSecretName)

color.Default.Fprintf(out, "Checking for kaniko secret [%s/%s]...\n", b.Namespace, b.PullSecretName)
client, err := kubernetes.Client()
if err != nil {
return nil, fmt.Errorf("getting Kubernetes client: %w", err)
}

secrets := client.CoreV1().Secrets(b.Namespace)

if b.PullSecret == "" {
logrus.Debug("No pull secret specified. Checking for one in the cluster.")

if _, err := secrets.Get(b.PullSecretName, metav1.GetOptions{}); err != nil {
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.PullSecretPath == "" {
return nil, fmt.Errorf("secret %s does not exist. No path specified to create it", b.PullSecretName)
}

return b.createSecretFromFile(secrets)
}
if b.PullSecretPath == "" {
// 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

b.PullSecretPath = defaultKanikoSecretPath
return func() {}, nil
}
return func() {}, nil
}

secretData, err := ioutil.ReadFile(b.PullSecret)
func (b *Builder) createSecretFromFile(secrets typedV1.SecretInterface) (func(), error) {
secretData, err := ioutil.ReadFile(b.PullSecretPath)
if err != nil {
return nil, fmt.Errorf("reading pull secret: %w", err)
return nil, fmt.Errorf("cannot create secret %s from path %s. reading pull secret: %w", b.PullSecretName, b.PullSecretPath, err)
}

secret := &v1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: b.PullSecretName,
Expand All @@ -68,7 +77,7 @@ func (b *Builder) setupPullSecret(out io.Writer) (func(), error) {
constants.DefaultKanikoSecretName: secretData,
},
}

b.PullSecretPath = constants.DefaultKanikoSecretName
if _, err := secrets.Create(secret); err != nil {
return nil, fmt.Errorf("creating pull secret %q: %w", b.PullSecretName, err)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/skaffold/build/cluster/secret_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ func TestCreateSecret(t *testing.T) {
Cluster: &latest.ClusterDetails{
Timeout: "20m",
PullSecretName: "kaniko-secret",
PullSecret: tmpDir.Path("secret.json"),
PullSecretPath: tmpDir.Path("secret.json"),
Namespace: "ns",
},
},
Expand Down Expand Up @@ -95,7 +95,7 @@ func TestExistingSecretNotFound(t *testing.T) {
// should fail to retrieve an existing secret
_, err = builder.setupPullSecret(ioutil.Discard)

t.CheckErrorContains("checking for existing kaniko secret", err)
t.CheckErrorContains("secret kaniko-secret does not exist. No path specified to create it", err)
})
}

Expand Down
8 changes: 4 additions & 4 deletions pkg/skaffold/schema/defaults/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -262,12 +262,12 @@ func setDefaultClusterTimeout(cluster *latest.ClusterDetails) error {

func setDefaultClusterPullSecret(cluster *latest.ClusterDetails) error {
cluster.PullSecretMountPath = valueOrDefault(cluster.PullSecretMountPath, constants.DefaultKanikoSecretMountPath)
if cluster.PullSecret != "" {
absPath, err := homedir.Expand(cluster.PullSecret)
if cluster.PullSecretPath != "" {
absPath, err := homedir.Expand(cluster.PullSecretPath)
if err != nil {
return fmt.Errorf("unable to expand pullSecret %s", cluster.PullSecret)
return fmt.Errorf("unable to expand pullSecret %s", cluster.PullSecretPath)
}
cluster.PullSecret = absPath
cluster.PullSecretPath = absPath
random := ""
if cluster.RandomPullSecret {
uid, _ := uuid.NewUUID()
Expand Down
4 changes: 2 additions & 2 deletions pkg/skaffold/schema/defaults/defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ func TestSetDefaultsOnCluster(t *testing.T) {
Build: latest.BuildConfig{
BuildType: latest.BuildType{
Cluster: &latest.ClusterDetails{
PullSecret: "path/to/pull/secret",
PullSecretPath: "path/to/pull/secret",
},
},
},
Expand All @@ -178,7 +178,7 @@ func TestSetDefaultsOnCluster(t *testing.T) {
Build: latest.BuildConfig{
BuildType: latest.BuildType{
Cluster: &latest.ClusterDetails{
PullSecret: "path/to/pull/secret",
PullSecretPath: "path/to/pull/secret",
PullSecretMountPath: path,
},
},
Expand Down
4 changes: 2 additions & 2 deletions pkg/skaffold/schema/latest/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,8 @@ type ClusterDetails struct {
// HTTPSProxy for kaniko pod.
HTTPSProxy string `yaml:"HTTPS_PROXY,omitempty"`

// PullSecret is the path to the Google Cloud service account secret key file.
PullSecret string `yaml:"pullSecret,omitempty"`
// PullSecretPath is the path to the Google Cloud service account secret key file.
PullSecretPath string `yaml:"pullSecretPath,omitempty"`

// PullSecretName is the name of the Kubernetes secret for pulling base images
// and pushing the final image. If given, the secret needs to contain the Google Cloud
Expand Down
14 changes: 12 additions & 2 deletions pkg/skaffold/schema/v2beta4/upgrade.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,26 @@ import (
// 1. Additions:
// 2. Removals:
// 3. Updates:
// + Rename `buildpack` to `buildpacks`
// pullSecret renamed to pullSecretPath
// Rename `buildpack` to `buildpacks`
func (c *SkaffoldConfig) Upgrade() (util.VersionedConfig, error) {
var newConfig next.SkaffoldConfig
pkgutil.CloneThroughJSON(c, &newConfig)
newConfig.APIVersion = next.Version

err := util.UpgradePipelines(c, &newConfig, upgradeOnePipeline)

return &newConfig, err
}

func upgradeOnePipeline(_, _ interface{}) error {
func upgradeOnePipeline(oldPipeline, newPipeline interface{}) error {
oldBuild := &oldPipeline.(*Pipeline).Build
newBuild := &newPipeline.(*next.Pipeline).Build

// rename: cluster.PullSecretPath cluster.PullSecretName
if c := oldBuild.Cluster; c != nil {
newBuild.Cluster.PullSecretPath = c.PullSecret
}

return nil
}
2 changes: 2 additions & 0 deletions pkg/skaffold/schema/v2beta4/upgrade_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ profiles:
cache: {}
cluster:
pullSecretName: e2esecret
pullSecret: secret.json
namespace: default
test:
- image: gcr.io/k8s-skaffold/skaffold-example
Expand Down Expand Up @@ -136,6 +137,7 @@ profiles:
cache: {}
cluster:
pullSecretName: e2esecret
pullSecretPath: secret.json
namespace: default
test:
- image: gcr.io/k8s-skaffold/skaffold-example
Expand Down
4 changes: 2 additions & 2 deletions pkg/skaffold/schema/versions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ build:
context: ./examples/app1
kaniko: {}
cluster:
pullSecret: /secret.json
pullSecretPath: /secret.json
pullSecretName: secret-name
namespace: nskaniko
timeout: 120m
Expand Down Expand Up @@ -326,7 +326,7 @@ func withClusterBuild(secretName, mountPath, namespace, secret string, timeout s
b := latest.BuildConfig{BuildType: latest.BuildType{Cluster: &latest.ClusterDetails{
PullSecretName: secretName,
Namespace: namespace,
PullSecret: secret,
PullSecretPath: secret,
PullSecretMountPath: mountPath,
Timeout: timeout,
}}}
Expand Down