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

New Endpoint: /upload or /publish extension #18

Open
podedra92 opened this issue Jun 2, 2023 · 8 comments
Open

New Endpoint: /upload or /publish extension #18

podedra92 opened this issue Jun 2, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@podedra92
Copy link
Contributor

We currently have the Add cli command, would be beneficial to expose this via API.
This would prevent the need to exec into the pods running in the cluster to add a new extension.

@podedra92 podedra92 changed the title New Endpoint: /upload or publish endpoint New Endpoint: /upload or /publish extension Jun 2, 2023
@code-asher code-asher added the enhancement New feature or request label Aug 15, 2023
@alexp73
Copy link

alexp73 commented Jan 25, 2024

As a part of our internal project we need to implement new /add and /remove endpoints. Please, see POC below (you can copy the yaml to editor.swagger.io). The idea is to repeat CLI interface for add and remove commands with one exception: the user can provide "extension" field (to upload extensions) or "extensionUrl" (to add extension using URL).
Please, check and comment, any suggestions would be really useful. Thanks.

openapi: 3.0.3
info:
  title: Coder Marketplace New APIs
  description: |-
    This is a proposal for new "Add extension" and "Remove extension" API endpoints
  version: 0.0.1
servers:
  - url: https://example.com/api
paths:
  /add:
    post:
      tags:
        - extensions
      summary: Add extension(s)
      description: Add new extension(s) to the marketplace, extension or extension-url parameter should be provided
      operationId: addExtension
      requestBody:
        content:
          multipart/form-data:
            schema:
              type: object
              properties:
                extensions-dir:
                  type: string
                artifactory:
                  type: string
                repo:
                  type: string
                extension:
                  type: string
                  format: binary
                extensionUrl:
                  type: string
        required: true
      responses:
        '201':
          description: Extension added
        '400':
          description: Unable to add extension
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/AddApiResponse'
  /remove/{extensionId}:
    delete:
      tags:
        - extensions
      summary: Remove extension
      description: Remove an extension, one version or all versions, if "all" keyword provided
      operationId: removeExtension
      parameters:
        - name: extensionId
          in: path
          description: Extension ID or "all" keyword
          required: true
          schema:
            type: string
      responses:
        '400':
          description: Unable to remove extension
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/RemoveApiResponse'
components:
  schemas:
    RemoveApiResponse:
      type: object
      properties:
        message:
          type: string
          example: Unable to remove extension
    AddApiResponse:
      type: object
      properties:
        message:
          type: string
          example: Unable to add extension

@code-asher
Copy link
Member

This looks really good! Some thoughts:

  1. Can we omit extensions-dir, artifactory, and repo for /add? The marketplace would already know all these values. If we do need them, then I imagine the remove endpoint also needs them.
  2. I think I would expect the /remove endpoint to take all as a query parameter to mimic how the cli does it, so something like /remove/{id}?all.
  3. While the /add, /remove endpoints do make sense and I see how they mirror the cli, I think it would be more REST-like to use /extensions and /extensions/{id}.
  4. Authentication is something we will have to figure out if this gets added. Maybe it can even just be basic http auth. Or, we could make the new endpoints opt-in and users have to secure the endpoints themselves.

@alexp73
Copy link

alexp73 commented Jan 29, 2024

Thank you for your response.

  1. You are right. Just confirmed we can re-use them.
  2. Yes, I have mentioned "all" in the description and going to support it.
  3. Yes, agree with that. I will use POST /extensions and DELETE /extensions{extensionId} or DELETE /extensions{extensionId@all} (@ because is used for versions now: BREAKING CHANGE: Amend version identifier in ID #24) as you suggested.
  4. Can we do authentication as a next step or do you think it should be the part of the same PR?

@code-asher
Copy link
Member

code-asher commented Feb 1, 2024

Yes, I have mentioned "all" in the description and going to support it.
/extensions{extensionId@all}

Ohhhh I see, yes this makes sense, I misread it as /extensions/all. I like that, maybe we should do that in the cli as well, right now the cli expects remove ext --all rather than remove ext@all, but we can think about changing the cli later.

Can we do authentication as a next step or do you think it should be the part of the same PR?

It does not need to be part of the same PR, but I would say at the very least we disable these endpoints by default, just to make sure we do not accidentally release with the endpoints exposed.

@alexp73
Copy link

alexp73 commented Feb 5, 2024

Ohhhh I see, yes this makes sense, I misread it as /extensions/all. I like that, maybe we should do that in the cli as well, right now the cli expects remove ext --all rather than remove ext@all, but we can think about changing the cli later.

Agreed, let's think about cli changes later.

Can we do authentication as a next step or do you think it should be the part of the same PR?

It does not need to be part of the same PR, but I would say at the very least we disable these endpoints by default, just to make sure we do not accidentally release with the endpoints exposed.

Noted, I will disable the endpoints by default. I think I will add some environment variables support: ENABLE_PUBLISHING_ENDPOINTS, MAX_UPLOAD_FILE_SIZE, MAX_FILES and UPLOADS_FOLDER.

@code-asher
Copy link
Member

code-asher commented Feb 5, 2024

I think I will add some environment variables support: ENABLE_PUBLISHING_ENDPOINTS, MAX_UPLOAD_FILE_SIZE, MAX_FILES and UPLOADS_FOLDER.

Works for me! Should we prefix these with MARKETPLACE or CODE_MARKETPLACE (a bit long but being unique seems like a good idea)?

@alexp73
Copy link

alexp73 commented Feb 6, 2024

I think I will add some environment variables support: ENABLE_PUBLISHING_ENDPOINTS, MAX_UPLOAD_FILE_SIZE, MAX_FILES and UPLOADS_FOLDER.

Works for me! Should we prefix these with MARKETPLACE or CODER_MARKETPLACE (a bit long but being unique seems like a good idea)?

Sure, I will add the MARKETPLACE prefix.

@alexp73
Copy link

alexp73 commented Feb 22, 2024

Quick update on this one. Functionally, everything is done and UT added but the changes are still inside our organisation. We are going to setup the process to contribute the changes back, using our organisation's account. It will take some time (2-3 weeks, I hope not more). In the mean time, I'm starting to work on protecting these endpoints by authentication.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants