-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat: Add cluster opt for host client basic auth #182
Conversation
8acaa03
to
1e18e85
Compare
cb6d9e9
to
d00ad76
Compare
**User value:** This commit will let users set the basic auth tokens for each flintlock host. To keep things simple, users will create a secret in the same namespace as their cluster. The secret can be called whatever they like, and they provide the name of this secret in the `MvmCluster.Spec`: ``` spec: placement: staticPool: basicAuthSecret: mybasicsecret hosts: - controlplaneAllowed: true endpoint: 192.168.0.31:9090 ``` The option is under the `placement.staticPool`, since it will only be in the non-dynamic placement that users will know the endpoint address ahead of time. How auth is handled in the dynamic scheduling case is coming soon. The host endpoint is relevant because that is what the controller will use to look up the associated auth token in the secret. Users will provide a map of the auth info in the secret data: ``` apiVersion: v1 kind: Secret metadata: name: mybasicsecret namespace: default type: Opaque data: 192.168.0.31: Zm9v # foo, in this case ``` **Implementation:** - In the secret example, note the absence of the `:` + port which would make the secret invalid. In the implementation, we remove the `:port` from the `endpoint`. To stop doing this, we could split the `endpoint` into `address` and `port`, and then gather those together for the `failureDomain`. - I have refactored the flintlock client factory func to receive a variadic array of opts that we build into a config. This just makes it nicer to add more options to the client, without having to expand the signature. - I have not done any TLS here deliberately to keep the change small. - The client simply adds the token to the request authorization header. **Testing:** - I have manually tested using Tilt + a local flintlock. - Unit testing is interesting, since there are not any outcomes that I can look for, since all the changes are on the client, which we use fakes for anyway. I will continue to think on 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.
Very minor comment about the verbosity level. But thats no essentail at all.
token := string(tokenSecret.Data[host]) | ||
|
||
if token == "" { | ||
m.V(2).Info( //nolint:gomnd // this magic number is fine |
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 actually take off the verbosity of 2.
m.V(2).Info( //nolint:gomnd // this magic number is fine | |
m.Info( |
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.
eh i'll do that in the next one 😈
Part of liquidmetal-dev/flintlock#356
User value:
This commit will let users set the basic auth tokens for each flintlock
host.
To keep things simple, users will create a secret in the same namespace
as their cluster. The secret can be called whatever they like, and they
provide the name of this secret in the
MvmCluster.Spec
:The option is under the
placement.staticPool
, since it will only be inthe non-dynamic placement that users will know the endpoint address
ahead of time. How auth is handled in the dynamic scheduling case is
coming soon.
The host endpoint is relevant because that is what the controller will
use to look up the associated auth token in the secret. Users will
provide a map of the auth info in the secret data:
Implementation:
:
+ port which wouldmake the secret invalid.
In the implementation, we remove the
:port
from theendpoint
. Tostop doing this, we could split the
endpoint
intoaddress
andport
, and then gather those together for thefailureDomain
.variadic array of opts that we build into a config. This just makes it
nicer to add more options to the client, without having to expand the
signature.
Testing:
can look for, since all the changes are on the client, which we use
fakes for anyway. I will continue to think on it.
Checklist: