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

tests: Adds e2e-test-auth-img project #1929

Closed

Conversation

claudiubelu
Copy link
Contributor

@claudiubelu claudiubelu commented Apr 14, 2021

The gcr.io/k8s-staging-e2e-test-auth-img and k8s.gcr.io/kubernetes-e2e-test-auth-img registries are meant to be private, to be used in a few E2E test scenarios, and it is meant to replace the gcr.io/authenticated-image-pulling.

An auth token with read-only access will have to be generated afterwards, and then baked into the E2E tests.

Related: #1459
Related: #1528

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Apr 14, 2021
@k8s-ci-robot k8s-ci-robot requested review from dims and nikhita April 14, 2021 15:24
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/access Define who has access to what via IAM bindings, role bindings, policy, etc. area/artifacts Issues or PRs related to the hosting of release artifacts for subprojects wg/k8s-infra labels Apr 14, 2021
@claudiubelu claudiubelu force-pushed the adds-e2e-test-auth-images branch from ef90d41 to c63b2a4 Compare April 14, 2021 15:25
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 14, 2021
@claudiubelu claudiubelu force-pushed the adds-e2e-test-auth-images branch from c63b2a4 to 31ab047 Compare April 16, 2021 12:47
@claudiubelu claudiubelu changed the title tests: Adds e2e-test-auth-images project tests: Adds e2e-test-auth-img project Apr 16, 2021
@claudiubelu claudiubelu force-pushed the adds-e2e-test-auth-images branch from 31ab047 to b54948d Compare April 16, 2021 13:26
@ameukam
Copy link
Member

ameukam commented Apr 16, 2021

@claudiubelu Please add a comment in the images.yaml file as suggested: https://github.com/kubernetes/k8s.io/tree/main/k8s.gcr.io#creating-staging-repos.

@claudiubelu claudiubelu force-pushed the adds-e2e-test-auth-images branch from b54948d to f941a0e Compare April 16, 2021 14:25
@claudiubelu
Copy link
Contributor Author

@claudiubelu Please add a comment in the images.yaml file as suggested: https://github.com/kubernetes/k8s.io/tree/main/k8s.gcr.io#creating-staging-repos.

Ty!

@ameukam
Copy link
Member

ameukam commented Apr 16, 2021

/lgtm
/hold
/assign @spiffxp @BenTheElder

@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 Apr 16, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 2021
@spiffxp
Copy link
Member

spiffxp commented May 12, 2021

I have this sneaking suspicion we can't get away with k8s.gcr.io/kubernetes-e2e-test-auth-img being private, while the rest of k8s.gcr.io is public.

EDIT: correct, we cannot accomplish what the description is asking for

ref: https://cloud.google.com/container-registry/docs/access-control#public

Within a project's host, it is not possible to publicly serve only specific images. If you have specific images you want to make public:

  • Take care to keep them in a separate host location which you make public, or
  • Create a new project to hold a publicly accessible images.

@spiffxp
Copy link
Member

spiffxp commented May 12, 2021

I also feel like "should the tests that use this even exist?" remains unanswered: kubernetes/kubernetes#97026 (comment)

ok that comment is less relevant, this PR is for authenticated-image-pulling, into which I have not dug deeply

Copy link
Member

@spiffxp spiffxp left a comment

Choose a reason for hiding this comment

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

An auth token with read-only access will have to be generated afterwards

Can you describe that a little bit more? I feel like that should be part of this PR. I suspect we can get away with either copying or refactoring what we use in ensure-conformance.sh to at least ensure the service account that will be authed is present:

# ensure_conformance_serviceaccount ensures that:
# - a serviceaccount of the given name exists in PROJECT
# - it can write to the given bucket
# - it has a private key stored in a secret in PROJECT accessible to the given group
function ensure_conformance_serviceaccount() {
local name="${1}"
local bucket="${2}"
local secret_accessors="${3}"
local email="$(svc_acct_email "${PROJECT}" "${name}")"
local secret="${name}-key"
local private_key_file="${TMPDIR}/key.json"
color 6 "Ensuring service account exists: ${email}"
ensure_service_account "${PROJECT}" "${name}" "Grants write access to ${bucket}"
color 6 "Ensuring ${PROJECT} contains secret ${secret} with private key for ${email}"
ensure_serviceaccount_key_secret "${PROJECT}" "${secret}" "${email}"
color 6 "Empowering ${secret_accessors} to access secret: ${secret}"
ensure_secret_role_binding \
"projects/${PROJECT}/secrets/${secret}" \
"group:${secret_accessors}" \
"roles/secretmanager.secretAccessor"
color 6 "Empowering ${email} to write to ${bucket}"
empower_svcacct_to_write_gcs_bucket "${email}" "${bucket}"
}

