Make cilium install idempotent

[lfundaro]

[No tests yet, waiting for discussion with Authors]

This PR makes the command cilium install idempotent.
It checks the error returned from the API and continues
in case the resource has been created already.

Fixes: #205

Signed-off-by: Lorenzo Fundaró lorenzofundaro@gmail.com

[lfundaro]

No tests yet, since there remains the question whether cilium install should be idempotent.

[pchaigno]

From the issue reactions, it looks like it would be appreciated :-)

Could you update the PR with tests? I switched to draft mode in the meantime.

[lfundaro]

From the issue reactions, it looks like it would be appreciated :-)

Could you update the PR with tests? I switched to draft mode in the meantime.

sure 👍 . I'll ping you back when I have something to review. Cheers.

[lfundaro]

@pchaigno, one question, in this PR we are skipping the installation of certificates if we see there is a resource already there. Is this the behavior that we want ? I'm thinking some edge cases:

1. As an operator, if I want to update all cilium certificates I could just issue cilium install and expect that all certificates would be refreshed. In this case, instead of skipping like in this block we would upsert the certificate, i.e.: if the k8s secret already exits, we just refresh data with a new cert, otherwise we create a new k8s secret.
2. From a security perspective, we may want cilium-cli to be authoritative on the installation process as a whole, i.e.: we guarantee that installed certificates come exclusively from the cli instead from unknown sources.

wdyt ?

I'm leaning towards upsert but I'm open to discuss.

[pchaigno]

AFAIU, option 1 also means the command won't actually be idempotent. Probably worth asking the wider community for opinions on #development (although note many folks are currently OOO with the long weekend).

[lfundaro]

AFAIU, option 1 also means the command won't actually be idempotent. Probably worth asking the wider community for opinions on #development (although note many folks are currently OOO with the long weekend).

yes, we would lose idempotency, but if option 2 is more important then we may as well forget about that property. I will ask on a wider forum. Cheers !

[ti-mo]

As an operator, if I want to update all cilium certificates I could just issue cilium install and expect that all certificates would be refreshed. In this case, instead of skipping like in this block we would upsert the certificate, i.e.: if the k8s secret already exits, we just refresh data with a new cert, otherwise we create a new k8s secret.

Making cilium install perform cert upserts would be surprising to me as an operator. This probably belongs in cilium upgrade (following Helm behaviour) or might even warrant a new cilium rotate or cilium encryption rotate command (for clustermesh/wg/ipsec).

As discussed on Slack, introducing skips does not add idempotency. It will cause headaches when we make this install routine create more resources and a user runs cilium install instead of cilium upgrade. Either we merge cilium upgrade into cilium install and make the whole lot fully idempotent (including diffs and dry-run), or we simply point the user to cilium upgrade when one of the resources created in cilium install already exists, with a graceful exit code.

[lfundaro]

or we simply point the user to cilium upgrade when one of the resources created in cilium install already exists, with a graceful exit code.

I would lean towards this approach. In practical terms, it yields the same value as merging upgrade into install and is waaaay much simpler. 😄 👍

I'll ping you both when I have a commit.

Cheers !

[lfundaro]

closing this PR and opening #282 since this is no longer about making install idempotent. cc @ti-mo @pchaigno