migration: Default CK8S_DRY_RUN_INSTALL to true

[simonklb]

Warning

This is a public repository, ensure not to disclose:

• personal data beyond what is necessary for interacting with this pull request, nor
• business confidential information, such as customer names.

What kind of PR is this?

Required: Mark one of the following that is applicable:

• kind/feature
• kind/improvement
• kind/deprecation
• kind/documentation
• kind/clean-up
• kind/bug
• kind/other

Optional: Mark one or more of the following that are applicable:

Important

Breaking changes should be marked kind/admin-change or kind/dev-change depending on type
Critical security fixes should be marked with kind/security

• kind/admin-change
• kind/dev-change
• kind/security
• [kind/adr](set-me)

What does this PR do / why do we need this PR?

Without this the script fails when it's unset.

Information to reviewers

Checklist

• Proper commit message prefix on all commits
• Change checks:
  • The change is transparent
  • The change is disruptive
  • The change requires no migration steps
  • The change requires migration steps
  • The change updates CRDs
  • The change updates the config and the schema
• Documentation checks:
  • The public documentation required no updates
  • The public documentation required an update - [link to change](set-me)
• Metrics checks:
  • The metrics are still exposed and present in Grafana after the change
  • The metrics names didn't change (Grafana dashboards and Prometheus alerts required no updates)
  • The metrics names did change (Grafana dashboards and Prometheus alerts required an update)
• Logs checks:
  • The logs do not show any errors after the change
• PodSecurityPolicy checks:
  • Any changed Pod is covered by Kubernetes Pod Security Standards
  • Any changed Pod is covered by Gatekeeper Pod Security Policies
  • The change does not cause any Pods to be blocked by Pod Security Standards or Policies
• NetworkPolicy checks:
  • Any changed Pod is covered by Network Policies
  • The change does not cause any dropped packets in the NetworkPolicy Dashboard
• Audit checks:
  • The change does not cause any unnecessary Kubernetes audit events
  • The change requires changes to Kubernetes audit policy
• Falco checks:
  • The change does not cause any alerts to be generated by Falco
• Bug checks:
  • The bug fix is covered by regression tests

[elastisys-staffan]

Nice catch! How did you find it? It is set here and here which I assumed would cover all cases. There are identical checks in several places, should we change them as well?

[simonklb]

Nice catch! How did you find it?

While developing and running a migration snippet directly instead of via ck8s upgrade.

It is set here and here which I assumed would cover all cases.

Any reason why you didn't add it to the prepare template? It was a prepare snippet in my case.

I've added it here: dd51e0b let me know if there is a reason for it to not be there and I'll revert it!

There are identical checks in several places, should we change them as well?

Indeed we should! 1552d39

[elastisys-staffan]

Nice catch! How did you find it?

While developing and running a migration snippet directly instead of via ck8s upgrade.

It is set here and here which I assumed would cover all cases.

Any reason why you didn't add it to the prepare template? It was a prepare snippet in my case.

I've added it here: dd51e0b let me know if there is a reason for it to not be there and I'll revert it!

There are identical checks in several places, should we change them as well?

Indeed we should! 1552d39

The original task left out the prepare step so that's why it's not there. My understanding is that the helper functions that implements dry-run should only be used for apply jobs. Prepare jobs is for local config changes using the yq helpers.

I'm not against adding it here as well, but I would like to hear @Ajarmar 's take on it.

[simonklb]

Nice catch! How did you find it?

While developing and running a migration snippet directly instead of via ck8s upgrade.

It is set here and here which I assumed would cover all cases.

Any reason why you didn't add it to the prepare template? It was a prepare snippet in my case.
I've added it here: dd51e0b let me know if there is a reason for it to not be there and I'll revert it!

There are identical checks in several places, should we change them as well?

Indeed we should! 1552d39

The original task left out the prepare step so that's why it's not there. My understanding is that the helper functions that implements dry-run should only be used for apply jobs. Prepare jobs is for local config changes using the yq helpers.

I'm not against adding it here as well, but I would like to hear @Ajarmar 's take on it.

cc @aarnq as well

In this specific case, I've added a "pre-flight" / acceptance test as a prepare snippet which uses (read-only) Helm commands.

[simonklb]

Here:

compliantkubernetes-apps/scripts/migration/crossplane.sh

Line 56 in d02faf7

if ! echo "${values}" | helm_do "${1}" diff "${helm_diff_args[@]}"; then

And in use here: https://github.com/elastisys/compliantkubernetes-apps/blob/5c3a466f6fa364d1b987ba4e0636a658a4414d10/migration/v0.50/prepare/90-module-node-local-dns-acceptance.sh

[aarnq]

Should the default really be to mutate things?

Also shouldn't get / read operations be performed even if we dry-run, so why do you need to run it with false from prepare snippets given the current state of it being only implemented for things normally done in apply snippets? I think limiting it would make it safer for use in that way in prepare snippets, right? 🤔

[simonklb]

Should the default really be to mutate things?

Good point! 9f7c42f

Also shouldn't get / read operations be performed even if we dry-run, so why do you need to run it with false from prepare snippets given the current state of it being only implemented for things normally done in apply snippets? I think limiting it would make it safer for use in that way in prepare snippets, right? 🤔

I'm not sure what you're after here, limit how and where?

[aarnq]

Should the default really be to mutate things?

Good point! 9f7c42f

Also shouldn't get / read operations be performed even if we dry-run, so why do you need to run it with false from prepare snippets given the current state of it being only implemented for things normally done in apply snippets? I think limiting it would make it safer for use in that way in prepare snippets, right? 🤔

I'm not sure what you're after here, limit how and where?

I'm not sure either, I think it doesn't apply anymore as you changed the default 😄

[simonklb]

@elastisys-staffan @Ajarmar any last words?