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

Add variables needed to access prow-build-cluster on EKS #28951

Merged
merged 2 commits into from
Mar 9, 2023
Merged

Add variables needed to access prow-build-cluster on EKS #28951

merged 2 commits into from
Mar 9, 2023

Conversation

xmudrii
Copy link
Member

@xmudrii xmudrii commented Mar 7, 2023

This PR adds environment variables, volume mounts, and volumes to the needed Prow components to access the prow-build-cluster on EKS.

Part of kubernetes/k8s.io#4686

/assign @upodroid @BenTheElder @ameukam @dims

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/config Issues or PRs related to code in /config area/prow/bump Updates to the k8s prow cluster sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Mar 7, 2023
@dims
Copy link
Member

dims commented Mar 7, 2023

/lgtm
/approve
/hold until you are ready @xmudrii

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. 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 7, 2023
upodroid
upodroid approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@upodroid upodroid left a comment

Choose a reason for hiding this comment

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

LGTM

@upodroid
Copy link
Member

upodroid commented Mar 7, 2023

This change has no effect till a kubeconfig that is accessing an EKS cluster is loaded

@ameukam
Copy link
Member

ameukam commented Mar 7, 2023

FYI @cjwagner @listx
Probably get feedback from them before we remove the hold.

TerryHowe
TerryHowe reviewed Mar 7, 2023
View reviewed changes
config/prow/cluster/deck_deployment.yaml
@@ -64,6 +64,13 @@ spec:
# Use KUBECONFIG envvar rather than --kubeconfig flag in order to provide multiple configs to merge.
- name: KUBECONFIG
value: "/etc/kubeconfig/config:/etc/kubeconfig-build-test-infra-trusted/kubeconfig:/etc/kubeconfig-build-k8s-prow-builds/kubeconfig:/etc/kubeconfig-build-rules-k8s/kubeconfig"
# AWS_ variables needed to assume role to access the prow-build-cluster EKS cluster.
- name: AWS_ROLE_ARN
value: arn:aws:iam::468814281478:role/Prow-EKS-Admin
Copy link
Member

Choose a reason for hiding this comment

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

Not for me to judge what is acceptable here, but seems like this could be locked down a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you mean not sharing it publicly, that would be more complicated and we also share ARNs for other stuff for some other jobs and components, so it shouldn't be an issue.

Copy link
Member

Choose a reason for hiding this comment

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

I'd have to dig through the code to see what permissions Prow-EKS-Admin has, but it just seems like deck, hook, etc might not need full root typer permissions. If that has been working in the past, it probably isn't an issue though.

Copy link
Member

Choose a reason for hiding this comment

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

The tokens used by Prow to access build clusters have always been a service account bound to the cluster-admin role(I agree it is overprivileged).

I have configured it to do the same thing, that role will have cluster-admin permissions.

https://docs.prow.k8s.io/docs/build-clusters/
https://docs.prow.k8s.io/docs/getting-started-deploy/#run-test-pods-in-different-clusters

@listx
Copy link
Contributor

listx commented Mar 7, 2023

I am not familiar with AWS so maybe @TerryHowe comment needs addressing (although I do not know what is meant by locking down these values).

/lgtm
/cc @chases2 who is oncall

@k8s-ci-robot
Copy link
Contributor

@listx: GitHub didn't allow me to request PR reviews from the following users: is, oncall, who.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

I am not familiar with AWS so maybe @TerryHowe comment needs addressing (although I do not know what is meant by locking down these values).

/lgtm
/cc @chases2 who is oncall

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.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2023
@ameukam
Copy link
Member

ameukam commented Mar 8, 2023

+1

Deferring LGTM to @chases2.

cjwagner
cjwagner approved these changes Mar 9, 2023
View reviewed changes
Copy link
Member

@cjwagner cjwagner left a comment

Choose a reason for hiding this comment

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

Looks ok. I'll unhold tomorrow.
/hold

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, dims, upodroid, xmudrii

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

@cjwagner
Copy link
Member

cjwagner commented Mar 9, 2023

/unhold

@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 9, 2023
@k8s-ci-robot k8s-ci-robot merged commit a428370 into kubernetes:master Mar 9, 2023
@xmudrii xmudrii deleted the prow-eks branch March 9, 2023 19:41
@TerryHowe TerryHowe mentioned this pull request Mar 31, 2023
Closed
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@upodroid upodroid upodroid approved these changes

@TerryHowe TerryHowe TerryHowe left review comments

@cjwagner cjwagner cjwagner approved these changes

@cblecker cblecker Awaiting requested review from cblecker

@dims dims Awaiting requested review from dims

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

@chases2 chases2 Awaiting requested review from chases2

Assignees

@dims dims

@listx listx

@BenTheElder BenTheElder

@ameukam ameukam

@cjwagner cjwagner

@upodroid upodroid

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 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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@xmudrii @dims @upodroid @ameukam @listx @k8s-ci-robot @cjwagner @TerryHowe @BenTheElder