-
Notifications
You must be signed in to change notification settings - Fork 200
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
Add validation webhook for KafkaUser #968
Conversation
if util.ObjectManagedByClusterRegistry(kafkaCluster) { | ||
// referencing remote Kafka clusters is not allowed | ||
logMsg = fmt.Sprintf("KafkaCluster CR '%s' in the namespace '%s' is a remote resource", clusterName, clusterNamespace) | ||
allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("clusterRef"), kafkaUser.Spec.ClusterRef, logMsg)) | ||
} |
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.
Question: Won't this cause issues with clreg replicated KafkaUsers from remote K8s clusters and Kafka clusters?
I didn't see any guard against validating KafkaUsers replicated by clreg.
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.
Hmmm yea you are right, we probably shouldn't have this restriction here
If we add the |
// avoid panic if the user wants to create a kafka user but the cluster is in plaintext mode | ||
if kafkaCluster.Spec.ListenersConfig.SSLSecrets == nil && kafkaUser.Spec.PKIBackendSpec == nil { | ||
logMsg = fmt.Sprintf("KafkaCluster CR '%s' in namespace '%s' is in plaintext mode, "+ | ||
"therefore 'spec.pkiBackendSpec' must be provided to create certificate", clusterName, clusterNamespace) | ||
allErrs = append(allErrs, field.Invalid(field.NewPath("spec").Child("pkiBackendSpec"), kafkaUser.Spec.PKIBackendSpec, logMsg)) | ||
} | ||
} |
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.
Note to myself: This check is not validate against a Kafka cluster within a Istio mesh since the broker certs are issued by Istio, not Koperator
Please add this validation also into this PR: #1019 |
Description
Add validation webhook implementation for
KafkaUser
resource creation and update, see the test cases for example misconfigurations and corresponding rejection messagesType of Change
Checklist