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

Virtual Pipeline Plugin #3310

Open
thavlice opened this issue May 14, 2023 · 30 comments
Open

Virtual Pipeline Plugin #3310

thavlice opened this issue May 14, 2023 · 30 comments
Labels
bot-check-complete Automated hosting checks passed hosting-request Request to host a component in jenkinsci security-audit-done The hosting request code passed the security audit with success

Comments

@thavlice
Copy link

Repository URL

https://github.com/thavlice/jenkins-report-virtual-pipeline

New Repository Name

virtual-pipeline-plugin

Description

Virtual Pipeline Jenkins plugin allows the user to define marks (using regular expressions) that can be found in the main log and them visualize then on the Project and Job Page.

This plugin helps to automate manual searching for key information in the main log of Jenkins builds.

GitHub users to have commit permission

@thavlice
@judovana

Jenkins project users to have release permission

thavlice
judovana

Issue tracker

Jira

@thavlice thavlice added the hosting-request Request to host a component in jenkinsci label May 14, 2023
@jenkins-cert-app
Copy link
Collaborator

Security audit, information and commands

The security team is auditing all the hosting requests, to ensure a better security by default.

This message informs you that a Jenkins Security Scan was triggered on your repository.
It takes ~10 minutes to complete.

Commands

The bot will parse all comments, and it will check if any line start with a command.

Security team only:

  • /audit-ok => the audit is complete, the hosting can continue 🎉.
  • /audit-skip => the audit is not necessary, the hosting can continue 🎉.
  • /audit-findings => the audit reveals some issues that require corrections ✏️.

Anyone:

  • /request-security-scan => the findings from the [Jenkins Security Scan](https://www.jenkins.io/doc/developer/security/scan/) were corrected, this command will re-scan your repository 🔍.
  • /audit-review => the findings from the audit were corrected, this command will ping the security team to review the findings 👀. It's only applicable when the previous audit required changes.

Only one command can be requested per comment.

(automatically generated message, version: 1.17.14)

@jenkins-cert-app jenkins-cert-app added the security-audit-todo The security team needs to audit the hosting request code label May 14, 2023
@github-actions
Copy link

Hello from your friendly Jenkins Hosting Checker

It appears you have some issues with your hosting request. Please see the list below and correct all issues marked Required. Your hosting request will not be approved until these issues are corrected. Issues marked with Warning or Info are just recommendations and will not stall the hosting process.

  • ⛔ Required: The following usernames in 'Jenkins project users to have release permission' need to log into Artifactory: thavlice (reports are re-synced hourly, wait to re-check for a bit after logging in)

You can re-trigger a check by editing your hosting request or by commenting /hosting re-check

@jenkins-cert-app
Copy link
Collaborator

The Jenkins Security Scan discovered 11 finding(s) 🔍.
For each of them, either apply the recommended correction, suppress the warning or provide a justification.

Once you're done, either re-run the scan with /request-security-scan or request the Security team to review your justifications with /audit-review.


Stapler: Missing POST/RequirePOST annotation

You can find detailed information about this finding here.

VirtualPipelineInputAdvanced.java#101
Potential CSRF vulnerability: If DescriptorImpl#doCheckNumberOfLineToDisplay connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST
VirtualPipelineInputAdvanced.java#84
Potential CSRF vulnerability: If DescriptorImpl#doCheckMaxContentLength connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST
VirtualPipelineInputAdvanced.java#72
Potential CSRF vulnerability: If DescriptorImpl#doCheckEndMark connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST
VirtualPipelineInputAdvanced.java#60
Potential CSRF vulnerability: If DescriptorImpl#doCheckStartMark connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST
VirtualPipelineInputSimple.java#46
Potential CSRF vulnerability: If DescriptorImpl#doCheckRegex connects to user-specified URLs, modifies state, or is expensive to run, it should be annotated with @POST or @RequirePOST

Stapler: Missing permission check

You can find detailed information about this finding here.

VirtualPipelineInputAdvanced.java#101
Potential missing permission check in DescriptorImpl#doCheckNumberOfLineToDisplay
VirtualPipelineInputAdvanced.java#84
Potential missing permission check in DescriptorImpl#doCheckMaxContentLength
VirtualPipelineInputAdvanced.java#72
Potential missing permission check in DescriptorImpl#doCheckEndMark
VirtualPipelineInputAdvanced.java#60
Potential missing permission check in DescriptorImpl#doCheckStartMark
VirtualPipelineInputSimple.java#46
Potential missing permission check in DescriptorImpl#doCheckRegex
VirtualPipelineOffsetAction.java#31
Potential missing permission check in VirtualPipelineOffsetAction#doSearchOffset

@jenkins-cert-app jenkins-cert-app added security-audit-needs-correction The security audit revealed issues that must be corrected from the hosting request and removed security-audit-todo The security team needs to audit the hosting request code labels May 14, 2023
@thavlice
Copy link
Author

/hosting re-check

@github-actions
Copy link

Hello from your friendly Jenkins Hosting Checker

It looks like you have everything in order for your hosting request. A human volunteer will check over things that I am not able to check for (code review, README content, etc) and process the request as quickly as possible. Thank you for your patience.

Hosting team members can host this request with /hosting host

@github-actions github-actions bot added bot-check-complete Automated hosting checks passed and removed needs-fix labels May 14, 2023
@NotMyFault
Copy link
Member

Virtual Pipeline Jenkins plugin allows the user to define marks (using regular expressions) that can be found in the main log and them visualize then on the Project and Job Page.

Based on this explanation, I recommend choosing a more fitting name for the plugin. I can imagine pipeline-log-filter or pipeline-log-highlighter suits better.

@alecharp
Copy link
Contributor

Hello @thavlice, I have some comment on your repository.

@thavlice
Copy link
Author

thavlice commented Jun 2, 2023

/request-security-scan

@jenkins-cert-app jenkins-cert-app added security-audit-todo The security team needs to audit the hosting request code and removed security-audit-needs-correction The security audit revealed issues that must be corrected from the hosting request labels Jun 2, 2023
@jenkins-cert-app
Copy link
Collaborator

The Jenkins Security Scan did not find anything dangerous with your plugin, congratulations! 🎉


💡 The Security team recommends that you are setting up the scan in your repository by following our guide.

@jenkins-cert-app jenkins-cert-app added security-audit-done The hosting request code passed the security audit with success and removed security-audit-todo The security team needs to audit the hosting request code labels Jun 2, 2023
@thavlice
Copy link
Author

thavlice commented Jun 2, 2023

Hello @thavlice, I have some comment on your repository.

* could you run `mvn tidy:pom` to re-arrange the `pom.xml` content

* some of the dependencies declared don't seem to be required

* could you switch https://github.com/thavlice/jenkins-report-virtual-pipeline/blob/65ea7487fe793ee722b3bb777c31a1f28ef83074/pom.xml#L77-L81 for https://github.com/jenkinsci/jackson2-api-plugin?

* rather than https://github.com/thavlice/jenkins-report-virtual-pipeline/blob/main/src/main/webapp/img/list.svg, can you please use the symbols. You can see how on https://weekly.ci.jenkins.io/design-library/Symbols/

* you are defining `css` on some jelly files. Please see `adjunct` here https://www.jenkins.io/doc/developer/views/exposing-bundled-resources/#adjuncts

* I would suggest to add the license header on the java classes

* I'm not sure https://github.com/thavlice/jenkins-report-virtual-pipeline/blob/65ea7487fe793ee722b3bb777c31a1f28ef83074/src/main/java/io/jenkins/plugins/VirtualPipeline/VirtualPipelineOffsetAction.java#L87-L89 exists. You would have a 404 on the icon I guess

Hi @alecharp,
I've implemented the suggested changes apart from the adjunct one as I am not sure how to link the css files (in webapp folder) using the <st:adjunct>.

Would appreciate deeper explanation of what is expected. Thanks in advance.

@NotMyFault
Copy link
Member

Virtual Pipeline Jenkins plugin allows the user to define marks (using regular expressions) that can be found in the main log and them visualize then on the Project and Job Page.

Based on this explanation, I recommend choosing a more fitting name for the plugin. I can imagine pipeline-log-filter or pipeline-log-highlighter suits better.

is yet to address.

@thavlice
Copy link
Author

thavlice commented Jun 9, 2023

Virtual Pipeline Jenkins plugin allows the user to define marks (using regular expressions) that can be found in the main log and them visualize then on the Project and Job Page.

Based on this explanation, I recommend choosing a more fitting name for the plugin. I can imagine pipeline-log-filter or pipeline-log-highlighter suits better.

is yet to address.
Hi @NotMyFault,

I believe that "highlight" might be a better description, but "filter" might seem a little off as there is already existing plugin called Log File Filter wit a different functionality.

Initially, the name "virtual pipeline" comes from the results of the "highlighting" of logs. The results should somewhat mimic the "breaking down build into smaller stages/steps" philosophy of a Pipeline for Freestyle project, thus being "Virtual pipeline", as in fact it has nothing to do with Pipeline apart from somewhat mimicking the behaviour.

I am not sure what is the ideal name, something involving "highlighter" might be the closest thing. I'll try to come up with something, appreciate the feedback.

(I added a reference to my thesis to README, which can make the purpose of this plugin and its name clearer)

@judovana
Copy link
Contributor

Will shot my irrelevant brainstorming.

(build)log-shortener(-plugin)
(build)log-grapper(-plugin)
(build)log-visualiser(-plugin)
pipeline-like-visualiser(-plugin)
(build)log-abstract/epitome(-plugin)
(build)log-digester(-plugin)
(build)log-digest-viewer(-plugin)
(build)log2pipe-view(-plugin)
(build)log2pipe-summary(-plugin)
(build)log2pipeline-view(-plugin)
(build)log2pipeline-summary(-plugin)

And recalling NotMyFualt's, as they are quite good:
pipeline-log-filter(-plugin)
pipeline-log-highlighter(-plugin)

Where I like all where pip-like kewyords appear.

One nit (just crossing mind). The implementations slightly lacks some Arrows, so the final extracted log would look indeed lilke pipe. Easy to add, but most likely wasting of space. Maybe optional? maybe just centred | between lines would be enough?

@NotMyFault
Copy link
Member

Will shot my irrelevant brainstorming.

(build)log-shortener(-plugin) (build)log-grapper(-plugin) (build)log-visualiser(-plugin) pipeline-like-visualiser(-plugin) (build)log-abstract/epitome(-plugin) (build)log-digester(-plugin) (build)log-digest-viewer(-plugin) (build)log2pipe-view(-plugin) (build)log2pipe-summary(-plugin) (build)log2pipeline-view(-plugin) (build)log2pipeline-summary(-plugin)

And recalling NotMyFualt's, as they are quite good: pipeline-log-filter(-plugin) pipeline-log-highlighter(-plugin)

Where I like all where pip-like kewyords appear.

One nit (just crossing mind). The implementations slightly lacks some Arrows, so the final extracted log would look indeed lilke pipe. Easy to add, but most likely wasting of space. Maybe optional? maybe just centred | between lines would be enough?

I think pipeline-log-filter makes a good fit :)

@mawinter69
Copy link
Contributor

Is it correct that this plugin is intended for use in freestyle jobs only?
Then I think any mentioning of pipeline in the artifactid is very misleading. Also in the index.jelly

When searching for pipeline in the plugin manager or on https://plugins.jenkins.io/ and this plugin is shown I would expect it has something to do with pipeline jobs which it hasn't.

Also the DisplayName of the Publisher (Virtual Pipeline) is not so fitting in my eyes.

My suggestion would be logline-marker for the plugin id and Mark Log Lines for the Publisher display name.

@mawinter69
Copy link
Contributor

The approach to read the log file in a publisher has 2 severe disadvantages.

  1. Accessing the log file directly is discouraged. The method Run#getLogFile() is deprecated. Although there is no implementation for freestyle, for pipeline jobs it is theoretically possible that logs are not stored in the file system.
  2. And any publisher that is running afterwards is ignored.

I suggest to implement this as a BuildWrapper in combination with a ConsoleLogFilter (See the timestamper plugin) how this is done.
Add to this a RunListener that after the run has finished creates all the actions and attaches them to the run.

@judovana
Copy link
Contributor

Is it correct that this plugin is intended for use in freestyle jobs only? Then I think any mentioning of pipeline in the artifactid is very misleading. Also in the index.jelly

Do you see any reason why it should fail if used in pipeline job?

...

My suggestion would be logline-marker for the plugin id and Mark Log Lines for the Publisher display name.

(build)log(line)-flow-visualiser?

@judovana
Copy link
Contributor

The approach to read the log file in a publisher has 2 severe disadvantages.

1. Accessing the log file directly is discouraged. The method `Run#getLogFile()` is deprecated. Although there is no implementation for freestyle, for pipeline jobs it is theoretically possible that logs are not stored in the file system.

2. And any publisher that is running afterwards is ignored.

The only thing the plugin have to do, is to not fail if the parsing fails - eg if buildlog is missing. Then all other publishers will normally run, right?

I suggest to implement this as a BuildWrapper in combination with a ConsoleLogFilter (See the timestamper plugin)

Using it as build wrapper will limit the plugin quite a lot. Righ now you can use it with any buildwrapper, by implementing it as build wrapper, it will make it not usable with any other build wrapers, and generally make any its usage very inconvenient.

how this is done. Add to this a RunListener that after the run has finished creates all the actions and attaches them to the run.

@mawinter69
Copy link
Contributor

mawinter69 commented Jun 26, 2023

Is it correct that this plugin is intended for use in freestyle jobs only? Then I think any mentioning of pipeline in the artifactid is very misleading. Also in the index.jelly

Do you see any reason why it should fail if used in pipeline job?

At the moment the plugin can't be used at all in pipeline jobs due to the restriction in here
Seems that statement is wrong

...

My suggestion would be logline-marker for the plugin id and Mark Log Lines for the Publisher display name.

(build)log(line)-flow-visualiser?

