Extract env var value read into a util #160

orenbm · 2020-09-10T09:01:07Z

We would like to better UT the logic of reading a value from the env
and defaulting to a default value if it's not present. We also have the
logic that transforms this env var value into an int or a duration (and
maybe more in the future) and it is best that this logic is UTed and is
implemented in its own util. This improves the readability of the
config.go file and of the util functions themselves.

This will also let us use these utils in other projects that consume the
authn-client without implementing them there as well.

pkg/authenticator/config/config.go

sigalsax · 2020-09-10T09:32:02Z

pkg/utils/environment_variable.go

+	}
+
+	envVarValue := envVarValueOrDefault(envVarName, defaultValue)
+	envVarValueDuration, err := time.ParseDuration(envVarValue)


why is this needed when you can just get it via 6*time.seconds?

but what if the value is from the env and not the default value? in that case it will be a string.

that's why i think it's more simple to have them both as strings. this will be the case for every env var regardless to the expected type. The value is always a string and the logic that converts it to the required type is in the util. it will create a uniformity throughout configurable values in this repo and in our other k8s projects.

the 6 would be from the env and timeseconds would be from the code

i prefer not to do that

yep, this won't work in the current product. i get a ERROR: 2020/09/10 11:56:22 config.go:128: CAKC010E Failed to parse CONJUR_TOKEN_TIMEOUT. Reason: time: missing unit in duration 6 error.

@izgeri when this env var was created how did you intend to set it? with 6m0s? do you know who defined this?
do you see a problem with changing this to an int input so it will be CONJUR_TOKEN_TIMEOUT="6" instead of CONJUR_TOKEN_TIMEOUT="6m0s"?

once we get to a decision i will update the README with the valid value pattern.

aha, i can see that this env var was used before as:

- name: CONJUR_TOKEN_TIMEOUT + value: "15s"

(saw it in Slack) so i wouldn't change the UX to be of int to not break changes.

@izgeri can you please review the commit i added with the readme change?

I'm pretty sure that CONJUR_TOKEN_TIMEOUT was added for dev troubleshooting, though I see it is documented in the README

I think it is wise to set a standard and enforce it, we should just make sure that the standard (and the default, btw) is clear

sigalsax · 2020-09-10T09:35:07Z

pkg/utils/environment_variable.go

+	defaultValue string,
+	envVarValueOrDefaultFunc EnvVarValueOrDefaultFunc,
+) (int, error) {
+	if envVarValueOrDefaultFunc == nil {


when would this not be nil? an example would help me understand what we are trying to accomplish here (:

IMO this breaking up of parameters style is really hard for my 👀 to follow

look at the UT :)

this is the way we can pass mocks to the functions in go

IMO this breaking up of parameters style is really hard for my 👀 to follow

i understand. i had a conversation about this with @sgnn7 yesterday. the alternative is a line that is too long which is an issue as well.

pkg/utils/environment_variable.go

sigalsax · 2020-09-10T09:40:34Z

pkg/authenticator/config/config.go


 	// DefaultClientCertRetryCountLimit is the amount of times we wait after successful
 	// login for the client certificate file to exist, where each time we wait for a second.
-	DefaultClientCertRetryCountLimit = 10
+	DefaultClientCertRetryCountLimit = "10"


are there minimum or maximum values for this limit? Can they put -5 for example? Would the logic hold up against negative numbers?

In SP we added a check for envValueInt < minValue with a minValue being 0 in order to avoid this

hmmmm. do you think it is needed or is it too low level?

do we want to not let users set (for example) CONJUR_TOKEN_TIMEOUT=-5? looks like too much of an effort to me.

I can totally see this being an issue especially because it is configurable no? It is coming from an environment and I don't trust user input 🙃

yes, but it adds another param that the function consumer needs to set where for most of the times it will be 0 and not interesting. i'll let @izgeri give her take on this before going forward.

I think we should have error checking to ensure sane and acceptable values are input for this param. is the param currently documented? in the README or just in the CONTRIB? if it's just for dev purposes / dev troubleshooting, we could just leave undocumented for now. if we're adding it AND documenting it, then we need to be more careful (and this would need a changelog entry)

I don't want to hold up this PR, but we should really consider doing a followup to allow for minimum/maximum range validation. In most cases, I'd bet that we can define hard-coded values for min and max values that are sensible and are liberal enough to cover all cases. For example, for certificate retries, we could set the minimum to 0 and the maximum to something astronomical (2**32 - 1?) If we don't add range checks, I don't think we know what would happen if someone sets retries or some other configured numbered value to -1 (I'm sure it's not in our integration testing).

by adding a minimum value we raise a few questions:

what is the minimum value of CONJUR_TOKEN_TIMEOUT?

what is the minimum value of CONJUR_CLIENT_CERT_RETRY_COUNT_LIMIT?

what do we do in case the given value is less than the minimum? Do we fail the app or use the default value?

does the answer to question kubernetes authenticator client works with conjur v5 #3 changes how we handle cases where the user gave an invalid value for CONJUR_TOKEN_TIMEOUT?

while these are good questions, i don't want to ask them in this PR and in this release that we are preparing. Once this is merged and the release is out there, we can look back at these questions and decide properly.

created an issue for this: #162

pkg/utils/environment_variable.go

izgeri · 2020-09-10T12:16:39Z

pkg/authenticator/config/config.go

 	}
+	defaultConfig.ClientCertRetryCountLimit = clientCertRetryCountLimit

 	return defaultConfig, nil


if we are updating this, can we change the name of this var? it's not the default config, it's the config that the client will use based on the default config and the user-provided config

sure, nice catch.

izgeri · 2020-09-10T13:31:45Z

pkg/utils/environment_variable.go

+	"github.com/cyberark/conjur-authn-k8s-client/pkg/log"
+)
+
+var defaultEnvVarValueOrDefault = envVarValueOrDefault


