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

feature(director): s3 update, make it compatible with other providers #806

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

p-8-z
Copy link

@p-8-z p-8-z commented Jun 30, 2023

I've update aws sdk to latest version and made director/screenshots/s3 package compatible with other providers.

References

  • I have updated the documentation. PR link here
  • I have added included automated tests
  • I acknowledge that I have tested that the change is backwards-compatible
  • Original issue / feature request / discussion: here

Use case

  • director/screenshots/s3 package compatible with other providers.

Example

Tested against Oracle S3 compatibility API

  director:
    build:
      dockerfile: packages/director/Dockerfile
      context: .

    image: agoldis/sorry-cypress-director:${VERSION:-latest}
    environment:
      DASHBOARD_URL: http://localhost:8080
      MONGODB_URI: 'mongodb://mongo:27017'
      MONGODB_DATABASE: 'sorry-cypress'
      EXECUTION_DRIVER: '../execution/mongo/driver'
      SCREENSHOTS_DRIVER: '../screenshots/s3.driver'
      GITLAB_JOB_RETRIES: 'false'
      PROBE_LOGGER: "false"
      #Bucket
      S3_URL: https://namespace.compat.objectstorage.region.oraclecloud.com
      S3_REGION: eu-frankfurt-1
      S3_BUCKET: sorry-cypress
      AWS_ACCESS_KEY_ID: 'key'
      AWS_SECRET_ACCESS_KEY: 'secret'
    ports:
      - 1234:1234
    depends_on:
      - mongo

};
const url = parseUrl(`${S3_URL}/${S3_BUCKET}/${key}`);

const signedGetUrlObject = await presigner.presign(
Copy link
Collaborator

Choose a reason for hiding this comment

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

@p-8-z This one is going to be stored in the DB and used for all subsequent reads. does it have an expiration?

import {
S3_ACL,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@p-8-z can you please elaborate - was it deprecated or smth?

S3_BUCKET,
S3_FORCE_PATH_STYLE,
S3_IMAGE_KEY_PREFIX,
S3_READ_URL_PREFIX,
Copy link
Collaborator

Choose a reason for hiding this comment

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

@p-8-z I believe some users wants the read url to be completely different from the write S3 bucket because, probably we'd keep it?

@agoldis
Copy link
Collaborator

agoldis commented Jul 9, 2023

@p-8-z thanks a lot for this contribution and the documentation. Please see the inline comments with a few questions,

@p-8-z
Copy link
Author

p-8-z commented Jul 16, 2023

@agoldis my first intention was to align url singing so even the Download URL could be sign so the resources are not just publicly available.
This would allow totally private S3 storage and more privacy.
But yes your concern is right, the signing of Download URL will expire and with signing v4 algorithm the maximum time of expiration is only 7 days.

So what this means is that with current architecture of storing Download URL after Uploading this is not possible.

My question is, is it possible to split this logic of uploading and downloading?
Director will just upload resource and stores the key into the database then API service would get the key from db and calculate and sign the url on request?

@p-8-z p-8-z marked this pull request as draft July 16, 2023 13:30
@agoldis
Copy link
Collaborator

agoldis commented Jul 17, 2023

@p-8-z
Yes - it is possible, you need to generate a new signed URL every time an artifact is returned from the APO

sequenceDiagram
    box Purple Write
    participant cypress
    participant director
    participant DB
    end

    box Green Read
    participant S3
    participant browser
    participant API
    end

    cypress->>director: post artifacts
    director->>DB: save S3 object URL
    director-->>cypress: upload url
    cypress->>S3: upload
    
    browser->>API: get entity with artifacts
    API->>API: generate signed URL
    API-->>browser: signed URL
    browser->>S3: get object via signed URL
    S3-->>browser: object
   

@Rohmilchkaese
Copy link

I'm super grateful for your work @p-8-z thanks !

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.

None yet

3 participants