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.

Sign up for GitHub

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

Jump to bottom

switch reversed old/new objects to validation #19030

Closed
wants to merge 1 commit into from
Closed

switch reversed old/new objects to validation #19030

wants to merge 1 commit into from

Conversation

deads2k
Copy link
Contributor

@deads2k deads2k commented Mar 20, 2018

New objects come first. This means that our update validation never checked to make sure the new image was valid. That means we could have invalid data sitting somewhere. Fun.

@bparees @miminar this is how the annotation from that controller got in there.
@openshift/api-review what shall we do about this one....

/hold

holding while we sort out what to do about the damage.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 20, 2018
@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 20, 2018
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k

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:

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2018
@deads2k
Copy link
Contributor Author

deads2k commented Mar 20, 2018

@openshift/api-review what shall we do about this one....

images are pretty tightly controlled. We may be able to simply remove the bad signature annotations and most data will be clean.

@deads2k deads2k mentioned this pull request Mar 20, 2018
Closed
@liggitt
Copy link
Contributor

liggitt commented Mar 20, 2018 •
edited
Loading

images are pretty tightly controlled. We may be able to simply remove the bad signature annotations and most data will be clean.

that's about the only reasonable automated solution I see

@deads2k
Copy link
Contributor Author

deads2k commented Mar 20, 2018

/retest

1 similar comment
@deads2k
Copy link
Contributor Author

deads2k commented Mar 20, 2018

/retest

@bparees
Copy link
Contributor

bparees commented Mar 20, 2018

ultimately I think there are 3 things we need to fix:

  1. fix the validation (this pr)
  2. proceed w/ @deads2k suggestion to fix prepareforupdate to strip the bad data
  3. fix the imagesignaturecontroller to stop adding the annotation.

note that item (3) must go in concurrent w/ this PR or else it's going to start getting errors on update.

@bparees
Copy link
Contributor

bparees commented Mar 20, 2018

@miminar i suggest you create a single PR that has all 3 items (and then we're going to need to backport it to 3.9 and 3.7)

@miminar
Copy link

miminar commented Mar 20, 2018

ack, on it

@miminar miminar mentioned this pull request Mar 20, 2018
Merged
openshift-merge-robot added a commit that referenced this pull request Mar 22, 2018
@openshift-merge-robot
Merge pull request #19037 from miminar/naughty-image-signature-annota…
1453679 
…tion

Automatic merge from submit-queue (batch tested with PRs 18905, 18968, 19016, 19037, 19056).

Drop image signature annotations

Because they were never meant to be allowed.

Includes and superseds #19030 (kudos to @deads2k)
Closes #19011

Resolves: [rhbz#1557607](https://bugzilla.redhat.com/show_bug.cgi?id=1557607)

/assign @bparees @deads2k 
/cc @mfojtik
@openshift-bot
Copy link
Contributor

@deads2k: PR needs rebase.

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.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2018
@deads2k deads2k closed this Mar 23, 2018
@deads2k deads2k deleted the image-01-strategy branch July 3, 2018 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfojtik mfojtik Awaiting requested review from mfojtik

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@deads2k @openshift-ci-robot @liggitt @bparees @miminar @openshift-bot