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

Extension registration #2750

Conversation

samuelcostae
Copy link
Contributor

Description

Draft PR to get feedback and questions .
This changes aim to Provide an endpoint where Extension can register themselves in the cluster and list its protected indices that should not be accessed by any user.

Category: new feature

Issues Resolved

#2595

Testing

A unique unit test added so far to assist devlopment

Check List

  • New functionality includes testing
  • New functionality has been documented
  • [x ] Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

It looks like this is duplicating the list of extensions that are specified by the OpenSearch extension yaml file and then loaded into the extension manager.

What is you're thinking about reconcile having two sources of information about extensions?

@samuelcostae
Copy link
Contributor Author

@peternied Thanks a million for the offer, I eventually figured out the put the request was arriving at the new class, and it was because i hadn't implemented a Validator (Added a dummy one in the meantime to always return true).

I have two questions here regarding the requests. I'm not sure what is the process to add a document to a indice in the OS cluster. In the draft PR i have i used
client.index(new IndexRequest("index_name").source(contentAsNode));

Is that the correct approach to add a document in a cluster?

I also think im missing something pretty obvious regarding why the request's body im sending here on ExtensionRegistrationApiActionTest.java

RestHelper.HttpResponse responsea = keyStoreRestHelper.executePutRequest( ENDPOINT, correctExtensionRequest);

AS the body / content is arriving empty, even though from other tests it seems pretty straight forward.

Thanks

@samuelcostae samuelcostae reopened this May 9, 2023
@samuelcostae
Copy link
Contributor Author

samuelcostae commented May 9, 2023

It looks like this is duplicating the list of extensions that are specified by the OpenSearch extension yaml file and then loaded into the extension manager.

What is you're thinking about reconcile having two sources of information about extensions?

From the discussion on #2553, I had understood that extensions would communicate with OS or Security via REST requests, mainly the posts before/after this @cwperks 's post: #2553 (comment) .

What is you're thinking about reconcile having two sources of information about extensions?

Having two sources is most of times not good, on this PR I had no intention to add a new source of truth for the extension data. My understanding is that wherever the extension would have its information stored (either the extensions.yaml or whichever external souce to be defined/discussed in the future), this endpoint would be touched with that info and the relevant info would be saved to the index.

But I might be missing some context regarding how the best approach to saving info to internally an index.


@Override
protected void handlePut(RestChannel channel, final RestRequest request, final Client client, final JsonNode content) throws IOException {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The request body arrives here empty .

An im missing something obvious?

@peternied
Copy link
Member

We spoke about this offline but to recap, #2595 doesn't seem critical path and introduces complexity that is better addressed later when more of the systems have matured.

With the current constraints, I do not see a path forward for this pull request and I am closing it out.

@peternied peternied closed this May 9, 2023
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.

2 participants