-
Notifications
You must be signed in to change notification settings - Fork 291
Allow additionalConfig settings to overwrite default security plugin settings #1000
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
base: main
Are you sure you want to change the base?
Conversation
…ttings Signed-off-by: Craig Perkins <[email protected]>
r.reconcilerContext.AddConfig("plugins.security.system_indices.enabled", "true") | ||
r.reconcilerContext.AddConfig("plugins.security.system_indices.indices", string(systemIndices)) | ||
|
||
if len(r.instance.Spec.General.AdditionalConfig) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cwperks even with this change, if we have to add the plugins.security.audit.type
with additionalConfig
will it create 2 values of plugins.security.audit.type
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it better to add it under spec.security
?
spec:
security:
auditType: "log4j"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding @swoehrl-mw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it won't create 2 values. It will overwrite the default and log that out accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cwperks can should be able to get this in #1011, can you update the readme with some details https://github.com/opensearch-project/opensearch-k8s-operator/blob/main/docs/userguide/main.md.
Thanks
@getsaurabh02
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
apologies for not addressing this earlier, pushing a commit in a few min
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added documentation
Signed-off-by: Craig Perkins <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR correctly fixes the usage of spec.general.additionalConfig
.
additionalConfig
is meant to be added to opensearch.yml
, but untill now it's been incorrectly used as extra environment variables.
opensearch-k8s-operator/opensearch-operator/pkg/builders/cluster.go
Lines 863 to 875 in 3372c11
// Append additional config to env vars, use General.AdditionalConfig by default, overwrite with Bootstrap.AdditionalConfig | |
extraConfig := cr.Spec.General.AdditionalConfig | |
if cr.Spec.Bootstrap.AdditionalConfig != nil { | |
extraConfig = cr.Spec.Bootstrap.AdditionalConfig | |
} | |
keys := helpers.SortedKeys(extraConfig) | |
for _, k := range keys { | |
env = append(env, corev1.EnvVar{ | |
Name: k, | |
Value: extraConfig[k], | |
}) | |
} |
opensearch-k8s-operator/opensearch-operator/pkg/reconcilers/cluster.go
Lines 143 to 153 in 3372c11
extraConfig := helpers.MergeConfigs(r.instance.Spec.General.AdditionalConfig, nodePool.AdditionalConfig) | |
sts := builders.NewSTSForNodePool( | |
username, | |
r.instance, | |
nodePool, | |
nodePoolConfig.ConfigHash, | |
r.reconcilerContext.Volumes, | |
r.reconcilerContext.VolumeMounts, | |
extraConfig, | |
) |
opensearch-k8s-operator/opensearch-operator/pkg/builders/cluster.go
Lines 574 to 581 in 3372c11
// Append additional config to env vars | |
keys := helpers.SortedKeys(extraConfig) | |
for _, k := range keys { | |
sts.Spec.Template.Spec.Containers[0].Env = append(sts.Spec.Template.Spec.Containers[0].Env, corev1.EnvVar{ | |
Name: k, | |
Value: extraConfig[k], | |
}) | |
} |
Can you remove the parts that generate those extra env vars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One more thing: could you add unit tests for this change?
Description
This PR seeks to build a way to overwrite the default security plugin settings from here. I'm not able to overwrite this settings by adding
plugins.security.audit.type: log4j
tospec.general.additionalConfig
. I can confirm that these changes produce the desired output by looking at the output being printed here.I think we should also consider adding
plugins.security.allow_default_init_securityindex: true
to the default security config values because this explicitly tells OpenSearch to init the security index from any suppled yaml files.Issues Resolved
Resolves #736
Check List
make lint
)If CRDs are changed:
make manifests
) and also copied into the helm chartPlease refer to the PR guidelines before submitting this pull request.
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.