new Cluster resource

[nader-ziada]

Fixes: #177

To enable tasks to connect to an external cluster other than the host
cluster, for example to deploy a app.

Proposed changes

• new resource of type Cluster
• cluster resource adds a step to create a kubeconfig for target cluster using a new binary kubeconfig-writer
• integration test to check kubeconfig
• resource validation to check for cluster resource
• remove clusters from pipeline and pipeline params types
• change examples to use cluster resource
• add documentation in using.md

[bobcatfish]

Lookin good!

I think you're planning to do this already, but it would be great to have some basic docs for the kubeconfig binary itself

[bobcatfish]

maybe the path should be configurable?

[bobcatfish]

thanks for figuring out how to get the secrets to work! 🎉

left some comments, mostly I think with a couple of changes we can have slightly fewer, but more focused and eaiser to debug, tests

[bobcatfish]

awesome!

[bobcatfish]

🎉

[bobcatfish]

might be worth mentioning this in the docs on how to use it

[nader-ziada]

it just how you create a secret, which I link to the docs

[bobcatfish]

kk

[bobcatfish]

if you put the logic in case v1alpha1.PipelineResourceTypeCluster: into a separate function, you could test that function directly instead of testing it a this level

if we did that, id be happy to remove these cases completely (or just keep one at most)

[nader-ziada]

But these cases are testing two different things,

• cluster resource with plain text is validating the step is added to the build
• cluster resource with secrets is validating the env from the secrets are added to the build step

[bobcatfish]

oh yeah for sure, we'd still have the 2 different tests, but we could have them at a different level.

caveat: i made a bit of a mistake and i thought this was the reconciler test! so i think what im saying is a lot less important now

however i still think this logic could be factored a bit differently so that it's slightly more clear and testable (esp as we keep adding more types of resources)

for example if you had functions like these:

func getClusterStep(r *v1alpha1.PipelineResource) (*corev1.Container, error) {
	clusterResource, err := v1alpha1.NewClusterResource(r)
	if err != nil {
		return nil, fmt.Errorf("error creating cluster resource %q, resource must be invalid: %s", r.Name, err)
	}

	var envVars []corev1.EnvVar
	for _, sec := range clusterResource.Secrets {
		ev := corev1.EnvVar{
			Name: strings.ToUpper(sec.FieldName),
			ValueFrom: &corev1.EnvVarSource{
				SecretKeyRef: &corev1.SecretKeySelector{
					LocalObjectReference: corev1.LocalObjectReference{
						Name: sec.SecretName,
					},
					Key: sec.SecretKey,
				},
			},
		}
		envVars = append(envVars, ev)
	}

	clusterContainer := &corev1.Container{
		Name:  "kubeconfig",
		Image: *kubeconfigWriterImage,
		Args: []string{
			"-clusterConfig", clusterResource.String(),
		},
		Env: envVars,
	}
	return clusterContainer, nil
}

// or you could even make this functional, have it return a new build object!
func addBuildStep(b *buildv1alpha1.Build, c *corev1.Container) {
	buildSteps := append([]corev1.Container{*c}, b.Spec.Steps...)
	b.Spec.Steps = buildSteps
}

You could write unit tests to cover both of the above, which due to their more specific interface don't even need task runs or tasks to test.

Then inside of AddInputResource you could have:

		case v1alpha1.PipelineResourceTypeCluster:
			{
				clusterContainlusterContainerer, err := getClusterStep(resource)
				if err != nil {
					return nil, fmt.Errorf("couldnt add cluster resource to task %q: %s", task.Name, err)
				}
				addBuildStep(build, clusterContainlusterContainerer)
			}
		}

And suddenly since AddInputResource is only calling out to 2 functions, both of which we have unit tested well, it's a lot less important (i would say not even worth it) to have test coverage at this level, and we can save ourselves a lot of boiler plate in our test code.

[nader-ziada]

I will refactor the func out of the main block of code, but would still like to keep the table test to be testing the external interface (contract) of the AddInputResource functionality. Testing the private function ties down the implementation. wdyt?

[shashwathi]

I agree with @pivotal-nader-ziada thought of testing the functionality instead of implementation. Unit tests should provide the confidence to refactor code without everything completely breaking apart. Testing private functions tend to restrict the refactor scope without altering the tests.

[bobcatfish]

