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.

Sign up for GitHub

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

feat(update-security-json): Update security.json when secret is updated #744

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(update-security-json): Update security.json when secret is updated #744

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1a7bb57
feat(update-security-json): Update security.json when secret is updated
mcarroll1 Jan 6, 2025
d79db2d
updating test to use basic auth
mcarroll1 Jan 16, 2025
285c662
address probesRequireAuth copy issue
mcarroll1 Feb 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 17 additions & 7 deletions api/v1beta1/solrcloud_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,13 @@ package v1beta1

import (
"fmt"
"github.com/go-logr/logr"
zkApi "github.com/pravega/zookeeper-operator/api/v1beta1"
"math/rand"
"strconv"
"strings"

"github.com/go-logr/logr"
zkApi "github.com/pravega/zookeeper-operator/api/v1beta1"

"k8s.io/apimachinery/pkg/util/intstr"

corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -681,7 +682,8 @@ type ManagedUpdateOptions struct {
// The maximum number of pods that can be unavailable during the update.
// Value can be an absolute number (ex: 5) or a percentage of the desired number of pods (ex: 10%).
// Absolute number is calculated from percentage by rounding down.
// If the provided number is 0 or negative, then all pods will be allowed to be updated in unison.
// If the provided number is 0, then all pods will be allowed to be updated in unison.
// Negatives are not allowed.
//
// Defaults to 25%.
//
Expand All @@ -691,7 +693,8 @@ type ManagedUpdateOptions struct {
// The maximum number of replicas for each shard that can be unavailable during the update.
// Value can be an absolute number (ex: 5) or a percentage of replicas in a shard (ex: 25%).
// Absolute number is calculated from percentage by rounding down.
// If the provided number is 0 or negative, then all replicas will be allowed to be updated in unison.
// If the provided number is 0 , then all replicas will be allowed to be updated in unison.
// Negatives are not allowed.
//
// Defaults to 1.
//
Expand Down Expand Up @@ -1621,6 +1624,15 @@ const (
Basic AuthenticationType = "Basic"
)

type BootstrapSecurityJson struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

[-0] If I'm reading this right, the YAML path for this property would be .solrSecurity.bootstrapSecurityJson.bootstrapSecurityJson?

Would have to think a bit about how to fix it, but the redundancy there feels like it'd be confusing to users IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, this is a bit ugly. I'll wait for resolution on the naming change before changing this

SecurityJsonSecret *corev1.SecretKeySelector `json:"bootstrapSecurityJson,omitempty"`

// Flag to indicate if the operator should overwrite an existing security.json if there are changes
// as compared to the underlying secret
// +optional
Overwrite bool `json:"overwrite,omitempty"`
}

type SolrSecurityOptions struct {
// Indicates the authentication plugin type that is being used by Solr; for now only "Basic" is supported by the
// Solr operator but support for other authentication plugins may be added in the future.
Expand Down Expand Up @@ -1649,7 +1661,5 @@ type SolrSecurityOptions struct {

// Configure a user-provided security.json from a secret to allow for advanced security config.
// If not specified, the operator bootstraps a security.json with basic auth enabled.
// This is a bootstrapping config only; once Solr is initialized, the security config should be managed by the security API.
// +optional
BootstrapSecurityJson *corev1.SecretKeySelector `json:"bootstrapSecurityJson,omitempty"`
BootstrapSecurityJson *BootstrapSecurityJson `json:"bootstrapSecurityJson,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

[0] I'm a bit torn on the CRD/interface for this functionality. Continuing to have "bootstrap" in the name feels a little misleading if this becomes something that isn't just about initial creation.

But is it worth the hassle of deprecating this section and creating a nearly identical one with a clearer name, idk.

Ignore my musing here, just thinking aloud.

}
22 changes: 21 additions & 1 deletion api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

52 changes: 31 additions & 21 deletions config/crd/bases/solr.apache.org_solrclouds.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10205,29 +10205,37 @@ spec:
description: |-
Configure a user-provided security.json from a secret to allow for advanced security config.
If not specified, the operator bootstraps a security.json with basic auth enabled.
This is a bootstrapping config only; once Solr is initialized, the security config should be managed by the security API.
properties:
key:
description: The key of the secret to select from. Must be
a valid secret key.
type: string
name:
default: ""
bootstrapSecurityJson:
description: SecretKeySelector selects a key of a Secret.
properties:
key:
description: The key of the secret to select from. Must
be a valid secret key.
type: string
name:
default: ""
description: |-
Name of the referent.
This field is effectively required, but due to backwards compatibility is
allowed to be empty. Instances of this type with an empty value here are
almost certainly wrong.
More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
type: string
optional:
description: Specify whether the Secret or its key must
be defined
type: boolean
required:
- key
type: object
x-kubernetes-map-type: atomic
overwrite:
description: |-
Name of the referent.
This field is effectively required, but due to backwards compatibility is
allowed to be empty. Instances of this type with an empty value here are
almost certainly wrong.
More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names
type: string
optional:
description: Specify whether the Secret or its key must be
defined
Flag to indicate if the operator should overwrite an existing security.json if there are changes
as compared to the underlying secret
type: boolean
required:
- key
type: object
x-kubernetes-map-type: atomic
probesRequireAuth:
description: |-
Flag to indicate if the configured HTTP endpoint(s) used for the probes require authentication; defaults
Expand Down Expand Up @@ -10428,7 +10436,8 @@ spec:
The maximum number of pods that can be unavailable during the update.
Value can be an absolute number (ex: 5) or a percentage of the desired number of pods (ex: 10%).
Absolute number is calculated from percentage by rounding down.
If the provided number is 0 or negative, then all pods will be allowed to be updated in unison.
If the provided number is 0, then all pods will be allowed to be updated in unison.
Negatives are not allowed.

Defaults to 25%.
x-kubernetes-int-or-string: true
Expand All @@ -10440,7 +10449,8 @@ spec:
The maximum number of replicas for each shard that can be unavailable during the update.
Value can be an absolute number (ex: 5) or a percentage of replicas in a shard (ex: 25%).
Absolute number is calculated from percentage by rounding down.
If the provided number is 0 or negative, then all replicas will be allowed to be updated in unison.
If the provided number is 0 , then all replicas will be allowed to be updated in unison.
Negatives are not allowed.

Defaults to 1.
x-kubernetes-int-or-string: true
Expand Down
28 changes: 19 additions & 9 deletions controllers/solrcloud_controller_basic_auth_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,12 @@ var _ = FDescribe("SolrCloud controller - Basic Auth", func() {
solrCloud.Spec.SolrSecurity = &solrv1beta1.SolrSecurityOptions{
AuthenticationType: solrv1beta1.Basic,
BasicAuthSecret: basicAuthSecretName,
BootstrapSecurityJson: &corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{Name: "my-security-json"},
Key: util.SecurityJsonFile,
BootstrapSecurityJson: &solrv1beta1.BootstrapSecurityJson{
SecurityJsonSecret: &corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{Name: "my-security-json"},
Key: util.SecurityJsonFile,
},
Overwrite: false,
},
}
})
Expand Down Expand Up @@ -324,10 +327,11 @@ func expectBasicAuthConfigOnPodTemplateWithGomega(g Gomega, solrCloud *solrv1bet
// if the zookeeperRef has ACLs set, verify the env vars were set correctly for this initContainer
allACL, _ := solrCloud.Spec.ZookeeperRef.GetACLs()
if allACL != nil {
g.Expect(expInitContainer.Env).To(HaveLen(10), "Wrong number of env vars using ACLs and Basic Auth")
g.Expect(expInitContainer.Env[len(expInitContainer.Env)-2].Name).To(Equal("SOLR_OPTS"), "Env var SOLR_OPTS is misplaced the Solr Pod env vars")
g.Expect(expInitContainer.Env[len(expInitContainer.Env)-1].Name).To(Equal("SECURITY_JSON"), "Env var SECURITY_JSON is misplaced the Solr Pod env vars")
testACLEnvVarsWithGomega(g, expInitContainer.Env[3:len(expInitContainer.Env)-2], true)
g.Expect(expInitContainer.Env).To(HaveLen(11), "Wrong number of env vars using ACLs and Basic Auth")
g.Expect(expInitContainer.Env[len(expInitContainer.Env)-3].Name).To(Equal("SOLR_OPTS"), "Env var SOLR_OPTS is misplaced the Solr Pod env vars")
g.Expect(expInitContainer.Env[len(expInitContainer.Env)-2].Name).To(Equal("SECURITY_JSON"), "Env var SECURITY_JSON is misplaced the Solr Pod env vars")
g.Expect(expInitContainer.Env[len(expInitContainer.Env)-1].Name).To(Equal("SECURITY_JSON_OVERWRITE"), "Env var SECURITY_JSON_OVERWRITE is misplaced the Solr Pod env vars")
testACLEnvVarsWithGomega(g, expInitContainer.Env[3:len(expInitContainer.Env)-3], true)
} // else this ref not using ACLs

expectPutSecurityJsonInZkCmd(g, expInitContainer)
Expand All @@ -353,9 +357,15 @@ func expectPutSecurityJsonInZkCmd(g Gomega, expInitContainer *corev1.Container)
g.Expect(expInitContainer).To(Not(BeNil()), "Didn't find the setup-zk InitContainer in the sts!")
expCmd := "solr zk cp zk:/security.json /tmp/current_security.json >/dev/null 2>&1; " +
"GET_CURRENT_SECURITY_JSON_EXIT_CODE=$?; if [ ${GET_CURRENT_SECURITY_JSON_EXIT_CODE} -eq 0 ]; then " +
"if [ ! -s /tmp/current_security.json ] || grep -q '^{}$' /tmp/current_security.json ]; then " +
"if [ ! -s /tmp/current_security.json ] || grep -q '^{}$' /tmp/current_security.json; then " +
"echo $SECURITY_JSON > /tmp/security.json; solr zk cp /tmp/security.json zk:/security.json >/dev/null 2>&1; " +
" echo 'Blank security.json found. Put new security.json in ZK'; fi; elif [ ${GET_CURRENT_SECURITY_JSON_EXIT_CODE} -eq 1 ]; then " +
" echo 'Blank security.json found. Put new security.json in ZK'; " +
"elif [ \"${SECURITY_JSON_OVERWRITE}\" = true ] && [ \"$(cat /tmp/current_security.json)\" != \"$(echo $SECURITY_JSON)\" ]; then " +
" echo $SECURITY_JSON > /tmp/security.json; solr zk cp /tmp/security.json zk:/security.json >/dev/null 2>&1; " +
" echo 'Diff found. Overwriting security.json in ZK'; " +
" else " +
" echo 'Not overwriting security.json'; fi; " +
"elif [ ${GET_CURRENT_SECURITY_JSON_EXIT_CODE} -eq 1 ]; then " +
" echo $SECURITY_JSON > /tmp/security.json; solr zk cp /tmp/security.json zk:/security.json >/dev/null 2>&1; " +
" echo 'No security.json found. Put new security.json in ZK'; fi"
g.Expect(expInitContainer.Command[2]).To(ContainSubstring(expCmd), "setup-zk initContainer not configured to bootstrap security.json!")
Expand Down
2 changes: 1 addition & 1 deletion controllers/solrcloud_controller_tls_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -937,7 +937,7 @@ func expectZkSetupInitContainerForTLSWithGomega(g Gomega, solrCloud *solrv1beta1
g.Expect(zkSetupInitContainer.Command[2]).To(ContainSubstring(expChrootCmd), "ZK Setup command does init the chroot")
expNumVars := 3
if solrCloud.Spec.SolrSecurity != nil && solrCloud.Spec.SolrSecurity.BasicAuthSecret == "" {
expNumVars = 4 // one more for SECURITY_JSON
expNumVars = 5 // two more for SECURITY_JSON and SECURITY_JSON_OVERWRITE
}
g.Expect(zkSetupInitContainer.Env).To(HaveLen(expNumVars), "Wrong number of envVars for zk-setup init container")
}
Expand Down
36 changes: 23 additions & 13 deletions controllers/util/solr_security_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,20 @@ import (
b64 "encoding/base64"
"encoding/json"
"fmt"
"math/rand"
"regexp"
"strings"
"time"

solr "github.com/apache/solr-operator/api/v1beta1"
"github.com/apache/solr-operator/controllers/util/solr_api"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"math/rand"
"regexp"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"strings"
"time"
)

const (
Expand All @@ -49,10 +50,11 @@ const (
// Utility struct holding security related config and objects resolved at runtime needed during reconciliation,
// such as the secret holding credentials the operator should use to make calls to secure Solr
type SecurityConfig struct {
SolrSecurity *solr.SolrSecurityOptions
CredentialsSecret *corev1.Secret
SecurityJson string
SecurityJsonSrc *corev1.EnvVarSource
SolrSecurity *solr.SolrSecurityOptions
CredentialsSecret *corev1.Secret
SecurityJson string
SecurityJsonSrc *corev1.EnvVarSource
SecurityJsonOverwrite bool
}

// Given a SolrCloud instance and an API service client, produce a SecurityConfig needed to enable Solr security
Expand Down Expand Up @@ -117,6 +119,7 @@ func reconcileForBasicAuthWithBootstrappedSecurityJson(ctx context.Context, clie

// supply the bootstrap security.json to the initContainer via a simple BASE64 encoding env var
security.SecurityJson = string(bootstrapSecret.Data[SecurityJsonFile])
security.SecurityJsonOverwrite = false
basicAuthSecret = authSecret
}

Expand All @@ -136,6 +139,7 @@ func reconcileForBasicAuthWithBootstrappedSecurityJson(ctx context.Context, clie
} else {
// stash this so we can configure the setup-zk initContainer to bootstrap the security.json in ZK
security.SecurityJson = string(bootstrapSecret.Data[SecurityJsonFile])
security.SecurityJsonOverwrite = false
security.SecurityJsonSrc = &corev1.EnvVarSource{
SecretKeyRef: &corev1.SecretKeySelector{
LocalObjectReference: corev1.LocalObjectReference{Name: bootstrapSecret.Name}, Key: SecurityJsonFile}}
Expand Down Expand Up @@ -166,13 +170,14 @@ func reconcileForBasicAuthWithUserProvidedSecret(ctx context.Context, client *cl

// is there a user-provided security.json in a secret?
// in this config, we don't need to enforce the user providing a security.json as they can bootstrap the security.json however they want
if sec.BootstrapSecurityJson != nil {
securityJson, err := loadSecurityJsonFromSecret(ctx, client, sec.BootstrapSecurityJson, instance.Namespace)
if sec.BootstrapSecurityJson != nil && sec.BootstrapSecurityJson.SecurityJsonSecret != nil {
securityJson, err := loadSecurityJsonFromSecret(ctx, client, sec.BootstrapSecurityJson.SecurityJsonSecret, instance.Namespace)
if err != nil {
return nil, err
}
security.SecurityJson = securityJson
security.SecurityJsonSrc = &corev1.EnvVarSource{SecretKeyRef: sec.BootstrapSecurityJson}
security.SecurityJsonSrc = &corev1.EnvVarSource{SecretKeyRef: sec.BootstrapSecurityJson.SecurityJsonSecret}
security.SecurityJsonOverwrite = sec.BootstrapSecurityJson.Overwrite
} // else no user-provided secret, no sweat for us

return security, nil
Expand Down Expand Up @@ -240,11 +245,16 @@ func cmdToPutSecurityJsonInZk() string {
cmd := " solr zk cp zk:/security.json /tmp/current_security.json >/dev/null 2>&1; " +
" GET_CURRENT_SECURITY_JSON_EXIT_CODE=$?; " +
"if [ ${GET_CURRENT_SECURITY_JSON_EXIT_CODE} -eq 0 ]; then " + // JSON already exists
"if [ ! -s /tmp/current_security.json ] || grep -q '^{}$' /tmp/current_security.json ]; then " + // File doesn't exist, is empty, or is just '{}'
Copy link
Contributor

Choose a reason for hiding this comment

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

[Q] I guess the unmatched ] is a bash syntax error that pre-exists your PR? I wonder how we didn't notice it sooner: presumably it would've caused errors for the initContainer or elsewhere?

Anyways, great catch!

"if [ ! -s /tmp/current_security.json ] || grep -q '^{}$' /tmp/current_security.json; then " + // File doesn't exist, is empty, or is just '{}'
" echo $SECURITY_JSON > /tmp/security.json;" +
" solr zk cp /tmp/security.json zk:/security.json >/dev/null 2>&1; " +
" echo 'Blank security.json found. Put new security.json in ZK'; " +
"fi; " + // TODO: Consider checking a diff and still applying over the top
"elif [ \"${SECURITY_JSON_OVERWRITE}\" = true ] && [ \"$(cat /tmp/current_security.json)\" != \"$(echo $SECURITY_JSON)\" ]; then " + // We want to overwrite the security config if there's a diff
Copy link
Contributor

Choose a reason for hiding this comment

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

[Q] Am I reading this correctly, that the first two blocks both upload the JSON and only differ in the message they echo out? There might be a better way to structure that to avoid the duplication...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed that this bit of bash could probably be optimized. I don't immediately see a way of re-arranging the conditions. Maybe I can just create function that takes "message" as an input?

" echo $SECURITY_JSON > /tmp/security.json;" +
" solr zk cp /tmp/security.json zk:/security.json >/dev/null 2>&1; " +
" echo 'Diff found. Overwriting security.json in ZK'; " +
" else " +
" echo 'Not overwriting security.json'; fi; " +
"elif [ ${GET_CURRENT_SECURITY_JSON_EXIT_CODE} -eq 1 ]; then " + // JSON doesn't exist, but not other error types
" echo $SECURITY_JSON > /tmp/security.json;" +
" solr zk cp /tmp/security.json zk:/security.json >/dev/null 2>&1; " +
Expand Down
8 changes: 5 additions & 3 deletions controllers/util/solr_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ package util

import (
"fmt"
"sort"
"strconv"
"strings"

solr "github.com/apache/solr-operator/api/v1beta1"
appsv1 "k8s.io/api/apps/v1"
corev1 "k8s.io/api/core/v1"
Expand All @@ -28,9 +32,6 @@ import (
"k8s.io/apimachinery/pkg/util/intstr"
"k8s.io/utils/pointer"
"k8s.io/utils/ptr"
"sort"
"strconv"
"strings"
)

const (
Expand Down Expand Up @@ -1263,6 +1264,7 @@ func generateZKInteractionInitContainer(solrCloud *solr.SolrCloud, solrCloudStat

if security != nil && security.SecurityJson != "" {
envVars = append(envVars, corev1.EnvVar{Name: "SECURITY_JSON", ValueFrom: security.SecurityJsonSrc})
envVars = append(envVars, corev1.EnvVar{Name: "SECURITY_JSON_OVERWRITE", Value: strconv.FormatBool(security.SecurityJsonOverwrite)})
if solrCloud.Spec.SolrZkOpts != "" {
envVars = append(envVars, corev1.EnvVar{Name: "ZKCLI_JVM_FLAGS", Value: solrCloud.Spec.SolrZkOpts})
}
Expand Down
3 changes: 3 additions & 0 deletions docs/solr-cloud/solr-cloud-crd.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1126,9 +1126,12 @@ spec:
bootstrapSecurityJson:
name: my-custom-security-json
key: security.json
overwrite: false
```
For `Basic` authentication, if you don't supply a `security.json` Secret, then the operator assumes you are bootstrapping the security configuration via some other means.

If `overwrite` is set to `true`, the security.json for the cluster will be updated if there is a difference between the underlying secret and the security.json in ZK.

Refer to the example `security.json` shown in the Authorization section above to help you get started crafting your own custom configuration.

#### Basic Authentication
Expand Down
Loading