Add support for Kinesis logging endpoint #177

kellymclaughlin · 2020-12-15T17:52:29Z

This requires moving to version 2.1.0 of go-fastly. If there's other work that needs to be done before that can happen that's fine, but I wanted to go ahead and get the PR out there even if it can't be merged right away.

Integralist

Only a few minor comments. Otherwise looking good 👍🏻

pkg/app/run_test.go

pkg/logging/kinesis/create.go

Integralist · 2020-12-16T16:20:54Z

pkg/logging/kinesis/create.go

+	"github.com/fastly/go-fastly/v2/fastly"
+)
+
+// CreateCommand calls the Fastly API to create Amazon Kinesis logging endpoints.


I think this should be singular, not plural for 'endpoint'

Suggested change

// CreateCommand calls the Fastly API to create Amazon Kinesis logging endpoints.

// CreateCommand calls the Fastly API to create an Amazon Kinesis logging endpoint.

Integralist · 2020-12-16T16:35:31Z

pkg/logging/kinesis/create.go

+	c.CmdClause.Flag("name", "The name of the Kinesis logging object. Used as a primary key for API access").Short('n').Required().StringVar(&c.EndpointName)
+	c.CmdClause.Flag("service-id", "Service ID").Short('s').StringVar(&c.manifest.Flag.ServiceID)
+	c.CmdClause.Flag("version", "Number of service version").Required().IntVar(&c.Version)
+
+	c.CmdClause.Flag("stream-name", "The Amazon Kinesis stream to send logs to").Required().StringVar(&c.StreamName)
+	c.CmdClause.Flag("access-key", "The access key associated with the target Amazon Kinesis stream").Required().StringVar(&c.AccessKey)
+	c.CmdClause.Flag("secret-key", "The secret key associated with the target Amazon Kinesis stream").Required().StringVar(&c.SecretKey)
+
+	c.CmdClause.Flag("region", "The AWS region where the Kinesis stream exists").Required().StringVar(&c.Region)
+	c.CmdClause.Flag("format", "Apache style log formatting").Action(c.Format.Set).StringVar(&c.Format.Value)
+	c.CmdClause.Flag("format-version", "The version of the custom logging format used for the configured endpoint. Can be either 2 (default) or 1").Action(c.FormatVersion.Set).UintVar(&c.FormatVersion.Value)
+	c.CmdClause.Flag("response-condition", "The name of an existing condition in the configured endpoint, or leave blank to always execute").Action(c.ResponseCondition.Set).StringVar(&c.ResponseCondition.Value)
+	c.CmdClause.Flag("placement", "Where in the generated VCL the logging call should be placed, overriding any format_version default. Can be none or waf_debug").Action(c.Placement.Set).StringVar(&c.Placement.Value)


I'm thinking the flags should be grouped by 'required' followed by 'optional'...

Suggested change

c.CmdClause.Flag("name", "The name of the Kinesis logging object. Used as a primary key for API access").Short('n').Required().StringVar(&c.EndpointName)

c.CmdClause.Flag("service-id", "Service ID").Short('s').StringVar(&c.manifest.Flag.ServiceID)

c.CmdClause.Flag("version", "Number of service version").Required().IntVar(&c.Version)

c.CmdClause.Flag("stream-name", "The Amazon Kinesis stream to send logs to").Required().StringVar(&c.StreamName)

c.CmdClause.Flag("access-key", "The access key associated with the target Amazon Kinesis stream").Required().StringVar(&c.AccessKey)

c.CmdClause.Flag("secret-key", "The secret key associated with the target Amazon Kinesis stream").Required().StringVar(&c.SecretKey)

c.CmdClause.Flag("region", "The AWS region where the Kinesis stream exists").Required().StringVar(&c.Region)

c.CmdClause.Flag("format", "Apache style log formatting").Action(c.Format.Set).StringVar(&c.Format.Value)

c.CmdClause.Flag("format-version", "The version of the custom logging format used for the configured endpoint. Can be either 2 (default) or 1").Action(c.FormatVersion.Set).UintVar(&c.FormatVersion.Value)

