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

upcoming: [M3-8306] - Hide CORS and SSL for OBJ Gen2 #10776

Merged
merged 7 commits into from
Aug 16, 2024

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Aug 13, 2024

Description 📝

CORS (Cross Origin Sharing) is not available for endpoint types E2 and E3. We will not be supporting SSL/TLS either at the moment.

Changes 🔄

For Gen2 (E2 & E3):

  • Hide CORS and display notice
  • Disable/Hide SSL/TLS Tab

Target release date 🗓️

N/A

Preview 📷

Before After
Screenshot 2024-08-12 at 10 15 02 PM Screenshot 2024-08-12 at 10 14 49 PM

How to test 🧪

Note

We have E2E tests ticket M3-8440 to cover all these scenarios separately.

Prerequisites

  • Enable MSW and ensure OBJ Gen2 flag is enabled

Verification steps

Note

This will work without refreshing the page only after you've navigated to the bucket details page, as the bucket names are randomly generated each time due to MSW.

  • Select a bucket
  • When on details page, observe SSL/TLS tab is not rendered
  • Go to Access tab and observe CORS is not rendered

As an Author I have considered 🤔

Check all that apply

  • 👀 Doing a self review
  • ❔ Our contribution guidelines
  • 🤏 Splitting feature into small PRs
  • ➕ Adding a changeset
  • 🧪 Providing/Improving test coverage
  • 🔐 Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • 👣 Providing comprehensive reproduction steps
  • 📑 Providing or updating our documentation
  • 🕛 Scheduling a pair reviewing session
  • 📱 Providing mobile support
  • ♿ Providing accessibility support

enabled &&
((isObjectStorageGen2Enabled && Boolean(endpoints)) ||
(isObjMultiClusterEnabled && Boolean(regions)) ||
Boolean(clusters));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just cleaned this up, logic should remain same.

Copy link
Contributor

Choose a reason for hiding this comment

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

Much better😮‍💨

);

const { data: bucketsData } = useObjectStorageBuckets(
isObjectStorageGen2Enabled
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to call this query when it's not Gen2, so I'm disabling it otherwise.

@jaalah-akamai jaalah-akamai marked this pull request as ready for review August 13, 2024 02:47
@jaalah-akamai jaalah-akamai requested a review from a team as a code owner August 13, 2024 02:47
@jaalah-akamai jaalah-akamai requested review from mjac0bs, hkhalil-akamai and harsh-akamai and removed request for a team August 13, 2024 02:47
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

This will work without refreshing the page only after you've navigated to the bucket details page, as the bucket names are randomly generated each time due to MSW.

Can you clarify these verification steps? I must not be reading them right/am not super familiar with this project, because I wasn't able to verify the changes. I turned the Gen2 flag on and the MSW on. (Multicluster was already on, since it's on in LD.) Then I navigated to http://localhost:3000/object-storage/buckets, and to a mock bucket's details page. I still saw CORS and SSL/TSL. I tried toggling the flag off->on + the MSW off-> on the bucket details page, and still saw CORS and SSL. Same with landing page and then navigating to bucket details.

I was unsure about the state of the Multicluster flag, but tried with it off and Gen2 on too.

…sSelect.tsx

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
@jaalah-akamai
Copy link
Contributor Author

Can you clarify these verification steps?

Seems like you had the steps right. Here's a screen recording

example.mov

Copy link

github-actions bot commented Aug 14, 2024

Coverage Report:
Base Coverage: 82.62%
Current Coverage: 82.62%

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

You know what I think happened? I must have had #10771 checked out. 🤦🏼‍♀️ I double-checked that it was a "Jaalah OBJ PR", but evidently not the right one.

✅ Confirmed CORS and SSL are hidden for OBJ Gen 2.

Screenshot 2024-08-14 at 1 41 15 PM
Screenshot 2024-08-14 at 1 41 29 PM

Approving since the lonely "Learn more." is non-blocking.

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Aug 14, 2024
@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Aug 15, 2024
@@ -208,7 +207,19 @@ export const AccessSelect = React.memo((props: Props) => {
</Link>
.
</Typography>
) : null}
) : (
// TODO: OBJGen2 - We need to handle link in upcoming PR
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we/Do we have a ticket to track this?

enabled &&
((isObjectStorageGen2Enabled && Boolean(endpoints)) ||
(isObjMultiClusterEnabled && Boolean(regions)) ||
Boolean(clusters));
Copy link
Contributor

Choose a reason for hiding this comment

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

Much better😮‍💨

@jaalah-akamai jaalah-akamai merged commit 02d51cb into linode:develop Aug 16, 2024
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Object Storage Gen2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants