-
Notifications
You must be signed in to change notification settings - Fork 24
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
NETOBSERV-489: Use authorize feature from console plugin API #175
NETOBSERV-489: Use authorize feature from console plugin API #175
Conversation
fcda917
to
6eb5ff1
Compare
api/v1alpha1/flowcollector_types.go
Outdated
//+kubebuilder:default:=false | ||
// sendAuthToken is a flag to enable or disable forwarind auth token | ||
// if enabled, this overide loki.sendAuthToken | ||
ForwardUserAuthToken bool `json:"forwardUserAuthToken,omitempty"` |
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.
Following the API recommendations, discouraging the use of bools, maybe I'd move this to a string property. Something like: userAuthToken: {BLOCK | FORWARD}
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.
maybe we should merge with FlowCollectorLoki.SendAuthToken
to a single enum then ? 🤔
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 changed the value to an enum.
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.
maybe we should merge with
FlowCollectorLoki.SendAuthToken
to a single enum then ? thinking
FlowCollectorLoki.SendAuthToken
apply to both FLP and Console plugin while this new value only apply to the Console Plugin.
Also FlowCollectorLoki.SendAuthToken
is still required for FLP when the user token forwarding is enabled.
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.
Yes. My thinking was about having FlowCollectorLoki.SendAuthToken
as:
enum:
- IGNORE
- SERVICE_ACCOUNTS
- MIXED
type: string
With proper documentation to explain MIXED
behavior
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.
Done
/ok-to-test |
New image: ["quay.io/netobserv/network-observability-operator:15353db"]. It will expire after two weeks. |
ce99aa7
to
a4485cb
Compare
/ok-to-test |
New image: ["quay.io/netobserv/network-observability-operator:df99708"]. It will expire after two weeks. |
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. I haven't tested it.
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: OlivierCazade 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 |
This PR enable the new option from the console from this PR:
netobserv/network-observability-console-plugin#198