Comment on lines 130 to 70
PRIVATE_PROJECTS=(
e2e-test-auth-img
)

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is a pattern we want to encourage more of?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2021
@spiffxp
Copy link
Member

spiffxp commented May 13, 2021

/cc @thockin @listx @hh @saschagrunert @jeremyrickard
Given that we can't use k8s.gcr.io for this, I have a couple questions/thoughts:

  • should we be thinking of promoting to a private repo, or should we just use a one-off private repo? if the latter, should we drop the "k8s-staging" from the name for this one-off repo?
  • should we be thinking about cross-cloud private artifacts for registry.k8s.io? or do we just decide to take the hit and have the world hit this one private repo in GCR whenever they run their kubernetes e2e tests?

@k8s-ci-robot k8s-ci-robot requested review from hh and jeremyrickard May 13, 2021 00:03
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 23, 2021
@claudiubelu claudiubelu force-pushed the adds-e2e-test-auth-images branch from 1056d82 to 1d4284f Compare June 23, 2021 16:31
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: claudiubelu
To complete the pull request process, please ask for approval from spiffxp after the PR has been reviewed.

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

@k8s-ci-robot k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 23, 2021
@ameukam
Copy link
Member

ameukam commented Jun 23, 2021

/lgtm
gentle ping @BenTheElder @spiffxp

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2021
Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

conceptually seems OK but not comfortable approving the whole thing myself given prior debate about how this should be done / named / ...

groups/sig-testing/groups.yaml Show resolved Hide resolved
The gcr.io/k8s-staging-e2e-test-auth-img and gcr.io/kubernetes-e2e-test-auth-img
registries are meant to be private, to be used in a few E2E test scenarios, and it is
meant to replace the gcr.io/authenticated-image-pulling.

An auth token with read-only access will have to be generated afterwards, and then
baked into the E2E tests.
@claudiubelu claudiubelu force-pushed the adds-e2e-test-auth-images branch from 1d4284f to b23a500 Compare July 15, 2021 11:28
@k8s-ci-robot
Copy link
Contributor

New changes are detected. LGTM label has been removed.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 15, 2021
@claudiubelu
Copy link
Contributor Author

So, the question remains: can this be made private? If so, how? It seems that kubernetes/k8s.io changed since this was initially proposed, so this just simply creates a new project, it's no longer trying to make it private at the moment.

@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 30, 2021
@k8s-ci-robot k8s-ci-robot added sig/k8s-infra Categorizes an issue or PR as relevant to SIG K8s Infra. and removed wg/k8s-infra labels Sep 29, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 28, 2021
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 27, 2022
@BenTheElder
Copy link
Member

This was always done on a separate registry, and it was always a GCR registry.

I don't think the purpose of the test is to test cloud providers at all, just the flow of an image that requires credentials.

I don't see a reasonable better alternative proposed here, other than perhaps giving up on the test ...

I'm going to drop a link in the SIG slack, I think this PR deserved a follow-up answering @claudiubelu's questions.

@BenTheElder
Copy link
Member

should we be thinking of promoting to a private repo, or should we just use a one-off private repo? if the latter, should we drop the "k8s-staging" from the name for this one-off repo?

IMO: yes. we should give it a one-off, more descriptive name if we do this.

should we be thinking about cross-cloud private artifacts for registry.k8s.io? or do we just decide to take the hit and have the world hit this one private repo in GCR whenever they run their kubernetes e2e tests?

Proposing one alternative (maybe) ... can we rely on running a simple authenticated registry in-cluster as part of the test? And just not serve a public endpoint for this purpose at all. We could probably bake up a docker image that is an authenticated registry with some image pre-pushed to it, the question is mostly of if we can run an in-cluster registry sufficiently portably for test purposes. I think the answer might be "no, we can't" (TLS? CRI <-> pod reachability?), but perhaps worth exploring.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/access Define who has access to what via IAM bindings, role bindings, policy, etc. area/artifacts Issues or PRs related to the hosting of release artifacts for subprojects area/audit Audit of project resources, audit followup issues, code in audit/ 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. lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. sig/k8s-infra Categorizes an issue or PR as relevant to SIG K8s Infra. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants