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

auditor fails to parse valid image references #191

Closed
listx opened this issue Mar 7, 2020 · 6 comments · Fixed by #193 or #199
Closed

auditor fails to parse valid image references #191

listx opened this issue Mar 7, 2020 · 6 comments · Fixed by #193 or #199
Assignees
Labels
priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.

Comments

@listx
Copy link
Contributor

listx commented Mar 7, 2020

During the backfill kubernetes/k8s.io#632, a buggy behavior was observed where the auditor could not parse certain image references.

The following images all triggered a TRANSACTION REJECTED: could not determine source registry: error:

gcr.io/k8s-artifacts-prod/cluster-api-aws/cluster-api-aws-controller@sha256:050848d5a92c8c794af31726b982e8c29aff67e6043e9861e6c71d22d282aa6d
gcr.io/k8s-artifacts-prod/cluster-api-aws/cluster-api-aws-controller@sha256:465842186bfa79cf037163d2c632f544bf8ef0f42065351e6fef7fca7ae58fde
gcr.io/k8s-artifacts-prod/cluster-api-aws/cluster-api-aws-controller@sha256:6600f3e68b8139babd91d52339103c2f9cf0f61d87abcfb08140059047a05f50
gcr.io/k8s-artifacts-prod/cluster-api-aws/cluster-api-aws-controller@sha256:cdf2581188c5de863f6e3bf3180c4d537e5bac25998ceb17f38076d7a28d08ad
gcr.io/k8s-artifacts-prod/cluster-api-aws/cluster-api-aws-controller@sha256:dc5d27778cb5e341d7703001e6009c34ddd89ddcdcf63efaf8edeef22b2aa8ee
gcr.io/k8s-artifacts-prod/e2e-test-images/agnhost@sha256:1dfec5637a7010d6c0955c26f0a752266fa2646ed2bf8e6ad745cdcfcb611db8
gcr.io/k8s-artifacts-prod/e2e-test-images/agnhost@sha256:435e92e8fa45c3bfadb2c12374ea5bea2173d1afbeff572cdca4ffee115a1be7
gcr.io/k8s-artifacts-prod/e2e-test-images/agnhost@sha256:bfd170a7683c58cb23c08a8ed3516fcfaa3180503be43a2be9a9deb0e1dfb0f2
gcr.io/k8s-artifacts-prod/e2e-test-images/agnhost@sha256:d3c06db4ef3ac9f309819e52a401ce2393aef76d2e9657e60daab9027c281855
gcr.io/k8s-artifacts-prod/e2e-test-images/agnhost@sha256:d9d5a169f27bac0c58cce541eeef3db7c2ddd70eca09b48da484a04564a6b1f9
gcr.io/k8s-artifacts-prod/kas-network-proxy/proxy-agent@sha256:43273b274ee48f7fd7fc09bc82e7e75ddc596ca219fd9b522b1701bebec6ceff
gcr.io/k8s-artifacts-prod/kas-network-proxy/proxy-agent@sha256:7bcbdf4cb26400ac576b33718000f0b630290dcf6380be3f60e33e5ba0461d31
gcr.io/k8s-artifacts-prod/kas-network-proxy/proxy-agent@sha256:8735603bbd7153b8bfc8d2460481282bb44e2e830e5b237738e5c3e2a58c8f45
gcr.io/k8s-artifacts-prod/kas-network-proxy/proxy-agent@sha256:99bade313218f3e6e63fdeb87bcddbf3a134aaa9e45e633be5ee5e60ddaac667
gcr.io/k8s-artifacts-prod/kas-network-proxy/proxy-agent@sha256:c1ccf44d6b6fe49fc8506f7571f4a988ad69eb00c7747cd2b307b5e5b125a1f1
gcr.io/k8s-artifacts-prod/kas-network-proxy/proxy-server@sha256:101863ca4affcb2121b785ab34f87321a3f954dd2146ae7069eea87ce1bd7059
gcr.io/k8s-artifacts-prod/kas-network-proxy/proxy-server@sha256:504a5296bbe2d84b22bd15be13cc9cc0ef1c9f1f841ad1965bcf544d4f8246a0
gcr.io/k8s-artifacts-prod/kas-network-proxy/proxy-server@sha256:aa225a770fb144d26c341b0918cd35ca402658135b850f0857b88b4925e17426
gcr.io/k8s-artifacts-prod/kas-network-proxy/proxy-server@sha256:cecee42a9997c0ea06dfcaddfc6a940e4faf52ca31f8dc44f4e7e454c3c8d957
gcr.io/k8s-artifacts-prod/kas-network-proxy/proxy-server@sha256:e65695c53ad76b0b7c24e6df6954afbc6d7454197215149b29670af30304f26c

correction: the images are located in the {asia,eu,us} regions --- but they have been removed from the above paste for simplicity

@listx
Copy link
Contributor Author

listx commented Mar 9, 2020

So the right way to fix this is to inject some much-needed unit-tests for the Audit() function. The problem with the existing implementation is that there is no way to test it without actually going over the network (to log things, etc). I'll need to break it up and add some unit tests along the way.

@listx
Copy link
Contributor Author

listx commented Mar 10, 2020

/reopen

This isn't fixed yet.

@k8s-ci-robot k8s-ci-robot reopened this Mar 10, 2020
@k8s-ci-robot
Copy link
Contributor

@listx: Reopened this issue.

In response to this:

/reopen

This isn't fixed yet.

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.

@listx listx self-assigned this Mar 10, 2020
@listx listx added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Mar 10, 2020
@bartsmykla
Copy link

Hi @listx. I see you are working on fixing the issue, but could you maybe give few words of summary why parsing this image references was failing?

@listx
Copy link
Contributor Author

listx commented Mar 12, 2020

Hi @listx. I see you are working on fixing the issue, but could you maybe give few words of summary why parsing this image references was failing?

The parsing logic was too limited (very brittle!) and didn't cover enough cases. #199 makes the parsing more robust, mainly by empowering the Contains() method to better gauge whether a given promoter manifest is responsible for the child manifest digest in the Pub/Sub payload.

@listx
Copy link
Contributor Author

listx commented Mar 14, 2020

I created #200 to add more tests, including an e2e test. I then made #201 to just make sure I could reproduce the behavior discussed in this bug (which I did), so #199 and #200 are ripe for merging. See #201 (comment) for more info.

After we get those changes merged, I think I will create 1 more PR to fix #183. Then it should be ready for being deployed into prod.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now.
Projects
None yet
3 participants