Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Michael Albinus]

Filipp Gunbin <fgunbin@fastmail.fm> writes:

Hi Filipp,

> +@samp{kubectl get pods} returns pods in the current context and
> +namespace.  Current namespace can be changed with @samp{kubectl config
> +set-context --current --namespace=<name>}.  After invoking this or
> +other command which modifies Kubernetes environment outside of Emacs,
> +call @code{tramp-cleanup-all-connections} to reset Tramp cache data.

Can we automate this? I mean, when pod data are cached, and Tramp
detects a changed namespace, the cached data should be flushed?

Tramp does something similar with other connection methods, for example
it checks "uname -sr" on remote hosts, and caches the result. Whenever a
new connection to a host is established, Tramp calls again "uname -sr",
and compares with the cached value. If the values differ, all cached
data for this connection are flushed.

Is there a similar way to retieve (and cache) the current context and
namespace for pods?

Best regards, Michael.

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Filipp Gunbin]

Hi Michael,

On 22/10/2022 11:53 +0200, Michael Albinus wrote:

> Filipp Gunbin <fgunbin@fastmail.fm> writes:
>
> Hi Filipp,
>
>> +@samp{kubectl get pods} returns pods in the current context and
>> +namespace.  Current namespace can be changed with @samp{kubectl config
>> +set-context --current --namespace=<name>}.  After invoking this or
>> +other command which modifies Kubernetes environment outside of Emacs,
>> +call @code{tramp-cleanup-all-connections} to reset Tramp cache data.
>
> Can we automate this? I mean, when pod data are cached, and Tramp
> detects a changed namespace, the cached data should be flushed?
>
> Tramp does something similar with other connection methods, for example
> it checks "uname -sr" on remote hosts, and caches the result. Whenever a
> new connection to a host is established, Tramp calls again "uname -sr",
> and compares with the cached value. If the values differ, all cached
> data for this connection are flushed.
>
> Is there a similar way to retieve (and cache) the current context and
> namespace for pods?

(I've actually wrote a message to emacs-devel about this, but then
decided it's just simpler to advise resetting cache, and didn't send the
message; maybe I should have asked first)

AFAIU, the general way would be to call "kubectl config view -o json",
then calculate checksum of the output and cache it.  This would catch
any change in context (namespace it just one case of many).

However, there're two things to consider here:

- "config view" command is not instantaneous: on my machine it's about
  100ms, and calling it on each (say) host completion would be annoying.

- Your example with uname is different in that if uname output changed,
  it means that the host environment changed, perhaps without local user
  knowing about it.  With kubectl, it's the local user which did some
  change (locally), and I see no problem in requiring her/him to reset
  Tramp cache after that.

Filipp

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Michael Albinus]

Filipp Gunbin <fgunbin@fastmail.fm> writes:

> Hi Michael,

Hi Filipp,

>> Can we automate this? I mean, when pod data are cached, and Tramp
>> detects a changed namespace, the cached data should be flushed?
>>
>> Tramp does something similar with other connection methods, for example
>> it checks "uname -sr" on remote hosts, and caches the result. Whenever a
>> new connection to a host is established, Tramp calls again "uname -sr",
>> and compares with the cached value. If the values differ, all cached
>> data for this connection are flushed.
>>
>> Is there a similar way to retieve (and cache) the current context and
>> namespace for pods?
>
> (I've actually wrote a message to emacs-devel about this, but then
> decided it's just simpler to advise resetting cache, and didn't send the
> message; maybe I should have asked first)

No problem that's what code review is good for :-)

> AFAIU, the general way would be to call "kubectl config view -o json",
> then calculate checksum of the output and cache it.  This would catch
> any change in context (namespace it just one case of many).

Yep, but I would recommend "kubectl config view --context=$(kubectl
config current-context) -o json". We don't need the information about
other context.

> However, there're two things to consider here:
>
> - "config view" command is not instantaneous: on my machine it's about
>   100ms, and calling it on each (say) host completion would be annoying.

I don't recommend it for hostname completion. I recommend it for opening
a connection; the cleanup shall happen in tramp-maybe-open-connection
via a hook.

If a user changes the context while there is an active connection in
Tramp, she will be lost. But this isn't an Emacs/Tramp specific
situation, so we don't need to care or document it.

> - Your example with uname is different in that if uname output changed,
>   it means that the host environment changed, perhaps without local user
>   knowing about it.  With kubectl, it's the local user which did some
>   change (locally), and I see no problem in requiring her/him to reset
>   Tramp cache after that.

Sure, but if we can this automate, it doesn't hurt. People tend to
forget to follow such recommendations for cleanup, and we will be blamed
with error reports about invalid cache entries then.

I'm just working on a patch, will show it to you for confirmation prior
commitment.

> Filipp

Best regards, Michael.

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Michael Albinus]

Michael Albinus <michael.albinus@gmx.de> writes:

Hi Filipp,

> Yep, but I would recommend "kubectl config view --context=$(kubectl
> config current-context) -o json". We don't need the information about
> other context.

Oops. The better way is "kubectl config view -o json --minify".

Best regards, Michael.

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Michael Albinus]

[-- Attachment #1: Type: text/plain, Size: 357 bytes --]

Michael Albinus <michael.albinus@gmx.de> writes:

Hi Filipp,

>> Yep, but I would recommend "kubectl config view --context=$(kubectl
>> config current-context) -o json". We don't need the information about
>> other context.
>
> Oops. The better way is "kubectl config view -o json --minify".

Patch is appended, could you pls test?

Best regards, Michael.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 4583 bytes --]

diff --git a/lisp/tramp-container.el b/lisp/tramp-container.el
index e104babe..5603fa0f 100644
--- a/lisp/tramp-container.el
+++ b/lisp/tramp-container.el
@@ -130,6 +130,15 @@ see its function help for a description of the format."
               (list nil name))
             names)))

+(defun tramp-kubernetes--current-context-data (vec)
+  "Return current context data as JSON string."
+  (with-temp-buffer
+    (when (zerop
+	   (tramp-call-process
+	    vec tramp-kubernetes-program nil t nil
+	    "config" "view" "--minify" "-o" "json"))
+      (buffer-string))))
+
 ;;;###tramp-autoload
 (defvar tramp-default-remote-shell) ;; Silence byte compiler.

@@ -165,6 +174,7 @@ see its function help for a description of the format."
                                    ("-it")
                                    ("--")
 			           ("%l")))
+		(tramp-config-check tramp-kubernetes--current-context-data)
                 (tramp-remote-shell ,tramp-default-remote-shell)
                 (tramp-remote-shell-login ("-l"))
                 (tramp-remote-shell-args ("-i" "-c"))))
diff --git a/lisp/tramp-sh.el b/lisp/tramp-sh.el
index d74afc84..39043482 100644
--- a/lisp/tramp-sh.el
+++ b/lisp/tramp-sh.el
@@ -4472,7 +4472,8 @@ process to set up.  VEC specifies the connection."
   ;; Check whether the output of "uname -sr" has been changed.  If
   ;; yes, this is a strong indication that we must expire all
   ;; connection properties.  We start again with
-  ;; `tramp-maybe-open-connection', it will be caught there.
+  ;; `tramp-maybe-open-connection', it will be caught there.  The same
+  ;; check will be applied with the function kept in`tramp-config-check'.
   (tramp-message vec 5 "Checking system information")
   (let* ((old-uname (tramp-get-connection-property vec "uname"))
 	 (uname
@@ -4481,8 +4482,23 @@ process to set up.  VEC specifies the connection."
 	      old-uname
 	    (tramp-set-connection-property
 	     vec "uname"
-	     (tramp-send-command-and-read vec "echo \\\"`uname -sr`\\\"")))))
-    (when (and (stringp old-uname) (not (string-equal old-uname uname)))
+	     (tramp-send-command-and-read vec "echo \\\"`uname -sr`\\\""))))
+	 (config-check-function
+	  (tramp-get-method-parameter vec 'tramp-config-check))
+	 (old-config-check
+	  (and config-check-function
+	       (tramp-get-connection-property vec "config-check-data")))
+	 (config-check
+	  (and config-check-function
+	       ;; If we are in `make-process', we don't need to recompute.
+	       (if (and old-config-check
+			(tramp-get-connection-property vec "process-name"))
+		   old-config-check
+		 (tramp-set-connection-property
+		  vec "config-check-data"
+		  (tramp-compat-funcall config-check-function vec))))))
+    (when (and (stringp old-uname) (stringp uname)
+	       (not (string-equal old-uname uname)))
       (tramp-message
        vec 3
        "Connection reset, because remote host changed from `%s' to `%s'"
@@ -4490,6 +4506,15 @@ process to set up.  VEC specifies the connection."
       ;; We want to keep the password.
       (tramp-cleanup-connection vec t t)
       (throw 'uname-changed (tramp-maybe-open-connection vec)))
+    (when (and (stringp old-config-check) (stringp config-check)
+	       (not (string-equal old-config-check config-check)))
+      (tramp-message
+       vec 3
+       "Connection reset, because remote configuration changed from `%s' to `%s'"
+       old-config-check config-check)
+      ;; We want to keep the password.
+      (tramp-cleanup-connection vec t t)
+      (throw 'uname-changed (tramp-maybe-open-connection vec)))

     ;; Try to set up the coding system correctly.
     ;; CCC this can't be the right way to do it.  Hm.
diff --git a/lisp/tramp.el b/lisp/tramp.el
index 57063154..933bac7e 100644
--- a/lisp/tramp.el
+++ b/lisp/tramp.el
@@ -281,6 +281,13 @@ pair of the form (KEY VALUE).  The following KEYs are defined:
     Until now, just \"ssh\"-based, \"sshfs\"-based and
     \"adb\"-based methods do.

+  * `tramp-config-check'
+    A function to be called with one argument, VEC.  It should
+    return a string which is used to check, whether the
+    configuration of the remote host has been changed (which
+    would require to flush the cache data).  This string is kept
+    as connection property \"config-check-data\".
+
   * `tramp-copy-program'
     This specifies the name of the program to use for remotely copying
     the file; this might be the absolute filename of scp or the name of

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Filipp Gunbin]

Hi Michael,

Generally it works, thank you very much!

Test case:

1) kubectl config set-context --current --namespace=main
2) dired /kubernetes:myapp-67cd597654-wxdjn:/
3) tramp-cleanup-this-connection
4) kubectl config set-context --current --namespace=
5) again dired /kubernetes:myapp-67cd597654-wxdjn:/ - I get an error and
  cache is cleared

However, now auto-completing /kubernetes: shows this, why 2?

--8<---------------cut here---------------start------------->8---
In this buffer, type RET to select the completion near point.

2 possible completions:
kubernetes:
--8<---------------cut here---------------end--------------->8---

Another issue: if at step 5 I instead use existing dired window and
type, say, "i" on a subdirectory, then I get the same-looking error, but
the Tramp cache is not cleared (perhaps this is not
Kubernetes-specific).

Also please see comments below.

On 24/10/2022 20:54 +0200, Michael Albinus wrote:

> Michael Albinus <michael.albinus@gmx.de> writes:
>
> Hi Filipp,
>
>>> Yep, but I would recommend "kubectl config view --context=$(kubectl
>>> config current-context) -o json". We don't need the information about
>>> other context.
>>
>> Oops. The better way is "kubectl config view -o json --minify".
>
> Patch is appended, could you pls test?
>
> Best regards, Michael.
>
> diff --git a/lisp/tramp-container.el b/lisp/tramp-container.el
> index e104babe..5603fa0f 100644
> --- a/lisp/tramp-container.el
> +++ b/lisp/tramp-container.el
> @@ -130,6 +130,15 @@ see its function help for a description of the format."
>                (list nil name))
>              names)))
>
> +(defun tramp-kubernetes--current-context-data (vec)
> +  "Return current context data as JSON string."
> +  (with-temp-buffer
> +    (when (zerop
> +	   (tramp-call-process
> +	    vec tramp-kubernetes-program nil t nil
> +	    "config" "view" "--minify" "-o" "json"))
> +      (buffer-string))))
> +

Let's remove --minify.

Also, maybe save the checksum (simple md5 would do it), not the full
listing?  The listing may contain sensitive data (for example, mine
shows my OAuth id token).

