Skip to content

[extension/awslogs_encoding] Add support for VPC flow logs in plain text file format #38897

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 21 commits into from
Apr 1, 2025
Merged

[extension/awslogs_encoding] Add support for VPC flow logs in plain text file format #38897

merged 21 commits into from
Apr 1, 2025

Conversation

constanca-m
Copy link
Contributor

@constanca-m constanca-m commented Mar 24, 2025 •
edited
Loading

Description

Add support for VPC flow logs sent to S3 in plain text format.

Link to tracking issue

Fixes #38896.

Testing

There are new unit tests added.

Documentation

Comments in the code and unit tests should be enough.

Fields are mapped this way:

Flow log field Attribute Available? Currently supported?
version aws.vpc.flow.log.version 🔴
account-id cloud.account.id 🟢
interface-id aws.eni.id 🔴
srcaddr source.address or network.peer.address 🔴
pkt-srcaddr source.address 🟢
dstaddr destination.address or network.peer.address 🔴
pkt-dstaddr destination.address 🟢
srcport source.port 🟢
dstport destination.port 🟢
protocol network.protocol.name 🟢
packets aws.vpc.flow.packets 🔴
bytes aws.vpc.flow.bytes 🔴
start aws.vpc.flow.start 🔴
end log timestamp N/A
action aws.vpc.flow.action 🔴
log-status aws.vpc.flow.status 🔴
vpc-id aws.vpc.id 🔴
subnet-id aws.vpc.subnet.id 🔴
instance-id host.id 🔴
tcp-flags network.tcp.flags 🔴
type network.type 🟢
region cloud.region 🟢
az-id aws.az.id 🔴
sublocation-type aws.sublocation.type 🔴
sublocation-id aws.sublocation.id 🔴
pkt-src-aws-service aws.vpc.flow.source.service 🔴
pkt-dst-aws-service aws.vpc.flow.destination.service 🔴
flow-direction network.io.direction 🟢
traffic-path aws.vpc.flow.traffic_path 🔴
ecs-cluster-arn aws.ecs.cluster.arn 🟢 🔴
ecs-cluster-name aws.ecs.cluster.name 🔴 🔴
ecs-container-instance-arn aws.ecs.container.instance.arn 🔴 🔴
ecs-container-instance-id aws.ecs.container.instance.id 🔴 🔴
ecs-container-id aws.ecs.container.id 🔴 🔴
ecs-second-container-id aws.ecs.second.container.arn 🔴 🔴
ecs-service-name aws.ecs.service.name 🔴 🔴
ecs-task-definition-arn aws.ecs.task.definition.arn 🔴 🔴
ecs-task-arn aws.ecs.task.arn 🟢 🔴
ecs-task-id aws.ecs.task.id 🟢 🔴
reject-reason aws.vpc.flow.reject_reason 🔴

@constanca-m constanca-m requested a review from a team as a code owner March 24, 2025 10:35
@constanca-m constanca-m requested a review from mwear March 24, 2025 10:35
@github-actions github-actions bot requested a review from axw March 24, 2025 10:36
atoulme
atoulme reviewed Mar 24, 2025
View reviewed changes
extension/encoding/awslogsencodingextension/config.go
formatVPCFlowLog = "vpc_flow_log"

fileFormatPlainText = "plain-text"
fileFormatParquet = "parquet"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotta document those somewhere eventually. Not in this PR, it's fine.

@github-actions github-actions bot mentioned this pull request Mar 25, 2025
Closed
axw
axw requested changes Mar 25, 2025
View reviewed changes
Copy link
Contributor

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, but I think we should be translating fields to semantic conventions, and considering additions to SemConv where necessary.

axw
axw reviewed Mar 27, 2025
View reviewed changes
Copy link
Contributor

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, nearly there.

...oding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/vpc_flow_log_unmarshaler.go
Comment on lines +67 to +83
var errGzipReader error
gzipReader, ok := v.gzipPool.Get().(*gzip.Reader)
if !ok {
gzipReader, errGzipReader = gzip.NewReader(bytes.NewReader(content))
} else {
errGzipReader = gzipReader.Reset(bytes.NewReader(content))
}
if errGzipReader != nil {
if gzipReader != nil {
v.gzipPool.Put(gzipReader)
}
return plog.Logs{}, fmt.Errorf("failed to decompress content: %w", errGzipReader)
}
defer func() {
_ = gzipReader.Close()
v.gzipPool.Put(gzipReader)
}()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for now, but at some point we should look at extracting this into some common code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Too many unmarshalers having this :)

...oding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/vpc_flow_log_unmarshaler.go Outdated
Comment on lines 289 to 292
case "srcaddr":
record.Attributes().PutStr("source.layer.address", value)
case "dstaddr":
record.Attributes().PutStr("destination.layer.address", value)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default format only includes srcaddr and dstaddr, and not pkt-srcaddr or pkt-dstaddr. I think in that case we should still set the standard SemConv fields where we can. Also, as mentioned in the other thread, I think if all 4 are available then we should set https://opentelemetry.io/docs/specs/semconv/attributes-registry/network/#network-peer-address depending on the traffic direction.

Unfortunately the default format doesn't include traffic direction either, otherwise I think we could use that to pick the relevant address for source.address or destination.address, and the other for network.peer.address.

The only other option I can think of is: if pkt-srcaddr and pkt-dstaddr are not present, use srcaddr and dstaddr for source.address and destination.address. May be incorrect in the presence of NAT, but we can document this and how to resolve it (i.e. by including pkt-*addr).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default format only includes srcaddr and dstaddr, and not pkt-srcaddr or pkt-dstaddr.

We aren't settling for the default format. Even a custom format is included in the first line. That's what we are following, so I think we should handle this case already.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand we'll handle all the formats, what I meant is that a user may have configured a VPC flow log with just the default format, in which case pkt-* won't be present.

axw
axw reviewed Mar 27, 2025
View reviewed changes
extension/encoding/awslogsencodingextension/internal/unmarshaler/vpc-flow-log/const.go Outdated
// protocolNames are needed to know the name of the protocol number given by the field
// protocol in a flow log record.
// See https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml.
var protocolNames = [143]string{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the indices are all correct now. https://www.iana.org/assignments/protocol-numbers/protocol-numbers.xhtml says "nsh" is 145, but we only have 143 entries.

What I was suggesting before is to use this syntax:

var protocolNames = [...]string{
  0: "hopopt",
  // etc.
  255: "reserved",
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, you are absolutely correct 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But then the easier way would be to have a map[int]string, don't you think? Otherwise we need empty values for numbers that do not map to any protocol name.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a fair tradeoff when there's only 256 entries.

Copy link
Contributor Author

@constanca-m constanca-m Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.