search

jetstack / jetstack-secure

Open source components of Jetstack Secure
https://www.jetstack.io/jetstack-secure/
Apache License 2.0
252 stars 24 forks source link

feat(onboarding): Add onboarding values to the helm chart #510

Closed tfadeyi closed 5 months ago

tfadeyi commented 5 months ago

Adds cluster onboarding values to the agent helm chart.

Change:

Note: Wait until v0.1.44 agent is released before merging

hawksight commented 5 months ago

@tfadeyi I had a couple of q's for context:

tfadeyi commented 5 months ago

@tfadeyi I had a couple of q's for context:

* What is the significance of "onboarding"?

We are adding these fields in order to allow users to configure the necessary fields in order to connect/onboard a cluster to VCP without going through UI or CLI. https://venafi.atlassian.net/browse/VC-22924

image

Adding those fields will make the backend create a new cluster connection element in VCP.

* Could these keys just be under `config.clusterName` & `config.clusterDescription`?

That's true the could be, just thought it would make it more clear that these fields would be used to onboard a cluster to VCP.

* Are the two fields `clusterName` & `clusterDescription` optional?

Yeah these fields are optional

Hopefully this https://venafi.atlassian.net/browse/VC-22924 will give more context on the problem

achuchev commented 5 months ago

@hawksight All good points. Let me know if you want to understand more about the proposed change.

@tfadeyi I agree with Peter that the new options have to be under config.

hawksight commented 5 months ago

@tfadeyi thanks for the explanation. What would happen if somewhere were to change the clusterName and clusterDescription fields? They would just change on the VCP?

I was thinking that if they are a "set once and never change" type field, then it makes sense to consolidate them under their own heading onboarding. But if they are just optional and can be changed on an ongoing basis, then I think it makes more sense to have them sit alongside the other configurable variables:

helm show values oci://$VENAFI_REGISTRY_PRIVATE_US/charts/venafi-kubernetes-agent | yq .
config:
  server: "https://api.venafi.cloud/"
  clientId: "XXXXXXXXXXXXXXX"
  clusterName: "black-pearl"
  clusterDescription: "The fastest cluster on the seven seas"

It also feels a little more natural and consistent with the jetstack-agent config.

Also thinking about the user install experience, it just makes the command a tiny bit shorter:

# This
helm diff upgrade --install venafi-kubernetes-agent oci://$VENAFI_REGISTRY_PRIVATE_US/charts/venafi-kubernetes-agent --namespace $VENAFI_NAMESPACE --create-namespace --set config.clientId="$VENAFI_CLIENT_ID" --set config.clusterName="abcdefg" --set config.clusterDescription="Alphabet" --version "0.1.43" --registry-config "${VENAFI_CREDS_LOCAL_PATH}/${VENAFI_CRED_NAME}/config.json"
# As opposed to this:
helm diff upgrade --install venafi-kubernetes-agent oci://$VENAFI_REGISTRY_PRIVATE_US/charts/venafi-kubernetes-agent --namespace $VENAFI_NAMESPACE --create-namespace --set config.clientId="$VENAFI_CLIENT_ID" --set config.onboarding.clusterName="abcdefg" --set config.onboarding.clusterDescription="Alphabet" --version "0.1.43" --registry-config "${VENAFI_CREDS_LOCAL_PATH}/${VENAFI_CRED_NAME}/config.json"

Other note

Perhaps we should remove the chart bump from here and here and do that explicitly as it's own commit before creating a tag / release on this repo?

achuchev commented 5 months ago

@hawksight: The cluster name and description will be preserved in VCP even if the agent reports new values. This ensures consistency and avoids unintended updates.

tfadeyi commented 5 months ago

Perhaps we should remove the chart bump from https://github.com/jetstack/jetstack-secure/pull/502 and here and do that explicitly as it's own commit before creating a tag / release on this repo?

I've already made a new release, didn't see the message in time. I'll make a new release