-
Notifications
You must be signed in to change notification settings - Fork 25
VC-41203: Allow users to select the Machine Hub mode #653
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
Conversation
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 good! I don't think we can merge with the flag as-is because I think it should be hidden or have a disclaimer (or both).
This looks great!
pkg/agent/config.go
Outdated
c.PersistentFlags().BoolVar( | ||
&cfg.MachineHubMode, | ||
"machine-hub", | ||
false, | ||
"Enables MachineHub mode. The agent will push data to CyberArk MachineHub. Can be used in conjunction with --venafi-cloud.", | ||
) |
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.
suggestion: I agree that this is probably not required in the long term, we can just enable it if configuration is provided.
In the short term, I think this feature will be rolled out slowly and we'll want it to be there for testing before it's ready for production.
With that in mind, I think it makes sense to have this flag, but hidden by default or else with a description that indicates the feature is for testing only. What do you think?
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 like the idea of hiding it so that we can roll this out without waiting. I'll do that.
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. Here is how I describe --machine-hub
:
This is a hidden feature flag we use to build the "Machine Hub" feature gradually without impacting customers. Once the feature is GA, we will turn this flag "on" by default.
// CredentialsSecretName is the name of a Kubernetes Secret in the same | ||
// namespace as the agent, which will be watched for a username and password | ||
// to send to CyberArk Identity for authentication. | ||
CredentialsSecretName string `yaml:"credentialsSecretName"` |
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.
suggestion: do you think we could include the secret watcher in this PR, or should that be left for a separate PR? It's definitely something we'll need!
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'll add the watcher to the next PR 😅
@maelvls what's the latest on this? I think once tests are passing we can probably get merged, right? |
Hey, I keep forgetting about this PR. It looks ready to be merged, all feedback seems to have been addressed. I'll resolve the conflicts and hope that I don't get 429s during the go-licenses check 😅 |
4cd49ed
to
82295ef
Compare
Co-authored-by: Ashley Davis <[email protected]>
82295ef
to
129b53d
Compare
I rebased, it should be good to go. I hope I didn't make any mistake when migrating to |
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
Seems like a great base to build upon, thank you 😁
Ref: VC-41203
Still to be done:
Context
Up to now, Venafi Kubernetes Agent (this project) was already able to push to two different backends (Jetstack Secure, and Venafi Cloud). We want Machine Hub to act as another backend ("control plane").
Unlike Jetstack Secure and Venafi Cloud that were exclusive to each other, it was decided that the Venafi Cloud backend could be used alongside the Machine Hub backend. The intention is (probably) to make the transition painless.
What am I adding?
This PR originates from Ashley's prototype (#652); I've added a new flag (
--machine-hub
) as a temporary hidden feature flag just to make sure we don't change the existing behavior unexpectedly. Later on, the way people will use the feature will be to just configure themachineHub
field on config.yaml.Just to clarify: the Machine Hub integration won't be working yet after this PR. It's just an empty shell for now. This PR is just about adding a way to turn on the feature, but doesn't implement the feature.
Things to consider
Why
--machine-hub
when everything must be configured in the config file?You might have noticed that I could have just used the presence of the
machineHub
field in the configuration to turn on the feature, e.g.:Since we want to merge things to master, we need a way to turn on/off this feature. So we've decided to have a hidden flag,
--machine-hub
, that gates the feature.Later on, once the feature is ready, we will turn on this flag by default (it will thus become useless).
Testing
I've added two new unit tests to check that the configuration logic works.
Here is the manual testing I've done:
Case 1: Machine Hub only
Case 2: Machine Hub + Venafi Cloud Key Pair Auth mode
For this test, I've used the tenant https://ven-tlspk.venafi.cloud/. To access the API key, use the user
[email protected]
and the password is visible in the page Production Accounts (private to Venafi). Then go to the settings and find the API key, and set it in the env varAPIKEY
.I got:
That's seemingly because https://ven-tlspk.venafi.cloud/ no longer has the feature flat enabled; from Datadog: