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-8090] - OMC - Update scope.region type to reflect new API changes #10451

Closed
wants to merge 5 commits into from

Conversation

abailly-akamai
Copy link
Contributor

@abailly-akamai abailly-akamai commented May 9, 2024

Description 📝

In alpha, we're seeing a payload error when POSTing to the object-storage/keys.

This is due to this PR having been merged to alpha and adding strictness to what is passed to the body of the endpoint in the absence of regions_allowed flag (Object MultiCluster)

The fact this was not caught is due to two reasons:

  • passing a default empty region as a string
  • weakening the type definition during our development phase (which admittedly would have not prevented the bug because of bullet point 1)

Changes 🔄

  • make Scope.region required and nullable
  • reflect changes in front-end code

Target release date 🗓️

5/13/2024

Preview 📷

Before After
Screen.Recording.2024-05-08.at.22.20.50.mov
Screen.Recording.2024-05-08.at.22.20.01.mov

How to test 🧪

Prerequisites

  • use ALPHA

Reproduction steps

Verification steps (verify in both ALPHA & PROD)

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

@abailly-akamai abailly-akamai changed the title fix: [M3-8090] - OMC - Update region type to reflect API changes fix: [M3-8090] - OMC - Update scope.region type to reflect new API changes May 9, 2024
@abailly-akamai abailly-akamai self-assigned this May 9, 2024
@abailly-akamai abailly-akamai marked this pull request as ready for review May 9, 2024 02:28
@abailly-akamai abailly-akamai requested a review from a team as a code owner May 9, 2024 02:28
@abailly-akamai abailly-akamai requested review from hana-akamai, carrillo-erik, cpathipa and dwiley-akamai and removed request for a team May 9, 2024 02:28
Copy link

github-actions bot commented May 9, 2024

Coverage Report:
Base Coverage: 82%
Current Coverage: 82%

Copy link
Contributor

@jdamore-linode jdamore-linode left a comment

Choose a reason for hiding this comment

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

Thanks for tackling this @abailly-akamai!

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for the details description 📝 !

Confirmed I was able to create an access key with the flag off using alpha ✅

@@ -72,7 +72,7 @@ export const getDefaultScopes = (buckets: ObjectStorageBucket[]): Scope[] =>
bucket_name: thisBucket.label,
cluster: thisBucket.cluster,
permissions: 'none' as AccessType,
region: thisBucket.region ?? '',
region: thisBucket.region ?? null,
Copy link
Contributor

@cpathipa cpathipa May 9, 2024

Choose a reason for hiding this comment

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

The API spec states 11.1.1
If "regions" is present, but its value is "null" or an empty list, the server must reject the request.

I would suggest we could remove the regions field in this case instead making it required with null value.

region: thisBucket.region ?? undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see my comment, you need to align with the API team on this

Copy link
Member

Choose a reason for hiding this comment

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

If regions is a new field for OMC, wouldn't it makes sense to just completely omit it here?

Suggested change
region: thisBucket.region ?? null,

Copy link
Contributor Author

@abailly-akamai abailly-akamai May 9, 2024

Choose a reason for hiding this comment

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

this is why ideally we need to two types and two payloads (depending on the flag value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We shouldn't be adjusting types based on a feature's availability. It weakens type safety.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, that's a great point. I'm curious how people handle this problem in general

The only thing that comes to mind is maintaining two separate type interfaces.

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 think we've been doing those "temporary" types often, then clean them up when the feature is in production.

I put an cafe item to discuss this. the overhead of doing two interfaces/payloads is extra work but worth it IMO. some features take a long time to see the day so the chances are something like this can happen when API amends its contract without considering it a breaking change

Copy link
Contributor

Choose a reason for hiding this comment

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

In this case with two interfaces It also leads to rewrite or duplicate most of the API OBJ methods in API v4.

Copy link
Member

Choose a reason for hiding this comment

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

Also a great point @cpathipa

Static types are hard 😣

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are. Not to mention we are "lying" about what true types are in our SDK by doing it this way

packages/api-v4/src/object-storage/types.ts Show resolved Hide resolved
@abailly-akamai
Copy link
Contributor Author

@cpathipa have you checked the threads and confirmed with the API team? You may be dealing with outdated specs or miscommunication between parties.

Also

If "regions" is present, but its value is "null" or an empty list, the server must reject the request.

are we talking about the same thing here?

"regions": [
   {
     "id": "us-east",
     "s3_endpoint": "us-east-12.linodeobjects.com"
   },
   {
     "id": "us-west",
     "s3_endpoint": "us-west-99.linodeobjects.com"
   }
 ],

@abailly-akamai
Copy link
Contributor Author

@cpathipa feel free to update this PR as you see fit for your project and the upcoming release

@cpathipa
Copy link
Contributor

cpathipa commented May 9, 2024

@abailly-akamai We should be good for now for addressing the issue of having empty string in the regions field.

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Able to create limited keys ✅
Code review ✅

Comment on lines 25 to +27
export interface ScopeRequest extends Omit<Scope, 'cluster'> {
// @TODO OBJ Multicluster: Omit 'region' as well when API changes get released to prod
cluster?: string;
region?: string;
region: string | null;
Copy link
Contributor

Choose a reason for hiding this comment

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

ScopeRequest doesn't actually get used anywhere in the codebase apparently, so we can get rid of it

Screenshot 2024-05-09 at 11 54 24 AM

@abailly-akamai
Copy link
Contributor Author

closing since this breaking change has since been reverted in this APIv4 PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants