feat (kumactl) kumactl can communicate to kuma-cp over https #633
Conversation
kumactl and kuma-dp communicates to kuma-cp over http. In case kuma-cp is behind a https reverese proxy, kumactl cannot connect to it. Added support to upgrade connection to https and disable client security check for ssl.
|
Hey, @sudeeptoroy |
|
Hi @jakubdyszkiewicz , yes sure, in scenarios where kuma-cp is deployed behind a reverse proxy, kuma-cp native endpoints will be available on reverse proxy with different hostnames. kuma-cp <------------ >|reverse -proxy (https)|< ============== >( kumactl/kuma-dp) I have started with #597, once done i can submit the fix under this PR so that the functionality is better understood. |
catalog is autoconfigured with default, but in certain situations where kuma-cp is behind a reverse proxy the autoconfigured values are not correct. added config variables which will be used to update catalog content to the right values as needed by an operator.
|
How would the connection to XDS/SDS work here? Don't you have to also configure the equivalent of |
| @@ -43,6 +43,8 @@ store: | |||
| bootstrapServer: | |||
| # Port of Server that provides bootstrap configuration for dataplanes | |||
| port: 5682 # ENV: KUMA_BOOTSTRAP_SERVER_PORT | |||
| # Public URL to reach Bootstrap server. ex: https://bootstrap.kuma.com:1234, its autoconfigured if blank | |||
| publicUrl: # ENV: KUMA_BOOTSTRAP_SERVER_PUBLIC_URL | |||
There was a problem hiding this comment.
See CatalogConfig:
type BootstrapApiConfig struct {
Url string
}
type DataplaneTokenApiConfig struct {
LocalUrl string
PublicUrl string
}
type AdminApiConfig struct {
LocalUrl string
PublicUrl string
}
type MonitoringAssignmentApiConfig struct {
Url string
}
I think instead of introducing publicUrl in every server it's better to include CatalogConfig to YAML and ENV config. this way you can override the catalog fields with your public URLs.
There was a problem hiding this comment.
yeah, i can do that..
There was a problem hiding this comment.
i can make changes for bootstrap and admin url.. how about sds.. i am guessing its going to sit with sdsServer config..
Also if i move the config to catalogConfig it may not very intuitive to the end user as catalog is an internal concept and not exposed to them yet..
There was a problem hiding this comment.
If you expose it, the catalog will be just a part of Kuma Config.
About XDS/SDS. Why not just set the KUMA_GENERAL_ADVERTISED_HOSTNAME to the address of your reverse proxy for Kuma CP? For API Server/Bootstrap/Admin it didn't work because the change of http:// -> https://, but I think here it should be ok.
yes, you are right..sds works but xds is still insecure.. and the way to do that would be to extent kuma-dp to take additional arguments via which user can specify the cert/key to use for upgrading the xdx connection.. i wanted to discuss with you before starting the xds work and hence did not include with this PR.. do you have any other suggestion.. |
SDS is secured, not sure if extra reverse proxy won't break anything here. About securing XDS. If we are talking about simple TLS, we can follow similar pattern that we've got for SDS which is:
This way we don't need to manually distribute certificate next to every Kuma DP instance. mTLS would be more complicated, but I don't think XDS contains such sensitive data to do mTLS. |
|
Thanks for your reply, it has clarified few doubt. i would still need suggestion for making further progress on other points. agreement: Let me explain the need for SDS endpoint and XDS client cert for envoy.
Let me know your thoughts around it. |
|
Thanks for explanation, now I understand more. SDS needs to be encrypted at all points, because it exchanges sensitive data and it works in a very similar way as XDS, so we probably use the same pattern for both. With SDS you don't want Contour to terminate TLS, therefore my question: can you use Can the hostname for Admin/SDS/XDS be the same with support for different ports? |
Introduced catalog under apiserver to expose bootstrap and monitoring server public url sdsServer url is also exposed as config
Thanks for your reply.
I will continue to make progress on XDS and let you know any new findings. |
|
hi @jakubdyszkiewicz, this PR is up for review |
removed previously added code and added trusted_ca to initiate a TLS connection
Renamed sds publicUrl to publicHostPort
…ture/client_https_support
pkg/config/sds/config.go
Outdated
| } | ||
| } | ||
|
|
||
| // Envoy SDS server configuration | ||
| type SdsServerConfig struct { | ||
| // Port of GRPC server that Envoy connects to | ||
| GrpcPort int `yaml:"grpcPort" envconfig:"kuma_sds_server_grpc_port"` | ||
| // Public HostPort to reach SDS server | ||
| PublicHostPort string `yaml:"publicHostPort" envconfig:"kuma_sds_server_public_host_port"` |
There was a problem hiding this comment.
Can we include this in the catalog instead? Just like MADS server.
I think there should be also XDS server there, because my guess is that you would also like to override this.
There was a problem hiding this comment.
yes, i can move it to catalog seems to be a better place to manage all overwrites.
XDS is derived via AdvertisedHostname. let me know if you are ok to derive from this new variable.
There was a problem hiding this comment.
Just realised, XDS is a part of bootstrapServer config. Probably we can keep it that way. Let me know.
| @@ -13,6 +13,7 @@ type configParameters struct { | |||
| XdsConnectTimeout time.Duration | |||
| AccessLogPipe string | |||
| DataplaneTokenPath string | |||
| CertBytes string | |||
| } | |||
There was a problem hiding this comment.
I think you will also want to override the XdsHost and XdsPort, right?
pkg/xds/bootstrap/generator.go
Outdated
| @@ -70,6 +75,13 @@ func (b *bootstrapGenerator) generateFor(proxyId core_xds.ProxyId, dataplane *co | |||
| if request.AdminPort != 0 { | |||
| adminPort = request.AdminPort | |||
| } | |||
| var cert string = "" | |||
| if b.xdsCertFile != "" { | |||
| c, e1 := ioutil.ReadFile(b.xdsCertFile) | |||
There was a problem hiding this comment.
can we have a little bit descriptive names like certBytes, err?
There was a problem hiding this comment.
also I think we should handle this error instead, not ignore it
|
@jakubdyszkiewicz its up for review, the CI did not run for some reason |
| } | ||
|
|
||
| type SdsApiConfig struct { | ||
| HostPort string `yaml:"hostPort" envconfig:"kuma_sds_server_public_host_port"` |
There was a problem hiding this comment.
Can we make this consistent to MonitoringAssignmentApiConfig?
Make this URL and use grpc://host:port format
pkg/core/bootstrap/autoconfig.go
Outdated
| "localhost", | ||
| cfg.BootstrapServer.Params.XdsHost, |
There was a problem hiding this comment.
Why are we adding Xds hostname to the SDS Server cert?
There was a problem hiding this comment.
As XDS is serving TLS so i added xds host to the cert. This created cert can be added to config when kuma-cp starts. It can be removed too. please suggest.
There was a problem hiding this comment.
Please remove it. If you need such a shared certificate you can generate it yourself pretty easily with kumactl generate tls-certificate providing multiple hostnames.
xds is already configured via bootstrap config so I have not included it in catalog. I feel XDS config is better with bootstrap config. however let me know your thoughts. |
|
You are right. Bootstrap config is enough there. |
|
I have make the changes you mentioned but the ci is breaking. Any clue on what might be wrong? |
|
I just re-run the checks, let's see what happens. |
|
@jakubdyszkiewicz any additional feedback here besides making sure that the checks pass? Is there any documentation that needs to be created in addition to this PR? |
thanks @subnetmarco it failed again at the same place. |
| } | ||
|
|
||
| type BootstrapApiConfig struct { | ||
| Url string | ||
| Url string `yaml:"url" envconfig:"kuma_bootstrap_server_public_url"` |
There was a problem hiding this comment.
We name the env vars the same as YAML path, so the env should be KUMA_API_SERVER_CATALOG_BOOTSTRAP_URL
other envs:
KUMA_API_SERVER_CATALOG_MONITORING_ASSIGNMENT_URL
KUMA_API_SERVER_CATALOG_SDS_URL
pkg/api-server/config_ws_test.go
Outdated
| "url": "" | ||
| }, | ||
| "sds": { | ||
| "hostPort": "" |
There was a problem hiding this comment.
this should be url
There was a problem hiding this comment.
yeah, not sure how i missed it..
|
Hi @jakubdyszkiewicz, thanks for fixing the CI. Its up for your review. |
|
@subnetmarco about docs: About securing XDS server - it's optional for now. I think we should document everything after we put TLS on every server and support this in kumactl/kuma-dp. |
|
Thank you for working together on this @sudeeptoroy Looking forward to your next contributions! 🎉 |
Thanks @jakubdyszkiewicz, i have a few more use case and looking forward to collaborate more closely. |
|
@sudeeptoroy, if you're interested in some contributor swag that we're planning right now, shoot me an email at kevin.chen@konghq.com so I make sure you get some once they're ready :) Thanks a bunch for this contribution |
Summary
kumactl and kuma-dp communicates to kuma-cp over http. In case kuma-cp is behind a https reverese proxy,
kumactl cannot connect to it.
Added support to upgrade connection to https and disable client security check for ssl.
this PR will enable https://github.com/Kong/kuma/issues/597