Remove the obsolete --use-letsencrypt, add --insecure-skip-tls-verify in the register-member command #95
Conversation
in the register-member command.
metlos
requested review from
MatousJobanek,
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19,
mfrancisc,
drpaneas and
fbm3307
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #95 +/- ##
==========================================
+ Coverage 68.02% 68.71% +0.68%
==========================================
Files 44 44
Lines 3240 3273 +33
==========================================
+ Hits 2204 2249 +45
+ Misses 841 825 -16
- Partials 195 199 +4
|
pkg/cmd/adm/register_member.go
Outdated
| @@ -36,6 +36,8 @@ const ( | |||
| TokenExpirationDays = 3650 | |||
| ) | |||
|
|
|||
| var invalidKubeConfigError = errors.New("invalid kubeconfig file") | |||
There was a problem hiding this comment.
only a detail - is there any reason for defining it as a global var?
There was a problem hiding this comment.
I think it is a recommended practice. See https://github.com/Djarvur/go-err113?tab=readme-ov-file#reports (which is a linter we don't use ATM).
There was a problem hiding this comment.
ok, but why? I mean, what is the benefit of it, and what is the disadvantage of doing it the other way return fmt.Errorf(...) as we do all over the place?
I can be wrong, but most of the projects do exactly it - initialize the error when returning them.
There was a problem hiding this comment.
I think if done using a var, you can use it with errors.Is(), which becomes more handy when the error is public.
There was a problem hiding this comment.
which is not in our case... I would vote for keeping it consistent with the rest of the code.
pkg/cmd/adm/register_member.go
Outdated
| targetAuth := sourceAuth.DeepCopy() | ||
| targetAuth.Token = token | ||
| targetAuth.TokenFile = "" | ||
| targetAuth.Username = "" | ||
| targetAuth.Password = "" | ||
| targetAuth.Impersonate = "" | ||
| targetAuth.ImpersonateUID = "" | ||
| targetAuth.ImpersonateGroups = []string{} | ||
| targetAuth.ImpersonateUserExtra = map[string][]string{} | ||
| targetAuth.Exec = nil | ||
| targetAuth.AuthProvider = nil |
There was a problem hiding this comment.
wasn't the original approach for auth section easier? I mean setting only those values that we need to set compared to deleting everything that we don't wan to have set?
There was a problem hiding this comment.
Hmm... I just wanted to use 1 approach for both auth and cluster sections (which is now also copied). I chose copying and then clearing/re-setting fields that we don't need or want to redefine.
But thinking about it further I am not 100% sure how to do this best. IMHO it is not important whether we just "pick what we want" or "clear what we don't want" but rather what fields are actually applicable.
After thinking about it more, I am not so sure anymore that, especially in the case of cluster, the fields in the kubeconfig that the caller uses to connect to the cluster are going to be the same as the ones needed by the kubeconfig used by the SAs within the cluster. Things like Cluster.TLSServerName, Cluster.CertificateAuthority or indeed Cluster.Server might actually be different, am I right?
I didn't want to complicate it and just use 1 approach for everything (copy and reset) but maybe we need something completely different...
Let's talk about this next year :)
There was a problem hiding this comment.
I would vote for "setting only applicable fields" which is more close to "pick what we want"
how about this one:
- set only applicable fields for auth
- copy the whole cluster context if the "insecure" flag is not define (as all CA/TLS fields are applicable in this case)
- if the "insecure" flag is used, then set only
ServerandInsecureSkipTLSVerify
that should be easy to read and understand.
But TBH, either option is fine. Let's go for something that is gonna be easy to maintain & read and keep it simple, we can improve it later if needed.
There was a problem hiding this comment.
copy the whole cluster context if the "insecure" flag is not define (as all CA/TLS fields are applicable in this case)
I don't think this is universally true. The CA/TLS can be different for communication from outside the cluster or within the cluster. We use the passed in kubeconfig to talk to the cluster from the outside but also to compose a new kubeconfig that should be used by the SA to communicate within the cluster. E.g. https://docs.openshift.com/container-platform/4.17/security/certificate_types_descriptions/user-provided-certificates-for-api-server.html vs https://docs.openshift.com/container-platform/4.17/security/certificate_types_descriptions/service-ca-certificates.html.
There was a problem hiding this comment.
I may have been overthinking this. The members and host will connect to each other "from outside" usually, too. If the connection requirements between the servers are different from what the caller of the ksctl command must use, then I think it is fair to require manual setup.
There was a problem hiding this comment.
As of now, we don't differentiate between:
- two clusters
- one cluster
we use the public API url in both cases, which means that it talks to the other cluster like it was a separate one. Let's not over-complicate this and keep it simple to support the most common happy-path. We can improve it later if needed.
pkg/cmd/adm/register_member.go
Outdated
| @@ -86,7 +106,7 @@ func NewRegisterMemberCmd() *cobra.Command { | |||
| flags.MustMarkRequired(cmd, "host-kubeconfig") | |||
| cmd.Flags().StringVar(&commandArgs.memberKubeConfig, "member-kubeconfig", "", "Path to the kubeconfig file of the member cluster") | |||
| flags.MustMarkRequired(cmd, "member-kubeconfig") | |||
| cmd.Flags().BoolVar(&commandArgs.useLetsEncrypt, "lets-encrypt", true, "Whether to use Let's Encrypt certificates or rely on the cluster certs.") | |||
| cmd.Flags().Bool("insecure-skip-tls-verify", false, "Whether to ignore TLS verification errors during the connection to both host and member. If not specified, the value is inherited from the respective kubeconfig files.") | |||
There was a problem hiding this comment.
| cmd.Flags().Bool("insecure-skip-tls-verify", false, "Whether to ignore TLS verification errors during the connection to both host and member. If not specified, the value is inherited from the respective kubeconfig files.") | |
| cmd.Flags().Bool("insecure-skip-tls-verify", false, "Flag to decided whether to ignore TLS verification errors or not during the connection to both host and member. If not specified, the value is inherited from the respective kubeconfig files.") |
| waitForReadyTimeout time.Duration | ||
| } | ||
|
|
||
| func NewRegisterMemberCmd() *cobra.Command { | ||
| return newRegisterMemberCmd(registerMemberCluster) |
There was a problem hiding this comment.
For my knowledge, would this work without the wrapper?
so that they can be used on the clusters where the files are not available. Some other minor cosmetic changes.
metlos
force-pushed
the
remove-lets-encrypt-flag
branch
2 times, most recently
from
7fd02b0 to
14c564a
Compare
Remove the obsolete --use-letsencrypt, add --insecure-skip-tls-verify in the register-member command.
The kubeconfig generation for ksctl (using the SA token) is reworked such that it reuses as much as possible from the source kubeconfig (i.e. kubeconfig for host or member connection) provided by the user.