Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Insights Operator pulling and exposing data from the OCM API #683
Insights Operator pulling and exposing data from the OCM API #683
Changes from 12 commits
b0bddb0
156811e
520dfca
f380524
9072876
a9353f5
8e38be8
31ca41f
0113815
83eb79d
0f921cd
20a7617
0b42cc6
5eeb9ce
15ef7c0
141a6ea
5e84453
6089e73
bcebd4c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
who put that secret there? it is managed, so some operator has to sync the secret there :-)
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.
A good question - is this something that the installer does, or does something like the CVO sync the pull secret in
openshift-config
toopenshift-config-managed
?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.
looking at the code, it looks like it's just using the pull-secret from openshift-config directly, not openshift-config-managed.
so the expectation is that secret is maintained by someone (if it expires, your nodes will have issues pulling images), so i don't think it's the responsibility of this EP to worry about it, other than that this EP should address what happens if it can't talk to OCM because the token expires(or for any other reason), which i think the EP does already.
see: https://github.com/tremes/insights-operator/blob/154641a9fae7c5100f7dce3cf0eddf4b38d56cea/pkg/config/configobserver/configobserver.go#L78
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 guess the question was why the new
etc-pki-entitlement
secret is created in theopenshift-config-managed
namespace (rather thanopenshift-config
?).This requires a new
Role
&RoleBinding
definition to be able to create the secret in theopenshift-config-managed
namespace. Is that OK/acceptable? Comparing to extending our existing role to be able to create the secret in theopenshift-config
namespace.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.
Configuration in
openshift-config
are usually added manually by admins compared, given thatetc-pki-entitlement
is managed by an OpenShift component, Adam and I suggested it be managed in theopenshift-config-managed
namespace.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.
@mfojtik Hi, are you Ok if we resolve this conversation based on the comments above?
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'd add admission control for CSI volumes as a GA graduation criteria
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.
why?
Consumption of the content seems outside the scope of this EP. The GA version of this EP is a component that can pull the cert down and put it in a well-defined location that we are happy with, reliably+securely, that is debuggable, provides useful status info, etc.
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.
Can we resolve this point @bparees @adambkaplan ?