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

feat(quay): add quay plugin #68

Merged
merged 6 commits into from
Feb 7, 2023
Merged

Conversation

fmenesesg
Copy link
Collaborator

#60
This is the first version of a Quay plugin that shows the tags, timestamp, last_update, and SHA of an image related to a backstage entity.

@fmenesesg fmenesesg changed the title add quay plugin feat(quay): add quay plugin Jan 9, 2023
Copy link
Member

@tumido tumido left a comment

Choose a reason for hiding this comment

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

I haven't reviewed the code fully yet, however I have some architectural concerns here. Can you please explain me why do we need a backend plugin in here? It seems like it's just a API proxy which doesn't communicate with Backstage backend/database/catalog/anything at all. What is the justification for a backend plugin in here? Can't we either talk to Quay directly from the frontend or proxy it via the native Backstage proxy (similarly to what ArgoCD or Prometheus plugins do)?

@fmenesesg
Copy link
Collaborator Author

@tumido thanks for your feedback, and you are right, by now on this version, the backend plugin is just a proxy, but what I was thinking is that when we add the ability to get the vulnerabilities per tag, that logic should be on the backend plugin.

@tumido
Copy link
Member

tumido commented Jan 11, 2023

@fmenesesg hm, interesting thought, how would you imagine this being handled on the backend? As far as I know, Backstage has no notion of this kind in its catalog. Would we need to extend the database with a new model, perhaps? Is that what we want? Backstage's event processing is in its infancy as well currently, so any backend solution wouldn't be reactive, we would have to resort to a polling solution. So far it seems to me way like a way heavier and very complicated solution to implement something like that in backstage backend compared to fetching the data directly on frontend. If I compare this with for example Snyk integration, they also have a frontend plugin only. I think mainly because of the backstage backend not being capable/suitable for this type of processing.

However I may be missing something here. Do you have a solution in mind?

@fmenesesg
Copy link
Collaborator Author

@tumido what I have in mind was to mimic on the backend the same behavior that quay has on their code.

@tumido
Copy link
Member

tumido commented Jan 12, 2023

@fmenesesg well.. I'm afraid that adding that to Backstage backend serves not much of a purpose. The only benefit may be caching these results so we optimize traffic. However that would mean you'd either have to extend the backstage RBAC model or build a per-user cache. Either way I don't see a way how to create that easily in a safe and scalable fashion.

Quay itself is performing these queries on frontend... This is telling something about the UX of Quay API. I don't think we should solve that on Backstage backend.

@tumido
Copy link
Member

tumido commented Jan 12, 2023

I have 2 main concerns regarding the API calls aggregations into a backend plugin compared to many frontend requests.

Timeouts

The proposed solution via backend plugin results in following sequence, correct?

sequenceDiagram
    Backstage Front-end->>Backstage Back-end: /api/quayPlugin/getTags
    Backstage Back-end->>Quay API: /getTags?page=1
    Backstage Back-end->>Quay API: /getTags?page=2
    Backstage Back-end->>Quay API: /getTags?page=...
	Quay API-->>Backstage Back-end: Page 1 response
	Quay API-->>Backstage Back-end: Page 2 response
	Quay API-->>Backstage Back-end: Page x response
	Backstage Back-end-->>Backstage Front-end: Response
Loading

Imagine any of the queries from backend to Quay API erroring out. Backstage backend can either retry that query or return error. On error frontend would retry resulting in repeating all the queries again.

sequenceDiagram
    Backstage Front-end->>Backstage Back-end: /api/quayPlugin/getTags
    Backstage Back-end->>Quay API: /getTags?page=1
    Backstage Back-end->>Quay API: /getTags?page=2
    Backstage Back-end->>Quay API: /getTags?page=...
	Quay API-->>Backstage Back-end: Page 1 response
	Quay API-->>Backstage Back-end: Page 2 ERROR
	Backstage Back-end-->>Backstage Front-end: ERROR
    Backstage Front-end->>Backstage Back-end: retry /api/quayPlugin/getTags
Loading

If it is Backstage backend who retries a single query to Quay API, then there's a substantial risk of timing out the original request from backstage frontend.

sequenceDiagram
    Backstage Front-end->>Backstage Back-end: /api/quayPlugin/getTags
    Backstage Back-end->>Quay API: /getTags?page=1
    Backstage Back-end->>Quay API: /getTags?page=2
    Backstage Back-end->>Quay API: /getTags?page=...
	Quay API-->>Backstage Back-end: Page 1 response
	Quay API-->>Backstage Back-end: Page 2 ERROR
    Backstage Back-end->>Quay API: /getTags?page=2
	Quay API-->>Backstage Back-end: Page x response
    Note over Backstage Front-end: TIMEOUT
Loading

This is solvable by advanced caching or delayed processing/notifying user via webhooks/polling that the results are ready.

Request cancellations

Imagine user refreshing a page or leaves the page before receiving quay plugin response. The original backstage frontend to backstage backend request has to be cancelled. Than you'd have to create additional handler to cancel all pending async requests between Backstage backend and Quay API. Otherwise just a single user by hitting F5 multiple times can saturate the backstage backend bandwidth.

I'm not sure all of this work is a worthy investment just because of a single API endpoint. There are too many sideeffects that needs to be handled properly. I think the cost of maintenance is too big, compared to what you can already do via issuing multiple requests on the frontend side.

@fmenesesg
Copy link
Collaborator Author

Ok, I got what you said, I'm going to change the implementation to do something similar to what the Argo plugin does

@tumido
Copy link
Member

tumido commented Jan 25, 2023

@fmenesesg do you need any help to move this forward? I can contribute if you want. We'd love to have the plugin available. 🙂

@fmenesesg
Copy link
Collaborator Author

@tumido I'm out on vacation i will return on 2 weeks. If you want you can start with the removal of the backend plugin in order to use the built in proxy that backstage has. I will start working when I come back.

@tumido tumido self-assigned this Jan 30, 2023
@tumido
Copy link
Member

tumido commented Jan 31, 2023

I gave it a rework (+rebase), removed the backend plugin and used the Proxy plugin instead.

That being said, it still needs a bit more work on CORS since all proxied requests now return:

API calls must be invoked with an X-Requested-With header if called from a browser

So.. Work in progress, just sharing my current results.

@tumido
Copy link
Member

tumido commented Feb 7, 2023

@fmenesesg I've updated the docs to include your fix and resolved merge conflict. PTAL 🙂

Thank you for letting me collaborate on this 👍

@fmenesesg
Copy link
Collaborator Author

@tumido LGTM, Thanks for your help with this 👍

@fmenesesg I've updated the docs to include your fix and resolved merge conflict. PTAL slightly_smiling_face

Thank you for letting me collaborate on this +1

@tumido tumido merged commit 7d4970e into janus-idp:main Feb 7, 2023
@fmenesesg fmenesesg mentioned this pull request Feb 9, 2023
10 tasks
mareklibra pushed a commit to mareklibra/janus-idp-backstage-plugins that referenced this pull request Apr 5, 2023
jkilzi pushed a commit to jkilzi/janus-idp-backstage-plugins that referenced this pull request Jan 12, 2024
jkilzi pushed a commit to jkilzi/janus-idp-backstage-plugins that referenced this pull request Jan 12, 2024
jkilzi pushed a commit to jkilzi/janus-idp-backstage-plugins that referenced this pull request Jan 16, 2024
openshift-merge-bot bot pushed a commit that referenced this pull request Jan 17, 2024
* feat(orchestrator): add orchestrator plugins

Squashed and rebased. Credits to
- Guilherme Caponetto
- Tiago Dolphine
- Michael Anstis

who did the work before squash and rebase.

* feat(orchestrator): enable dynamic plugin