c.CmdClause.Flag("response-condition", "The name of an existing condition in the configured endpoint, or leave blank to always execute").Action(c.ResponseCondition.Set).StringVar(&c.ResponseCondition.Value)

c.CmdClause.Flag("placement", "Where in the generated VCL the logging call should be placed, overriding any format_version default. Can be none or waf_debug").Action(c.Placement.Set).StringVar(&c.Placement.Value)

c.CmdClause.Flag("name", "The name of the Kinesis logging object. Used as a primary key for API access").Short('n').Required().StringVar(&c.EndpointName)

c.CmdClause.Flag("version", "Number of service version").Required().IntVar(&c.Version)

c.CmdClause.Flag("stream-name", "The Amazon Kinesis stream to send logs to").Required().StringVar(&c.StreamName)

c.CmdClause.Flag("access-key", "The access key associated with the target Amazon Kinesis stream").Required().StringVar(&c.AccessKey)

c.CmdClause.Flag("secret-key", "The secret key associated with the target Amazon Kinesis stream").Required().StringVar(&c.SecretKey)

c.CmdClause.Flag("region", "The AWS region where the Kinesis stream exists").Required().StringVar(&c.Region)

c.CmdClause.Flag("service-id", "Service ID").Short('s').StringVar(&c.manifest.Flag.ServiceID)

c.CmdClause.Flag("format", "Apache style log formatting").Action(c.Format.Set).StringVar(&c.Format.Value)

c.CmdClause.Flag("format-version", "The version of the custom logging format used for the configured endpoint. Can be either 2 (default) or 1").Action(c.FormatVersion.Set).UintVar(&c.FormatVersion.Value)

c.CmdClause.Flag("response-condition", "The name of an existing condition in the configured endpoint, or leave blank to always execute").Action(c.ResponseCondition.Set).StringVar(&c.ResponseCondition.Value)

c.CmdClause.Flag("placement", "Where in the generated VCL the logging call should be placed, overriding any format_version default. Can be none or waf_debug").Action(c.Placement.Set).StringVar(&c.Placement.Value)

Integralist · 2020-12-16T16:37:21Z

pkg/logging/kinesis/create.go

+	c.CmdClause.Flag("access-key", "The access key associated with the target Amazon Kinesis stream").Required().StringVar(&c.AccessKey)
+	c.CmdClause.Flag("secret-key", "The secret key associated with the target Amazon Kinesis stream").Required().StringVar(&c.SecretKey)
+
+	c.CmdClause.Flag("region", "The AWS region where the Kinesis stream exists").Required().StringVar(&c.Region)


Are we sure 'region' is required? It doesn't look to be in the API docs example (nor in Go-Fastly).

Yeah, it is required. The configuration validation will fail if it is not present. I see what you mean in the case of go-fastly. I followed the existing patterns of other logging endpoints when I did that work and they all seem to only have ServiceId and ServiceVersion as required. My guess is that the enforcement of required fields was deferred to the config validation, but maybe we want to revisit that.

Question:

The configuration validation will fail if it is not present.

I presume 'config validation' refers to the upstream API's own validation, would that be correct?

I see Go-Fastly currently doesn't define validation for the 'region' field, but if we're saying the API itself is expecting a region field otherwise it'll return an error, then we should ensure a validation check for the 'region' field is added into Go-Fastly (I have an existing PR open I can add this to).

But for the time being, this is fine to be marked as .Required() considering Go-Fastly doesn't currently have logic to catch the missing field.

Note: I can't actually test whether the API expects 'region' to be provided because it seems Kinesis logging isn't available on my personal Fastly account 😬 I did login as an admin to see if it's something I could enable as a feature flag but nothing obvious was coming up in my search.

Also, just FYI (most of which will be obvious to you): my perspective on this is that we have three tools at play here:

CLI > Go-Fastly > API

We should be able to trust the API to always have the right validation in place, but (as you know) by having validation outside of the API it means we can prevent slowing down a client trying to carry out operations that otherwise we know are just going to fail by the time they reach the API layer, so short-circuiting those requests is better for the client as far as the 'feedback loop' is concerned but also good for the API because it doesn't have to handle the request in the first place.

Considering Go-Fastly and CLI sit at effectively the same level (i.e. they exist on the client) we need to decide which 'tool' is responsible for handling validation of obvious issues with a client request, and for me that should be Go-Fastly as it's the thing that's ultimately fronting the API.

So my hope is that with the work @doramatadora has been doing on the new OpenAPI specification it will mean we can move to a place where we more concretely know what is a required field and have Go-Fastly handle validation and thus reduce duplicating logic in the CLI (although admittedly at this point in time the OpenAPI spec for Kinesis logging still doesn't indicate that 'region' is a required field nor does it show it as part of its example code so that would likely need updating).

I presume 'config validation' refers to the upstream API's own validation, would that be correct?

That's correct.

Note: I can't actually test whether the API expects 'region' to be provided because it seems Kinesis logging isn't available on my personal Fastly account grimacing I did login as an admin to see if it's something I could enable as a feature flag but nothing obvious was coming up in my search.

If you send me the service id in slack I can enable this for you.

I'll also follow-up on the docs side of things and open a change request if needed to have it indicate region as required.

Integralist · 2020-12-16T16:42:27Z

pkg/logging/kinesis/delete.go

+	"github.com/fastly/go-fastly/v2/fastly"
+)
+
+// DeleteCommand calls the Fastly API to delete Amazon Kinesis logging endpoints.


I think this should be singular, not plural for 'endpoint'

Suggested change

// DeleteCommand calls the Fastly API to delete Amazon Kinesis logging endpoints.

// DeleteCommand calls the Fastly API to delete an Amazon Kinesis logging endpoint.

Integralist · 2020-12-16T17:21:28Z

pkg/logging/kinesis/describe.go

+	fmt.Fprintf(out, "Service ID: %s\n", kinesis.ServiceID)
+	fmt.Fprintf(out, "Version: %d\n", kinesis.ServiceVersion)
+	fmt.Fprintf(out, "Name: %s\n", kinesis.Name)
+	fmt.Fprintf(out, "Stream name: %s\n", kinesis.StreamName)
+	fmt.Fprintf(out, "Region: %s\n", kinesis.Region)
+	fmt.Fprintf(out, "Access key: %s\n", kinesis.AccessKey)
+	fmt.Fprintf(out, "Secret key: %s\n", kinesis.SecretKey)
+	fmt.Fprintf(out, "Format: %s\n", kinesis.Format)
+	fmt.Fprintf(out, "Format version: %d\n", kinesis.FormatVersion)
+	fmt.Fprintf(out, "Response condition: %s\n", kinesis.ResponseCondition)
+	fmt.Fprintf(out, "Placement: %s\n", kinesis.Placement)


I think we should create a new /pkg/text/lines.go to make it easier to render output for newline information (like logging endpoints) and then we'd also be consistent with other packages in the CLI that use the text package.

Something generic like this would suffice: https://play.golang.org/p/csgKDzLZPeu

Which means you could do something like:

import "github.com/fastly/cli/pkg/text" text.PrintLines(out, text.Lines{ "Service ID": kinesis.ServiceID, "Version": kinesis.ServiceVersion, "Name": kinesis.Name, ... })

@phamann what do you think about this...

If we combined this generic implementation (see my comment above) with textio.NewPrefixWriter then we could probably end up reducing quite a few of the existing Print<T> functions (e.g. PrintBackend, PrintDictionaryItemKV etc).

You could even take it a step further and add in the ability to pass in an optional callback function for those edge cases such as PrintDictionaryItem where they only print lines that have values (although to be fair that's probably logic you'd shift back up into the consuming package). But I'm just 'brain dumping' ideas at the moment.

👍 to this idea from me. I'd say this is probably out of scope for this PR, but I think it would be a good change to implement broadly.

Agree on both accounts:

Very useful abstraction and would reduce repetition across the codebase.

Out of scope of this PR.

👍🏻 👌🏻

Integralist · 2020-12-16T17:47:21Z

pkg/logging/kinesis/root.go

+func NewRootCommand(parent common.Registerer, globals *config.Data) *RootCommand {
+	var c RootCommand
+	c.Globals = globals
+	c.CmdClause = parent.Command("kinesis", "Manipulate Fastly service version Kinesis logging endpoints")


I'm not a fan of the wording used in the CLI currently as it doesn't read like a correct sentence.

Suggested change

c.CmdClause = parent.Command("kinesis", "Manipulate Fastly service version Kinesis logging endpoints")

c.CmdClause = parent.Command("kinesis", "Manipulate a Kinesis logging endpoint for a specific Fastly service version")

This could be something I fix more broadly in a separate PR though.

Oh, actually -- is this command even reachable (after looking at Exec)?

Oh, actually -- is this command even reachable (after looking at Exec)?

All command groups have unreachable roots in the tree. This is the root for this group of commands, the text is only used in help output when, for instance, fastly logging --help is ran.

Integralist · 2020-12-16T17:54:18Z

pkg/logging/kinesis/list.go

+		return nil
+	}
+
+	fmt.Fprintf(out, "Service ID: %s\n", c.Input.ServiceID)


Similar to my earlier comment I think a new generic text implementation could be useful here as well.

kellymclaughlin · 2020-12-21T17:41:49Z

pkg/logging/kinesis/create.go

+
+	// required
+	c.CmdClause.Flag("name", "The name of the Kinesis logging object. Used as a primary key for API access").Short('n').Required().StringVar(&c.EndpointName)
+	c.CmdClause.Flag("service-id", "Service ID").Required().Short('s').StringVar(&c.manifest.Flag.ServiceID)


I noticed that service-id is not marked as required in many or maybe all of the logging endpoint code. I've set it for Kinesis in this PR, but I'll address the inconsistency in the rest of the logging endpoints in another PR.

Integralist · 2021-01-04T09:59:40Z

@kellymclaughlin just checking, was this PR ready for another review?

kellymclaughlin · 2021-01-04T15:14:57Z

@Integralist Yes, this should be good for another look. Thanks!

Integralist · 2021-01-05T17:01:03Z

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/v2 v2.0.0 h1:dCPOEbTXy8ic5CGj6YibPNvRYG01y15H7kHTlC4d57k=
 github.com/fastly/go-fastly/v2 v2.0.0/go.mod h1:+gom+YR+9Q5I4biSk/ZjHQGWXxqpRxC3YDVYQcRpZwQ=


can we run a go mod tidy to see if we can get the v2.0.0 go-fastly dependency cleared from this file.

Integralist · 2021-01-05T17:17:23Z

pkg/logging/kinesis/delete.go

+	c.Globals = globals
+	c.manifest.File.Read(manifest.Filename)
+	c.CmdClause = parent.Command("delete", "Delete a Kinesis logging endpoint on a Fastly service version").Alias("remove")
+	c.CmdClause.Flag("service-id", "Service ID").Required().Short('s').StringVar(&c.manifest.Flag.ServiceID)


@kellymclaughlin so interestingly I'm not sure if service-id needs to be 'required' in the sense that later on in Exec we have...

serviceID, source := c.manifest.ServiceID() if source == manifest.SourceUndefined { return errors.ErrNoServiceID }

If we inspect the ServiceID() method it's checking first the flag and then the manifest file for a service id before erroring if both are missing...

cli/pkg/compute/manifest/manifest.go

Lines 52 to 63 in c0f3373

// ServiceID yields a ServiceID.

func (d *Data) ServiceID() (string, Source) {

if d.Flag.ServiceID != "" {

return d.Flag.ServiceID, SourceFlag

}

if d.File.ServiceID != "" {

return d.File.ServiceID, SourceFile

}

return "", SourceUndefined

}

Oh interesting. Ok, I'll remove the Required and move the service-id option to the optional section then.

Integralist

@kellymclaughlin this LGTM but before merging could you first rebase master.

Depending on when you see this notification you might need to wait until I've gotten someone to approve this PR (which will cut a new 'patch' CLI release v0.21.2).

Once that's done and you've rebased, then your PR can be cut as a 'minor' release v0.22.0 (along with your Splunk PR).

Thanks!

kellymclaughlin force-pushed the add-kinesis-logging-endpoint branch from 3c70a40 to 0bfb086 Compare December 15, 2020 18:20

Integralist reviewed Dec 16, 2020

View reviewed changes

kellymclaughlin force-pushed the add-kinesis-logging-endpoint branch from ed2d1e6 to 7eb1d32 Compare December 21, 2020 17:17

kellymclaughlin commented Dec 21, 2020

View reviewed changes

kellymclaughlin requested a review from Integralist January 4, 2021 15:15

Integralist reviewed Jan 5, 2021

View reviewed changes

kellymclaughlin requested a review from Integralist January 5, 2021 18:45

Integralist approved these changes Jan 6, 2021

View reviewed changes

kellymclaughlin added 5 commits January 6, 2021 10:56

Add support for Kinesis logging endpoint

75eccff

Code reorganization and comment re-wording for Kinesis endpoint

944cf3d

Indicate service-id as required for Kinesis operations

cacda3a

Fix comments typo in Kinesis create and update files

8bbe7a1

Make service-id optional again and clean up go.sum file

42e6b99

kellymclaughlin force-pushed the add-kinesis-logging-endpoint branch from 62ab724 to 42e6b99 Compare January 6, 2021 17:57

Integralist merged commit 7bc9416 into fastly:master Jan 7, 2021

Integralist added the enhancement New feature or request label Jan 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Kinesis logging endpoint #177

Add support for Kinesis logging endpoint #177

kellymclaughlin commented Dec 15, 2020

Integralist left a comment

Integralist Dec 16, 2020

Integralist Dec 16, 2020

Integralist Dec 16, 2020

kellymclaughlin Dec 17, 2020

Integralist Dec 18, 2020

Integralist Dec 18, 2020

Integralist Dec 18, 2020

kellymclaughlin Dec 21, 2020

Integralist Dec 16, 2020

Integralist Dec 16, 2020

Integralist Dec 16, 2020

kellymclaughlin Dec 17, 2020

phamann Dec 18, 2020

Integralist Dec 18, 2020

Integralist Dec 16, 2020

phamann Dec 18, 2020

Integralist Dec 16, 2020

kellymclaughlin Dec 21, 2020

Integralist commented Jan 4, 2021

kellymclaughlin commented Jan 4, 2021

Integralist Jan 5, 2021

kellymclaughlin Jan 5, 2021

Integralist Jan 5, 2021

kellymclaughlin Jan 5, 2021

kellymclaughlin Jan 5, 2021

Integralist left a comment

	// CreateCommand calls the Fastly API to create Amazon Kinesis logging endpoints.
	// CreateCommand calls the Fastly API to create an Amazon Kinesis logging endpoint.

	// DeleteCommand calls the Fastly API to delete Amazon Kinesis logging endpoints.
	// DeleteCommand calls the Fastly API to delete an Amazon Kinesis logging endpoint.

	c.CmdClause = parent.Command("kinesis", "Manipulate Fastly service version Kinesis logging endpoints")
	c.CmdClause = parent.Command("kinesis", "Manipulate a Kinesis logging endpoint for a specific Fastly service version")

	// ServiceID yields a ServiceID.
	func (d *Data) ServiceID() (string, Source) {
	if d.Flag.ServiceID != "" {
	return d.Flag.ServiceID, SourceFlag
	}

	if d.File.ServiceID != "" {
	return d.File.ServiceID, SourceFile
	}

	return "", SourceUndefined
	}

Add support for Kinesis logging endpoint #177

Add support for Kinesis logging endpoint #177

Conversation

kellymclaughlin commented Dec 15, 2020

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

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

Integralist commented Jan 4, 2021

kellymclaughlin commented Jan 4, 2021

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 left a comment

Choose a reason for hiding this comment