Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Explicitly specifying nonexistent trigger plan is silently treated as deploy #1268

Closed
porridge opened this issue Jan 14, 2020 · 4 comments · Fixed by #1336 or #1345
Closed

Explicitly specifying nonexistent trigger plan is silently treated as deploy #1268

porridge opened this issue Jan 14, 2020 · 4 comments · Fixed by #1336 or #1345

Comments

@porridge
Copy link
Member

porridge commented Jan 14, 2020

What happened:

I specified a non-existent plan name in a parameter's trigger attribute. The validation passed and I run an update:

$ cat params.yaml 
apiVersion: kudo.dev/v1beta1
parameters:
- name: NAME
  required: False
  trigger: nonexistent
$ PATH=/home/porridge/Desktop/work/mesosphere/code/kudo/bin:$PATH kubectl kudo package verify .
package is valid
$ PATH=/home/porridge/Desktop/work/mesosphere/code/kudo/bin:$PATH kubectl kudo update --instance minimal-foo6 -p NAME=minimal-foo6-changed
Instance minimal-foo6 was updated.$

This resulted in the deploy plan running.

$ kubectl describe instance minimal-foo6
Name:         minimal-foo6
Namespace:    default
Labels:       kudo.dev/operator=minimal
Annotations:  kudo.dev/last-applied-instance-state: {"operatorVersion":{"name":"minimal-0.6.0"},"parameters":{"NAME":"minimal-foo6-changed"}}
API Version:  kudo.dev/v1beta1
Kind:         Instance
Metadata:
  Creation Timestamp:  2020-01-14T12:27:20Z
  Generation:          2
  Resource Version:    10888
  Self Link:           /apis/kudo.dev/v1beta1/namespaces/default/instances/minimal-foo6
  UID:                 407a9322-d6f7-4f11-a38f-dbcef3983871
Spec:
  Operator Version:
    Name:  minimal-0.6.0
  Parameters:
    NAME:  minimal-foo6-changed
Status:
  Aggregated Status:
    Status:  COMPLETE
  Plan Status:
    Deploy:
      Last Finished Run:  2020-01-14T12:28:06Z
      Name:               deploy
      Phases:
        Name:    main
        Status:  COMPLETE
        Steps:
          Name:    app
          Status:  COMPLETE
      Status:      COMPLETE
      UID:         0804dd5f-d45b-49ba-b519-07a01730e3c4
Events:
  Type    Reason        Age               From                 Message
  ----    ------        ----              ----                 -------
  Normal  PlanStarted   7s (x2 over 53s)  instance-controller  Execution of plan deploy started
  Normal  PlanFinished  7s (x2 over 53s)  instance-controller  Execution of plan deploy finished with status COMPLETE
$

What you expected to happen:

Verification should fail, and an error should be returned at update time complaining about the wrong plan name.

How to reproduce it (as minimally and precisely as possible):

See above.

@zen-dog
Copy link
Contributor

zen-dog commented Jan 14, 2020

This is by design: we use update and deploy plans as the fallback when the triggered plan couldn't be found.

@porridge
Copy link
Member Author

It's fine to use update or deploy if there is no trigger plan specified at all. However silently falling back to a different plan than explicitly specified is dangerous. A typo can have dear consequences.

@gerred agreed when I asked him before filing this issue.

@zen-dog
Copy link
Contributor

zen-dog commented Jan 14, 2020

However silently falling back to a different plan than explicitly specified is dangerous.

ah, that I agree with. I'll fix this as part of admission webhook work

@ANeumann82
Copy link
Member

This wasn't fixed in #1188 so I fixed it in another PR

zen-dog added a commit that referenced this issue Feb 13, 2020
…trigger

Summary:
This is a followup to #1336 which verified that triggered plans are actually defined in the OV. This PR adds this logic to the instance webhook.

Fixes #1268

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
zen-dog pushed a commit that referenced this issue Feb 17, 2020
…trigger (#1345)

Summary:
This is a followup to #1336 which verified that triggered plans are actually defined in the OV. This PR adds this logic to the instance webhook.

Fixes #1268

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
runyontr pushed a commit that referenced this issue Mar 11, 2020
…trigger (#1345)

Summary:
This is a followup to #1336 which verified that triggered plans are actually defined in the OV. This PR adds this logic to the instance webhook.

Fixes #1268

Signed-off-by: Aleksey Dukhovniy <alex.dukhovniy@googlemail.com>
Signed-off-by: Thomas Runyon <runyontr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment