-
Notifications
You must be signed in to change notification settings - Fork 466
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
cluster-logging: Add LokiStack tokenized auth proposal #1503
cluster-logging: Add LokiStack tokenized auth proposal #1503
Conversation
e6bae1d
to
54e0b5b
Compare
54e0b5b
to
f396132
Compare
The EP looks really good -- it follows the referenced workflow with CCO and it provides practical implementation details such as YAML configurations and environment variables which should make it a valuable resource for practical implementation for other operator authors following this path to support tokenized auth on all three major cloud providers. |
Inactive enhancement proposals go stale after 28d of inactivity. See https:/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale |
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.
Just some minor fixes.
My only big question about this is the fact that we require customers to pre-provision IAM resources, which IIUC, render CCO completely redundant as the Loki Operator with the RoleARN and some extra config from the storage secret would be able to make STS/WIF workflow work by itself.
I say this because from my POV if an operator creates a CredentialRequest
, this resource has all the necessary information for creating the IAM resources we are currently asking to be pre-provisioned. So currently our proposal, to me, feels like we are supporting STS for STS clusters in Manual mode and the other Modes I think it would also work but creating the CredentialRequest
would only make CCO create extra IAM resources that wouldn't be used.
Maybe I'm misunderstanding something but I wanted to bring it up
value: /var/run/secrets/openshift/serviceaccount/token | ||
``` | ||
|
||
__Note:__ The Azure Region value is populated from the azure object storage secret referenced by the `lokistack.spec.storage.secret.name`. |
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 think we can remove this, this seems to be from one of the first iterations of this document, region seems to also be in the secret created by CCO so we can use that.
To frame this a little bit more, we are not implementing all CCO modes of operation except the short-lived tokens mode. This is the only one supported on all big three providers for the platform's operators. In addition the team behind CCO adds enablements (OCP Console OLM UI + CredentialsRequest CR additions) which we will gradually make use upon availability (i.e. AWS since 4.14 and Azure in 4.15). In fact in
data:
bucketnames: loki-bucket
roleARN: the-full-role-arn-as-created-manually-by-the-cloud-admin
data:
bucketnames: loki-bucket The reason not asking for a In summary one needs to zoom out a bit and look on the CCO enablement from the UI to the operator lifecycle and lastly on the exchange of a "Your don't need to know where you get your credentials for Loki to access S3. If your Cloud Admin created a role for you, Loki-Operator and CCO will make things travel for you." |
Yeah this is the context I was missing/forgetting thanks a lot for the explanation |
Inactive enhancement proposals go stale after 28d of inactivity. See https:/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale |
Inactive enhancement proposals go stale after 28d of inactivity. See https:/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
/remove-lifecycle stale |
#1555 is changing the enhancement template in a way that will cause the header check in the linter job to fail for existing PRs. If this PR is merged within the development period for 4.16 you may override the linter if the only failures are caused by issues with the headers (please make sure the markdown formatting is correct). If this PR is not merged before 4.16 development closes, please update the enhancement to conform to the new template. |
fa09df2
to
072184f
Compare
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.
Updated to reflect the current implementation state of the Loki Operator.
- name: TENANT_ID | ||
value: "<azure tenant id>" | ||
- name: SUBSCRIPTION_ID | ||
value: "<azure subscription id>" |
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.
Should we mention the optional REGION
variable here?
038b14c
to
b68d421
Compare
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.
lgtm
/approve |
b68d421
to
0ae27fa
Compare
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.
after this from my side, it's lgtm
77bce1e
to
dfe01fb
Compare
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.
more small fixes I noticed when giving it a final review
4e5ac54
to
689f16f
Compare
@periklis: all tests passed! 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. |
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.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoaoBraveCoding, periklis 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 |
Based upon #1339 the following enhancement proposes the required changes in operating Loki Operator in STS-enabled clusters incl. HCP/ROSA.
Pre-requisites in upstream:
cc @xperimental @cahartma @jcantrill @alanconway @JoaoBraveCoding @btaani