adding feature to set TLS configuration #776
Conversation
|
@ysdongAmazon could help to review this? |
|
@sshver could help to approve and trigger the workflow? |
|
@sshver once this done, do i require to update helm chart at https://github.com/aws/eks-charts/tree/master/stable/appmesh-controller? |
| // TLSVersion helper function returns the TLS Version ID for the version name passed. | ||
| tlsVersion, err := k8sapiflag.TLSVersion(tlsOpt.minVersion) | ||
| if err != nil { | ||
| setupLog.Error(err, "TLS version invalid") |
There was a problem hiding this comment.
I think we should be exiting on failure to honor this setting. The same applies to the cipher suites.
There was a problem hiding this comment.
Also it would be good to handle the interpreting of the tls options in main up front, and just pass the values into this function. That would keep any argument handling logic in a single place, and make sure it happens right away on startup similar to other argument parsing failures.
There was a problem hiding this comment.
@joesbigidea Thank for highlighting on exiting on failure. For second point, my opinion is that when NewManager is called, it will callback on setting the TLS config, if we exit with failure, the controller will not able to start anyway. Furthermore, this config is expecting the callback function https://github.com/kubernetes-sigs/controller-runtime/blob/v0.14.6/pkg/manager/manager.go#L253
May b u could show me how u organize the argument handling logic and i could refer to it. Hehe.. I am not the expert in this domain nor familiar on the organization for this project and need your help to guide me.
| // This function get the option from command argument (tlsConfig), check the validity through k8sapiflag | ||
| // and set the config for webhook server. | ||
| // refer to https://pkg.go.dev/k8s.io/component-base/cli/flag | ||
| func tlsConfigSetting(cfg *tls.Config, tlsOpt tlsConfig) { |
There was a problem hiding this comment.
This probably warrants a unit test.
There was a problem hiding this comment.
@joesbigidea i dint see any unit-test current for the main.go from makefile refer to https://github.com/aws/aws-app-mesh-controller-for-k8s/blob/master/Makefile#L35. Could u help me on this?
|
@kangsheng89 Since we want to get this change out quickly @sshver made a few small structural changes to this in a separate PR. That PR has been manually tested and we'll go ahead and get that one merged and released. Thank you for your contribution. |
|
Thank you! @joesbigidea and @sshver |
Issue #, if available:
Description of changes:
This is enable user to configure TLS setting for the controller manager
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.