That would also be fine for me

@mawinter69
Copy link
Contributor

The approach to read the log file in a publisher has 2 severe disadvantages.

1. Accessing the log file directly is discouraged. The method `Run#getLogFile()` is deprecated. Although there is no implementation for freestyle, for pipeline jobs it is theoretically possible that logs are not stored in the file system.

2. And any publisher that is running afterwards is ignored.

The only thing the plugin have to do, is to not fail if the parsing fails - eg if buildlog is missing. Then all other publishers will normally run, right?

Yes.

I suggest to implement this as a BuildWrapper in combination with a ConsoleLogFilter (See the timestamper plugin)

Using it as build wrapper will limit the plugin quite a lot. Righ now you can use it with any buildwrapper, by implementing it as build wrapper, it will make it not usable with any other build wrapers, and generally make any its usage very inconvenient.

That is not true, you can have as many build wrapper as you want. There is no limitation to the plugin I would say.

how this is done. Add to this a RunListener that after the run has finished creates all the actions and attaches them to the run.

@mawinter69
Copy link
Contributor

by using a ConsoleLogFilter you can even make the plugin work with pipeline jobs.

@judovana
Copy link
Contributor

judovana commented Jun 26, 2023

by using a ConsoleLogFilter you can even make the plugin work with pipeline jobs.

Hmm. That s part of BuildWrapper... But probably worthy to investigate.

@NotMyFault
Copy link
Member

The plugin's functionality would make a good PR to https://plugins.jenkins.io/text-finder/, which supports pipelines already, alongside freestyle jobs.

@mawinter69
Copy link
Contributor

by using a ConsoleLogFilter you can even make the plugin work with pipeline jobs.

Hmm. That s part of BuildWrapper... But probably worthy to investigate.

Seems it can already work with pipeline, still the ConsoleLogFilter approach might have advantages, e.g. it would allow to configure it globally.
Note that you can also enable the ConsoleLogFilter by setting a jobProperty theoretically afaik.

@judovana
Copy link
Contributor

The plugin's functionality would make a good PR to https://plugins.jenkins.io/text-finder/, which supports pipelines already, alongside freestyle jobs.

Not necessarily. Text finder plugin does its job really well, and should remain in doing that.
Some time ago, I created a PR to text-finder plugin, whcih allow several textfinders - jenkinsci/text-finder-plugin#47 it was never accepted, and so I keep living in my fork text-finders-plugin (note the s). And am still thinking how to deal with it. If to try to push it to text-finder-plugin again, or try to move text-finders-plugin from personal space to jenkinsci. Later I added to text-finders-plugin feature, to set dispaly name based on the match. Yet agian, I thought about moving it to text-finder-plugin but realised that it shoudl be plugin on its own.

Long story short, the border between one plugin one thing and correctly, and to much shared code, lmerge functionality is thin.

In this plugin, I would definitely vote for having it standalone.

@judovana
Copy link
Contributor

by using a ConsoleLogFilter you can even make the plugin work with pipeline jobs.

Hmm. That s part of BuildWrapper... But probably worthy to investigate.

Seems it can already work with pipeline, still the ConsoleLogFilter approach might have advantages, e.g. it would allow to configure it globally. Note that you can also enable the ConsoleLogFilter by setting a jobProperty theoretically afaik.

Interestign idea. TY!

@NotMyFault
Copy link
Member

Some time ago, I created a PR to text-finder plugin, whcih allow several textfinders - jenkinsci/text-finder-plugin#47

That was 4 years ago. I'd recommend trying it again and reaching out to the maintainers of the plugin, if it's not up for adoption. I don't believe hosting another plugin doing something similar because the initial PR didn't address the concerns of the plugin maintainers is the right direction to go to.

@judovana
Copy link
Contributor

All concerns were cleared, but the final ack was never given. And maintainers are still the same. it seems that multiple textfinders were added at the end. 🥇 I will rise the discussion about display name again. TY!

@thavlice
Copy link
Author

Just to check, I'll do the following things:

  • Name changes to not confuse with Pipe
    • change the id to log-flow-visualizer (alternatively I liked the name log-mark-highlighter)
    • change the display name to Log Flow Visualizer
  • Investigate compatibility with Pipeline Jobs
    • check and possibly implement BuildWrapper approach
    • check and possibly implement ConsoleLogFilter approach
  • As of now, aiming to publish as standalone plugin
  • other minor implementation fixes

Summed up from the conversation, let me know if I missed something or there is something else to be done for the plugin.

@judovana
Copy link
Contributor

Sounds correct to me. TY!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot-check-complete Automated hosting checks passed hosting-request Request to host a component in jenkinsci security-audit-done The hosting request code passed the security audit with success
Projects
None yet
Development

No branches or pull requests

6 participants