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

Deploy statusreconciler for prow.k8s.io #12258

Merged
merged 1 commit into from
Apr 23, 2019
Merged

Deploy statusreconciler for prow.k8s.io #12258

merged 1 commit into from
Apr 23, 2019

Conversation

stevekuznetsov
Copy link
Contributor

Signed-off-by: Steve Kuznetsov skuznets@redhat.com

/assign @cjwagner @fejta

@stevekuznetsov
Deploy statusreconciler for prow.k8s.io
349be1b 
Signed-off-by: Steve Kuznetsov <skuznets@redhat.com>
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/prow Issues or PRs related to prow size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/prow/bump Updates to the k8s prow cluster sig/testing Categorizes an issue or PR as relevant to SIG Testing. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 18, 2019
@k8s-ci-robot k8s-ci-robot requested review from amwat and ixdy April 18, 2019 15:19
stevekuznetsov
stevekuznetsov commented Apr 18, 2019
View reviewed changes
prow/cluster/BUILD.bazel
@@ -32,6 +32,7 @@ release(
component("prowjob", "customresourcedefinition"),
component("pushgateway", "deployment"),
component("sinker", "deployment"),
component("statusreconciler", "deployment"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see build and crier use RBAC -- I don't fully understand when we can and cannot -- should I use it for status-reconciler too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would assume we can everywhere so long as it's properly configured?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjwagner had some comments about turning off legacy authorization on the cluster?

Copy link
Member

Choose a reason for hiding this comment

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

I think our cluster is ignoring RBAC because legacy authorization is enabled and I think it needs to be enabled until we figure out how to configure RBAC correctly for plank, deck, and sinker to be authenticated and authorized to create/list/delete pods in separate build clusters. I'm not entirely certain about either of those points though.

prow/cluster/statusreconciler_deployment.yaml
ports:
- name: http
containerPort: 8888
- --blacklist=kubernetes/kubernetes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI

fejta
fejta approved these changes Apr 18, 2019
View reviewed changes
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

prow/cluster/starter.yaml
kind: ServiceAccount
metadata:
namespace: default
name: "statusreconciler"
Copy link
Contributor

Choose a reason for hiding this comment

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

Does every service really needs it's own service account as opposed to one scoped for prow or some larger subset thereof?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

More granular permissions are better IMO

@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. labels Apr 18, 2019
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5616aa868b0ef6884b3e9868e3e3d631f3a68d28

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, fejta, stevekuznetsov

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

/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 Apr 23, 2019
@k8s-ci-robot k8s-ci-robot merged commit 3ed05bc into kubernetes:master Apr 23, 2019
@rofuentes rofuentes mentioned this pull request Apr 25, 2019
Closed
@bobcatfish bobcatfish mentioned this pull request Aug 5, 2019
Closed
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

@cjwagner cjwagner cjwagner approved these changes

@amwat amwat Awaiting requested review from amwat

@ixdy ixdy Awaiting requested review from ixdy

Assignees

@fejta fejta

@cjwagner cjwagner

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@stevekuznetsov @k8s-ci-robot @cjwagner @fejta