Skip to content

Conversation

saschagrunert
Copy link
Member

This is needed to run changelog e2e/integration tests within the CI.

Needed by: #1023

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 20, 2020
@k8s-ci-robot k8s-ci-robot added area/release-eng Issues or PRs related to the Release Engineering subproject sig/release Categorizes an issue or PR as relevant to SIG Release. labels Jan 20, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: saschagrunert
To complete the pull request process, please assign tpepper
You can assign the PR to them by writing /assign @tpepper in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@justaugustus
Copy link
Member

/hold
What's the intent here?
Seems like we're getting ready to build a token into a public image that has write access to our repos...

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2020
@saschagrunert
Copy link
Member Author

/hold
What's the intent here?
Seems like we're getting ready to build a token into a public image that has write access to our repos...

Ah yes 🙈, I did not take into consideration that the token has write access to the repo. We would finally need a new token with read-only access to k/k (like every user has). Let me prepare the PR accordingly...

@saschagrunert saschagrunert changed the title Add GITHUB_TOKEN to releng-ci-bazel container image WIP: Add GITHUB_TOKEN to releng-ci-bazel container image Jan 20, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 20, 2020
This is needed to run changelog e2e/integration tests within the CI.

Signed-off-by: Sascha Grunert <[email protected]>
secrets:
- kmsKeyName: projects/kubernetes-release-test/locations/global/keyRings/anago/cryptoKeys/k8s-release-robot
secretEnv:
GITHUB_TOKEN: <TOKEN>
Copy link
Member Author

Choose a reason for hiding this comment

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

@justaugustus I think we would need a new token here, which has read-only access to k/k for parsing the release notes from it.

@hoegaarden
Copy link
Contributor

I think there is some confusion:

The token here would only be available when building the image, i.e. while docker build runs. We don't need and want the token then.

For #1023, we'd want the token at the runtime of the tests.

However, and I know I repeat myself, I don't think we should implement any tool doing things like this (e.g. git pushing) if we can avoid doing so, but rather have GCB do it. I think a tool should ideally prepare the git repo locally and hand off to another GCB task which does the pushing (or pulling or whatever interaction with the outside world). Skimming the code really quick, I think specifically for #1023 that is straight forward. This would also mean, we get one step closer to not running random release steps on a devs laptop (which we have no control or idea about).

@saschagrunert
Copy link
Member Author

I think there is some confusion:

The token here would only be available when building the image, i.e. while docker build runs. We don't need and want the token then.

For #1023, we'd want the token at the runtime of the tests.

However, and I know I repeat myself, I don't think we should implement any tool doing things like this (e.g. git pushing) if we can avoid doing so, but rather have GCB do it.

We need this token primarily to generate the release notes, not for pushing content to any remote.

I think a tool should ideally prepare the git repo locally and hand off to another GCB task which does the pushing (or pulling or whatever interaction with the outside world). Skimming the code really quick, I think specifically for #1023 that is straight forward. This would also mean, we get one step closer to not running random release steps on a devs laptop (which we have no control or idea about).

Okay I see. I will follow up on another PR where we can continue the discussion.

@hoegaarden
Copy link
Contributor

hoegaarden commented Jan 22, 2020

We need this token primarily to generate the release notes, not for pushing content to any remote.

If we need a token (for e2e testing or anything else), we could (there are other options, maybe better ones):

  • create a test cloudbuilder conf, e.g. .../gcb/test/cloudbuild.yaml) which
    • checks out k/release (to the PR commit, is that possible?)
    • runs the tests, e.g. bazel test or go test ./...
    • has a secret (a different one then the one we use for the stage/release process!)
    • exposes that secret again via secretEnv in the GCB task
  • leverage the prow builder thing to kick that test off as a pre-submit

For this, I think we'd want to have a different kms keyring / secret which is manageable by non-SIG-chairs, for easier management. I am not sure how feasible that all is, considering that we have multiple GCP projects and there is no automation for that (AFAIK) available (yet), and I think we'd want to make sure we have the projects in sync. (If that's already there, yiihhaa, I'd love to introduce a new secret to play with sendgrid or similar).

