gardener / ops-toolbelt

Useful tools and operations guide for gardener landscapes
Apache License 2.0
15 stars 26 forks source link

Fixes the path for cacert used in etcdctl. #119

Closed ishan16696 closed 7 months ago

ishan16696 commented 7 months ago

What this PR does / why we need it: Fixes the precalculated argument value of cacert path used in etcdctl command. Removed the etcd_host is used as precalculated argument for etcdctl --endpoints.

Which issue(s) this PR fixes: Fixes #118 and #120

Special notes for your reviewer:

Release note:

Fixes the precalculated argument CA certificate path used for the etcdctl command.
petersutter commented 7 months ago

/invite @mimiteto

gardener-robot commented 7 months ago

@petersutter Command "/invite @mimiteto" failed with "Reviews may only be requested from collaborators. One or more of the users or teams you specified is not a collaborator of the gardener/ops-toolbelt repository.".

Additional Information ``` Redacted in public. Check backend logs. ```
mimiteto commented 7 months ago

Discussed with @ishan16696 in private chat - both are legit issues and I agree with the fixes. Also, he was nice to update the cert paths that to be referred as vars (as I missed that as well). I believe my /lgtm will not work. Also, it's I think currently it's somewhat usable (just as before).

ishan16696 commented 7 months ago

I found one more issue. I will open a issue to explain it and fix that in this PR. /do-not-merge

ishan16696 commented 7 months ago

I found one more issue. I will open a issue to explain it and fix that in this PR.

Explained the issue here: https://github.com/gardener/ops-toolbelt/issues/120

mimiteto commented 7 months ago

Ok, I see your issue. Let's discuss here, so we don't have to check 2 different threads. When we reach a conclusion we can mention it in the issue. IMO there are 4 options:

I think the least-desired option is option 2 - it's slow and error prone. Then option 3 seems to be bringing confusion (hence the issue @ishan16696 opened). Option 4 needs yet another shell script to be implemented and will need to be adjusted again for the cases where this is executed as ops-pod and either within chroot or not. Option 1 is the safest and easiest to implement, but usage is a bit harder for operators. @ishan16696 wdyt?

ishan16696 commented 7 months ago

Hi @mimiteto , I was also leaning towards the option 1 first then I found a way to make it work, Can you check my latest commit and see if it make sense to you.

mimiteto commented 7 months ago

I guess you can do it like that as well. Also, you don't need to use awk - hostname (or the env HOSTNAME) will give you the same information as uname -a | awk '{print $2}' but this does not make any meaningful difference.

mimiteto commented 7 months ago

Also, as I was looking here does it make sense to mention the possible etcd addresses in the cheatsheet in some way?

ishan16696 commented 7 months ago

I guess in cheatsheet also it can be replaced with a general terms like this https://${etcd_host}:${etcd_port}. And we can also add a function there which sets the etcd_host correctly. Would it make sense ?

mimiteto commented 7 months ago

Looks nice.

ishan16696 commented 7 months ago

I have removed some timeout flags in this commit as I feel not every command of etcdctl requires them. Only compact and defrag command requires them, so these flags can be added by operator if required.

ishan16696 commented 7 months ago

/squash-merge /removed-do-not-merge

gardener-robot commented 7 months ago

@ishan16696 Command /squash-merge is not known.

gardener-robot commented 7 months ago

@ishan16696 Command /removed-do-not-merge is not known.

mimiteto commented 7 months ago

/lgtm