>  ;;;###tramp-autoload
>  (defvar tramp-default-remote-shell) ;; Silence byte compiler.
>
> @@ -165,6 +174,7 @@ see its function help for a description of the format."
>                                     ("-it")
>                                     ("--")
>  			           ("%l")))
> +		(tramp-config-check tramp-kubernetes--current-context-data)
>                  (tramp-remote-shell ,tramp-default-remote-shell)
>                  (tramp-remote-shell-login ("-l"))
>                  (tramp-remote-shell-args ("-i" "-c"))))
> diff --git a/lisp/tramp-sh.el b/lisp/tramp-sh.el
> index d74afc84..39043482 100644
> --- a/lisp/tramp-sh.el
> +++ b/lisp/tramp-sh.el
> @@ -4472,7 +4472,8 @@ process to set up.  VEC specifies the connection."
>    ;; Check whether the output of "uname -sr" has been changed.  If
>    ;; yes, this is a strong indication that we must expire all
>    ;; connection properties.  We start again with
> -  ;; `tramp-maybe-open-connection', it will be caught there.
> +  ;; `tramp-maybe-open-connection', it will be caught there.  The same
> +  ;; check will be applied with the function kept in`tramp-config-check'.
>    (tramp-message vec 5 "Checking system information")
>    (let* ((old-uname (tramp-get-connection-property vec "uname"))
>  	 (uname
> @@ -4481,8 +4482,23 @@ process to set up.  VEC specifies the connection."
>  	      old-uname
>  	    (tramp-set-connection-property
>  	     vec "uname"
> -	     (tramp-send-command-and-read vec "echo \\\"`uname -sr`\\\"")))))
> -    (when (and (stringp old-uname) (not (string-equal old-uname uname)))
> +	     (tramp-send-command-and-read vec "echo \\\"`uname -sr`\\\""))))
> +	 (config-check-function
> +	  (tramp-get-method-parameter vec 'tramp-config-check))
> +	 (old-config-check
> +	  (and config-check-function
> +	       (tramp-get-connection-property vec "config-check-data")))
> +	 (config-check
> +	  (and config-check-function
> +	       ;; If we are in `make-process', we don't need to recompute.
> +	       (if (and old-config-check
> +			(tramp-get-connection-property vec "process-name"))
> +		   old-config-check
> +		 (tramp-set-connection-property
> +		  vec "config-check-data"
> +		  (tramp-compat-funcall config-check-function vec))))))
> +    (when (and (stringp old-uname) (stringp uname)
> +	       (not (string-equal old-uname uname)))
>        (tramp-message
>         vec 3
>         "Connection reset, because remote host changed from `%s' to `%s'"
> @@ -4490,6 +4506,15 @@ process to set up.  VEC specifies the connection."
>        ;; We want to keep the password.
>        (tramp-cleanup-connection vec t t)
>        (throw 'uname-changed (tramp-maybe-open-connection vec)))
> +    (when (and (stringp old-config-check) (stringp config-check)
> +	       (not (string-equal old-config-check config-check)))
> +      (tramp-message
> +       vec 3
> +       "Connection reset, because remote configuration changed from `%s' to `%s'"
> +       old-config-check config-check)

Currently, we'll get huge config listings here (at the level 3)?  If we
change to checksums, that'd be not very useful.  So perhaps don't output
those strings?

> +      ;; We want to keep the password.
> +      (tramp-cleanup-connection vec t t)
> +      (throw 'uname-changed (tramp-maybe-open-connection vec)))
>
>      ;; Try to set up the coding system correctly.
>      ;; CCC this can't be the right way to do it.  Hm.
> diff --git a/lisp/tramp.el b/lisp/tramp.el
> index 57063154..933bac7e 100644
> --- a/lisp/tramp.el
> +++ b/lisp/tramp.el
> @@ -281,6 +281,13 @@ pair of the form (KEY VALUE).  The following KEYs are defined:
>      Until now, just \"ssh\"-based, \"sshfs\"-based and
>      \"adb\"-based methods do.
>
> +  * `tramp-config-check'
> +    A function to be called with one argument, VEC.  It should
> +    return a string which is used to check, whether the

Comma not needed?

> +    configuration of the remote host has been changed (which
> +    would require to flush the cache data).  This string is kept
> +    as connection property \"config-check-data\".
> +
>    * `tramp-copy-program'
>      This specifies the name of the program to use for remotely copying
>      the file; this might be the absolute filename of scp or the name of

Also, my added paragraph in manual can be removed now.

Thanks.
Filipp

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Michael Albinus]

Filipp Gunbin <fgunbin@fastmail.fm> writes:

> Hi Michael,

Hi Filipp,

> Generally it works, thank you very much!

Thanks for checking!

> Test case:
>
> 1) kubectl config set-context --current --namespace=main
> 2) dired /kubernetes:myapp-67cd597654-wxdjn:/
> 3) tramp-cleanup-this-connection
> 4) kubectl config set-context --current --namespace=
> 5) again dired /kubernetes:myapp-67cd597654-wxdjn:/ - I get an error and
>   cache is cleared
>
> However, now auto-completing /kubernetes: shows this, why 2?
>
> In this buffer, type RET to select the completion near point.
>
> 2 possible completions:
> kubernetes:

Hmm. It says 2 possible completions, but doesn't show anything. Not sure
what this means.

Anayway, I've added a new user option to Tramp,
tramp-completion-use-cache. If you set it to nil, Tramp doesn't use
cached connections for host name completion. This shall fix your use case.

> Another issue: if at step 5 I instead use existing dired window and
> type, say, "i" on a subdirectory, then I get the same-looking error, but
> the Tramp cache is not cleared (perhaps this is not
> Kubernetes-specific).

Cannot say anything, let's see whether it is still present after the
following changes.

>> +(defun tramp-kubernetes--current-context-data (vec)
>> +  "Return current context data as JSON string."
>> +  (with-temp-buffer
>> +    (when (zerop
>> +	   (tramp-call-process
>> +	    vec tramp-kubernetes-program nil t nil
>> +	    "config" "view" "--minify" "-o" "json"))
>> +      (buffer-string))))
>> +
>
> Let's remove --minify.
>
> Also, maybe save the checksum (simple md5 would do it), not the full
> listing?  The listing may contain sensitive data (for example, mine
> shows my OAuth id token).

Right, there's too much data. And Tramp shouldn't expose sensible
data. Finally, I've changed this to

--8<---------------cut here---------------start------------->8---
# kubectl config view -o jsonpath='{.contexts[?(@.name == "<current-context-name>")]}'
--8<---------------cut here---------------end--------------->8---

with <current-context-name> being the name of the current context. This
shows just the fields in the current context, and nothing else - that's
exactly what we need for the check.

>> +    (when (and (stringp old-config-check) (stringp config-check)
>> +	       (not (string-equal old-config-check config-check)))
>> +      (tramp-message
>> +       vec 3
>> +       "Connection reset, because remote configuration changed from `%s' to `%s'"
>> +       old-config-check config-check)
>
> Currently, we'll get huge config listings here (at the level 3)?  If we
> change to checksums, that'd be not very useful.  So perhaps don't output
> those strings?

With the change above, the check string is much shorter. Yes, we could
use checksums, but this will prevent us to understand why the connection
cache has been cleared.

And this shouldn't happen too often, so I guess a message of level 3 is
still useful and not too annoying.

>> +  * `tramp-config-check'
>> +    A function to be called with one argument, VEC.  It should
>> +    return a string which is used to check, whether the
>
> Comma not needed?

Don't know. In the Tramp sources, both "check, whether" and "check
whether" is used, so it is inconsistent at all :-) Let's keep it, until
somebody teaches me to change it this way or the other in all places.

>>    * `tramp-copy-program'
>>      This specifies the name of the program to use for remotely copying
>>      the file; this might be the absolute filename of scp or the name of
>
> Also, my added paragraph in manual can be removed now.

Done.

I've committed my last version of the patch, because I believe it is
more simple to discuss about. Further comments welcome!

> Thanks.
> Filipp

Best regards, Michael.

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Yuri Khan]

On Tue, 25 Oct 2022 at 21:51, Michael Albinus <michael.albinus@gmx.de> wrote:

+    […] It should
+    return a string which is used to check, whether the
+    configuration of the remote host has been changed (which
+    would require to flush the cache data).

> > Comma not needed?
>
> Don't know. In the Tramp sources, both "check, whether" and "check
> whether" is used, so it is inconsistent at all :-) Let's keep it, until
> somebody teaches me to change it this way or the other in all places.

“whether the configuration <…> has been changed” is a subordinate (aka
dependent) clause. In English, those are separated with a comma only
when coming before the main clause. Here, it comes after the main
clause, so a comma looks like a Germanism.

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Robert Pluim]

>>>>> On Tue, 25 Oct 2022 22:43:07 +0700, Yuri Khan <yuri.v.khan@gmail.com> said:

    Yuri> On Tue, 25 Oct 2022 at 21:51, Michael Albinus <michael.albinus@gmx.de> wrote:
    Yuri> +    […] It should
    Yuri> +    return a string which is used to check, whether the
    Yuri> +    configuration of the remote host has been changed (which
    Yuri> +    would require to flush the cache data).

    >> > Comma not needed?
    >> 
    >> Don't know. In the Tramp sources, both "check, whether" and "check
    >> whether" is used, so it is inconsistent at all :-) Let's keep it, until
    >> somebody teaches me to change it this way or the other in all places.

    Yuri> “whether the configuration <…> has been changed” is a subordinate (aka
    Yuri> dependent) clause. In English, those are separated with a comma only
    Yuri> when coming before the main clause. Here, it comes after the main
    Yuri> clause, so a comma looks like a Germanism.

True, but Iʼm more concerned about the "require to flush" bit, which
should be "require flushing" :-)


Robert
-- 

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Yuri Khan]

On Tue, 25 Oct 2022 at 22:48, Robert Pluim <rpluim@gmail.com> wrote:

>     Yuri> +    configuration of the remote host has been changed (which
>     Yuri> +    would require to flush the cache data).
>
> […] Iʼm more concerned about the "require to flush" bit, which
> should be "require flushing" :-)

Right. Or “require us to flush”.

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Michael Albinus]

Yuri Khan <yuri.v.khan@gmail.com> writes:

Dear Yuri and Robert,

> On Tue, 25 Oct 2022 at 22:48, Robert Pluim <rpluim@gmail.com> wrote:
>
>>     Yuri> +    configuration of the remote host has been changed (which
>>     Yuri> +    would require to flush the cache data).
>>
>> […] Iʼm more concerned about the "require to flush" bit, which
>> should be "require flushing" :-)
>
> Right. Or “require us to flush”.

Please push any change which is worth to improve the Tramp manual. I'm
not a native English speaker, and I'm notorious in doing it wrong.

Thanks, and best regards, Michael.

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Filipp Gunbin]

Hi Michael,

Something messed up in the mailing list (or in my Gnus config :-), I see
only some messages in this thread from today.  Also, the ordering is odd
in web archive:
https://lists.gnu.org/archive/html/emacs-devel/2022-10/msg01946.html

Anyway: I'll test the new patch a bit later, now just wanted to return
to current-context thing.  I'm still sure that we need to cache full
config, because a user may change context as easily as a namespace.  In
fact, a context _is_ a namespace, in a sense, just higher-level
namespace.

Also, context refers to "cluster" by name, and cluster config also may
change, etc.

About checksum: it should not be very important to see what changed,
because the user did the change herself.  It's more important to avoid
storing sensitive data.

Filipp

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Michael Albinus]

Filipp Gunbin <fgunbin@fastmail.fm> writes:

> Hi Michael,

Hi Filipp,

> Something messed up in the mailing list (or in my Gnus config :-), I see
> only some messages in this thread from today.

I recommend to read such MLs via news.gmane.io (as I do). No problems today.

> Anyway: I'll test the new patch a bit later, now just wanted to return
> to current-context thing.  I'm still sure that we need to cache full
> config, because a user may change context as easily as a namespace.  In
> fact, a context _is_ a namespace, in a sense, just higher-level
> namespace.

My proposal is to check all fields of current-context. Like this:

--8<---------------cut here---------------start------------->8---
# kubectl config view -o jsonpath='{.contexts[?(@.name == "docker-desktop")]}'
{"context":{"cluster":"docker-desktop","user":"docker-desktop"},"name":"docker-desktop"}
--8<---------------cut here---------------end--------------->8---

This includes the name of the current context ("docker-desktop"), and if
the user switches to another context, or changes an attribute of the
current context, it will differ.

> Also, context refers to "cluster" by name, and cluster config also may
> change, etc.

Yes, but we cannot check everything. Compare it to an ssh connection,
which uses a Host entry in .ssh/config. If a user changes the Hostname
part of that Host entry, it isn't detected either by Tramp. The user is
responsible for this kind of changes, and the consequences.

And remember, this discussion started because you were concerned about a
changed namespace, nothing else.

> About checksum: it should not be very important to see what changed,
> because the user did the change herself.  It's more important to avoid
> storing sensitive data.

My proposal avoids to store sensitive data. The authentication token
you've mentioned is part of the Users config. We don't see it anymore
according to the latest patch.

> Filipp

Best regards, Michael.

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Filipp Gunbin]

Hi Michael,

On 25/10/2022 19:48 +0200, Michael Albinus wrote:

> Filipp Gunbin <fgunbin@fastmail.fm> writes:
>
>> Something messed up in the mailing list (or in my Gnus config :-), I see
>> only some messages in this thread from today.
>
> I recommend to read such MLs via news.gmane.io (as I do). No problems
> today.

After "make bootstrap", I now see the messages, don't know what was the
cause.

>> Anyway: I'll test the new patch a bit later, now just wanted to return
>> to current-context thing.  I'm still sure that we need to cache full
>> config, because a user may change context as easily as a namespace.  In
>> fact, a context _is_ a namespace, in a sense, just higher-level
>> namespace.
>
> My proposal is to check all fields of current-context. Like this:
>
> # kubectl config view -o jsonpath='{.contexts[?(@.name == "docker-desktop")]}'
> {"context":{"cluster":"docker-desktop","user":"docker-desktop"},"name":"docker-desktop"}
>
> This includes the name of the current context ("docker-desktop"), and if
> the user switches to another context, or changes an attribute of the
> current context, it will differ.
>
>> Also, context refers to "cluster" by name, and cluster config also may
>> change, etc.
>
> Yes, but we cannot check everything. Compare it to an ssh connection,
> which uses a Host entry in .ssh/config. If a user changes the Hostname
> part of that Host entry, it isn't detected either by Tramp. The user is
> responsible for this kind of changes, and the consequences.
> 
> And remember, this discussion started because you were concerned about a
> changed namespace, nothing else.

I think I now agree with you, let's use this approach for now.

I'll give it more testing and will write back.

Thanks,
Filipp

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Filipp Gunbin]

Hi Michael,

I've tested your patch a bit more.  Good news is that strange completion
behavior ("2" displayed when actually no completions) is gone, perhaps
recent "make bootstrap" helped.

The problem with cache (not) clearing remains, although with
tramp-completion-use-cache nil it doesn't show.  I've opened bug#58782
for that.

Nitpicks:

+(defun tramp-kubernetes--current-context-data (vec)
+  "Return Kubernetes current context data as JSONPATH string."

The result is just json, not jsonpath (jsonpath is an expression
language).

-  ;; `tramp-maybe-open-connection', it will be caught there.
+  ;; `tramp-maybe-open-connection', it will be caught there.  The same
+  ;; check will be applied with the function kept in`tramp-config-check'.
                                                     ^
Space missing.

Thank you again,
Filipp

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Michael Albinus]

Filipp Gunbin <fgunbin@fastmail.fm> writes:

> Hi Michael,

Hi Filipp,

> The problem with cache (not) clearing remains, although with
> tramp-completion-use-cache nil it doesn't show.  I've opened bug#58782
> for that.

Will check.

> Nitpicks:
>
> +(defun tramp-kubernetes--current-context-data (vec)
> +  "Return Kubernetes current context data as JSONPATH string."
>
> The result is just json, not jsonpath (jsonpath is an expression
> language).
>
> -  ;; `tramp-maybe-open-connection', it will be caught there.
> +  ;; `tramp-maybe-open-connection', it will be caught there.  The same
> +  ;; check will be applied with the function kept in`tramp-config-check'.
>                                                      ^
> Space missing.

Thanks, fixed in master.

> Thank you again,
> Filipp

Best regards, Michael.

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Filipp Gunbin]

Hi Michael,

While I briefly looked at the patch, I have a doubt that it will help,
see below.

On 24/10/2022 17:07 +0200, Michael Albinus wrote:

> Filipp Gunbin <fgunbin@fastmail.fm> writes:
>
>> Hi Michael,
>
> Hi Filipp,
>
>>> Can we automate this? I mean, when pod data are cached, and Tramp
>>> detects a changed namespace, the cached data should be flushed?
>>>
>>> Tramp does something similar with other connection methods, for example
>>> it checks "uname -sr" on remote hosts, and caches the result. Whenever a
>>> new connection to a host is established, Tramp calls again "uname -sr",
>>> and compares with the cached value. If the values differ, all cached
>>> data for this connection are flushed.
>>>
>>> Is there a similar way to retieve (and cache) the current context and
>>> namespace for pods?
>>
>> (I've actually wrote a message to emacs-devel about this, but then
>> decided it's just simpler to advise resetting cache, and didn't send the
>> message; maybe I should have asked first)
>
> No problem that's what code review is good for :-)
>
>> AFAIU, the general way would be to call "kubectl config view -o json",
>> then calculate checksum of the output and cache it.  This would catch
>> any change in context (namespace it just one case of many).
>
> Yep, but I would recommend "kubectl config view --context=$(kubectl
> config current-context) -o json". We don't need the information about
> other context.

First, I think we do need all context info.  "kubectl config
use-context" is no different from other configuration changes, which a
user may do "outside" of Emacs at any time.

>> However, there're two things to consider here:
>>
>> - "config view" command is not instantaneous: on my machine it's about
>>   100ms, and calling it on each (say) host completion would be annoying.
>
> I don't recommend it for hostname completion. I recommend it for opening
> a connection; the cleanup shall happen in tramp-maybe-open-connection
> via a hook.
>
> If a user changes the context while there is an active connection in
> Tramp, she will be lost. But this isn't an Emacs/Tramp specific
> situation, so we don't need to care or document it.

Second, I (seem to) understand what the uname check is for.  It may help
if some host on the network is recreated.  Or if a user changes network
and the same hostname/IP starts referring to another machine.  But here,
first of all I stumbled upon this problem:

- set some namespace
- open container
- .. then your connection somehow closes, which can be simulated by
  tramp-cleanup-this-connection...
- change namespace
- at this point, if you type "/kubernetes:", you'll get your old pod
  name in the completion

I think completion is much more important in case of Kubernetes than
when dealing with normal hosts: pods usually will have unpredictable
technical names with random parts.  So to get around it, we'll have to
refresh context on every completion.  Doing this when opening connection
doesn't have much sense: you anyway get an error, and you anyway won't
know proper pod names.

We could try checking config file modtime, its faster then kubectl
config view.  https://kubernetes.io/docs/reference/kubectl/ says:

--8<---------------cut here---------------start------------->8---
For configuration, kubectl looks for a file named config in the
$HOME/.kube directory. You can specify other kubeconfig files by setting
the KUBECONFIG environment variable or by setting the --kubeconfig flag.
--8<---------------cut here---------------end--------------->8---

But then we have to follow this, and require the user to tell Emacs
about --kubeconfig in use (if any).

So I end up at the conclusion that in this case it's better for user to
learn that she should reset Tramp cache.  Our help could become more
confusing than helping.

WDYT?

Thanks,
Filipp

Re: master 55eabe96c9: ; Improve manual for Tramp kubernetes method

[Filipp Gunbin]

On 24/10/2022 23:41 +0300, Filipp Gunbin wrote:

> Second, I (seem to) understand what the uname check is for.  
[...]

I think you can ignore my second point, I now see what you mean.  We've
got a closed connection, get an outdated completion, try to open
connection, get an error and at this moment the cache is cleared.  Next
time the completion will be alright.

The first (getting full config) still holds.  I'll now test the patch,
thanks!

Filipp