However, even if it has one more "indirection" I am starting to like the prow -> builder -> GCB pre-submit, I think it could allow us to manage pretty much everything (esp. secrets included) ourselves, given we get the access stuff right on all GCP projects involved.

Alternatively, there is a way to store secrets in prow, but I am not sure how that exactly works. IIRC someone from test-infra creates a k8s secret which we then can use in our prow jobs? 🤷 Other folks (@justaugustus, @calebamiles, and others) know way more about that.

Does this make sense? What's y'all's preference?

  • Manage the secrets in GCP and do the pre-submit via prow -> builder -> GCB
  • Manage secrets in the prow cluster and run the tests directly on prow

@justaugustus
Copy link
Member

justaugustus commented Jan 27, 2020

@saschagrunert -- Apologies for dropping a hold on this and not getting back to you with something more detailed.

That said, I agree with @hoegaarden's assessment.

If we need a token (for e2e testing or anything else), we could (there are other options, maybe better ones):

  • create a test cloudbuilder conf, e.g. .../gcb/test/cloudbuild.yaml) which

Yep, this could be a variant in the variants.yaml for a stage in the release flow.

  • checks out k/release (to the PR commit, is that possible?)

Yep, there's a pull URI/branch that we can use (the exact form escapes me at the moment).

For this, I think we'd want to have a different kms keyring / secret which is manageable by non-SIG-chairs, for easier management. I am not sure how feasible that all is, considering that we have multiple GCP projects and there is no automation for that (AFAIK) available (yet), and I think we'd want to make sure we have the projects in sync. (If that's already there, yiihhaa, I'd love to introduce a new secret to play with sendgrid or similar).

Agreed that we want to minimize the amount of creds we have to manage.
I'm hesitant to introduce too much more into the mix while we work to migrate the release process from Google-owned to K8s-owned GCP projects.

We could also potentially use the new Secret Manager (https://cloud.google.com/secret-manager/docs), though I haven't had a chance to play around with that yet.

However, even if it has one more "indirection" I am starting to like the prow -> builder -> GCB pre-submit, I think it could allow us to manage pretty much everything (esp. secrets included) ourselves, given we get the access stuff right on all GCP projects involved.

Glad the flow sounds good to you :)

Alternatively, there is a way to store secrets in prow, but I am not sure how that exactly works. IIRC someone from test-infra creates a k8s secret which we then can use in our prow jobs? 🤷 Other folks (@justaugustus, @calebamiles, and others) know way more about that.

Does this make sense? What's y'all's preference?

  • Manage the secrets in GCP and do the pre-submit via prow -> builder -> GCB
  • Manage secrets in the prow cluster and run the tests directly on prow

I think we should start with managing the secret in Prow, since it's what we've got today.
We can work towards getting this into the GCB builder later on.


Going back to the /hold, having the GitHub token in the environment is not required; it should only be needed in the environment for actually running the tool (which doesn't need to happen in the container build steps, as Hannes mentioned).

What I'd like to see additionally (separate PR) is the removal of any flags within our tools which allow for passing tokens via command line.

As we output logs in GCB, Prow, and elsewhere (pasting logs to debug in issues/Slack), having sensitive creds on the command line open us up to accidentally exposure (mentioned here as well).

We should get sensitive information into the environment ahead of the command's execution (pass by env variable, environment file).

@saschagrunert
Copy link
Member Author

Thank you for all the feedback regarding this change. In the long term we need to have a GITHUB_TOKEN for real integration testing anyway, but for now lets close this one since the changes are out of scope. Let's follow-up on #1023

@saschagrunert saschagrunert deleted the github-token branch January 27, 2020 08:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/release-eng Issues or PRs related to the Release Engineering subproject cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. sig/release Categorizes an issue or PR as relevant to SIG Release. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants