VC-35630: User can now omit the server field in config when using the Venafi Connection mode
#566
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
For context, EU customers need to change the URL of the VCP API to point to https://api.venafi.eu. To do that, and imagining that they are using the VenafiConnection authentication, they may try to use the `spec.vcp.url` field on their VenafiConnection resource and find that this field doesn’t do anything because the Helm chart's `config.server` is set to https://api.venafi.cloud by default. Another possible scenario is that EU customers may end up with a VenafiConnection configured with the `spec.vcp.url` field set to `https://api.venafi.eu`. This VenafiConnection will have been already working well with venafi-enhanced-issuer and approver-policy-enterprise. Once they try to switch the Agent to the VenafiConnection auth method, they will see that it doesn’t work because the Agent picks up the default value in the Agent’s helm chart, i.e., ``` config: server: https://api.venafi.cloud. ```
I caught this thanks to golangci-lint's ineffassign which showed the warning "ineffectual assignment to err".
golangci-lint had been showing this warning for a while... The actual warning is "impossible condition: nil != nil" from gopls' nilness linter.
maelvls
commented
Aug 30, 2024
| @@ -269,186 +259,3 @@ func run(test testcase) func(t *testing.T) { | |||
| assert.Equal(t, test.expectReadyCondMsg, got.Status.Conditions[0].Message) | |||
| } | |||
| } | |||
|
|
|||
| func fakeVenafiCloud(t *testing.T) (*httptest.Server, *x509.Certificate) { | |||
There was a problem hiding this comment.
I moved this to the testutil.go package.
maelvls
commented
Aug 30, 2024
Comment on lines
-291
to
-293
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
I've removed this because the linter was complaining. Looks like an error checked left over during a refactoring.
maelvls
commented
Aug 30, 2024
pkg/testutil/testutil.go
Outdated
Comment on lines
194
to
200
| var assertFn AssertRequest | ||
| assertFnMu := sync.Mutex{} | ||
| setAssert = func(setAssert AssertRequest) { | ||
| assertFnMu.Lock() | ||
| defer assertFnMu.Unlock() | ||
| assertFn = setAssert | ||
| } |
There was a problem hiding this comment.
I needed to make sure that the request's host was correct, that's why I wrote this clunky code to set an assertion... I'd like to have a clearer way of doing that, or even better: not have to assert the host on the request...
maelvls
force-pushed
the
disable-config-server-field
branch
from
55b08d8 to
d03c705
Compare
wallrj
approved these changes
Sep 3, 2024
There was a problem hiding this comment.
Thanks @maelvls
The changes look good to me and the tests pass on my laptop.
Additionally, I rebased #562 on this branch and re-ran the e2e/test.sh script and observed the new log message and the agent successfully posted the data to the EU API.
$ ./hack/e2e/test.sh
...
+ grep -q 'Data sent successfully'
2024/09/03 08:22:13 Preflight agent version: development ()
2024/09/03 08:22:13 ignoring the server field specified in the config file. In Venafi Connection mode, this field is not needed. Use the VenafiConnection's spec.vcp.url field instead.
2024/09/03 08:22:13 Venafi Connection mode was specified, using Venafi Connection authentication.
2024/09/03 08:22:13 ignoring venafi-cloud.upload_path. In Venafi Connection mode, this field is not needed.
2024/09/03 08:22:13 ignoring venafi-cloud.uploader_id. In Venafi Connection mode, this field is not needed.
2024/09/03 08:22:13 Prometheus was enabled.
Running prometheus server on port :8081
2024/09/03 08:22:58 Posting data to: https://api.venafi.eu/
2024/09/03 08:22:59 Data sent successfully.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Source: VC-35630
Problem
EU customers need to change the URL of the VCP API to point to https://api.venafi.eu/. To do that, and imagining that they are using the VenafiConnection authentication, they may try to use the
spec.vcp.urlfield on their VenafiConnection resource and find that this field doesn’t do anything because the Helm chart'sconfig.serveris set to https://api.venafi.cloud/ by default.Another possible scenario is that EU customers may end up with a VenafiConnection configured with the
spec.vcp.urlfield set tohttps://api.venafi.eu/. This VenafiConnection will have been already working well with venafi-enhanced-issuer and approver-policy-enterprise. Once they try to switch the Agent to the VenafiConnection auth method, they will see that it doesn’t work because the Agent picks up the default value in the Agent’s helm chart, i.e.,Solution
In this PR, I propose to ignore the
serverfield specified in the config file in Venafi Connection mode. By default, the Helm chart setsservertohttps://api.venafi.cloud, which means the user may get confused as to what this field does.To avoid confusion, the logs now show the following message when
serverisn't empty:This way, the user will know that what they need to do in case they see the
serverfield and think "is that what I need to change?".I've also clarified in the Helm chart that
serveris only used for Jetstack Secure OAuth, Jetstack Secure API Token, and Venafi Cloud Keypair authentication.