Support sasl kafka endpoint options in Fastly CLI #161

kellymclaughlin · 2020-10-26T22:00:05Z

Add the following options to the Kafka logging subcommands in order to support SASL:

parse-log-kevvals - Indicates the log format should be parsed as key-value pairs
max-batch-size - The maximum size of a log batch in bytes
auth-method - one of: plain, scram-sha256, scram-sha512
username - Username for authentication
password - Password for authentication

mccurdyc · 2020-10-28T13:12:29Z

@kellymclaughlin I haven't reviewed yet, but just noting I think this will require us to update the go-fastly version in the Go module to get the latest version with these new fields, right?

fastly/go-fastly#227

phamann

Thanks for the contribution @kellymclaughlin, looking good overall just want to clean up some of the flag types.

Also made me realise that Kingpin really needs a flag.RequiredIfSet(*otherFlag) flag clause to decorate flag dependencies to avoid us having to write the validation logic for these relationships.

pkg/logging/kafka/create.go

pkg/logging/kafka/update.go

mccurdyc

I agree with @phamann's comments, but also added a couple comments of my own.

Otherwise, things look good to me.

mccurdyc · 2020-10-29T14:58:33Z

pkg/logging/kafka/create.go

+		}
+	}
+
+	if !c.UseSASL.Value && (c.AuthMethod.Value != "" || c.User.Value != "" || c.Password.Value != "") {


Disregard the following, but it was a thought that crossed my mind.

I based my final judgement on how cat and gcloud handle extraneous flags.

thought - Is this block really necessary? In other words isn't this safe to just accept, but not do anything with these values since the values are only set in the above block?

Good question. It's not necessary. The reason I added this was trying avoid unexpected results and unnecessary debugging. My reasoning was if a user gives the auth method, username, or password they probably meant to enable SASL and by accepting the invalid set of options we've possibly added to the confusion. The hope is by enforcing this at creation time we're saving them and possibly ourselves some time. Happy to remove it though if you think it's not worth it.

It is probably worth giving the user feedback when the options they used make no sense. My $0.02 it should stay. The more Go Way to do it would be to move this invalid case as far up in the function as possible.

pkg/logging/kafka/kafka_integration_test.go

mccurdyc · 2020-10-29T15:14:28Z

pkg/logging/kafka/kafka_test.go

+			},
+		},
+		{
+			name: "verify disabling SASL",


We may be able to disregard the following because the CLI will require that credentials always be supplied when SASL is enabled, but we should probably confirm that you can't enable, disable and re-renabled without supplying credentials in the API.

TL;DR - we should ensure that when we disable SASL, we removed the stored SASL creds i.e., make sure these zero values make it through to the backend.

Thanks for including this test!! It got me thinking...

I just want someone else to confirm my thoughts and that the following is true on the following:

enable SASL (i.e., send credentials to Fastly)

disable SASL

This will "delete" the SASL credentials, right?

re-enable SASL, but don't supply credentials.

This may require us to confirm in go-fastly that sending empty string values to go-fastly will actually make it through to the Fastly backend.

Basically, I want to ensure that we don't have a possible security issue where if SASL was ever enabled, it couldn't be disabled and then re-enabled without credentials.

Agreed, the code to ensure that we remove the stored creds is here. Right now the API doesn't establish a relationship among any of the Kafka configuration fields and I reflected that in my go-fastly changes. I think ideally we'd implement this logic once in the API versus in each consumer, but for now I wanted to deal with it here until I worked out the best way to deal with it more generally.

pkg/logging/kafka/update.go

* Use NegatableBoolVar instead of two explicit flags * Use EnumVar for --auth-method to restrict valid input values * Remove some unnecessary test formatting * Remove obsolete test cases

kellymclaughlin · 2020-10-29T20:38:05Z

Thanks for the contribution @kellymclaughlin, looking good overall just want to clean up some of the flag types.

Also made me realise that Kingpin really needs a flag.RequiredIfSet(*otherFlag) flag clause to decorate flag dependencies to avoid us having to write the validation logic for these relationships.

Thanks for looking over this @phamann! I just came across the NegatableBool yesterday evening myself and I didn't know about EnumVar. I've made those changes.

As far as Kingpin, I would have definitely enjoyed a RequiredIfSet option. I also found the builtin validation stuff in the library not usable in our context which is too bad. I also noticed we're relying on the v3-unstable branch of Kingpin and I see that the author has ceased contributing to the library and has created an entirely new library for args parsing. This is my first foray into the cli code, but I wonder if there has been any consideration of moving to a new args parsing library that is more actively supported and offer us some better features? If there is ever discussion of that I'd be happy to be involved and be part of a divide and conquer effort to get it done.

jaredmorrow

Some small nits related to https://github.com/golang/go/wiki/CodeReviewComments#indent-error-flow

jaredmorrow · 2020-11-06T23:37:54Z

pkg/logging/kafka/create.go

+		}
+	}
+
+	if !c.UseSASL.Value && (c.AuthMethod.Value != "" || c.User.Value != "" || c.Password.Value != "") {


It is probably worth giving the user feedback when the options they used make no sense. My $0.02 it should stay. The more Go Way to do it would be to move this invalid case as far up in the function as possible.

pkg/logging/kafka/create.go

pkg/logging/kafka/update.go

…lin/cli into kelly/add-kafka-sasl-fields

phamann

Thanks @kellymclaughlin. This is looking good now. Just some edits to make the flag descriptions consistent with the rest of the project. You'll need to regenerate the app help for the tests to pass once accepted. Then we're good to go.

pkg/logging/kafka/create.go

pkg/logging/kafka/update.go

Remove periods from flag descriptions Co-authored-by: Patrick Hamann <patrick@fastly.com>

phamann

LGTM 🎉

Thanks for the contribution @kellymclaughlin and apologies its taken so long to get over the line.

Integralist

One very minor comment, also it would be good to have these new fields documented. I've left a comment on the most recent jira ticket here for reference: here

pkg/app/run_test.go

Integralist · 2020-11-16T17:05:39Z

go.sum

@@ -32,6 +32,8 @@ github.com/emirpasic/gods v1.12.0 h1:QAUIPSaCu4G+POclxeqb3F+WPpdKqFGlw36+yOzGlrg
 github.com/emirpasic/gods v1.12.0/go.mod h1:YfzfFFoVP/catgzJb4IKIqXjX78Ha8FMSDh3ymbK86o=
 github.com/fastly/go-fastly v1.16.0 h1:RMHvkzZ52J60+jSPK+6BLodIsx4OBlaoB18XmL7C0+Y=
 github.com/fastly/go-fastly v1.16.0/go.mod h1:jILbTQnU/K/7XHQNzQWd1O7hbXIcp6dKrxfRWqU6xfk=


@kellymclaughlin I just pulled your branch and ran make all and found that the go.sum was updated for me to remove the v1.16.0 dependency. Are you OK to update this PR with that change?

@Integralist Ok, I've pushed that change.

Thanks @kellymclaughlin - just to let you know that Patrick and I are still chipping away at the Go-Fastly v2 merge/release (things got complicated 😬) but we expect to have it finished today.

👍 It looks like something has changed in the last day or so on the Github side with setting environment variables for the CI actions and it's now causing the checks to fail. Let me know if you would like me to try to run that down for this PR.

Yeah we had to fix that here: https://github.com/fastly/terraform-provider-fastly/pull/340/files#diff-15c806aa509538190832852f439e9921a23bec2da81f95ed0e4bf13c14e5b160R55

Integralist · 2020-11-17T16:11:41Z

Looks like there is an add-path used as well somewhere and that I believe also needs to use the new github environment files? https://docs.github.com/en/free-pro-team@latest/actions/reference/workflow-commands-for-github-actions#environment-files

kellymclaughlin · 2020-11-17T16:50:27Z

Third time's a charm, but got the CI checks happy again.

Support sasl kafka endpoint options in Fastly CLI

6d6a0d3

phamann requested a review from mccurdyc October 27, 2020 11:40

kellymclaughlin added 2 commits October 28, 2020 15:21

Bump the go-fastly version to pull in kafka SASL changes

5bf4ebc

Merge branch 'master' into kelly/add-kafka-sasl-fields

7a16ab8

phamann requested changes Oct 29, 2020

View reviewed changes

phamann added the enhancement New feature or request label Oct 29, 2020

mccurdyc reviewed Oct 29, 2020

View reviewed changes

kellymclaughlin added 2 commits October 29, 2020 13:55

Updates based on review feedback

3015eff

* Use NegatableBoolVar instead of two explicit flags * Use EnumVar for --auth-method to restrict valid input values * Remove some unnecessary test formatting * Remove obsolete test cases

Clean up help output so run_test passes

34e3558

Merge branch 'master' into kelly/add-kafka-sasl-fields

2530b04

kellymclaughlin requested review from phamann and mccurdyc November 4, 2020 21:43

jaredmorrow suggested changes Nov 6, 2020

View reviewed changes

kellymclaughlin added 4 commits November 9, 2020 09:27

Move failure cases up in functions per code review

4485587

Merge branch 'kelly/add-kafka-sasl-fields' of github.com:kellymclaugh…

8c0f4da

…lin/cli into kelly/add-kafka-sasl-fields

Remove some extraneous whitespace in Kafka test cases

ecd0124

Merge branch 'master' into kelly/add-kafka-sasl-fields

83f13a3

kellymclaughlin requested a review from jaredmorrow November 9, 2020 17:16

phamann requested changes Nov 10, 2020

View reviewed changes

kellymclaughlin and others added 2 commits November 10, 2020 08:33

Apply suggestions from code review

0fd5778

Remove periods from flag descriptions Co-authored-by: Patrick Hamann <patrick@fastly.com>

Update the app help test to remove periods from kafka flag descriptions

6f48f1d

phamann approved these changes Nov 11, 2020

View reviewed changes

Integralist reviewed Nov 16, 2020

View reviewed changes

pkg/app/run_test.go Show resolved Hide resolved

Integralist reviewed Nov 16, 2020

View reviewed changes

kellymclaughlin added 2 commits November 17, 2020 07:56

go.sum file cleanup

d256350

Update how GOPATH is set in pr_test github action file

8849e39

Update how PATH is set in pr_test github action file

9e05949

Update how PATH and GOPATH are set for tests in pr_test action file

8585157

Integralist approved these changes Nov 19, 2020

View reviewed changes

Integralist merged commit e9802d1 into fastly:master Nov 19, 2020

kellymclaughlin deleted the kelly/add-kafka-sasl-fields branch November 19, 2020 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support sasl kafka endpoint options in Fastly CLI #161

Support sasl kafka endpoint options in Fastly CLI #161

kellymclaughlin commented Oct 26, 2020

mccurdyc commented Oct 28, 2020

phamann left a comment •

edited

Loading

mccurdyc left a comment

mccurdyc Oct 29, 2020

kellymclaughlin Oct 29, 2020

jaredmorrow Nov 6, 2020

mccurdyc Oct 29, 2020

kellymclaughlin Oct 29, 2020

kellymclaughlin commented Oct 29, 2020

jaredmorrow left a comment

jaredmorrow Nov 6, 2020

phamann left a comment

phamann left a comment

Integralist left a comment

Integralist Nov 16, 2020

kellymclaughlin Nov 17, 2020

Integralist Nov 17, 2020

kellymclaughlin Nov 17, 2020

Integralist Nov 17, 2020

Integralist commented Nov 17, 2020

kellymclaughlin commented Nov 17, 2020

Support sasl kafka endpoint options in Fastly CLI #161

Support sasl kafka endpoint options in Fastly CLI #161

Conversation

kellymclaughlin commented Oct 26, 2020

mccurdyc commented Oct 28, 2020

phamann left a comment • edited Loading

Choose a reason for hiding this comment

mccurdyc 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

kellymclaughlin commented Oct 29, 2020

jaredmorrow left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phamann left a comment

Choose a reason for hiding this comment

phamann left a comment

Choose a reason for hiding this comment

Integralist 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

Integralist commented Nov 17, 2020

kellymclaughlin commented Nov 17, 2020

phamann left a comment •

edited

Loading