Provide https support for health check#912
Conversation
Signed-off-by: Boris Kuschel <[email protected]>
sunsingerus
left a comment
There was a problem hiding this comment.
PR looks fine, but there are two items that would be nice to clarify in more details. In particularly, not sure about build .sh script and let's address DSN() generation section.
| -X ${REPO}/pkg/version.BuiltAt=${NOW} \ | ||
| " \ | ||
| ${GCFLAGS:-} \ | ||
| $GCFLAG "${GCFLAGS:-}" \ |
There was a problem hiding this comment.
Can you, please, clarify, why would you like to create $GCFLAG as a separate var and not to add '-gcflags' to $GCFLAGS ?
There was a problem hiding this comment.
The original method had issues with the quotations being passed into go build. It wasn't parsing correctly, either wrapping the entire thing in single quotes (including the flag specificier) or each argument in single quotes. I was using go 1.17. I am not sure if it's particular to that version but doing it this way avoids the issue.
|
|
||
| if [[ "" != "${GCFLAGS:-}" ]]; then | ||
| GCFLAGS="-gcflags ${GCFLAGS:-}" | ||
| GCFLAG='-gcflags' |
There was a problem hiding this comment.
See my comment below on the call line
| // 1. Metrics requests | ||
| // 2. Schema maintenance | ||
| // User credentials can be specified in additional ClickHouse config files located in `chUsersConfigsPath` folder | ||
| CHScheme string `json:"chScheme" yaml:"chScheme"` |
There was a problem hiding this comment.
This section is kind of deprecated and is kept for backward-compatibility only. That's OK, it's just nice extra.
There was a problem hiding this comment.
I saw, wasn't sure, i thought it was no harm.
| func (c *Connection) connect(ctx context.Context) { | ||
| c.l.V(2).Info("Establishing connection: %s", c.params.GetDSNWithHiddenCredentials()) | ||
| dbConnection, err := databasesql.Open("clickhouse", c.params.GetDSN()) | ||
| dbConnection, err := databasesql.Open("clickhouse", c.params.GetDSN()+"?tls_config=tls-settings") |
There was a problem hiding this comment.
Just thinking aloud - may be these params should be not added to GetDSN(), but produced by GetDSN()?
There was a problem hiding this comment.
Especially taking into account, that DSN is already aware of the scheme (be it http/https)
There was a problem hiding this comment.
I think I ended going this way because of this unit was importing the go driver here, so the crypto init/registration had to also happen here. The specific registration key is currently hard coded on unit initialization and wanted to confine the hardcoded key to the scope of this unit.
I can make it a constant constant ("tls-settings") in the ConnectionParam unit.
|
@sunsingerus Do you think this is ok to merge? |
Thanks for taking the time to contribute to
clickhouse-operator!Please, read carefully instructions on how to make a Pull Request.
This will help a lot for maintainers to adopt your Pull Request.
Important items to consider before making a Pull Request
Please check items PR complies to:
next-releasebranch, not intomasterbranch1. More info--
1 If you feel your PR does not affect any Go-code or any testable functionality (for example, PR contains docs only or supplementary materials), PR can be made into
masterbranch, but it has to be confirmed by project's maintainer.