Skip to content

Conversation

sebastienbeau
Copy link
Member

When installing the optional module server_environment_data_encryption.

If you create an elastic backend and save it (without creating index) the index_prefix_name is correctly saved inside a data_encrypted object linked to the se.backend.

Then if you add an index, it will try to read the index_prefix_name on the related "se.backend.elastic" and break everything (index name are always empty).

I would like to understand why we have to redifine the field index_prefix_name on the se.backend.spec.abstract.

I will try to spend more time on it (for now I can live with my fix).

@simahawk can you try if you still have the issue on your side if I remove the code on V14? If yes how can I reproduce it ?

@sebastienbeau sebastienbeau added the help wanted Extra attention is needed label Mar 29, 2021
@sebastienbeau sebastienbeau requested a review from simahawk March 29, 2021 15:31
@simahawk
Copy link
Contributor

@sebastienbeau the main reason: server.env fails to find the field conf for the specific model.
That was the problem. If index prefix name is not coming from the env you'll run in trouble because you might override prod data from staging for instance. Now, if you tell me "let's fix server env" that might be feasible but IDK because inherits is implied.
Maybe a better approach, here, would be to use a computed field instead of the related.
Regarding your issue: data_enc should be tied to the specific backend and not to the generic one IMO.

@sebastienbeau
Copy link
Member Author

@simahawk thanks for your feedback I will investigate more and try to fix server env directly.
I keep this open until I propose a better solution

@sebastienbeau sebastienbeau force-pushed the 14.0-fix-server-env branch 2 times, most recently from 15b711e to 4550b54 Compare May 21, 2021 08:24
@sebastienbeau
Copy link
Member Author

@simahawk it's possible to have an example of server env config that you use for elastic ? so I can test it correctly ?
Thanks

@simahawk
Copy link
Contributor

simahawk commented Jul 8, 2021

[se_backend_algolia.whatever_tech_name]
index_prefix_name=foo
algolia_app_id = xxx
algolia_api_key = yyy
algolia_api_key_public = zzz

[se_backend_elasticsearch.whatever_tech_name]
index_prefix_name=foo
es_server_host=****

@sebastienbeau
Copy link
Member Author

Thanks

@sebastienbeau sebastienbeau changed the title [FIX] following patch break compatibility with server_environment_data_encryption [14.0][FIX] following patch break compatibility with server_environment_data_encryption Jul 13, 2021
@sebastienbeau
Copy link
Member Author

After some battle testing I think we should remove this "hack" of related field, because it's broken.

Indeed if you try the following

export SERVER_ENV_CONFIG="
[se_backend_elasticsearch.whatever_tech_name]
es_server_host=***
index_prefix_name=foo"

The value on the se_backend is set inside the field "server_env_defaults" when you open the se_backend_elasticsearch

So when you change the config

export SERVER_ENV_CONFIG="
[se_backend_elasticsearch.whatever_tech_name]
es_server_host=***
index_prefix_name=bar"

The value on the se_backend is set inside the field "server_env_defaults" is still "foo" and it will be updated when you open the se_backend_elasticsearch

So I propose to remove this code and to use the following config

export SERVER_ENV_CONFIG="
[se_backend_elasticsearch.whatever_tech_name]
es_server_host=***

[se_backend.whatever_tech_name]
index_prefix_name=bar"

With that config, it work. The only issue is that the field "index_prefix_name" is not in readonly, it's a bug in server-env, I will fix it

@sebastienbeau sebastienbeau added needs review and removed help wanted Extra attention is needed labels Nov 12, 2021
@sebastienbeau
Copy link
Member Author

@simahawk what do you think ?

@simahawk simahawk changed the title [14.0][FIX] following patch break compatibility with server_environment_data_encryption [14.0][FIX] Remove index_prefix_name from abstract model Nov 16, 2021
@simahawk
Copy link
Contributor

@sebastienbeau are you 100% that this works? Because I'm kind of sure it didn't work when I tested it locally, but it was on v13.
Before accepting it I'd like to make some tests on v14.

@sebastienbeau
Copy link
Member Author

I test it and it was working correctly locally.

@sebastienbeau sebastienbeau added this to the 14.0 milestone Dec 11, 2021
@github-actions
Copy link

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Apr 17, 2022
@github-actions github-actions bot closed this May 22, 2022
@sebastienbeau sebastienbeau reopened this Jun 11, 2022
@sebastienbeau sebastienbeau removed the stale PR/Issue without recent activity, it'll be soon closed automatically. label Jun 11, 2022
@github-actions
Copy link

github-actions bot commented Oct 9, 2022

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale PR/Issue without recent activity, it'll be soon closed automatically. label Oct 9, 2022
@github-actions github-actions bot closed this Nov 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review stale PR/Issue without recent activity, it'll be soon closed automatically.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants