integration: use restricted RBAC credentials for some tests by jazzdan · Pull Request #1695 · tilt-dev/tilt

jazzdan · 2019-05-29T15:04:05Z

No description provided.

jazzdan · 2019-05-29T15:55:46Z

For an explanation of what I'm scripting here see this article.

maiamcc

I uh... THINK this lgtm? (def try to get some of the tests to go red to make sure you're testing what you think you're testing.)

this is all opaque and weird enough that i'd recommend a lotttt of comments in the setup code about what's going on and why we have to do it this way, otherwise this'll be hell on the next person who has to change it

maiamcc · 2019-05-29T16:12:54Z

+
+func (f *k8sFixture) getSecrets() {
+	outWriter := bytes.NewBuffer(nil)
+	cmdStr := `kubectl get secrets -n tilt-integration -o json | jq -r '.items[] | select(.metadata.name | startswith("tilt-integration-user-token-")) | .data.token'`


diff between these two blocks is not immediately apparent, consider comments for readability

maiamcc · 2019-05-29T16:16:34Z

+		f.t.Fatalf("Error getting secrets: %v. Cmd: %s", err, cmdStr)
+	}
+
+	cmdStr = `kubectl get secrets -n tilt-integration -o json | jq -r '.items[] | select(.metadata.name | startswith("tilt-integration-user-token-")) | .data["ca.crt"]'`


if this kubectl command takes longer than a few hundred ms, might make sense to run the kubectl get secrets bit only once and run the output of that through jq -- i don't want to add multiple seconds of overhead to every integration test if we can help it

This code is only called if you enable restricted credentials in the test, so it's opt in.

ah gotcha 👍

maiamcc · 2019-05-29T16:22:27Z

+func (f *k8sFixture) SetRestrictedCredentials() {
+	f.getSecrets()
+
+	err := ioutil.WriteFile("/tmp/cert", []byte(f.cert), 0644)


worth a note about why we have to write it to disk instead of passing it to the cmd

...also it makes me a little sad that we're doing this unnecessarily for local integration tests, but 🤷‍♀

nicks

Have you considered making all the namespace setup into a separate reusable script, instead of baking it into the integration test fixture?

I expect we'll want to do this in other places. And other teams will want to use it for their tiltfile tests

jazzdan · 2019-05-29T17:09:32Z

@nicks I didn't, but I think that's a good idea!

nicks · 2019-05-29T18:56:29Z

+		f.t.Fatalf("Error writing cert: %v", err)
+	}
+	outWriter := bytes.NewBuffer(nil)
+	cmd := exec.CommandContext(f.ctx, "kubectl", "config", "set-credentials", "tilt-integration-user", "--client-certificate=/tmp/cert", fmt.Sprintf("--token=%s", f.token))


modifying the user's kubeconfig seems very dangerous and likely to break something (e.g., if the user ctrl-c's a test)

have you considered setting the KUBECONFIG env var to use a custom kubeconfig?

Yeah I think that would be a better approach and once I get this working at all I will do it that way instead.

nicks · 2019-05-29T18:57:12Z

+func (f *k8sFixture) SetRestrictedCredentials() {
+	f.getSecrets()
+
+	err := ioutil.WriteFile("/tmp/cert", []byte(f.cert), 0644)


are you aware of client-certificate-data so you can embed the data in the kubeconfig file?

Yes, but you can't specify that via kubectl config set-credentials and I'm trying to avoid having to parse/write a kubeconfig.

I don't think I can avoid it though, tragically.

have you tried kubectl config set? its instructions explicitly mention certificate-authority-data

Aha, no! Thank you for the tip

jazzdan · 2019-05-29T20:35:06Z

 		o.LabelSelector = ls.String()
 	}))

+	logger.Get(ctx).Infof("DERP namespace: %s", ns)


ignore this for now 😬

jazzdan · 2019-05-29T20:55:41Z

This is still very much a work in progress, but it should be much different than the last time you looked at it @nicks @maiamcc, but I have to head out for the day.

maiamcc

ooh this IS much better! 🙌

maiamcc · 2019-05-29T21:37:22Z

 func (f *k8sFixture) CreateNamespaceIfNecessary() {
 	outWriter := bytes.NewBuffer(nil)
 	cmd := exec.CommandContext(f.ctx, "kubectl", "apply", "-f", "namespace.yaml")
+	if f.usingOverriddenConfig {


instead of having to remember this every time you make a new Command object, consider an f.CommandContext(...) method that does this automatically (and maybe even passes the ctx for you). Otherwise, seems like a recipe of someone invoking a command wrong and breaking stuff in mysterious ways

maiamcc · 2019-05-29T21:38:30Z

+		f.t.Fatalf("Error getting secrets: %v. Cmd: %s", err, cmdStr)
+	}
+
+	cmdStr = `kubectl get secrets -n tilt-integration -o json | jq -r '.items[] | select(.metadata.name | startswith("tilt-integration-user-token-")) | .data["ca.crt"]'`


ah gotcha 👍

This reverts commit 859f223.

…that just set up the environment

This reverts commit abb5008.

jazzdan marked this pull request as ready for review May 29, 2019 15:50

jazzdan requested review from maiamcc and nicks May 29, 2019 15:51

maiamcc approved these changes May 29, 2019

View reviewed changes

nicks reviewed May 29, 2019

View reviewed changes

jazzdan commented May 29, 2019

View reviewed changes

Dan Miller added 9 commits May 29, 2019 16:47

integration: use restricted RBAC credentials for some tests

ed44883

use new user

99496a4

make jq a requirement

0973374

make another function private

565e619

add comment

518fcfb

wip

d135c98

more tweaks

7cb64a8

refactor to change KUBECONFIG, refactor in to functions

ef741bb

teardown tempdir fixture

2bf4f1c

jazzdan force-pushed the dmiller/ch2595/add-integration-test-for-cluster-w-namespace branch from 531d417 to 2bf4f1c Compare May 29, 2019 20:48

Dan Miller added 2 commits May 29, 2019 16:49

remove log statement

2a486c5

Set KUBECONFIG for Tilt commands

b73e785

maiamcc approved these changes May 29, 2019

View reviewed changes

nicks approved these changes May 30, 2019

View reviewed changes

jazzdan force-pushed the dmiller/ch2595/add-integration-test-for-cluster-w-namespace branch 2 times, most recently from 233ff6f to b73e785 Compare May 30, 2019 15:16

Dan Miller added 4 commits May 30, 2019 11:21

kick off new ci job

6c3b480

temp enable new watching code to see if test breaks

859f223

base64 decode token before using it

9330ad7

Revert "temp enable new watching code to see if test breaks"

abb5008

This reverts commit 859f223.

Dan Miller added 3 commits May 30, 2019 16:32

We actually don't need to set the kubeconfig for these kubectl calls …

51173f2

…that just set up the environment

Revert "Revert "temp enable new watching code to see if test breaks""

ea16156

This reverts commit abb5008.

use a cluster role binding so that we can read node config

5699964

jazzdan merged commit 0952f54 into master May 31, 2019

nicks deleted the dmiller/ch2595/add-integration-test-for-cluster-w-namespace branch February 3, 2022 16:53

Uh oh!

Conversation

jazzdan commented May 29, 2019

Uh oh!

jazzdan commented May 29, 2019

Uh oh!

maiamcc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicks left a comment

Choose a reason for hiding this comment

Uh oh!

jazzdan commented May 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jazzdan commented May 29, 2019

Uh oh!

maiamcc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants