Do not bring up the PostgreSQL or Kafka/Zookeeper services if specifying external services #604
Conversation
postgresql secret and defined an external postgresql server hostname.
|
I see this pattern over and over in our operator. Is it possible to make a method/function in Go to abstract this? |
|
Are you talking about the new code added or the following pattern ? For the latter, I'd keep those as it's the go pattern for error check/propagation up. For the new code added, I agree, I'd like to generalize it if possible, I'll work on the kafka side and see if we can use common code for secret checks and such. |
|
@abellotti How are you distinguishing an external postgres vs an internal postgres where we've autocreated the secret...is it the hostname being blank? |
|
Yes, a user must have provided a secret with a non-blank hostname. |
abellotti
changed the title
and leveraged the same to also skip kafka/zookeeper deployment if kafka secret and hostname were specified.
|
Checked commits abellotti/manageiq-pods@3c22858~...7cd301d with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint |
|
Keeping as WIP for now as it needs some more testing. |
|
Successfully tested using an external Kafka server after having fun with #607 (comment) - unwipping. |
abellotti
changed the title
But what does the generated secret look like? If it also has a hostname, then you can't tell them apart after the fact. |
| @@ -439,3 +451,13 @@ func (r *ReconcileManageIQ) createk8sResIfNotExist(cr *miqv1alpha1.ManageIQ, res | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func getSecretKeyValue(client client.Client, nameSpace string, secretName string, keyName string) string { | |||
There was a problem hiding this comment.
I think there's another file with secret methods, where this might belong better.
There was a problem hiding this comment.
Not that I saw, pod specific helpers (postgres, kafka, etc) to return the default secret name and default secret if not specified. One possibility is to try to move this to pkg/helpers/miq-components/util.go
There was a problem hiding this comment.
I don't see a better place for this right now.
| @@ -439,3 +451,13 @@ func (r *ReconcileManageIQ) createk8sResIfNotExist(cr *miqv1alpha1.ManageIQ, res | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func getSecretKeyValue(client client.Client, nameSpace string, secretName string, keyName string) string { | |||
There was a problem hiding this comment.
I don't see a better place for this right now.
| @@ -267,6 +267,12 @@ func (r *ReconcileManageIQ) generateMemcachedResources(cr *miqv1alpha1.ManageIQ) | |||
| } | |||
|
|
|||
| func (r *ReconcileManageIQ) generatePostgresqlResources(cr *miqv1alpha1.ManageIQ) error { | |||
| hostName := getSecretKeyValue(r.client, cr.Namespace, cr.Spec.DatabaseSecret, "hostname") | |||
| if hostName != "" { | |||
There was a problem hiding this comment.
@abellotti Should this be if hostName != "postgresql" {?
There was a problem hiding this comment.
No, this is what triggers external vs internal - #604 (comment)
There was a problem hiding this comment.
It's not obvious to me why this would be empty string since either the user creates the secret with a hostname or we do...
There was a problem hiding this comment.
It should stay if hostName != "", if the user specified a secret with a non-blank hostname we're done, we don't get to call that DefaultPostgresqlSecret (where we populate the secret for the local postgresql case), which is called on line 276 after the if clause.
There was a problem hiding this comment.
@bdunne Yeah I had the same question - I assumed when we generate it ourselves we don't specify a hostname?
Do not bring up:
they specified the postgresql secret with the external PostgreSQL hostname defined there.
service hostname defined.
Fixes: #529