Skip to content

CLOUDP-290847: Remove cert hash annotations #383

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+65 −85
Open

CLOUDP-290847: Remove cert hash annotations #383

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion controllers/om/deployment/testing_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func CreateFromReplicaSet(mongoDBImage string, forceEnterprise bool, rs *mdb.Mon
}

d.MergeReplicaSet(
replicaset.BuildFromStatefulSet(mongoDBImage, forceEnterprise, sts, rs.GetSpec(), rs.Status.FeatureCompatibilityVersion),
replicaset.BuildFromStatefulSet(mongoDBImage, forceEnterprise, sts, rs.GetSpec(), rs.Status.FeatureCompatibilityVersion, ""),
rs.Spec.AdditionalMongodConfig.ToMap(),
lastConfig.ToMap(),
zap.S(),
Expand Down
11 changes: 2 additions & 9 deletions controllers/om/process/om_process.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,15 @@ import (
mdbv1 "github.com/mongodb/mongodb-kubernetes/api/v1/mdb"
mdbmultiv1 "github.com/mongodb/mongodb-kubernetes/api/v1/mdbmulti"
"github.com/mongodb/mongodb-kubernetes/controllers/om"
"github.com/mongodb/mongodb-kubernetes/controllers/operator/certs"
"github.com/mongodb/mongodb-kubernetes/pkg/dns"
"github.com/mongodb/mongodb-kubernetes/pkg/util"
)

func CreateMongodProcessesWithLimit(mongoDBImage string, forceEnterprise bool, set appsv1.StatefulSet, dbSpec mdbv1.DbSpec, limit int, fcv string) []om.Process {
func CreateMongodProcessesWithLimit(mongoDBImage string, forceEnterprise bool, set appsv1.StatefulSet, dbSpec mdbv1.DbSpec, limit int, fcv string, tlsCertPath string) []om.Process {
hostnames, names := dns.GetDnsForStatefulSetReplicasSpecified(set, dbSpec.GetClusterDomain(), limit, dbSpec.GetExternalDomain())
processes := make([]om.Process, len(hostnames))

certificateFileName := ""
if certificateHash, ok := set.Annotations[certs.CertHashAnnotationKey]; ok {
certificateFileName = fmt.Sprintf("%s/%s", util.TLSCertMountPath, certificateHash)
}

for idx, hostname := range hostnames {
processes[idx] = om.NewMongodProcess(names[idx], hostname, mongoDBImage, forceEnterprise, dbSpec.GetAdditionalMongodConfig(), dbSpec, certificateFileName, set.Annotations, fcv)
processes[idx] = om.NewMongodProcess(names[idx], hostname, mongoDBImage, forceEnterprise, dbSpec.GetAdditionalMongodConfig(), dbSpec, tlsCertPath, set.Annotations, fcv)
}

return processes
Expand Down
8 changes: 4 additions & 4 deletions controllers/om/replicaset/om_replicaset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,15 @@ import (

// BuildFromStatefulSet returns a replica set that can be set in the Automation Config
// based on the given StatefulSet and MongoDB resource.
func BuildFromStatefulSet(mongoDBImage string, forceEnterprise bool, set appsv1.StatefulSet, dbSpec mdbv1.DbSpec, fcv string) om.ReplicaSetWithProcesses {
return BuildFromStatefulSetWithReplicas(mongoDBImage, forceEnterprise, set, dbSpec, int(*set.Spec.Replicas), fcv)
func BuildFromStatefulSet(mongoDBImage string, forceEnterprise bool, set appsv1.StatefulSet, dbSpec mdbv1.DbSpec, fcv string, tlsCertPath string) om.ReplicaSetWithProcesses {
return BuildFromStatefulSetWithReplicas(mongoDBImage, forceEnterprise, set, dbSpec, int(*set.Spec.Replicas), fcv, tlsCertPath)
}

// BuildFromStatefulSetWithReplicas returns a replica set that can be set in the Automation Config
// based on the given StatefulSet and MongoDB spec. The amount of members is set by the replicas
// parameter.
func BuildFromStatefulSetWithReplicas(mongoDBImage string, forceEnterprise bool, set appsv1.StatefulSet, dbSpec mdbv1.DbSpec, replicas int, fcv string) om.ReplicaSetWithProcesses {
members := process.CreateMongodProcessesWithLimit(mongoDBImage, forceEnterprise, set, dbSpec, replicas, fcv)
func BuildFromStatefulSetWithReplicas(mongoDBImage string, forceEnterprise bool, set appsv1.StatefulSet, dbSpec mdbv1.DbSpec, replicas int, fcv string, tlsCertPath string) om.ReplicaSetWithProcesses {
members := process.CreateMongodProcessesWithLimit(mongoDBImage, forceEnterprise, set, dbSpec, replicas, fcv, tlsCertPath)
replicaSet := om.NewReplicaSet(set.Name, dbSpec.GetMongoDBVersion())
rsWithProcesses := om.NewReplicaSetWithProcesses(replicaSet, members, dbSpec.GetMemberOptions())
rsWithProcesses.SetHorizons(dbSpec.GetHorizonConfig())
Expand Down
1 change: 0 additions & 1 deletion controllers/operator/certs/certificates.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ type certDestination string

const (
OperatorGeneratedCertSuffix = "-pem"
CertHashAnnotationKey = "certHash"
Copy link
Collaborator

Choose a reason for hiding this comment

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

q: was this annotation CertHashAnnotationKey and InternalCertAnnotationKey only used internally? User might use it for any reason? Even if not I see this as API change and we should add changelog mentioning that this has gone away + detail the reasoning behind the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is internal implementation detail. Users do not interact with this annotation: controller takes user proved secret with a certs, generates a concatenated cert and writes into a new secret. And from from that generated secret it calculates the hash which is being written into STS's annotation.

It is not an API change (since users do not interface with it). I do not think it is worth adding a change log about it.


Unused = "unused"
Database = "database"
Expand Down
13 changes: 0 additions & 13 deletions controllers/operator/construct/database_construction.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -466,14 +466,8 @@ func buildDatabaseStatefulSetConfigurationFunction(mdb databaseStatefulSetSource
appLabelKey: opts.ServiceName,
}

annotationFunc := statefulset.WithAnnotations(defaultStatefulSetAnnotations(opts.CertificateHash))
podTemplateAnnotationFunc := podtemplatespec.NOOP()

annotationFunc = statefulset.Apply(
annotationFunc,
statefulset.WithAnnotations(map[string]string{util.InternalCertAnnotationKey: opts.InternalClusterHash}),
)

if vault.IsVaultSecretBackend() {
podTemplateAnnotationFunc = podtemplatespec.Apply(podTemplateAnnotationFunc, podtemplatespec.WithAnnotations(secretsToInject.DatabaseAnnotations(mdb.GetNamespace())))
}
Expand Down Expand Up @@ -530,7 +524,6 @@ func buildDatabaseStatefulSetConfigurationFunction(mdb databaseStatefulSetSource
statefulset.WithServiceName(opts.ServiceName),
statefulset.WithReplicas(opts.Replicas),
statefulset.WithOwnerReference(opts.OwnerReference),
annotationFunc,
volumeClaimFuncs,
shareProcessNs,
statefulset.WithPodSpecTemplate(podtemplatespec.Apply(podTemplateModifications...)),
Expand Down Expand Up @@ -1057,12 +1050,6 @@ func DatabaseStartupProbe() probes.Modification {
)
}

func defaultStatefulSetAnnotations(certHash string) map[string]string {
return map[string]string{
certs.CertHashAnnotationKey: certHash,
}
}

// TODO: temprorary duplication to avoid circular imports
func NewDefaultPodSpecWrapper(podSpec mdbv1.MongoDbPodSpec) *mdbv1.PodSpecWrapper {
return &mdbv1.PodSpecWrapper{
Expand Down
8 changes: 3 additions & 5 deletions controllers/operator/construct/multicluster/multicluster_replicaset.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (

mdbv1 "github.com/mongodb/mongodb-kubernetes/api/v1/mdb"
mdbmultiv1 "github.com/mongodb/mongodb-kubernetes/api/v1/mdbmulti"
"github.com/mongodb/mongodb-kubernetes/controllers/operator/certs"
"github.com/mongodb/mongodb-kubernetes/controllers/operator/construct"
"github.com/mongodb/mongodb-kubernetes/mongodb-community-operator/pkg/util/merge"
"github.com/mongodb/mongodb-kubernetes/pkg/handler"
Expand Down Expand Up @@ -65,20 +64,19 @@ func WithStsOverride(stsOverride *appsv1.StatefulSetSpec) func(options *construc
}
}

func WithAnnotations(resourceName string, certHash string) func(options *construct.DatabaseStatefulSetOptions) {
func WithAnnotations(resourceName string) func(options *construct.DatabaseStatefulSetOptions) {
return func(options *construct.DatabaseStatefulSetOptions) {
options.Annotations = statefulSetAnnotations(resourceName, certHash)
options.Annotations = statefulSetAnnotations(resourceName)
}
}

func statefulSetName(mdbmName string, clusterNum int) string {
return fmt.Sprintf("%s-%d", mdbmName, clusterNum)
}

func statefulSetAnnotations(mdbmName string, certHash string) map[string]string {
func statefulSetAnnotations(mdbmName string) map[string]string {
return map[string]string{
handler.MongoDBMultiResourceAnnotation: mdbmName,
certs.CertHashAnnotationKey: certHash,
}
}

Expand Down
55 changes: 27 additions & 28 deletions controllers/operator/mongodbmultireplicaset_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,12 +171,32 @@ func (r *ReconcileMongoDbMultiReplicaSet) Reconcile(ctx context.Context, request
return r.updateStatus(ctx, &mrs, workflow.Failed(err), log)
}

// If tls is enabled we need to configure the "processes" array in opsManager/Cloud Manager with the
// correct tlsCertPath, with the new tls design, this path has the certHash in it(so that cert can be rotated
// without pod restart).
tlsCertPath := ""
internalClusterCertPath := ""
if mrs.Spec.Security.IsTLSEnabled() {
certSecretName := mrs.Spec.GetSecurity().MemberCertificateSecretName(mrs.Name)
internalClusterCertSecretName := mrs.Spec.GetSecurity().InternalClusterAuthSecretName(mrs.Name)
tlsCertHash := enterprisepem.ReadHashFromSecret(ctx, r.SecretClient, mrs.Namespace, certSecretName, "", log)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I see that tlsCertHash and tlsCertPath are calculated here and also in the other controllers/operator/mongodbreplicaset_controller.go. Can we move this logic to common controller?
I thought about method like:

func (r *ReconcileCommonController) tlsCertHashAndPath(ctx, ...) (string, string) {
  tlsCertHash := enterprisepem.ReadHashFromSecret(ctx, r.SecretClient, mrs.Namespace, certSecretName, "", log)
  if tlsCertHash == "" {
    return "", ""
  }
  
  return tlsCertHash, fmt.Sprintf("%s/%s", util.TLSCertMountPath, tlsCertHash)  
}

internalClusterCertHash := enterprisepem.ReadHashFromSecret(ctx, r.SecretClient, mrs.Namespace, internalClusterCertSecretName, "", log)

if internalClusterCertHash != "" {
internalClusterCertPath = fmt.Sprintf("%s%s", util.InternalClusterAuthMountPath, internalClusterCertHash)
}

if tlsCertHash != "" {
tlsCertPath = fmt.Sprintf("%s/%s", util.TLSCertMountPath, tlsCertHash)
}
}

// Recovery prevents some deadlocks that can occur during reconciliation, e.g. the setting of an incorrect automation
// configuration and a subsequent attempt to overwrite it later, the operator would be stuck in Pending phase.
// See CLOUDP-189433 and CLOUDP-229222 for more details.
if recovery.ShouldTriggerRecovery(mrs.Status.Phase != mdbstatus.PhaseRunning, mrs.Status.LastTransition) {
log.Warnf("Triggering Automatic Recovery. The MongoDB resource %s/%s is in %s state since %s", mrs.Namespace, mrs.Name, mrs.Status.Phase, mrs.Status.LastTransition)
automationConfigError := r.updateOmDeploymentRs(ctx, conn, mrs, true, log)
automationConfigError := r.updateOmDeploymentRs(ctx, conn, mrs, tlsCertPath, internalClusterCertPath, true, log)
reconcileStatus := r.reconcileMemberResources(ctx, &mrs, log, conn, projectConfig)
if !reconcileStatus.IsOK() {
log.Errorf("Recovery failed because of reconcile errors, %v", reconcileStatus)
Expand All @@ -188,7 +208,7 @@ func (r *ReconcileMongoDbMultiReplicaSet) Reconcile(ctx context.Context, request

status := workflow.RunInGivenOrder(publishAutomationConfigFirst,
func() workflow.Status {
if err := r.updateOmDeploymentRs(ctx, conn, mrs, false, log); err != nil {
if err := r.updateOmDeploymentRs(ctx, conn, mrs, tlsCertPath, internalClusterCertPath, false, log); err != nil {
return workflow.Failed(err)
}
return workflow.OK()
Expand Down Expand Up @@ -499,7 +519,7 @@ func (r *ReconcileMongoDbMultiReplicaSet) reconcileStatefulSets(ctx context.Cont
mconstruct.WithClusterNum(clusterNum),
Replicas(replicasThisReconciliation),
mconstruct.WithStsOverride(&stsOverride),
mconstruct.WithAnnotations(mrs.Name, certHash),
mconstruct.WithAnnotations(mrs.Name),
mconstruct.WithServiceName(mrs.MultiHeadlessServiceName(clusterNum)),
PodEnvVars(newPodVars(conn, projectConfig, mrs.Spec.LogLevel)),
CurrentAgentAuthMechanism(currentAgentAuthMode),
Expand Down Expand Up @@ -677,7 +697,7 @@ func (r *ReconcileMongoDbMultiReplicaSet) saveLastAchievedSpec(ctx context.Conte

// updateOmDeploymentRs performs OM registration operation for the replicaset. So the changes will be finally propagated
// to automation agents in containers
func (r *ReconcileMongoDbMultiReplicaSet) updateOmDeploymentRs(ctx context.Context, conn om.Connection, mrs mdbmultiv1.MongoDBMultiCluster, isRecovering bool, log *zap.SugaredLogger) error {
func (r *ReconcileMongoDbMultiReplicaSet) updateOmDeploymentRs(ctx context.Context, conn om.Connection, mrs mdbmultiv1.MongoDBMultiCluster, tlsCertPath, internalClusterCertPath string, isRecovering bool, log *zap.SugaredLogger) error {
reachableHostnames := make([]string, 0)

clusterSpecList, err := mrs.GetClusterSpecItems()
Expand Down Expand Up @@ -725,28 +745,7 @@ func (r *ReconcileMongoDbMultiReplicaSet) updateOmDeploymentRs(ctx context.Conte
}
log.Debugf("Existing process Ids: %+v", processIds)

certificateFileName := ""
internalClusterPath := ""

// If tls is enabled we need to configure the "processes" array in opsManager/Cloud Manager with the
// correct certFilePath, with the new tls design, this path has the certHash in it(so that cert can be rotated
// without pod restart), we can get the cert hash from any of the statefulset, here we pick the statefulset in the first cluster.
if mrs.Spec.Security.IsTLSEnabled() {
firstStatefulSet, err := r.firstStatefulSet(ctx, &mrs)
if err != nil {
return err
}

if hash := firstStatefulSet.Annotations[util.InternalCertAnnotationKey]; hash != "" {
internalClusterPath = fmt.Sprintf("%s%s", util.InternalClusterAuthMountPath, hash)
}

if certificateHash := firstStatefulSet.Annotations[certs.CertHashAnnotationKey]; certificateHash != "" {
certificateFileName = fmt.Sprintf("%s/%s", util.TLSCertMountPath, certificateHash)
}
}

processes, err := process.CreateMongodProcessesWithLimitMulti(r.imageUrls[mcoConstruct.MongodbImageEnv], r.forceEnterprise, mrs, certificateFileName)
processes, err := process.CreateMongodProcessesWithLimitMulti(r.imageUrls[mcoConstruct.MongodbImageEnv], r.forceEnterprise, mrs, tlsCertPath)
if err != nil && !isRecovering {
return err
}
Expand All @@ -759,7 +758,7 @@ func (r *ReconcileMongoDbMultiReplicaSet) updateOmDeploymentRs(ctx context.Conte
caFilePath := fmt.Sprintf("%s/ca-pem", util.TLSCaMountPath)

agentCertSecretName := mrs.GetSecurity().AgentClientCertificateSecretName(mrs.GetName())
status, additionalReconciliationRequired := r.updateOmAuthentication(ctx, conn, rs.GetProcessNames(), &mrs, agentCertSecretName, caFilePath, internalClusterPath, isRecovering, log)
status, additionalReconciliationRequired := r.updateOmAuthentication(ctx, conn, rs.GetProcessNames(), &mrs, agentCertSecretName, caFilePath, internalClusterCertPath, isRecovering, log)
if !status.IsOK() && !isRecovering {
return xerrors.Errorf("failed to enable Authentication for MongoDB Multi Replicaset")
}
Expand All @@ -768,7 +767,7 @@ func (r *ReconcileMongoDbMultiReplicaSet) updateOmDeploymentRs(ctx context.Conte

err = conn.ReadUpdateDeployment(
func(d om.Deployment) error {
return ReconcileReplicaSetAC(ctx, d, mrs.Spec.DbCommonSpec, lastMongodbConfig, mrs.Name, rs, caFilePath, internalClusterPath, nil, log)
return ReconcileReplicaSetAC(ctx, d, mrs.Spec.DbCommonSpec, lastMongodbConfig, mrs.Name, rs, caFilePath, internalClusterCertPath, nil, log)
},
log,
)
Expand Down
Loading
Loading