[disconnected] Add option to perform image signature verification - #4141
[disconnected] Add option to perform image signature verification#4141drosenfe wants to merge 1 commit into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 53m 03s |
cc9445e to
3372477
Compare
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 1h 59m 18s |
|
recheck |
3372477 to
5136acc
Compare
5136acc to
e1740a1
Compare
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 46m 57s |
e1740a1 to
565cc7d
Compare
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 3h 10m 08s |
565cc7d to
baf984e
Compare
|
Build failed (check pipeline). Post ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 41m 01s |
baf984e to
99839c9
Compare
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
rabi
left a comment
rabi
left a comment
There was a problem hiding this comment.
A question on the default otherwise good for me.
| mirror_location: "{{ disconnect_working_dir }}/mirror_location" | ||
| local_registry: "{{ disconnect_working_dir }}/local_registry" | ||
| oc_mirror_cert_manager_catalog_url: "{{ cifmw_cert_manager_catalog_url | default('registry.redhat.io/redhat/redhat-operator-index:v4.18') }}" | ||
| verify_image_signatures: "{{ cifmw_disconnected_verify_image_signatures | default(false) }}" |
There was a problem hiding this comment.
Image signature verification is true by default downstream right? Otherwise one has to use insecureAcceptAnything: true in /etc/containers/policy.json for default/registry.
There was a problem hiding this comment.
Image signature verification is not true by default downstream. This is from /etc/containers/policy.json:
"default": [ { "type": "insecureAcceptAnything" } ]
What I've found is that the test operator that runs tempest tests doesn't support image signature verification. I left the default at false to be consistent with current downstream jobs.
| delay: 30 | ||
|
|
||
| - name: Generate ClusterImagePolicies from mirror data | ||
| cifmw.general.generate_cluster_image_policies: |
| output_file = module.params.get("output_file") | ||
|
|
||
| # Red Hat public key: security.access.redhat.com/data/63405576.txt (between BEGIN and END, passed through | base64 -w0) | ||
| sigstore_key = """LS0tLS1CRUdJTiBQVUJMSUMgS0VZLS0tLS0KTUlJQ0lqQU5CZ2txaGtpRzl3MEJBUUVGQUFPQ0FnOEFNSUlDQ2dLQ0FnRUEwQVN5dUgyVExXdkJVcVBIWjRJcAo3NWc3RW5jQmtnUUhkSm5qenhBVzVLUVRNaC9zaUJvQi9Cb1NydGlQTXduQ2hiVENuUU9JUWVadURpRm5odUo3Ck0vRDNiN0pvWDBtMTIzTmNDU242N21BZGpCYTZCZzZrdWtaZ0NQNFpVWmVFU2FqV1gvRWp5bEZjUkZPWFc1N3AKUkRDRU40MkovallsVnF0K2c5K0dya2VyOFN6ODZIM2wwdGJxT2RqYnovVnhIWWh3RjBjdFVNSHN5VlJEcTJRUAp0cXpOWGxtbE1oUy9Qb0ZyNlI0dS83SENuL0srTGVnY08yZkFGT2I0MEt2S1NLS1ZENmxld1VaRXJob3AxQ2dKClhqRHRHbW1POWRHTUY3MW1mNkhFZmFLU2R5K0VFNmlTRjJBMlZ2OVFoQmF3TWlxMmtPekVpTGc0bkFkSlQ4d2cKWnJNQW1QQ3FHSXNYTkdaNC9RK1lUd3dsY2UzZ2xxYjVMOXRmTm96RWRTUjlOODVERVNmUUxRRWRZM0NhbHdLTQpCVDFPRWhFWDF3SFJDVTRkck1PZWo2Qk5XMFZ0c2NHdEhtQ3JzNzRqUGV6aHdOVDh5cGt5UytUMHpUNFRzeTZmClZYa0o4WVNIeWVuU3pNQjJPcDJidnNFM2dyWStzNzRXaEc5VUlBNkRCeGNUaWUxNU5Tekt3Znphb05XT0RjTEYKcDdCWThhYUhFMk1xRnhZRlgrSWJqcGtRUmZhZVFRc291REZkQ2tYRUZWZlBwYkQyZGs2RmxlYU1UUHV5eHRJVApnalZFdEdRSzJxR0NGR2lRSEZkNGhmVitlQ0E2M0pybzF6MHpvQk01QmJJSVEzK2VWRnd0M0FsWnA1VVZ3cjZkCnNlY3FraS95cm12M1kwZHFaOVZPbjNVQ0F3RUFBUT09Ci0tLS0tRU5EIFBVQkxJQyBLRVktLS0tLQ==""" |
There was a problem hiding this comment.
(non-blocking) suggestion: Let's move this below NO_POLICIES in upper case as a constant.
There was a problem hiding this comment.
Moved and capitalized in latest version.
| for mirror, source in unique_pairs: | ||
| name = mirror.split("/")[-1].replace(".", "-").replace(":", "-") | ||
| policy = { | ||
| "apiVersion": "config.openshift.io/v1", |
There was a problem hiding this comment.
(non-blocking) question: DO we need to check openshift deployed version? I think in > 4.18 this is correct. If not this is not applied and we're silently not running this.
We can comment this that requires specific ocp version to run, or retrieving the info by "oc explain clusterimagepolicy --recursive | head -1" to parametrice this line, or maybe just not running the python code at all.
There was a problem hiding this comment.
I added that openshift 4.19 or later is required to the Documentation description.
| "---\n" + "\n---\n".join(yaml.dump(p, sort_keys=False) for p in policies) | ||
| ) | ||
|
|
||
| print(f"Generated {len(policies)} ClusterImagePolicy objects in {output_file}") |
There was a problem hiding this comment.
(blocking) let's move this to module.log or put in a result field. This might bring runtime error of not able to parse JSON object.
There was a problem hiding this comment.
Changed to use logging module in latest version.
| cifmw.general.generate_cluster_image_policies: | ||
| input_dir: "{{ mirror_location }}/working-dir/cluster-resources" | ||
| output_file: "{{ disconnect_working_dir }}/ClusterImagePolicies.yaml" | ||
| """ |
There was a problem hiding this comment.
(non-blocking) suggestion: Let's add the RETURN explanation.
There was a problem hiding this comment.
RETURN explanation has been added.
| # Collect mirror-source pairs | ||
| pairs = [] | ||
|
|
||
| for fname in os.listdir(input_dir): |
There was a problem hiding this comment.
(blocking) suggestion: let's check this exists first so we can fail_json in case of not existing.
There was a problem hiding this comment.
Checking for existence of input_dir has been added.
| else "imageTagMirrors" | ||
| ) | ||
| for entry in doc["spec"].get(key, []): | ||
| source = entry["source"] |
There was a problem hiding this comment.
(non-blocking) suggestion: I'd check if both keys exists before trying to access them.
There was a problem hiding this comment.
Check for existence of source and mirrors has been added.
|
|
||
| result = { | ||
| "success": False, | ||
| "changed": False, |
There was a problem hiding this comment.
(blocking) suggestion: We need to change this as True somewhere. Might be good L134?
There was a problem hiding this comment.
Setting changed to True has been added to suggested line.
| type: str | ||
| output_file: | ||
| description: | ||
| - Absolute path to directory when ClusterImagePolicy file is created |
There was a problem hiding this comment.
(blocking) typo: I think this should be "to file" instead "to directory"
There was a problem hiding this comment.
Change to file instead of directory has been made.
|
|
||
| print(f"Generated {len(policies)} ClusterImagePolicy objects in {output_file}") | ||
|
|
||
| # Ensure some cluster image policies were created |
There was a problem hiding this comment.
(blocking) suggestion: let's move this to L 121, so we don't create a file the header.
There was a problem hiding this comment.
Suggested move has been made.
|
(blocking) suggestion: Also I think this new python code suitable to have some tests located at: tests/unit/modules/test_generate_cluster_image_policies.py. |
Add option to the existing configure openshift cluster for disconnected deployment hook to perform image signature verification. This may be used when oc mirror v2 has mirrored both container images and their cryptographic signatures. jira: https://redhat.atlassian.net/browse/OSPRH-35167 Signed-off-by: David Rosenfeld drosenfe@redhat.com
99839c9 to
4c9218a
Compare
|
Build succeeded (check pipeline). ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 19m 06s |
Not sure how to make molecule tests. Can you re-review the rest while I try to figure out molecule tests? |
3 participants
Add option to the existing configure openshift cluster for disconnected deployment hook to perform image signature verification. This may be used when oc mirror v2 has mirrored both container images and their cryptographic signatures.
jira: https://redhat.atlassian.net/browse/OSPRH-35167
Signed-off-by: David Rosenfeld drosenfe@redhat.com