bug 1727104: daemon/controller: handle empty ignition configs

[kikisdeliveryservice]

Wrap ignition validation in helper function that adds exception
for empty ign configs. Use helper validation function in update.go
and render_controller.go

Update and add unit tests

Closes: #888

[runcom]

Instead of letting an empty Ignition version trigger an error, let's bypass that for all MachineConfigs since it seems unnecessarily confusing for most users.

uhm, the version check when a ignition config is provided is actually valuable and Andrew has been telling us forever and that was probably why we added it.

Why don't we do like Colin said in #888 (comment) (since it's not a pointer)? that way we still leave the version check (w/o changing behavior between openshift versions) and allow for non-Ignition MC to go through as well w/o version

We still want to error out on:

apiVersion: machineconfiguration.openshift.io/v1
kind: MachineConfig
metadata:
  labels:
    machineconfiguration.openshift.io/role: worker
  name: 50-kargs
spec:
  kargs:
    - nosmt
  config:
    storage:
      files:
      - contents:
          source: data:,infra
        filesystem: root
        mode: 0644
        path: /etc/infra

[kikisdeliveryservice]

@runcom this PR only covers the case when an ignition version is empty - if an ignition version is provided, everything functions (including the checks) as it did before.

I think that errorring out for general empty ignition values across the board makes little sense as a UX (I get questions about this a lot) and is super confusing whether or not the user is submitting kargs. if empty why not just supply a default value (which this PR does) and let the checks process whether or not the ignition config is valid otherwise and meets our reconcilable criteria (including our ignition checks)? Lots of people submit otherwise valid configs whose only error is not having an ignition version even though we have said in the past we want to de-emphasize actual ignition as a customer facing tool and use it more under the hood.

cc: @cgwalters thoughts?

[runcom]

I think that errorring out for general empty ignition values across the board makes little sense as a UX (I get questions about this a lot) and is super confusing whether or not the user is submitting kargs. if empty why not just supply a default value (which this PR does) and let the checks process whether or not the ignition config is valid otherwise and meets our reconcilable criteria (including our ignition checks)?

The version check is to make sure the ignition embedded has the correct version to work with us and be understandable by the MCD - I'm not sure why people complain about this but it's something we enforce and it must be enforced to have valid ignition embedded (till we change it to something else?). It's a check for correctness, as annoying as it may be (when we move away from ignition hopefully that goes away as well and we can rely on kube types versions?)
Example, if someones provide Ignition version v3 w/o providing the version, it's completely wrong to assume v2 and inject v2 (the MCD is also gonna fail anyway).

The issue here is that we shouldn't validate the inner config for the MC when it's empty regardless of version (Colin provided a suggestion in the issue to just walk the struct and derive emptiness/changes) nor I believe we should add default w/o knowledge of what it's embedded in the MC itself, that's just skipping the check so we might as well get rid of the check as well cause it's not fully honored anymore.

[kikisdeliveryservice]

Example, if someones provide Ignition version v3 w/o providing the version, it's completely wrong to assume v2 and inject v2 (the MCD is also gonna fail anyway).

The question, I think, is failing on an empty version field or failing on the rest of the MC as a UX. Given past discussions of the place of ignition in MCs, I wanted to bring up the possibility of not failing on empty and using a default instead.

I can switch this only to kargs, but also wanted to bring up this possibility of using the MC as an MC that uses Ignition tools rather than functions strictly as an Ignition clone (as per our past discussions).

[cgwalters]

Right, my initial suggestion was to make it not an error to have an "empty" Ignition config, in other words it's OK to not have a version specified if the config is otherwise empty.

In other words replace all of our calls to validate.ValidateWithoutSource with a wrapper function that does something like:

func ValidateIgnition(cfg *Ignition) error {
  emptyIgnition := &Ignition{}
  if reflect.DeepEqual(&emptyIgnition, cfg) {
    return nil
  }
  return validate(cfg)
}

[kikisdeliveryservice]

Right, my initial suggestion was to make it not an error to have an "empty" Ignition config, in other words it's OK to not have a version specified if the config is otherwise empty.

Ok, gotcha.

[runcom]

if reflect.DeepEqual(&emptyIgnition, cfg) {

I wonder if that takes care of the comparison for emptiness but I think it will just fine since we're not moving to newer ignition config now so we don't risk a new added field faling the DeepEqual check 👍

[kikisdeliveryservice]

I think I was thinking too much about our future of MCs discussions, which, is not happening in this PR or soon.. 😄

if reflect.DeepEqual(&emptyIgnition, cfg) {

I wonder if that takes care of the comparison for emptiness but I think it will just fine since we're not moving to newer ignition config now so we don't risk a new added field faling the DeepEqual check +1

It should be fine, I'll let you know if I run into any trouble. I'll add a test for that as well to cover it.

[kikisdeliveryservice]

While working on this PR I noticed an error so this bug will fix BZ 1726866 in master and once approved and merged will be backported.

[kikisdeliveryservice]

/retest

[kikisdeliveryservice]

I will have to update TestReconcileAfterBadMC() since applying a bad MC will not result in a render config that is generated but then fails. With this PR a bad MC will result in no new render config being generated...

Looking at the e2e I think that there will end up being some redundancy between TestReconcileAfterBadMC() and the existing TestPoolDegradedOnFailToRender() if I fix it to account for the fixed validation since they are both now testing the same thing so I'll refactor them into one test.

[kikisdeliveryservice]

this really shouldn't be failing seeing weird things in e2e logs...

[kikisdeliveryservice]

/retest

[kikisdeliveryservice]

"Error: Error creating S3 bucket: TooManyBuckets: You have attempted to create more buckets than allowed"

/retest

[kikisdeliveryservice]

/retest

[runcom]

/lgtm
/retest

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Details

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

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[kikisdeliveryservice]

/retest

[openshift-ci-robot]

@kikisdeliveryservice: The Bugzilla bug is in an unrecognized state (ON_QA) and will not be moved to the MODIFIED state.

Details

In response to this:

bug 1727104: daemon/controller: handle empty ignition configs

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.