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.

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

ci-k8sio-cip: do not use secret #16883

Merged
merged 1 commit into from
Mar 23, 2020
Merged

Conversation

listx
Copy link
Contributor

@listx listx commented Mar 20, 2020

Ever since kubernetes/k8s.io#655, this job
should already run with an authenticated GCP SA (before the job's code
even runs). This means there is no need to use the keyfile to
authenticate (again), as it is redundant.

We still keep the -use-service-account flag for now because the
promoter currently still tries to extract OAUTH tokens as the GCP
account at the very beginning of its run. It's a nice sanity check.

Once we confirm that the OAUTH tokens do still get populated in the
business logic of the job, we don't need to keep this flag any more
because the GCRs that the promoter deals with today are all public (the
promoter was designed to deal with private repos), and so the very
first "read GCR" API calls do not need the OAUTH tokens for just reading
the repos. That takes care of the GCR read API calls.

As for the GCR write calls, the gcrane.Copy() method that perform the
GCR writes do their own authentication dance, but that is separate and
automatic as long as the GCP account is authenticated before that
function call.

This is why the -use-service-account flag is marked for future
deprecation in this commit.

Ever since kubernetes/k8s.io#655, this job
should already run with an authenticated GCP SA (before the job's code
even runs). This means there is no need to use the keyfile to
authenticate (again), as it is redundant.

We still keep the `-use-service-account` flag for now because the
promoter currently still tries to extract OAUTH tokens as the GCP
account at the very beginning of its run. It's a nice sanity check.

Once we confirm that the OAUTH tokens do still get populated in the
business logic of the job, we don't need to keep this flag any more
because the GCRs that the promoter deals with today are all public (the
promoter was designed to deal with private repos), and so the very
first "read GCR" API calls do not need the OAUTH tokens for just reading
the repos. That takes care of the GCR read API calls.

As for the GCR write calls, the gcrane.Copy() method that perform the
GCR writes do their own authentication dance, but that is separate and
automatic as long as the GCP account is authenticated before that
function call.

This is why the `-use-service-account` flag is marked for future
deprecation in this commit.
@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 Mar 20, 2020
@k8s-ci-robot k8s-ci-robot added area/config Issues or PRs related to code in /config sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Mar 20, 2020
@listx
Copy link
Contributor Author

listx commented Mar 20, 2020

/cc @fejta @thockin

Copy link
Contributor

@fejta fejta left a comment

Choose a reason for hiding this comment

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

/hold
Will take effect on merge
/hold cancel when you're ready for this to occur.

@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 23, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 23, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2020
@listx
Copy link
Contributor Author

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

listx commented Mar 23, 2020

Thanks!

@k8s-ci-robot k8s-ci-robot merged commit fc49fdc into kubernetes:master Mar 23, 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:

Ever since kubernetes/k8s.io#655, this job
should already run with an authenticated GCP SA (before the job's code
even runs). This means there is no need to use the keyfile to
authenticate (again), as it is redundant.

We still keep the -use-service-account flag for now because the
promoter currently still tries to extract OAUTH tokens as the GCP
account at the very beginning of its run. It's a nice sanity check.

Once we confirm that the OAUTH tokens do still get populated in the
business logic of the job, we don't need to keep this flag any more
because the GCRs that the promoter deals with today are all public (the
promoter was designed to deal with private repos), and so the very
first "read GCR" API calls do not need the OAUTH tokens for just reading
the repos. That takes care of the GCR read API calls.

As for the GCR write calls, the gcrane.Copy() method that perform the
GCR writes do their own authentication dance, but that is separate and
automatic as long as the GCP account is authenticated before that
function call.

This is why the -use-service-account flag is marked for future
deprecation in this commit.

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
Copy link
Contributor Author

listx commented Mar 24, 2020

The job is now failing with:

*looking at "/home/prow/go/src/github.com/kubernetes/k8s.io/k8s.gcr.io"
E0323 23:16:47.862361      12 token.go:57] could not execute cmd /usr/bin/gcloud auth --account=k8s-infra-gcr-promoter@k8s-artifacts-prod.iam.gserviceaccount.com print-access-token
E0323 23:16:47.862477      12 inventory.go:1141] could not get service account token for k8s-infra-gcr-promoter@k8s-artifacts-prod.iam.gserviceaccount.com
F0323 23:16:47.862495      12 cip.go:238] exit status 1

I've checked the k8s-artifacts-prod side of IAM configs and it appears that it was correctly actuated. So the settings look fine.

I thought about this a liitle more and I think this error is happnening because I forgot to define a serviceAccountName here for this job. Oops!

In my case it should be k8s-artifacts-prod because this job is already in the test-pods K8s namespace, as defined in kubernetes/k8s.io#655.

PR incoming...!

listx pushed a commit to listx/test-infra that referenced this pull request Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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 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
Development

Successfully merging this pull request may close these issues.

3 participants