Introduce configuration option to configure image pushing per kube-context

[corneliusweig]

This PR adds a new configuration option in the skaffold configuration file called local-cluster with values true/false. It will be useful for users working with a local setup but with non-standard kube-context names (i.e. other than minikube or docker-for-desktop). The effect of this option is to skip pushing images when the cluster is considered local. It therefore addresses #1158, #1205, and #1172.

I tried to make this new option fully backwards compatible. This required to make it optional, therefore it is implemented as a *bool. Unfortunately this breaks the config-set command, e.g. skaffold config set --global local-cluster true, because it cannot handle pointer types. Before this PR can be merged, this should be addressed (as well as adding some documentation). However, before moving on, I would like to get some feedback if you think this makes sense.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

---

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

[codecov-io]

Codecov Report

Merging #1355 into master will increase coverage by 0.01%.
The diff coverage is 48.93%.

@@            Coverage Diff             @@
##           master    #1355      +/-   ##
==========================================
+ Coverage   45.35%   45.36%   +0.01%     
==========================================
  Files         115      115              
  Lines        4773     4805      +32     
==========================================
+ Hits         2165     2180      +15     
- Misses       2383     2401      +18     
+ Partials      225      224       -1

Impacted Files	Coverage Δ
cmd/skaffold/app/cmd/config/util.go	27.45% <0%> (-6.29%)	⬇️
pkg/skaffold/build/local/types.go	0% <0%> (ø)	⬆️
cmd/skaffold/app/cmd/config/set.go	61.11% <92%> (+10.23%)	⬆️

---

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 aace573...4fcaff5. Read the comment docs.

[googlebot]

CLAs look good, thanks!

[nkubala]

@corneliusweig thanks for the PR! definitely a fan of this change. I have one concern with some of the changes you've made to how new values get set in the config, but other than that this is looking good

[nkubala]

@corneliusweig sorry for the super late reply here. Thanks for the explanations, I understand your rationale now; sorry for being a little slow originally :) this isn't as simple as I thought it would be originally.

I'd really like to avoid using yaml parsing if we can here though; it has limitations that using reflection can help us get around (see #872 for an example), especially when dealing with nested objects in the config.

I hacked together an alternative solution that involves switching on the fieldType of the config, and creating a new value object based on the string arg we pass in through the CLI args:

func setConfigValue(name string, value interface{}) error {
	cfg, err := getOrCreateConfigForKubectx()
	if err != nil {
		return err
	}

	fieldName := getFieldName(cfg, name)
	if fieldName == "" {
		return fmt.Errorf("%s is not a valid config field", name)
	}

	fieldValue := reflect.Indirect(reflect.ValueOf(cfg)).FieldByName(fieldName)
	fieldType := fieldValue.Type()
	val, err := getNewFieldValue(fieldType, value)
	if err != nil {
		return fmt.Errorf("%s is not a valid value for field %s", value, name)
	}

	reflect.ValueOf(cfg).Elem().FieldByName(fieldName).Set(val)

	return writeConfig(cfg)
}

func getNewFieldValue(fieldType reflect.Type, value interface{}) (reflect.Value, error) {
	valStr := value.(string)

	switch fieldType.String() {
	case "string":
		return reflect.ValueOf(valStr), nil
	case "*bool":
		if valStr == "" {
			return reflect.Zero(fieldType), nil
		}
		valBase, err := strconv.ParseBool(valStr)
		if err != nil {
			return reflect.Value{}, err
		}
		return reflect.ValueOf(&valBase), nil
	default:
		return reflect.Value{}, fmt.Errorf("unsupported type: %s", fieldType)
	}
}

It's a little less clean because it involves adding a new switch clause every time we add a field with a new type in the global config, but it gets around having to use yaml parsing to address the concept of a "nil" field value (e.g. true, false, and nil). I really feel like there's another solution for implementing "optional" values here using reflection, I'm just not seeing it.

Take a look at this and let me know what you think, in the meantime can you rebase your PR so CI passes? If we can't come up with anything better than we can decide between the two implementations and get this merged soon.

[corneliusweig]

Hi @nkubala,
no worries, better late than never ;). I better understand now, why you want to avoid the yaml unmarshalling. And I quite like your modifications. Apart from minor renamings, I used your patch as basis for a new commit.

My only worry is the cast value.(string) in set.go. Since value only has type constraint interface{}, this could blow up. However, it's in a private function and I have too little experience with this project to judge if this is acceptable. After all, you also explained that this is a deliberate choice to convey that the method is meant to not only work for strings.

Last, I would also like to add some documentation about the whole global config. What would be the best place for that?

[nkubala]

@corneliusweig yeah, after thinking about it some more I think it might actually make sense to just change the signature to setConfigValue(name string, value string). my reasoning here is that the value we're passing in is coming from cobra args, which are always strings, so there's no real reason to call them interface{} if we're just gonna cast them back. the change I recommended still holds though, because even though we're passing the value as a string, semantically it really has an embedded type (e.g. *bool) which we can infer using reflection.

so basically what I'm saying is, you can go ahead and change all these functions to accept the value parameter as a string and then get rid of the typecast :) then this LGTM

[nkubala]

oh, and in response to the global config: I realized how utterly terrible the documentation for that feature is a few days ago, sorry about that 😨 if you'd like to add some, it probably fits best in the Concepts section of the docs 👍

[corneliusweig]

Hi @nkubala , all done. I hope the documentation is somewhat helpful.
In addition, I also found what I think was an error in the documentation of the the gcr.io default-repo logic. Could you take a look?

[nkubala]

@corneliusweig awesome, a few tiny nits but this is basically there. And yes that was an error in the default repo docs, thanks for fixing that!

[nkubala]

@corneliusweig thanks for all your work on this!

[haf]

Also, do you have any idea as to why when running minikube with kvm or virtualbox, the docker daemon isn't properly configured to have access? And even if adding a service-account _json_key to the docker daemon in the virtual machine / guest, skaffold is not allowed to do the pull, but will reuse images from the guest docker daemon when those have been pulled manually.

For example, if your image is doing a FROM eu.gcr.io/my-project/my-svc an skaffold runs a non-host docker daemon, you'll have issues with skaffold

I'm commenting on this PR since it would seem that you're working on unifying how the k8s/docker context is managed in Skaffold and it would seem appropriate to fix both pull and push under the same credential.

[corneliusweig]

@haf It's impossible to really help you, because there is not enough context. For example, have you done the docker login after source <(minikube docker-env) and so on. Please take the time to collect all information and open a separate issue.

[haf]

@corneliusweig I've set up ALL logins possible before starting skaffold. I don't run source <(minikube docker-env) at all. I don't understand what information I'm supposed to give either, because I don't know how you expect it to work, and it's not documented how auth works between docker-on-host vs docker-in-guest setups. If you document how you expect me to configure private registries, I can create bug reports against the expected behaviour, but right now I'm just wandering around a desert.

[corneliusweig]

@haf So I guess you want to use a registry with access restrictions during your docker builds. That will require the following steps:

minikube start
source <(minikube docker-env) # this will set up the minikube docker daemon in your current shell
docker login ... # this will login in the minikube docker daemon
kubectl config use-context minikube
skaffold dev

Does that work for you? If not, please open a new issue.

[haf]

@corneliusweig No that doesn't work, because despite being able to PULL from within minikube, when skaffold runs docker to PULL the image it doesn't authenticate successfully.