I see what you're saying - I think this means we should push the interface changes up a level, i.e. updated the interface to AddInputResource so that it doesn't need to have TaskRuns and Tasks instantiated in order to test it :D

We can keep iterating on it.

Unit tests should provide the confidence to refactor code without everything completely breaking apart.

I dunno, I think we should limit the amount of code that one can change without also changing a unit test (and we should limit that to the "imperative shell").

I think very finely grained and focused unit tests put pressure on us to design our code better - and I think the fact that so many objects need to be instantiated for a lot of our existing unit tests is a sign we can improve the design.

Testing private functions tend to restrict the refactor scope without altering the tests.

I find that when we start wanting to test private functions, its often a sign that we can break up the code more, such that these private functions become public. For example, perhaps we want to have a package that is about clusters specifically, which could have a public function like getClusterStep

I find this tweet from dave cheney related and inspiring:

use packages like a knife to separate code that consumes functionality from code that provide functionality. Use unit tests along the cut line to lock in the behaviour of your packages.

[bobcatfish]

i feel like we already have a lot of integration tests - what if we had the helm integration test use a cluster resource instead, then we could just have the one test?

[nader-ziada]

I feel this test is specific to the creation of the kubeconfig file, I would prefer to remove the kaniko test instead, WDYT?

[shashwathi]

If the concern is only about time then we could consider refactoring tests to be parallel since all e2e tests are self contained in their namespaces.

[bobcatfish]

my concern is less about time and more about:

1. Maintaining the integration tests
2. Relying on integration tests for coverage

this is definitely a philosophical point of view, but i recently watched integration tests are a scam and i realized that relying too much on integration tests can cause us to be lax with our unit test coverage, which in turn promotes bad design choices.

This is besides the fact that integration tests are slow and when they fail, it's very hard to know what to do.

(Watch this space for my upcoming kubecon talk which is pretty much all about this!!)

So I would like to minimize the surface of what we test in our integration tests as much as possible, and focus instead of organizing and unit testing our code in such a way that we don't feel like we need to cover it in integration tests #the-possibly-unrealistic-ideal

I feel this test is specific to the creation of the kubeconfig file, I would prefer to remove the kaniko test instead, WDYT?

in an ideal world what i would really like is just one integration test that includes as much functionality as we think is reasonable, e.g. it uses a kube cluster, has a service account, builds and pushes an image.

Anyway feel free to ignore this and go ahead with this test but you'll probably see a PR from me in the future where I try to move our integration tests toward this ideal - we can always continue the discussion there :D

[nader-ziada]

for this specific case, I can't really merge this integration test checking for kubeconfig with the helm one because we would need a actual 2nd cluster to target by the helm deployment. The kaniko test we can delete, or maybe the hello-world?

[bobcatfish]

I can't really merge this integration test checking for kubeconfig with the helm one because we would need a actual 2nd cluster to target by the helm deployment.

oh i see, i didnt realize that the integration test wasn't actually using the kubeconfig

would you mind making a follow-up issue in our backlog that we should consider having an integration test that actually uses a second cluster?

alternatively:
we could use the cluster resource to (somehow) point at the current cluster, but maybe a different namespace or something?

[shashwathi]

I think its helpful to log if username password or token is used for authentication. If user has provided both and choice is made then it would be useful data point for debugging.

[shashwathi]

Great job @pivotal-nader-ziada :)

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pivotal-nader-ziada, shashwathi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [pivotal-nader-ziada]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[bobcatfish]

Looking good! if you get tired of the philosophical debate about the tests feel free to remove this and go ahead and merge:
/hold

/lgtm

(But if you can see the same dream I see, a dream of many narrowly focused unit tests and very very very few integration tests, i think we could make this even more awesome!)

[nader-ziada]

I like your dream of many unit tests and few integration tests @bobcatfish, let see what I can do :D

[knative-metrics-robot]

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
cmd/kubeconfigwriter/main.go	Do not exist	0.0%
pkg/apis/pipeline/v1alpha1/cluster_resource.go	Do not exist	59.4%
pkg/apis/pipeline/v1alpha1/pipelineparams_validation.go	76.3%	75.9%	-0.5
pkg/apis/pipeline/v1alpha1/pipelineresource_validation.go	0.0%	68.2%	68.2
pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go	84.6%	88.6%	4.0

[nader-ziada]

/retest

[bobcatfish]

/lgtm

[nader-ziada]

/hold cancel