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

adds basic checking of subscription state #52

Merged
merged 9 commits into from
Apr 23, 2024

Conversation

s-amann
Copy link
Contributor

@s-amann s-amann commented Apr 10, 2024

What this PR does

Before this PR:
Subscription state was not validated when making requests belonging to a subscription

After this PR:
Subscription state is validated, meaning, requests must be using a subscriptionID which has a 'registered' state stored in the frontends cache.

To add any subscription as registered to the frontend cache run

curl -X PUT localhost:8443/subscriptions/SUBSCRIPTION_ID?api-version=2.0 -H 'content-type: application/json' -d '{"state":"Registered"}'

Jira: ARO-6313
Link to demo recording:

Special notes for your reviewer

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

  • PR: The PR description is expressive enough and will help future contributors
  • Code: Write code that humans can understand and Keep it simple
  • Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
  • Upgrade: Impact of this change on upgrade flows was considered and addressed if required
  • Deployment: The deployment process was considered and addressed if required
  • Testing: New code requires new unit tests.
  • Documentation: Is the documentation updated? Either in the doc located in focus area, in the README or in the code itself.
  • Customers: Is this change affecting customers? Is the release plan considered?

Copy link

@AldoFusterTurpin AldoFusterTurpin left a comment

Choose a reason for hiding this comment

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

I know it is in draft mode, but just two minor suggestions in case they make sense to you. Thanks! 🙂

frontend/middleware_validatesubscription.go Show resolved Hide resolved
frontend/middleware_validatesubscription.go Show resolved Hide resolved
@s-amann s-amann marked this pull request as ready for review April 16, 2024 13:05
mbarnes
mbarnes previously approved these changes Apr 16, 2024
Copy link
Collaborator

@mbarnes mbarnes left a comment

Choose a reason for hiding this comment

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

Had a suggestion that would require a bit of refactoring, so I'll leave it up to you. LGTM otherwise.

frontend/middleware_validatesubscription.go Show resolved Hide resolved
frontend/middleware_validatesubscription.go Outdated Show resolved Hide resolved
frontend/middleware_validatesubscription.go Show resolved Hide resolved
Copy link
Contributor

@bennerv bennerv left a comment

Choose a reason for hiding this comment

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

can we get jira link in PR?

frontend/frontend.go Show resolved Hide resolved
frontend/middleware_validatesubscription.go Outdated Show resolved Hide resolved
frontend/middleware_validatesubscription.go Show resolved Hide resolved
frontend/middleware_validatesubscription.go Outdated Show resolved Hide resolved
@s-amann s-amann force-pushed the check-subscription-state branch from ced317c to 08bdf14 Compare April 16, 2024 19:12
Copy link
Collaborator

@mbarnes mbarnes left a comment

Choose a reason for hiding this comment

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

Couple questions about API versions and the subscription endpoint:

  • Can we remove /internal/api/subscription/register.go?
    I was going to suggest using a different middleware chain for this endpoint that omits MiddlewareValidateAPIVersion (since that's only meant for our API versions) but it looks like you've effectively done that already.

  • Should ArmSubscriptionAction be verfiying the api-version parameter is "2.0"?

@s-amann
Copy link
Contributor Author

s-amann commented Apr 17, 2024

Couple questions about API versions and the subscription endpoint:

  • Can we remove /internal/api/subscription/register.go?
    I was going to suggest using a different middleware chain for this endpoint that omits MiddlewareValidateAPIVersion (since that's only meant for our API versions) but it looks like you've effectively done that already.

I can delete subscription/register.go - I only needed this before removing the endpoint from our api version validation.

  • Should ArmSubscriptionAction be verfiying the api-version parameter is "2.0"?

I'm not sure about this, the docs say "system version 2.0" will be used, but it doesn't say anything about requiring 2.0

mbarnes
mbarnes previously approved these changes Apr 17, 2024
Copy link
Collaborator

@mbarnes mbarnes left a comment

Choose a reason for hiding this comment

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

LGTM. 👍 on moving subscription structs into the arm package.

bennerv
bennerv previously approved these changes Apr 18, 2024
@s-amann s-amann dismissed stale reviews from bennerv and mbarnes via 21dd948 April 22, 2024 12:11
@mjlshen mjlshen merged commit ed0bc71 into Azure:main Apr 23, 2024
4 checks passed
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.

5 participants