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

Improvement/osis 145 disable get usage api #139

Merged
merged 3 commits into from
Jan 10, 2024

Conversation

anurag4DSB
Copy link
Contributor

@anurag4DSB anurag4DSB commented Jan 9, 2024

PR Summary: Modification of getUsage API Behavior

Changes Introduced:

  • Altered the getUsage API to consistently return a NotImplemented status, regardless of UTAPI enablement. This ensures uniformity in API responses and decouples the behavior from UTAPI's operational state.

Implications:

  • The changes are primarily at the OSIS level and do not impact the existing setup in the Federation.
  • Federation retains its role in determining UTAPI enablement and configuring OSIS accordingly. However, even when Federation/S3C acknowledges UTAPI as enabled and configures OSIS, this will not activate UTAPI features in OSIS.

Federation Impact:

  • No alterations are required in Federation, except for an update to OSIS.
  • Future solutions to this implementation will require Java expertise, as Federation components remain unaffected.

Code and Output Samples:

  • The getUsage API now returns the following output:
$ curl -X 'GET' 'http://localhost:8443/api/v1/usage?tenant_id=123aaa4' -H 'accept: application/json' -H 'Authorization: Basic RDRJVDJBV1NCNTg4R081SjlUMDA6VUVFdTh0WWxzT0dHcmdmNERBaVNaRDZhcFZOUFVXcVJpUEcwblRCNg=='
{"timestamp":"2024-01-09T15:03:41.619+00:00","status":501,"error":"Not Implemented","path":"/api/v1/usage"}%
  • The getInfo API's not_implemented list now includes getUsage:
$curl -X 'GET' 'http://127.0.0.1:8443/api/info' -H 'accept: application/json'
{"platform_name":"scality","platform_version":"15.2.3","api_version":"1.0.0","status":"NORMAL","not_implemented":["getBucketLoggingId","deleteTenant","getUsage"],"logo_uri":"http://127.0.0.1:8443/scalitylogo.png","auth_modes":["Basic"],"services":{"s3":"http://localhost:8000","iam":"http://127.0.0.1:8443/"},"regions":["us-east-1"],"storage_classes":["default"],"iam":true}%

Generated by Copilot:

This pull request includes various changes to different files in the codebase. The most important changes are as follows:

@anurag4DSB anurag4DSB force-pushed the improvement/OSIS-145-disable-get-usage-API branch from 90bc170 to cc383dc Compare January 9, 2024 15:13
Comment on lines 32 to 20
// HttpURLConnection connection = null;
// try {
// // check if Utapi Endpoint is reachable
// connection = (HttpURLConnection) new java.net.URL(appEnv.getUtapiEndpoint()).openConnection();
// connection.setConnectTimeout(appEnv.getUtapiHealthCheckTimeout());
// connection.connect();
// } catch (SocketTimeoutException e) {
// logger.warn("Failed to connect to Utapi endpoint {} in timeout {}",appEnv.getUtapiEndpoint(), appEnv.getUtapiHealthCheckTimeout());
// return Health.down()
// .withDetail("error", e.getMessage())
// .build();
// } catch (IOException e) {
// logger.warn("an I/O error occurs while trying to connect {}.",appEnv.getUtapiEndpoint());
// return Health.down()
// .withDetail("error", e.getMessage())
// .build();
// } finally {
// if (connection != null) {
// connection.disconnect();
// }
// }
return Health.down().build();
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 will be refactored in the future as there are some edge cases where not setting the right configuration causes unexpected issues. Hence even if this is not callable as we commented code below, we have commented this code.

Choose a reason for hiding this comment

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

Can we just delete it, as it stays in the git history forever?

@anurag4DSB anurag4DSB marked this pull request as ready for review January 9, 2024 15:22
@anurag4DSB anurag4DSB requested a review from a team as a code owner January 9, 2024 15:22
Comment on lines 32 to 20
// HttpURLConnection connection = null;
// try {
// // check if Utapi Endpoint is reachable
// connection = (HttpURLConnection) new java.net.URL(appEnv.getUtapiEndpoint()).openConnection();
// connection.setConnectTimeout(appEnv.getUtapiHealthCheckTimeout());
// connection.connect();
// } catch (SocketTimeoutException e) {
// logger.warn("Failed to connect to Utapi endpoint {} in timeout {}",appEnv.getUtapiEndpoint(), appEnv.getUtapiHealthCheckTimeout());
// return Health.down()
// .withDetail("error", e.getMessage())
// .build();
// } catch (IOException e) {
// logger.warn("an I/O error occurs while trying to connect {}.",appEnv.getUtapiEndpoint());
// return Health.down()
// .withDetail("error", e.getMessage())
// .build();
// } finally {
// if (connection != null) {
// connection.disconnect();
// }
// }
return Health.down().build();

Choose a reason for hiding this comment

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

Can we just delete it, as it stays in the git history forever?

@@ -1 +1 @@
* @scality/object-lead @XinLiScality

Choose a reason for hiding this comment

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

😢

Suggested change
* @scality/object-lead @XinLiScality
* @scality/object-lead @XinLiScality

build.gradle Outdated
Copy link

@williamlardier williamlardier Jan 9, 2024

Choose a reason for hiding this comment

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

should the commit message say 2.2.2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it should, I will amend it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@anurag4DSB anurag4DSB force-pushed the improvement/OSIS-145-disable-get-usage-API branch from cc383dc to 2932e33 Compare January 10, 2024 13:52
@anurag4DSB
Copy link
Contributor Author

@williamlardierI removed the commented code in intermediate fix up commit 319f6e4

Copy link

@benzekrimaha benzekrimaha left a comment

Choose a reason for hiding this comment

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

lgtm

GetUsage API is disabled to unblock RING v9.3 release.
Removed Xin as she is not a part of Scality and changed lead group to
object. We still have branch protection rules in place for main branch.
This rule does not allow any merges without 2 approvals.
@anurag4DSB anurag4DSB force-pushed the improvement/OSIS-145-disable-get-usage-API branch from 2932e33 to 55c9125 Compare January 10, 2024 16:36
@anurag4DSB anurag4DSB merged commit 57ec0d9 into main Jan 10, 2024
6 checks passed
@anurag4DSB anurag4DSB deleted the improvement/OSIS-145-disable-get-usage-API branch January 10, 2024 16:50
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.

4 participants