what is happening on this line?

we set the function as a variable so it's clearer to the reader that the function being used in the one defined at the env of the file

izgeri · 2020-09-10T13:46:58Z

CHANGELOG.md

 - Username formatting now correctly only prints FullUsername field ([#126](https://github.com/cyberark/conjur-authn-k8s-client/issues/126))
+- Timeout for token retrievals is now correctly reset in consequent failures
+  ([cyberark/conjur-authn-k8s-client#158](https://github.com/cyberark/conjur-authn-k8s-client/issues/158))


Suggested change

- Username formatting now correctly only prints FullUsername field ([#126](https://github.com/cyberark/conjur-authn-k8s-client/issues/126))

- Timeout for token retrievals is now correctly reset in consequent failures

([cyberark/conjur-authn-k8s-client#158](https://github.com/cyberark/conjur-authn-k8s-client/issues/158))

- Username formatting now correctly only prints `FullUsername` field.

[cyberark/conjur-authn-k8s-client#126](https://github.com/cyberark/conjur-authn-k8s-client/issues/126))

- Timeout for token retrievals is now correctly reset in consecutive authentication failures.

[cyberark/conjur-authn-k8s-client#158](https://github.com/cyberark/conjur-authn-k8s-client/issues/158)

I am not sure the user impact from these changes is clear

for the first, is it that the log messages now print a more succinct name or a more detailed name? I'd call that out specifically

Logs now correctly print only the Conjur identity without the policy branch prefix.

for the second option, it's more like

When authentication fails, the exponential backoff retry is correctly reset so that it will continue to attempt to authenticate until backoff is exhausted.

thanks, that is much better

izgeri · 2020-09-10T13:47:16Z

CHANGELOG.md

+- Wait slightly for the client certificate file to exist after login before
+  raising an error.


What is the user impact of this change? How does it affect the UX?

the client will fail less frequently as we now wait for up to 10 seconds to read the cert instead of immediately reading it. We are also logging our efforts so that's another customer-facing change. would you change the entry according to this?

izgeri

left a handful of comments, still would be good for @sgnn7 to weigh in since he chatted with you about some of the implementation details

izgeri · 2020-09-10T13:48:41Z

README.md

@@ -38,7 +38,7 @@ questions, please contact us on [Discourse](https://discuss.cyberarkcommons.org/
 - `CONJUR_AUTHN_URL`: URL pointing to authenticator service endpoint
 - `CONJUR_AUTHN_LOGIN`: Host login for pod e.g. `namespace/service_account/some_service_account`
 - `CONJUR_SSL_CERTIFICATE`: Public SSL cert for Conjur connection
- `CONJUR_TOKEN_TIMEOUT`: Timeout for fetching a new token (defaults to 6 minutes). In most cases, this variable should not be modified.
+- `CONJUR_TOKEN_TIMEOUT`: Timeout for fetching a new token (defaults to 6 minutes). In most cases, this variable should not be modified. The value should be in a signed sequence of decimal numbers with optional fraction and unit suffix (e.g "6m0s")


this should be broken into multiple lines

a signed sequence of decimal numbers with optional fraction and unit suffix
I don't know what this means without the example. is there a standard we could link to here?

i took it from the golang docs for time: https://golang.org/pkg/time/#ParseDuration. i'll add a link

how about The value should be in a format that can be parsed with [time.ParseDuration](https://golang.org/pkg/time/#ParseDuration) (e.g "6m0s")?

izgeri · 2020-09-10T13:48:55Z

design/templates/solution_design.md

@@ -0,0 +1,71 @@
+# Solution Design - Template


does this indicate your branch may need a rebase?

izgeri · 2020-09-10T13:50:41Z

pkg/authenticator/config/config.go


 	// DefaultClientCertRetryCountLimit is the amount of times we wait after successful
 	// login for the client certificate file to exist, where each time we wait for a second.
-	DefaultClientCertRetryCountLimit = 10
+	DefaultClientCertRetryCountLimit = "10"


I think we should have error checking to ensure sane and acceptable values are input for this param. is the param currently documented? in the README or just in the CONTRIB? if it's just for dev purposes / dev troubleshooting, we could just leave undocumented for now. if we're adding it AND documenting it, then we need to be more careful (and this would need a changelog entry)

pkg/utils/environment_variable.go

izgeri · 2020-09-10T13:53:52Z

pkg/utils/environment_variable.go

+	}
+
+	envVarValue := envVarValueOrDefault(envVarName, defaultValue)
+	envVarValueDuration, err := time.ParseDuration(envVarValue)


I'm pretty sure that CONJUR_TOKEN_TIMEOUT was added for dev troubleshooting, though I see it is documented in the README

I think it is wise to set a standard and enforce it, we should just make sure that the standard (and the default, btw) is clear

orenbm · 2020-09-10T14:22:47Z

I think we should have error checking to ensure sane and acceptable values are input for this param. is the param currently documented? in the README or just in the CONTRIB? if it's just for dev purposes / dev troubleshooting, we could just leave undocumented for now. if we're adding it AND documenting it, then we need to be more careful (and this would need a changelog entry)

CONJUR_CLIENT_CERT_RETRY_COUNT_LIMIT is for dev purposes so i'm leaving it undocumented. It is verified to be an int value when parsing it.

orenbm · 2020-09-10T14:25:01Z

might be worth a comment - it's not a pattern I've seen before. maybe other devs would disagree but @sigalsax already had a question :)

@izgeri added a comment

orenbm · 2020-09-10T14:26:14Z

I'm pretty sure that CONJUR_TOKEN_TIMEOUT was added for dev troubleshooting, though I see it is documented in the README. I think it is wise to set a standard and enforce it, we should just make sure that the standard (and the default, btw) is clear

we enforce it in DurationFromEnvOrDefault. an error will be raised if the value is not in the valid format.

the error will be CAKC010E Failed to parse CONJUR_TOKEN_TIMEOUT. Reason: time: invalid duration <given-value>

diverdane

This is good stuff, thanks for moving this logic to a re-usable util package!
I've included some suggestions on how the code might be refactored to simplify and maybe help readability and code coverage.

diverdane · 2020-09-10T20:43:22Z

pkg/utils/environment_variable.go

+	if err != nil {
+		// We return -1 as we can't return nil for int. This value will not be used
+		// as err is not nil.
+		return -1, log.RecordedError(log.CAKC010E, envVarName, err.Error())


The convention that I've seen is that when returning an error along with other return values, the other return values should be set to their zero values (i.e. 0 for int returns).

I can't find anything official in Golang docs, but I did find this:
https://www.digitalocean.com/community/tutorials/handling-errors-in-go#:~:text=When%20a%20function%20returns%20multiple,value%20to%20a%20zero%20value.

When a function returns multiple values, including an error, convention requests that we return the error as the last item. When returning an error from a function with multiple return values, idiomatic Go code also will set each non-error value to a zero value. Zero values are, for example, an empty string for strings, 0 for integers, an empty struct for struct types, and nil for interface and pointer types, to name a few.

nice, great to learn this!

@sigalsax, @izgeri ^

So I am understanding correctly if we return an error (for example return -1, log.RecordedError(log.CAKC010E, envVarName, err.Error())), no matter what, Golang will zero the return values? So for example, -1 will be 0 because it is an integer?
@diverdane @orenbm

i haven't verified it but in any case we will return 0 to maintain the standard

pkg/utils/environment_variable.go

pkg/utils/environment_variable_test.go

diverdane · 2020-09-10T23:01:41Z

pkg/utils/environment_variable_test.go

+}
+
+func TestEnvironmentVariable(t *testing.T) {
+	Convey("IntFromEnvOrDefault", t, func() {


I think we're deprecating the use of Convey? But personally I'm fine with it... it tells the story of what you're testing.

@diverdane what are we moving to if not Convey

diverdane · 2020-09-10T23:21:05Z

pkg/authenticator/config/config.go


 	// DefaultClientCertRetryCountLimit is the amount of times we wait after successful
 	// login for the client certificate file to exist, where each time we wait for a second.
-	DefaultClientCertRetryCountLimit = 10
+	DefaultClientCertRetryCountLimit = "10"


I don't want to hold up this PR, but we should really consider doing a followup to allow for minimum/maximum range validation. In most cases, I'd bet that we can define hard-coded values for min and max values that are sensible and are liberal enough to cover all cases. For example, for certificate retries, we could set the minimum to 0 and the maximum to something astronomical (2**32 - 1?) If we don't add range checks, I don't think we know what would happen if someone sets retries or some other configured numbered value to -1 (I'm sure it's not in our integration testing).

We would like to better UT the logic of reading a value from the env and defaulting to a default value if it's not present. We also have the logic that transforms this env var value into an int or a duration (and maybe more in the future) and it is best that this logic is UTed and is implemented in its own util. This improves the readability of the config.go file and of the util functions themselves. This will also let us use these utils in other projects that consume the authn-client without implementing them there as well.

sigalsax

LGTM great work!

Implemented requested changes

orenbm requested a review from a team as a code owner September 10, 2020 09:01

orenbm requested a review from izgeri September 10, 2020 09:01

sigalsax reviewed Sep 10, 2020

View reviewed changes

pkg/authenticator/config/config.go Show resolved Hide resolved

sigalsax reviewed Sep 10, 2020

View reviewed changes

pkg/utils/environment_variable.go Outdated Show resolved Hide resolved

sigalsax reviewed Sep 10, 2020

View reviewed changes

pkg/utils/environment_variable.go Show resolved Hide resolved

orenbm force-pushed the env-var-util branch from 93dbbff to f3dcfda Compare September 10, 2020 10:16

izgeri reviewed Sep 10, 2020

View reviewed changes

orenbm force-pushed the env-var-util branch from f3dcfda to 0f936ed Compare September 10, 2020 12:26

izgeri reviewed Sep 10, 2020

View reviewed changes

orenbm force-pushed the env-var-util branch from 0f936ed to b036163 Compare September 10, 2020 13:33

izgeri reviewed Sep 10, 2020

View reviewed changes

izgeri previously requested changes Sep 10, 2020

View reviewed changes

orenbm force-pushed the env-var-util branch from b036163 to b45b147 Compare September 10, 2020 14:19

orenbm requested a review from izgeri September 10, 2020 14:29

orenbm force-pushed the env-var-util branch from b45b147 to 6eb0e94 Compare September 10, 2020 14:29

diverdane reviewed Sep 10, 2020

View reviewed changes

orenbm force-pushed the env-var-util branch 5 times, most recently from b206292 to bbea837 Compare September 13, 2020 10:14

orenbm added 2 commits September 13, 2020 13:15

Give example for CONJUR_TOKEN_TIMEOUT in README

18302b6

orenbm force-pushed the env-var-util branch from bbea837 to 18302b6 Compare September 13, 2020 10:16

sigalsax approved these changes Sep 13, 2020

View reviewed changes

orenbm merged commit f3f6d02 into master Sep 13, 2020

orenbm deleted the env-var-util branch September 13, 2020 10:39

		- Wait slightly for the client certificate file to exist after login before
		raising an error.

Extract env var value read into a util #160

Extract env var value read into a util #160

Conversation

orenbm commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenbm Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenbm Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenbm Sep 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izgeri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orenbm commented Sep 10, 2020

orenbm commented Sep 10, 2020

orenbm commented Sep 10, 2020 • edited Loading

diverdane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigalsax Sep 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sigalsax left a comment

Choose a reason for hiding this comment

orenbm Sep 10, 2020 •

edited

Loading

orenbm Sep 10, 2020 •

edited

Loading

orenbm Sep 10, 2020 •

edited

Loading

orenbm commented Sep 10, 2020 •

edited

Loading

sigalsax Sep 13, 2020 •

edited

Loading