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

fix: [M3-9022, M3-9027] - Filter regions by endpoints and fix unavailable regions #11432

Merged
merged 18 commits into from
Dec 19, 2024

Conversation

jaalah-akamai
Copy link
Contributor

@jaalah-akamai jaalah-akamai commented Dec 17, 2024

Description 📝

  1. Warning notices for unavailable Gen2 regions are not displaying the region names due to inconsistencies in error response formats.
  2. Regions displayed in the Object Gen2 create drawer and Access Keys list are not being filtered based on user endpoint capabilities.

Note

This code relies on the /endpoints, which is activated through customer tags and capabilities. If endpoints is not available, we fallback to using regions and clusters for bucket retrieval.

Changes 🔄

  • Regions themselves have capabilities, therefore we need to filter those by what is returned from the object-storage/endpoints endpoint. This has to be done in 3 places:
    1. The create bucket drawer when selecting a region
    2. The access key /object-storage/access-keys page when displaying regions
    3. The access key drawer when selecting "Show All"
  • For the unavailable regions, instead of passing the entire region to the component we only need the labels. This helped with some of the type differences.
  • New useObjectStorageRegions hook that filters regions by endpoints if user has Gen2 capabilities, otherwise falls back to all regions for backwards compatibility for legacy and multi-cluster.

Target release date 🗓️

Before 12/20 (Friday)

Preview 📷

Before After
bug fixed
Screenshot 2024-12-17 at 1 24 23 PM Gen2 regions should no longer be displayed if you don't have permission to access the cluster
Screenshot 2024-12-17 at 10 27 39 PM Gen2 regions showing! Screenshot 2024-12-17 at 10 27 24 PM
Screenshot 2024-12-17 at 10 18 24 PM Screenshot 2024-12-17 at 10 24 10 PM

How to test 🧪

Prerequisites

(How to setup test environment)

  • See me and I'll get you added to the engineering account with capabilities

Reproduction steps

(How to reproduce the issue, if applicable)

  • Go to /object-storage/buckets in PROD and observe unavailable regions derived from our endpoints are not displayed due to difference in how we formatted the errors.

Verification steps

(How to verify changes)

  • View locally or with preview link and observe unavailable regions should be displaying appropriately.
  • After creating a gen2 bucket, try creating an access key in that same region. You should see the bucket appear.
Author Checklists

As an Author, to speed up the review process, I considered 🤔

👀 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


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@jaalah-akamai jaalah-akamai requested a review from a team as a code owner December 17, 2024 23:26
@jaalah-akamai jaalah-akamai requested review from carrillo-erik and hkhalil-akamai and removed request for a team December 17, 2024 23:26
@jaalah-akamai jaalah-akamai added the Hotfix Hotfix: This is going to staging label Dec 17, 2024
@jaalah-akamai jaalah-akamai changed the base branch from develop to staging December 17, 2024 23:27
@jaalah-akamai jaalah-akamai self-assigned this Dec 17, 2024
@jaalah-akamai jaalah-akamai added the Release → Staging Pre-Release: Release → Staging label Dec 17, 2024

// Single pass through errors to collect all region labels
errors.forEach((error) => {
if ('endpoint' in error && error.endpoint) {
Copy link
Contributor Author

@jaalah-akamai jaalah-akamai Dec 17, 2024

Choose a reason for hiding this comment

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

The error here can be either a Gen1 or Gen2 error - hence the type guard to check if the property exists.

error: BucketError | BucketErrorGen2

interface UnavailableRegionsDisplayProps {
unavailableRegions: Region[];
interface UnavailableRegionLabelsProps {
regionLabels: string[];
Copy link
Contributor Author

@jaalah-akamai jaalah-akamai Dec 17, 2024

Choose a reason for hiding this comment

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

No need for an array of Region's since all we need are the label's. This allowed for cleaner typings since error formats are different. We couldn't pull from unavailableRegion.label since that wasn't a property on the new endpoint errors.

@jaalah-akamai
Copy link
Contributor Author

@cpathipa @coliu-akamai I'll get you access to the account for testing

Copy link

github-actions bot commented Dec 18, 2024

Coverage Report:
Base Coverage: 86.85%
Current Coverage: 86.85%

@jaalah-akamai jaalah-akamai changed the title fix: [M3-9022] - Filter regions by endpoints and fix unavailable regions fix: [M3-9022, M3-9027] - Filter regions by endpoints and fix unavailable regions Dec 18, 2024
'de-fra-2',
'sg-sin-2',
]);

Copy link
Contributor Author

@jaalah-akamai jaalah-akamai Dec 18, 2024

Choose a reason for hiding this comment

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

This is because we cannot add these to our OBJ data centers list in api until GA or until OBJ region capabilities is customer aware.

Copy link

@victoriaroan victoriaroan left a comment

Choose a reason for hiding this comment

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

vgood here: All looks good to me! I tested with both a user who has access to the limited access regions and a user who does not, and both functioned as expected/desired.

I'm only somewhat proficient with react and not at all with typescript, so my actual code review might not count for much, but I don't see anything glaringly wrong. 🙂 And as mentioned, functionality is good 👍

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

gonna review the functionality after retro! in between a lot of meetings today 😅

@coliu-akamai
Copy link
Contributor

functionality looking good too! will give a final approval/check after custom hook changes are pushed up 🚀

coliu-akamai
coliu-akamai previously approved these changes Dec 18, 2024
Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

reconfirming functionality + code review, thank you!

@jaalah-akamai jaalah-akamai added the Add'tl Approval Needed Waiting on another approval! label Dec 18, 2024
@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 466 passing tests on test run #12 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing466 Passing2 Skipped97m 48s

@@ -61,6 +63,7 @@ export const RegionSelect = <

const regionOptions = getRegionOptions({
currentCapability,
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.

This is temporary as noted in getRegionOptions until APINext makes Gen2 region capabilities customer-aware.

@@ -114,7 +117,7 @@ export const RegionSelect = <
<RegionOption
disabledOptions={disabledRegions[region.id]}
item={region}
key={key}
key={`${region.id}-${key}`}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

key which is the region label is no longer specific enough since we now can have multiple endpoints per region

@@ -60,9 +60,10 @@ export const BucketRateLimitTable = ({
<Box>
<FormLabel>
<Typography
{...typographyProps}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved the spreading of props to the top to allow overrides

regionsAffected.map((thisRegion) => (
<li key={thisRegion}>{thisRegion}</li>
regionsAffected.map((thisRegion, idx) => (
<li key={`${thisRegion}-${idx}`}>{thisRegion}</li>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same issue as above - multiple endpoints can exist in a region

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

✅ functionality as specified
✅ user without gen2 flags/capabilities doesn't see gen 2 regions (whoops typo)

thank you!

@jaalah-akamai jaalah-akamai merged commit ce7a06f into linode:staging Dec 19, 2024
22 of 23 checks passed
Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

Confirming bucket and access create flow are working as expected for legacy and gen1

Copy link

cypress bot commented Dec 19, 2024

Cloud Manager E2E    Run #7002

Run Properties:  status check failed Failed #7002  •  git commit ce7a06f613: Merge pull request #11432 from jaalah-akamai/M3-9022
Project Cloud Manager E2E
Branch Review staging
Run status status check failed Failed #7002
Run duration 30m 25s
Commit git commit ce7a06f613: Merge pull request #11432 from jaalah-akamai/M3-9022
Committer Jaalah Ramos
View all properties for this run ↗︎

Test results
Tests that failed  Failures 2
Tests that were flaky  Flaky 3
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 464
View all changes introduced in this branch ↗︎

Tests for review

Failed  cypress/e2e/core/linodes/rebuild-linode.spec.ts • 2 failed tests

View Output Video

Test Artifacts
rebuild linode > rebuilds a linode from Community StackScript Screenshots Video
rebuild linode > rebuilds a linode from Account StackScript Screenshots Video
Flakiness  switch-linode-state.spec.ts • 1 flaky test

View Output Video

Test Artifacts
switch linode state > powers off a linode from landing page Screenshots Video
Flakiness  resize-linode.spec.ts • 2 flaky tests

View Output Video

Test Artifacts
resize linode > resizes a linode by increasing size when offline: cold migration Screenshots Video
resize linode > resizes a linode by decreasing size Screenshots Video

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Add'tl Approval Needed Waiting on another approval! Hotfix Hotfix: This is going to staging Release → Staging Pre-Release: Release → Staging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants