Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd: Fix --authfile/--insecure flags #2242

Merged
merged 2 commits into from
Nov 28, 2023

Conversation

mauriciovasquezbernal
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal commented Nov 20, 2023

d1bccf6 ("gadgets/run: Support eBPF parameters") introduced a logic to remove the flags before calling GetGadgetInfo(), unfortunately it also broke the --authfile/--insecure flags as they were removed.

This commit changes the approach to use ParseEarlyFlags() to parse flags that are already known before contacting the server, like --insecure & --authfile, so they can be used by the server while it executes GetGadgetInfo().

Fixes: d1bccf6 ("gadgets/run: Support eBPF parameters")
Fix #2225

How to test

  1. Push a gadget to a insecure registry. cmd: flags for registry AuthOptions (--authfile/--insecure) doesn't seem to be working #2225 contains instructions for it. In my case I pushed it to 192.168.1.150:5000/foo:latest

I manually tested the following:

ig

sudo -E ig run 192.168.1.150:5000/foo:latest --insecure
INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME                  PID                  PPID                 COMM             UID                  GID                 RETVAL

ig + gadgectl using default unix socket

$ sudo -E ig daemon
INFO[0000] Experimental features enabled                
INFO[0000] starting Inspektor Gadget daemon at "unix:///var/run/ig/ig.socket"
...

$ sudo -E ./gadgetctl run 192.168.1.150:5000/foo:latest 
INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME                  PID                  PPID                 COMM             UID                  GID                 RETVAL

ig + gadgectl using tcp

$ sudo -E ig daemon -H tcp://127.0.0.1:1234
INFO[0000] Experimental features enabled                
INFO[0000] starting Inspektor Gadget daemon at "tcp://127.0.0.1:1234"

...
$ ./gadgetctl run 192.168.1.150:5000/foo:latest --remote-address tcp://127.0.0.1:1234 --insecure
INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME                  PID                  PPID                 COMM             UID                  GID                 RETVAL

kubectl gadget

$ ./kubectl-gadget run 192.168.1.150:5000/foo:latest --insecure -A 
INFO[0000] Experimental features enabled                
K8S.NODE              K8S.NAMESPACE         K8S.POD               K8S.CONTAINER        PID         PPID        COMM        UID         GID         RETVAL

TODO

  • Add integration tests for for --insecure

d1bccf6 ("gadgets/run: Support eBPF parameters") introduced a logic
to remove the flags before calling GetGadgetInfo(), unfortunately
it also broke the --authfile/--insecure flags as they were removed.

This commit changes the approach to use ParseEarlyFlags() to parse
flags that are already known before contacting the server, like
--insecure & --authfile, so they can be used by the server while it
executes GetGadgetInfo().

Fixes: d1bccf6 ("gadgets/run: Support eBPF parameters")

Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>
@mauriciovasquezbernal
Copy link
Member Author

I added an integration test for the --insecure flag, unfortunately doing the same for the --authfile isn't so easy as it'll require to configure native basic auth and TLS certificates in the registry. I'll open an issue to handle this later on.

Copy link
Member

@eiffel-fl eiffel-fl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it and it works fine:

$ sudo -E ./ig run localhost:5000/gadget/open --insecure
INFO[0000] Experimental features enabled                
RUNTIME.CONTAINERNAME          PID              COMM             UID              GID              RET   FNAME                           
elated_payne                   85381            bash             0                0                3     /etc/ld.so.cache                

I nonetheless do not understand what is wrong with the test as the image seems to be copied:

Copied [registry] ttl.sh/3285379a0774706d29b7142bb33e117faedaf732/trace_open:2242-merge => [registry] 10.244.1.20:5000/trace_open:2242-merge
Digest: sha256:28a480836f3a974cbb847b4c615c31bb24a3d83e8d729b8c6518cee357c0c8e4

@mqasimsarfraz
Copy link
Member

It seems that the copier pods keeps on restarting, perhaps we need pod restart policy Never. Maybe the failure is related to that? 🤔

@mauriciovasquezbernal
Copy link
Member Author

It seems that the copier pods keeps on restarting, perhaps we need pod restart policy Never. Maybe the failure is related to that? 🤔

I made this change, kicked the CI few times and it seems to be working fine now. Thanks!

Copy link
Member

@burak-ok burak-ok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
In other places we also have sleep commands but we could maybe change that

}
RunTestSteps(orasCpCmds, t)

// TODO: Ideally it should not depend on a real gadget, but we don't have a "test gadget" available yet.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created issue #2255 to track this.

integration/inspektor-gadget/run_insecure_test.go Outdated Show resolved Hide resolved
Signed-off-by: Mauricio Vásquez <mauriciov@microsoft.com>
Copy link
Member

@mqasimsarfraz mqasimsarfraz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested it and it works fine, thanks!

@mauriciovasquezbernal mauriciovasquezbernal merged commit 556d762 into main Nov 28, 2023
50 checks passed
@mauriciovasquezbernal mauriciovasquezbernal deleted the mauricio/fix-gadget-flags branch November 28, 2023 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cmd: flags for registry AuthOptions (--authfile/--insecure) doesn't seem to be working
4 participants