* fix(orchestrator): load HostDirectory from backend-app-api (#28)

Signed-off-by: Moti Asayag <[email protected]>
(cherry picked from commit 3ba02c6)

* fix(orchestrator): make the port config optional (#38)

* feat(orchestrator): handle assessment workflow and make optional the workflowsSource config (#35)

* feat: issue FLPATH-591 - Add assessment workflow type and display outputs (#21)

* Configure for assessment swf

* filter assessment on workflow definition page

* workflow columns

* workflow columns

* workflow columns

* workflow filter

* feat: Render assessment results supporting dynamic categories

* issue FLPATH-657: workflow label

* Revert "Configure for assessment swf"

This reverts commit b4048e1.

* Revert sonata service port

* Add key for workflow options category

* Fix workflow execution from choose btn

* Fix review comments

* address comments

* Fix review comment to switch to useRouteRef

* fix review comments

* fix review comments

---------

Co-authored-by: richardwang98 <[email protected]>

* feat: make optional the workflowsSource config (#25)

---------

Co-authored-by: anludke <[email protected]>

* feat(orchestrator): add backend endpoint for getting workflows from data index service (#39)

* feat: add backend for getting work flows from data index service

* feat: cherrypick - add backend for getting work flows from data index service

* feat: cherrypick - add backend for getting work flows from data index service

* feat: cherrypick - add backend for getting work flows from data index service

* chore(orchestrator): update dependencies

* feat(orchestrator): orchestrator plugin entry page & workflows table (#31)

https://issues.redhat.com/browse/FLPATH-686
https://issues.redhat.com/browse/FLPATH-682

* feat: introduced the workflows/overview endpoint (#36)

* FLPATH-702 : New endpoint to fetch workflow overview

* FLPATH-702 : iterate through all elements instead of picking the first element

* modified the map method

* calculate avg execution time change

* Add log message when http calls failing

* Fix lint issues

* rename variable

* paginated graphql object fetch

* Include workflowId in the result object

* Renamed to description

* to epoc timestamp

* suppress eslint warning

* epoc number to string

* make all string fields optional

* use a simple for loop

* extracted to a new method

* rename to lastTriggeredMs

* include uri in the WorkflowOverview type

* mark avgDurationMs as optional

* feat(orchestrator): introduced `workflows/:workflowId/overview` endpoint (#40)

* FLPATH-702 : New endpoint to fetch workflow overview

* FLPATH-702 : iterate through all elements instead of picking the first element

* modified the map method

* calculate avg execution time change

* Add log message when http calls failing

* Fix lint issues

* rename variable

* paginated graphql object fetch

* Include workflowId in the result object

* Renamed to description

* to epoc timestamp

* suppress eslint warning

* epoc number to string

* make all string fields optional

* use a simple for loop

* extracted to a new method

* rename to lastTriggeredMs

* include uri in the WorkflowOverview type

* mark avgDurationMs as optional

* fetch only one overview obj

* fix(orchestrator): make the port config optional (#37)

* feat(orchestrator): orchestrator plugin entry page & workflows table (#31)

https://issues.redhat.com/browse/FLPATH-686
https://issues.redhat.com/browse/FLPATH-682

* removed unnecessary method

---------

Co-authored-by: Guilherme Caponetto <[email protected]>
Co-authored-by: Jonathan Kilzi <[email protected]>

* feat(orchestrator): add Workflow Run List  (#30)

* feat(orchestrator): add Workflow Run List

FLPATH-693

A component listing running workflows is added.

* chore: move loading logic out of the OrchestratorPage

* chore(orchestrator): implement the WorkflowViewerFormatter (#41)

* chore: implement the WorkflowViewerFormatter: utility for converting WorkflowOverview backend data to data UI can display

* removed redundant export and removed redundant 'Interface' suffix from DataFormatter interface name

* Update plugins/orchestrator/src/dataFormatters/DataFormatter.ts

Co-authored-by: Guilherme Caponetto <[email protected]>

---------

Co-authored-by: Guilherme Caponetto <[email protected]>

* feat(orchestrator): fetch data input schema from `/management/processes` (#45)

* Fetch data input schema from /management/processes

* Fix some code smells

* Rename WorkflowProcess -> WorkflowInfo

* feat(orchestrator): implement the workflow viewer new UX (#32)

* feat(orechestrator): implement the workflow viewer new UX

* code review fixes

* visual fixes including skeleton for loading state

* chore(orchestrator): Renames the components displaying the tabs content

* chore(orchestrator): updates the OrchestratorClient

Lazy loads the baseUrl
Adds a method for calling GET /workflows/overview

* feat(orchestrator): orchestrator workflow execution page (#46)

Co-authored-by: Jonathan Kilzi <[email protected]>

* chore(orchestrator): updates the stories grouping (#50)

* chore(orchestrator): aligns the workflows table with the design (#51)

* chore(orchestrator): add Category to the procesInstance result (#52)

* fix(orchestrator): add links to workflows in the workflow list (#53)

* feat: added color icon to workflow details page last run status field (#55)

* feat(orchestrator): enable usage of local envelope for the workflow editor (#56)

* feat(orchestrator): add business key to assessment and pass on to workflow options (#42)

* feat(orchestrator): add page listing details of a workflow run (#49)

* feat(orchestrator): add feature flag for developer mode and config to enable/disable the integration with catalog (#58)

* feat(orchestrator): workflow editor modal (#59)

* feat(orchestrator): fix missing workflow type in overview (#60)

Signed-off-by: Gloria Ciavarrini <[email protected]>

* fix(orchestrator): fix some code smells (#63)

* fix(orchestrator): addresses sonarcloud issues (#65)

* fix(orchestrator): addressed a couple of issues (#64)

* fix(orchestrator): minor fixes (#67)

* feat(orchestrator): refactoring to use data-index to fetch workflow definitions (#57)

Co-authored-by: Guilherme Caponetto <[email protected]>

* fix(orchestrator): fix sonar issue (#69)

* feat(orchestrator): use assessment process id as bk for next workflows (#70)

* feat(orchestrator): execute workflow page new UX (#68)

* fix(orchestrator): theme for `monaco-editor` in `JsonTextAreaForm` component and mandatory `dataIndexService.url` (#71)

* fix(orchestrator): remove dependency on devmode in the entity provider (#72)

* fix(orchestrator): skip it if hardcoded specs do not exist (#74)

* feat(orchestrator): execute workflow page polishing (#75)

* feat(orchestrator): workflow instance result page (#73)

* chore(orchestrator): add codeowners to orchestrator

* fix(FLPATH-852): show WF description on the execution result page (#77)

* chore(orchestrator): add OWNERS file to each orchestrator package

* feat(orchestrator): migrates to the new UI (#78)

---------

Signed-off-by: Gloria Ciavarrini <[email protected]>
Co-authored-by: Moti Asayag <[email protected]>
Co-authored-by: richard wang <[email protected]>
Co-authored-by: anludke <[email protected]>
Co-authored-by: rhkp <[email protected]>
Co-authored-by: Jonathan Kilzi <[email protected]>
Co-authored-by: Jude Niroshan <[email protected]>
Co-authored-by: Marek Libra <[email protected]>
Co-authored-by: Bat-Zion Rotman <[email protected]>
Co-authored-by: yu zhao <[email protected]>
Co-authored-by: Gloria Ciavarrini <[email protected]>
Co-authored-by: Tiago Dolphine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants