-
Notifications
You must be signed in to change notification settings - Fork 392
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
OCPNODE-1632: Implement ImagePolicy and ClusterImagePolicy #3786
base: master
Are you sure you want to change the base?
Conversation
Skipping CI for Draft Pull Request. |
3663151
to
0af1660
Compare
9c5e936
to
324a6eb
Compare
324a6eb
to
9b0b0fe
Compare
ca46c02
to
bf82584
Compare
a154f35
to
4499485
Compare
1eeed3e
to
856ce2a
Compare
e2b3e60
to
68249ff
Compare
80eb398
to
2a00f2a
Compare
@QiWang19: This pull request references OCPNODE-1632 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
2e6f3d0
to
17ed01a
Compare
ready for review. |
/retest |
@QiWang19: This pull request references OCPNODE-1632 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
return namespaceJSONs, nil | ||
} | ||
|
||
func imagePolicyConfigFileList(namespaceJSONs map[string][]byte, sigstoreRegistriesConfigYaml []byte) []generatedConfigFile { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CRI-O has to this point no ability to correctly implement per-namespace policies (that has stalled in cri-o/cri-o#7046 , and my further work on that is tracked in https://issues.redhat.com/browse/RUN-1980 ), so isn’t this rather premature?
Of course the code proposed in this PR could be reviewed concurrently, but I don’t think it makes sense to merge it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternatively, it might make sense to add the feature that adds a CR to configure the cluster-wide-global /etc/container/policy.json
, right now — if that can be reasonably isolated from the other parts of the approved enhancement. It’s just the per-namespace policies that are not currently possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtrmac thank you for the clarification! Do you have plans to continue the CRI-O work (RUN-1980) in the near future?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m definitely interested seeing this through to completion, after all the effort, but I don’t think I can spare the time in the next ~month at least.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note https://issues.redhat.com/browse/OTA-1170 ; AFAICS that would benefit from MCO implementing ClusterImagePolicy
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that sounds like a plan. If we can deliver the namespaced policies in CRI-O v1.30 (late April) then we can pull those into the MCO implementation afterwards.
Focusing on the cluster policy sounds totally reasonable for now. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mtrmac @saschagrunert PR #4160 implementing ClusterImagePolicy is ready for review.
- get | ||
- list | ||
- watch | ||
- update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that that (Cluster)ImagePolicy -> MachineConfig pipe needs the read-access verbs. But I'm not clear on why this system:openshift:machine-config-operator:cluster-reader
role would need update
write access. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will clean this up.
Signed-off-by: Qi Wang <qiwan@redhat.com>
1cb83a4
to
f3f5618
Compare
Signed-off-by: Qi Wang <qiwan@redhat.com>
f3f5618
to
a041bc8
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: QiWang19 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/retest |
/test e2e-gcp-op-techpreview |
@QiWang19: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
/test e2e-gcp-op-techpreview |
Issues go stale after 90d of inactivity. Mark the issue as fresh by commenting If this issue is safe to close now please do so with /lifecycle stale |
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-sigs/prow repository. |
- What I did
Follow the design from the enhancement. Add the implementation to ContainerruntimConfig controller for
policy.json
configuration.- How to verify it
- Description for the changelog