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

github-jira-proxy, prow, plugin: add plugin that checks github webhooks #3516

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

avlitman
Copy link
Contributor

@avlitman avlitman commented Jul 9, 2024

What this PR does / why we need it:
This pr added in order to make sure jira webhook secret url is private and no reachable by github repo maintainers outside of redhat.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Jira-ticket: https://issues.redhat.com/browse/CNV-45739

Special notes for your reviewer:
this service needs to have a public URL github can reach.

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:

none

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Jul 9, 2024
@avlitman avlitman marked this pull request as draft July 9, 2024 13:52
@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L labels Jul 9, 2024
Copy link
Contributor

@dhiller dhiller left a comment

Choose a reason for hiding this comment

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

Looks good for a start @avlitman - comments inline.

Maybe @brianmcarey can chime in here on how we should configure the ingress rule - and what else is necessary to get it reachable from external sources.

@@ -0,0 +1,20 @@

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can consume from your own GitHub repository for a start. Other than that, this folder is reserved for proper external prow plugins, which the code here doesn't match (yet). Having said that, you can either build from your public repo in the images Dockerfile, or move your code to the robots folder.

@@ -546,6 +546,10 @@ external_plugins:
endpoint: http://referee:9900
events:
- issue_comment
- name: github-jira-proxy
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be removed until the code matches the setup for a proper external plugin.

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from dhiller. For more information see the Kubernetes Code Review Process.

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

@avlitman avlitman force-pushed the add-jira-proxy branch 2 times, most recently from e9a3d4d to 2b60454 Compare July 17, 2024 08:36
@avlitman avlitman marked this pull request as ready for review July 24, 2024 11:32
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 24, 2024
@dhiller
Copy link
Contributor

dhiller commented Jul 25, 2024

/uncc @davidvossel @vladikr
/cc @brianmcarey

Copy link
Member

@brianmcarey brianmcarey left a comment

Choose a reason for hiding this comment

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

Looks good @avlitman - I think it would be worth adding your pod to this list so that it is checked in the prow-deploy presubmit -

- name: wait for prow pods to be ready

@avlitman avlitman force-pushed the add-jira-proxy branch 2 times, most recently from 494a794 to fe14a8d Compare August 29, 2024 08:39
@avlitman
Copy link
Contributor Author

Looks good @avlitman - I think it would be worth adding your pod to this list so that it is checked in the prow-deploy presubmit -

- name: wait for prow pods to be ready

Added (:

@brianmcarey
Copy link
Member

@avlitman looks like your pod is failing to reach a running state for some reason - https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_project-infra/3516/pull-project-infra-prow-deploy-test/1829076461068226560

You should be able to run a test deploy against a kubevirtci cluster to check out why. You probably need ansible installed though.
https://github.com/kubevirt/project-infra/blob/main/github/ci/prow-deploy/hack/test.sh

@avlitman
Copy link
Contributor Author

@avlitman looks like your pod is failing to reach a running state for some reason - https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_project-infra/3516/pull-project-infra-prow-deploy-test/1829076461068226560

You should be able to run a test deploy against a kubevirtci cluster to check out why. You probably need ansible installed though.

https://github.com/kubevirt/project-infra/blob/main/github/ci/prow-deploy/hack/test.sh

So just install ansible and run it from my laptop?
Also any ideas why it failed?

@brianmcarey
Copy link
Member

@avlitman looks like your pod is failing to reach a running state for some reason - https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_project-infra/3516/pull-project-infra-prow-deploy-test/1829076461068226560
You should be able to run a test deploy against a kubevirtci cluster to check out why. You probably need ansible installed though.
https://github.com/kubevirt/project-infra/blob/main/github/ci/prow-deploy/hack/test.sh

So just install ansible and run it from my laptop? Also any ideas why it failed?

The test env may be missing the necessary secrets to start the pod - I will try to run it here locally to see and let you know.

@avlitman
Copy link
Contributor Author

avlitman commented Sep 1, 2024

@avlitman looks like your pod is failing to reach a running state for some reason - https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_project-infra/3516/pull-project-infra-prow-deploy-test/1829076461068226560
You should be able to run a test deploy against a kubevirtci cluster to check out why. You probably need ansible installed though.
https://github.com/kubevirt/project-infra/blob/main/github/ci/prow-deploy/hack/test.sh

So just install ansible and run it from my laptop? Also any ideas why it failed?

The test env may be missing the necessary secrets to start the pod - I will try to run it here locally to see and let you know.

Much appreciated! does it make sense to push changes to this pr to see if pull-project-infra-prow-deploy-test pass or it will abuse the ci too much?

@brianmcarey
Copy link
Member

@avlitman looks like your pod is failing to reach a running state for some reason - https://prow.ci.kubevirt.io/view/gs/kubevirt-prow/pr-logs/pull/kubevirt_project-infra/3516/pull-project-infra-prow-deploy-test/1829076461068226560
You should be able to run a test deploy against a kubevirtci cluster to check out why. You probably need ansible installed though.
https://github.com/kubevirt/project-infra/blob/main/github/ci/prow-deploy/hack/test.sh

So just install ansible and run it from my laptop? Also any ideas why it failed?

The test env may be missing the necessary secrets to start the pod - I will try to run it here locally to see and let you know.

Much appreciated! does it make sense to push changes to this pr to see if pull-project-infra-prow-deploy-test pass or it will abuse the ci too much?

It justs the one job so you should be ok to push whatever changes you want - the lane is pretty quick too so you wouldn't be generating too much load.

Copy link
Member

@brianmcarey brianmcarey left a comment

Choose a reason for hiding this comment

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

Yes it looks like it is missing a couple of secrets for the testing env

  Warning  FailedMount  30s (x9 over 2m38s)  kubelet            MountVolume.SetUp failed for volume "github-webhook-secret" : secret "github-webhook-secret" not found
  Warning  FailedMount  30s (x9 over 2m38s)  kubelet            MountVolume.SetUp failed for volume "jira-webhook-url" : secret "jira-webhook-url" not found

You can add dummy secrets here for testing - https://github.com/kubevirt/project-infra/blob/main/github/ci/prow-deploy/vars/kubevirtci-testing/secrets.yml

@avlitman avlitman force-pushed the add-jira-proxy branch 6 times, most recently from 4a0c234 to 707f44d Compare September 3, 2024 15:08
@kubevirt-bot kubevirt-bot added size/L and removed size/M labels Sep 3, 2024
Copy link
Member

@brianmcarey brianmcarey left a comment

Choose a reason for hiding this comment

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

Looks good @avlitman - just a couple of questions on the ingress.

- prow.ci.kubevirt.io
secretName: github-jira-proxy-tls
rules:
- host: prow.ci.kubevirt.io
Copy link
Member

Choose a reason for hiding this comment

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

We will need to use a different host name here as prow.ci.kubevirt.io is already in use by prow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in both places

pathType: Prefix
backend:
service:
name: github-jira-proxy
Copy link
Member

Choose a reason for hiding this comment

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

Should this be github-jira-proxy-service as that is what the service is named below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

Copy link
Member

@brianmcarey brianmcarey left a comment

Choose a reason for hiding this comment

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

Also can you add some details to the PR description on why we want to add this.

@avlitman
Copy link
Contributor Author

avlitman commented Sep 5, 2024

Also can you add some details to the PR description on why we want to add this.

@brianmcarey thanks a lot brian- fixed all your comments (:

Copy link
Member

@brianmcarey brianmcarey left a comment

Choose a reason for hiding this comment

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

/hold

This service should not be hosted on the kubevirt control plane cluster as there is still a risk of the mentioned secrets leaking.

@kubevirt-bot kubevirt-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants