We're not going to support all possible cases (ex looking here I also see a key 3 digest_null "" cipher_null ""), right?

Oh, good question!
Do we know all possible supported formats?

Oh, good question! Do we know all possible supported formats?

These are used in our tests, but there could be more combination with different formats.

I remember discussing (and tried to implement) this with @pchaigno during my on-boarding. IIRC, the outcome of the discussion was "It's a nice to have, but the burden of maintaining additional code might be heavier than the advantages brought".

I think my opinion didn't change, though I'm happy to discuss it and reconsider. My main question would be what problem do we aim to solve with this additional code? Any code we add will need maintenance, any abstraction will limit what users can do. Even if these inconveniences are small (as is likely the case here), we need to weight them against benefit(s). If the benefits are negligible (ex. the command looks nicer), then it's probably not worth it.

My main question would be what problem do we aim to solve with this additional code? Any code we add will need maintenance, any abstraction will limit what users can do. Even if these inconveniences are small (as is likely the case here), we need to weight them against benefit(s). If the benefits are negligible (ex. the command looks nicer), then it's probably not worth it.

I see your points, fair enough!

My arguments are:

1. For the users (who are not familiar with XFRM) Cilium CLI can be more friendly than bash commands.
2. Bash tools might have different behavior on different OSs.
3. Bash tools might not be installed on user boxes.
4. We already have key rotation support in Cilium CLI which does the same thingy and requires the same maintenance from our side. So, I think it should also be removed if we decide not to introduce this one.
5. Advanced users are still not limited to using bash tools.

CC: @smagnani96

1. For the users (who are not familiar with XFRM) Cilium CLI can be more friendly than bash commands.

I'd like to provide users the easiest way to interact with Cilium.
Our configurations might be hacky or tricky sometimes.
We could provide such CLI command with "bare minimum" support (ex. just aead algo), leaving advanced config to expert users who need specific settings.
After all, that's similar to how users can install Cilium (either via cli install or directly via helm).

2. Bash tools might have different behavior on different OSs.

In theory that's right, in practice I don't think we're using sorceries (echo, dd, xxd), so I wouldn't worry too much for this.

3. Bash tools might not be installed on user boxes.

I remember once I received an error xxd: unknown command upon issuing the following, but I don't recall the env:

kubectl create -n kube-system secret generic cilium-ipsec-keys --from-literal=keys="3 rfc4106(gcm(aes)) $(echo $(dd if=/dev/urandom count=20 bs=1 2> /dev/null | xxd -p -c 64)) 128"

4. We already have key rotation support in Cilium CLI which does the same thingy and requires the same maintenance from our side. So, I think it should also be removed if we decide not to introduce this one.

Partially agree. Unfortunately, the commands to patch the k8s secret are less immediate than a simple create, as shown https://github.com/cilium/cilium/blob/main/.github/actions/ipsec-key-rotate/action.yaml#L26-L39.

So the rotate command is definitively convenient to keep it (even though it does not support all algo formats).

5. Advanced users are still not limited to using bash tools.

The manual create secret command would still be available to users.

@pchaigno WDYT?

I still remember copying and pasting commands too quickly, failing to notice error messages (such as xxd: command not found) "hidden" in the output multiple times. If a cilium-cli create-key command existed, I must admit I’d prefer to use it.

However, cilium-cli create-key --auth-algo gcm-aes --wait-duration 1 doesn’t cover all current use cases. For example, cilium-cli --context should be considered for clustermesh scenarios, which is precisely Paul’s concern: any abstraction will limit what users can do.

My personal take is that we should incorporate the new CLI into all ipsec gh workflows as part of the pr, demonstrating that it is robust enough to handle our current use cases. Even if this can only help developers make less mistakes, I think it's of value.

Is this a WIP?

Is this a WIP?

Yes, I've converted to the draft.
Sorry for the delay.

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

/ci-ipsec-e2e

/ci-eks

@tamilmani1989 kind ping)

Hey @cilium/azure, could you please review the PR.

@cilium/azure (@tamilmani1989, @ungureanuvladvictor, @hemanthmalla, @tommyp1ckles), gentle ping. It would be great to move forward with this PR.

/test

/test