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

CLOUDP-130487 fixed operator crash in face of incomplete TLS config #1115

Closed

Conversation

hoyhbx
Copy link
Contributor

@hoyhbx hoyhbx commented Sep 11, 2022

What problem does this PR solve?

When TLS is enabled with an incomplete configuration (i.e. none of caConfigMapRef and caCertificateSecretRef is specified), the operator crashes. Upon inspection of the operator error log, we find that the crash happens when the operator is Ensuring TLS is correctly configured as it results in a segmentation fault due to a nil pointer reference.

It seems that this null pointer exception occurs while ensuring that the CA cert is configured during TLS config validation when the following condition is checked:

if mdb.Spec.Security.TLS.CaCertificateSecret != nil {
	caResourceName = mdb.TLSCaCertificateSecretNamespacedName()
	caData, err = secret.ReadStringData(secretGetter, caResourceName)
} else {
	caResourceName = mdb.TLSConfigMapNamespacedName()
	caData, err = configmap.ReadData(cmGetter, caResourceName)
}

Since Spec.Security.TLS.CaCertificateSecret is set to nil, mdb.TLSConfigMapNamespacedName is called:

func (m MongoDBCommunity) TLSConfigMapNamespacedName() types.NamespacedName {
	return types.NamespacedName{Name: m.Spec.Security.TLS.CaConfigMap.Name, Namespace: m.Namespace}
}

However, Spec.Security.TLS.CaConfigMap is also nil, so when the Name field is accessed a runtime error occurs as a nil pointer is dereferenced.

What changes were made and how does it work?

Currently, there is no validation for TLS spec:

func validateSpec(mdb mdbv1.MongoDBCommunity) error {
	if err := validateUsers(mdb); err != nil {
		return err
	}


	if err := validateArbiterSpec(mdb); err != nil {
		return err
	}


	if err := validateAuthModeSpec(mdb); err != nil {
		return err
	}


	return nil
}

I think we can include the following condition in the above code along with the function definition in controllers/validation/validation.go:

if err := validateTLSSpec(mdb); err != nil {
	return err
}
// Validate TLS specification
func validateTLSSpec(mdb mdbv1.MongoDBCommunity) error {
        if mdb.Spec.Security.TLS.Enabled && mdb.Spec.Security.TLS.CaConfigMap == nil && mdb.Spec.Security.TLS.CaCertificateSecret == nil {
		return fmt.Errorf("CaCertificateSecretRef/CaConfigMapRef not found.")
	}

	return nil
}

Code changes

  • Has Go code change
  • Has CI related scripts change

Tests

  • Unit test
  • E2E test
  • Manual test
  • No code

This is a simple fix and we suppose no test above is needed.

Side effects

  • Breaking backward compatibility
  • Other side effects:

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation

All Submissions:

  • Have you opened an Issue before filing this PR?
  • Have you signed our CLA?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Put closes #XXXX in your comment to auto-close the issue that your PR fixes (if such).

Copy link
Contributor

@slaskawi slaskawi left a comment

Choose a reason for hiding this comment

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

Great work! Can I ask to add a test for this? Just to make sure we won't break this in the future.

@hoyhbx
Copy link
Contributor Author

hoyhbx commented Sep 20, 2022

@slaskawi Will do

@slaskawi
Copy link
Contributor

Let's close this in favor of #1119 as that one is a bit more complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants