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

[BUG] can't set reindex.remote.allowlist #883

Open
karitham opened this issue Oct 23, 2024 · 6 comments
Open

[BUG] can't set reindex.remote.allowlist #883

karitham opened this issue Oct 23, 2024 · 6 comments
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@karitham
Copy link

What is the bug?

Setting reindex.remote.allowlist in spec.general.additionalConfig doesn't seem to apply.

I have been looking through the docs for this specific key but could not find it either. I am perhaps being mislead by the error message, but can't find any additional information on it.

This is the error that lead me to change the configuration:

{
  "error": {
    "root_cause": [
      {
        "type": "illegal_argument_exception",
        "reason": "[redacted] not allowlisted in reindex.remote.allowlist"
      }
    ],
    "type": "illegal_argument_exception",
    "reason": "[redacted] not allowlisted in reindex.remote.allowlist"
  },
  "status": 400
}

How can one reproduce the bug?

Here is part of my config file:

apiVersion: opensearch.opster.io/v1
kind: OpenSearchCluster
metadata:
  name: os
spec:
  security:
    config:
      adminCredentialsSecret:
        name: os-admin-creds
      securityConfigSecret:
        name: securityconfig-secret
    tls:
      http:
        generate: true
      transport:
        generate: true
  general:
    monitoring:
      enable: true
    serviceName: os
    version: 2.17.0
    pluginsList: ["repository-s3"]
    drainDataNodes: true
    additionalConfig:
      reindex.remote.allowlist: '["redacted", "redacted"]'
      s3.client.default.endpoint: minio.minio.svc.cluster.local:9000
      s3.client.default.protocol: http
      s3.client.default.path_style_access: "true"

      cluster.remote_store.state.enabled: "true"

      node.attr.remote_store.repository.remoteStore.settings.bucket: dmp-es-prod
      node.attr.remote_store.repository.remoteStore.settings.region: eu-central-1
      node.attr.remote_store.repository.remoteStore.type: s3

      node.attr.remote_store.segment.repository: remoteStore
      node.attr.remote_store.translog.repository: remoteStore
      node.attr.remote_store.state.repository: remoteStore

Looking at opensearch.yml (kubectl exec pods/os-nodes-0 -- cat /usr/share/opensearch/config/opensearch.yml) and it doesn't show up. Similarly, it doesn't show up in the dashboard.

I also don't see it in the configmap (os-config in my case).

What is the expected behavior?

For the keys to show up in the configuration file or to show up in the dashboard.

As mentioned earlier I may be being mislead by the error, since I can't find anything about this reindex in the opensearch docs.

@karitham karitham added bug Something isn't working untriaged Issues that have not yet been triaged labels Oct 23, 2024
@karitham
Copy link
Author

karitham commented Oct 24, 2024

I managed to investigate a bit more and when I query it, I get back a single string instead of the array needed by additionalConfig. Unfortunately it doesn't appear possible to pass an array directly to allowlist.

query

GET /_cluster/settings?include_defaults=true&filter_path=**.reindex.remote.allowlist

response

{
  "defaults": {
    "reindex": {
      "remote": {
        "allowlist": [
          "[\"redacted\"","\"redacted\"]"
        ]
      }
    }
  }
}

@prudhvigodithi prudhvigodithi added good first issue Good for newcomers and removed untriaged Issues that have not yet been triaged labels Oct 24, 2024
@Divyaasm
Copy link

Hi @karitham , can you contribute to this issue?

@karitham
Copy link
Author

Sure, I can contribute, but I don't know the overall design of the operator well enough.

Since the env var is set properly (and must exist as a string), then I guess it's only about editing whatever script pulls those variables from the env into the config.

@karitham
Copy link
Author

I've been exploring and it seems that the best solution would be to either:

  • Revamp the way the operator puts config into the containers, such that we don't use env and avoid having to use untyped strings anywhere
    • This would also help for discoverability, since nothing in k8s told me my options were actually being used. The env changed but I couldn't discover it anywhere else, and neither the logs nor the binary/operator spec told me whether it worked. I also tried using a bad key that doesn't exist and that just did nothing, so it might be preferable going down that route.
  • Fix the scripts that pull values from env. They seem to be in the build repo. I don't think it's fixable from there because they're untyped as well.
    • Fixing the script looks more like fixing the way the opensearch binary interprets -E flags, in which case I can't help with that (I don't know any java, so navigating that codebase is already too much).

I think both options are valid, but I might entirely be missing something.

@bshien
Copy link

bshien commented Nov 25, 2024

@prudhvigodithi @swoehrl-mw Could you help take a look at this? Thanks

@swoehrl-mw
Copy link
Collaborator

I know others reported problems with the way additionalConfig works in the past.

IMO the best option forward is to rework how additionalConfig is provided to the opensearch pods. If we switch this to a configmap that gets mounted into the pods, we can use the full yaml range in there.
This would mean changing the OpensearchCluster CRD to accept not just strings as values for additionalConfig (important is to make sure it stays backwards-compatible), and properly detect changes to the config and perform a rolling restart (right now this is implicitly detected as changes to the envvars). Best bet would be to store a hash of the configmap content as an annotation on the pods.

PRs are always welcome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
Status: 📦 Backlog
Development

No branches or pull requests

5 participants