-
Notifications
You must be signed in to change notification settings - Fork 25
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
[VC-28877] Publish venafi kubernetes agent chart #471
[VC-28877] Publish venafi kubernetes agent chart #471
Conversation
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
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.
Really good changes, thank you for refactoring this to include the right registry links etc.
Also repo instruction are good 👍
Couple of minor things and slight rant, but nothing to stop this merging.
|
||
### 1) Setup registry credentials | ||
> Learn [how to access the private Venafi OCI registries](https://docs.venafi.cloud/vaas/k8s-components/th-guide-confg-access-to-tlspk-enterprise-components/). |
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.
This is completely the right link, but what it links to is completely useless IMO.
Why would you have to enable this by talking to a human. Shouldn't this just be a feature enabled by default?
Surely being a Venafi Customer is enough to get access to a private revision of the venafi-kubernetes-agent
?
No action needed, just ranting.
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 raised an issue with the documentation team:
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.
This was added to the docs by PM in advance of the preview. The page will updated for the GA.
--namespace ${VENAFI_NAMESPACE} \ | ||
--set config.clientId="${VENAFI_CLIENT_ID}" | ||
``` | ||
|
||
Optionally if you need to change the backend to the EU Venafi Control Plane you can use: | ||
> To change the backend to the EU Venafi Control Plane, use the following Helm value: | ||
> `--set config.server="${VENAFI_SERVER_URL}"` |
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.
We haven't exported this variable previously, so maybe we include it, or just add this into the instructions if this might change often? Not sure what's best, but also happy to leave how it is and assume people know who to export the env var and what it should be exported 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.
I've hard coded the EU URL here instead.
|
||
1) A registry credential to allow helm to pull the chart from our private OCI registry. | ||
2) A service account key pair used by the agent to authenticate to the Venafi Control Plane. | ||
- `oci://registry.venafi.cloud/charts/venafi-kubernetes-agent` (public) |
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.
Is it worth a quick note to say that for this readme, we assume you are using the public registry?
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.
It starts with the following manual steps: | ||
|
||
1. Choose the next semver version number. | ||
This project has only ever incremented the "patch" number (never the "minor" number) regardless of the scope of the changes. |
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.
Seems a bit counter intuitive to the preceding sentence, but is entirely accurate.
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 considered just changing the version number to 1.0.0 and telling releasers to increment the minor version,
but decided that would require a wider discussion.
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, no need for action, I'm just arguing semantics 😂
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
Signed-off-by: Richard Wall <[email protected]>
@@ -7,7 +7,7 @@ replicaCount: 1 | |||
|
|||
image: | |||
# -- Default to Open Source image repository | |||
repository: quay.io/jetstack/preflight | |||
repository: quay.io/jetstack/venafi-agent |
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.
This is a more appropriate image, but later we should set up mirroring to registry.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.
Thanks you for constructively taking on board my comments. Changes are good 👍
It starts with the following manual steps: | ||
|
||
1. Choose the next semver version number. | ||
This project has only ever incremented the "patch" number (never the "minor" number) regardless of the scope of the changes. |
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, no need for action, I'm just arguing semantics 😂
Fixes: https://venafi.atlassian.net/browse/VC-28877
@dbarranco made some changes to Harbour to mirror the chart to the documented Venafi registries.
You can examine the Helm chart yourself, as follows:
Testing
I installed the chart in a Kind cluster on my laptop and was able to see the agent running and connecting to the Venafi Control Plane.