MCD: add ign validation check for mc.ignconfig

[kikisdeliveryservice]

- What I did

• Add ignition validation check on mc.ignconfig
• Added unit tests for ignition validation & update existing unit tests to use valid ign configs

Closes: #479

- How to verify it
create a proper machineconfig and oc create -f it - SUCCESS!:

apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfig
metadata:
  labels:
    machineconfiguration.openshift.io/role: worker
  name: good-test
spec:
  config:
    ignition:
      version: 2.2.0
    storage:
      files:
      - contents:
          source: data:,hello%20world%0A
          verification: {}
        filesystem: root
        mode: 420
        path: /home/core/prtest

create a bad/invalid machineconfig (here we are using a relative path which is bad) and apply it - Failure!

apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfig
metadata:
  labels:
    machineconfiguration.openshift.io/role: worker
  name: bad-test
spec:
  config:
    ignition:
      version: 2.2.0
    storage:
      files:
      - contents:
          source: data:,hello%20world%0A
          verification: {}
        filesystem: root
        mode: 420
        path: home/core/prtest

for the bad config daemon logs oc logs -p ... you should see something like:
I0226 02:48:52.743267 6205 update.go:668] Can't reconcile config worker-54afbf0177f8fb7dd414e3f606b7362d with worker-a59fd0f825db6a767251a2b3cd9563e2: Invalid Ignition config found: error: path not absolute

[runcom]

I do love this test and the general approach 👍

[runcom]

let's use the exported interface NewClusterDrivenDaemon directly, we'll be able to avoid making assumption on the object itself and avoid falling into stuff like #476

[kikisdeliveryservice]

thanks for this tip!

[kikisdeliveryservice]

Checking this out, the whole file should use that exported interface if you think that's the best thing. I'll update them all in another commit/PR once I get this generally working 😄

[kikisdeliveryservice]

Gah, rebase messed up my test. Fixing now.

[kikisdeliveryservice]

/test e2e-aws-op

[runcom]

Just a comment, I believe this is great :)

[runcom]

Lowercase error (rationale is that when you concat error bubbling up you don't want to have errors like Failed to: Because of something: Which failed)

[kikisdeliveryservice]

so you are saying rpt, err := validate...?

[runcom]

Nope, just the format of the error message :) (the actual sentence in the error, start it lowercase)

[kikisdeliveryservice]

Gotcha! I'll update the whole file this way to be consistent in a separate PR! 😄

[runcom]

@ajeddeloh ptal

[runcom]

/approve

[runcom]

As a follow up, I would create another failing case where the MC doesn't have the ignition version and it correctly fails. Not for this PR.

[kikisdeliveryservice]

sure thing!

[ajeddeloh]

LGTM aside from one possible typo.

[cgwalters]

I'm not opposed to doing this in the MCD. However, there is IMO a strong argument for doing it in the MCC, specifically the render controller.

It's the renderer which merges in the MC fragments and generates the "rendered" i.e. "master-" version. If we validate things there it happens once rather than on every node. (Or if it fails, on both a master and worker causing them to go degraded)

[ajeddeloh]

@cgwalters We should do it on both ends. Compared to actually applying changes validation is cheap. IMO if we can have different parts of the system check each other instead of trusting each other that makes a more robust system and debugging easier.

[cgwalters]

We should do it on both ends.

👍

[kikisdeliveryservice]

I think it makes more sense for the validation to be in once place bc in general ign validation doesn't say anything about the mc being reconcilable so the same mc that passes that basic validation in MCC can fail and degrade later in the MCD.

Thinking about UX, I think it's confusing to have a MC fail in multiple places ie MCC/MCD and have to check logs in both to understand what was wrong.

[kikisdeliveryservice]

/test e2e-aws

[cgwalters]

I think it makes more sense for the validation to be in once place

The logic for validating an Ignition config is in one place - Ignition itself right? We're talking about having two callers for the same code.

in general ign validation doesn't say anything about the mc being reconcilable

Yes - but the scenario here I think we're mainly worried about is the case where a user uploads a bad Ignition config. "Reconciliation" is about a difference between two configs.

Thinking about UX, I think it's confusing to have a MC fail in multiple places ie MCC/MCD and have to check logs in both to understand what was wrong.

I think we should expect admins to look at oc get machineconfigpool - and the status there rolls up into our high level operator status (oc get clusteroperator/machine-config) and from there rolls up into oc get clusterversion.

The MCC is in control of the pool and is a good place to report status.

[ajeddeloh]

I think it makes more sense for the validation to be in once place bc in general ign validation doesn't say anything about the mc being reconcilable so the same mc that passes that basic validation in MCC can fail and degrade later in the MCD.

I think that's fine to have failures in multiple places especially if it means that users can get feedback more immediately. The sooner into an update we can catch a potential problem the better.

Thinking about UX, I think it's confusing to have a MC fail in multiple places ie MCC/MCD and have to check logs in both to understand what was wrong.

I'm a bit out of my wheelhouse here, but it seems like we should make these kinds of errors (especially when a human is writing things) easy to surface. Even better would be to validate things before adding them to cluster (i.e. do a dry run and make sure it validates before actually applying a change). On CL we get a decent number of errors that are just from Ignition validation failure. Ignition in general tries to do as much as it can up front to make sure things are good, then also double check things when running (and do more checks it couldn't do when not running on the host).

[cgwalters]

Another argument for doing this in the MCC as well - today if a bad config is generated, then it will be provided to new nodes that boot via the MCS and passed to Ignition where the node will fail to boot entirely because it's actually Ignition running in the initramfs parsing it.

The MCD is about incremental updates post-install.

[kikisdeliveryservice]

@cgwalters & @ajeddeloh make great points. I will open a MCC PR separate from this one.

[cgwalters]

Even better would be to validate things before adding them to cluster (i.e. do a dry run and make sure it validates before actually applying a change).

Right...in Kubernetes land that gets into (AIUI) "validating admission webhooks". Or it looks like CRDs can have openAPI validation.

What you're talking about is more giving an error at oc create -f /path/to/mc.yaml. What I'm talking about is having the renderer not even try to provide it to a node at update time. What this PR is doing is validating it on each machine.

[kikisdeliveryservice]

currently walking @LorbusChris through testing the PR. :)

[kikisdeliveryservice]

gah aws at it again: level=error msg="\t* aws_internet_gateway.igw: error attaching EC2 Internet Gateway (igw-0b8e5771c75e84ef7): timeout while waiting for state to become 'success' (timeout: 2m0s)"

/test e2e-aws

[kikisdeliveryservice]

Rebased against master for @LorbusChris testing. No changes to my commits.

[LorbusChris]

Succesfully tested this on an AWS cluster. Thanks for the guidance @kikisdeliveryservice !

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ajeddeloh, kikisdeliveryservice, LorbusChris, runcom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [kikisdeliveryservice,runcom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment