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.

Sign up for GitHub

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

Jump to bottom

ci-k8sio-cip: use k8s service account #16917

Merged
merged 2 commits into from
Mar 24, 2020
Merged

ci-k8sio-cip: use k8s service account #16917

merged 2 commits into from
Mar 24, 2020

Conversation

listx
Copy link
Contributor

@listx listx commented Mar 24, 2020 •
edited
Loading

This is a follow-up to kubernetes/k8s.io#655 and
#16883.

/cc @fejta @thockin

Aside: I think the KSA name "k8s-artifacts-prod" is bad (too generic) and should be changed in the future, but let's do that later when we need to disambiguate.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/config Issues or PRs related to code in /config sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Mar 24, 2020
@listx listx mentioned this pull request Mar 24, 2020
Closed
@spiffxp
Copy link
Member

spiffxp commented Mar 24, 2020

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 24, 2020
@spiffxp
Copy link
Member

spiffxp commented Mar 24, 2020 •
edited
Loading

/hold
You'll need to update the PR to add the service account here https://github.com/kubernetes/test-infra/blob/master/prow/cluster/trusted_serviceaccounts.yaml

@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 Mar 24, 2020
@listx
Copy link
Contributor Author

listx commented Mar 24, 2020

@fejta: @spiffxp added a hold for now because it's not clear if the k8s-artifacts-prod KSA exists yet in the test-pods namespace, and he does not have access to create one.

Can you remove hold if you can confirm that one has been created (or is this step not necessary)? Thanks.

@fejta
Copy link
Contributor

fejta commented Mar 24, 2020 •
edited
Loading

These are self service, we apply what is declared on merge:

test-infra/prow/cluster/BUILD.bazel

Lines 63 to 73 in 0132e79

component("trusted_serviceaccounts", MULTI_KIND),
component(
"tune-sysctls",
"daemonset",
cluster = BUILD_CLUSTER,
),
component(
"build_serviceaccounts",
MULTI_KIND,
cluster = BUILD_CLUSTER,
),

As Aaron says, if you want a service account with that name to exist, you need to declare it (in the correct cluster where the job runs):

https://github.com/kubernetes/test-infra/blob/master/prow/cluster/trusted_serviceaccounts.yaml
https://github.com/kubernetes/test-infra/blob/master/prow/cluster/build_serviceaccounts.yaml

activate "k8s-artifacts-prod" KSA
47d48ba 
This follows the instructions posted at kubernetes#16917 (comment).
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/prow Issues or PRs related to prow area/prow/bump Updates to the k8s prow cluster and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 24, 2020
fejta
fejta approved these changes Mar 24, 2020
View reviewed changes
prow/cluster/trusted_serviceaccounts.yaml
apiVersion: v1
metadata:
annotations:
iam.gke.io/gcp-service-account: k8s-infra-gcr-promoter@k8s-artifacts-prod.iam.gserviceaccount.com
Copy link
Contributor

@fejta fejta Mar 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Note that you'll still need to run the gcloud command to authorize k8s-artifacts-prod's ability to authenticate as k8s-infra-gcr-promoter@k8s-artifacts-prod.iam.gserviceaccount.com to GCP.

https://github.com/kubernetes/test-infra/tree/master/workload-identity

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has already been done by @thockin; I've indepently verified that there is a "Workload Identity" role added to the k8s-infra-gcr-promoter@k8s-artifacts-prod.iam.gserviceaccount.com account from the IAM web ui for the k8s-artifacts-prod project which owns that GCP SA, so I think we're ready for merge.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 24, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, listx, spiffxp

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:

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

@listx
Copy link
Contributor Author

listx commented Mar 24, 2020

These are self service, we apply what is declared on merge:

test-infra/prow/cluster/BUILD.bazel

Lines 63 to 73 in 0132e79

component("trusted_serviceaccounts", MULTI_KIND),
component(
"tune-sysctls",
"daemonset",
cluster = BUILD_CLUSTER,
),
component(
"build_serviceaccounts",
MULTI_KIND,
cluster = BUILD_CLUSTER,
),

As Aaron says, if you want a service account with that name to exist, you need to declare it (in the correct cluster where the job runs):

https://github.com/kubernetes/test-infra/blob/master/prow/cluster/trusted_serviceaccounts.yaml
https://github.com/kubernetes/test-infra/blob/master/prow/cluster/build_serviceaccounts.yaml

ACK. ISTR these files before when you were first doing this stuff earlier this year Erick, thanks for the reminder.

@listx
Copy link
Contributor Author

listx commented Mar 24, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 24, 2020
@k8s-ci-robot k8s-ci-robot merged commit 377fa07 into kubernetes:master Mar 24, 2020
@k8s-ci-robot
Copy link
Contributor

@listx: Updated the job-config configmap in namespace default at cluster default using the following files:

  • key test-infra-trusted.yaml using file config/jobs/kubernetes/test-infra/test-infra-trusted.yaml

In response to this:

This is a follow-up to kubernetes/k8s.io#655 and
#16883.

/cc @fejta @thockin

Aside: I think the KSA name "k8s-artifacts-prod" is bad (too generic) and should be changed in the future, but let's do that later when we need to disambiguate.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

listx pushed a commit to listx/k8s.io that referenced this pull request Mar 25, 2020
switch Workload Identity grant to Prow's trusted cluster
cc0457c 
This is a followup to kubernetes/test-infra#16917.

The problem is that there are 2 completely separate Prow clusters, one
called k8s-prow-builds, which runs random presubmits from the public and
k8s-prow, which only runs trusted jobs. The ci-k8sio-cip job runs in the
trusted cluster, and so the Workload Identity grant must be for
"k8s-prow", not "k8s-prow-builds".

The incorrect grant to
"k8s-prow-builds.svc.id.goog[test-pods/k8s-artifacts-prod]" must
manually be removed.
@listx listx mentioned this pull request Mar 25, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fejta fejta fejta approved these changes

@thockin thockin Awaiting requested review from thockin

Assignees

@spiffxp spiffxp

@fejta fejta

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/config Issues or PRs related to code in /config area/prow/bump Updates to the k8s prow cluster area/prow Issues or PRs related to prow cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@listx @spiffxp @fejta @k8s-ci-robot