Skip to content
This repository has been archived by the owner on Jul 28, 2024. It is now read-only.

New properties API completion #403

Merged
merged 5 commits into from
Oct 16, 2023
Merged

New properties API completion #403

merged 5 commits into from
Oct 16, 2023

Conversation

boeboe
Copy link
Contributor

@boeboe boeboe commented Oct 13, 2023

Ported the work done in WASM plugins to expose the properties API.

  • Added all of the Getters in separate files
  • Added marshaling and marshaling functions in serialization.go
  • Added Istio and Envoy specific types in types.go
  • Added GetProperty functions in utils.go
  • Ported all documentation over
  • Adopted unit test input data reflecting real data (from an Istio based setup)

TBD with @mathetake @jcchavezs @nacx

  • The marshaling and unmarshaling functions should not be exclusive to properties api, as they are essential for the shared data API between different plugins as well
  • Some consideration might need to be taken on Envoy vs Istio specific logic, although this is now already cleanly separated in different files

@mathetake
Copy link
Member

The marshaling and unmarshaling functions should not be exclusive to properties api, as they are essential for the shared data API between different plugins as well

no we don't need for shared data as here we are having them only because it's the serialization format for properties vs shared data API doesn't have any format at the ABI level.

properties/pilot_test.go Outdated Show resolved Hide resolved
properties/pilot_test.go Outdated Show resolved Hide resolved
properties/types.go Outdated Show resolved Hide resolved
properties/types.go Outdated Show resolved Hide resolved
properties/types.go Outdated Show resolved Hide resolved
properties/types.go Outdated Show resolved Hide resolved
properties/types.go Outdated Show resolved Hide resolved
properties/types.go Outdated Show resolved Hide resolved
properties/types.go Outdated Show resolved Hide resolved
properties/types.go Outdated Show resolved Hide resolved
properties/types.go Outdated Show resolved Hide resolved
Comment on lines 84 to 91
const (
// InterceptionNone indicates that the workload is not using IPtables for traffic interception
None IstioTrafficInterceptionMode = iota
// InterceptionTproxy implies traffic intercepted by IPtables with TPROXY mode
Tproxy
// InterceptionRedirect implies traffic intercepted by IPtables with REDIRECT mode. This is our default mode
Redirect
)
Copy link
Member

Choose a reason for hiding this comment

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

please prefix the global "enum" by its type name as I suggested ^ to have clean namespace in the pkg

Suggested change
const (
// InterceptionNone indicates that the workload is not using IPtables for traffic interception
None IstioTrafficInterceptionMode = iota
// InterceptionTproxy implies traffic intercepted by IPtables with TPROXY mode
Tproxy
// InterceptionRedirect implies traffic intercepted by IPtables with REDIRECT mode. This is our default mode
Redirect
)
const (
// IstioTrafficInterceptionModeNone indicates that the workload is not using IPtables for traffic interception
IstioTrafficInterceptionModeNone IstioTrafficInterceptionMode = iota
// IstioTrafficInterceptionModeTproxy implies traffic intercepted by IPtables with TPROXY mode
IstioTrafficInterceptionModeTproxy
// IstioTrafficInterceptionModeRedirect implies traffic intercepted by IPtables with REDIRECT mode. This is our default mode
IstioTrafficInterceptionModeRedirect
)

@mathetake
Copy link
Member

talked offline - let's move the serde functions after landing this PR and place it under internal/serde directory so that we could reuse it later on for shared data API wrapper

@boeboe
Copy link
Contributor Author

boeboe commented Oct 16, 2023

@mathetake fixed the final linter warnings as well. if you give it a go with the changes I made based on your good feedback, we can merge.

Comment on lines +8 to +17
type EnvoyTrafficDirection int

const (
// Unspecified means that the direction is not specified.
Unspecified EnvoyTrafficDirection = iota
// Inbound means that the transport is used for incoming traffic.
Inbound
// Outbound means that the transport is used for outgoing traffic.
Outbound
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type EnvoyTrafficDirection int
const (
// Unspecified means that the direction is not specified.
Unspecified EnvoyTrafficDirection = iota
// Inbound means that the transport is used for incoming traffic.
Inbound
// Outbound means that the transport is used for outgoing traffic.
Outbound
)
type EnvoyTrafficDirection int
const (
// EnvoyTrafficDirectionUnspecified means that the direction is not specified.
EnvoyTrafficDirectionUnspecified EnvoyTrafficDirection = iota
// EnvoyTrafficDirectionInbound means that the transport is used for incoming traffic.
EnvoyTrafficDirectionInbound
// EnvoyTrafficDirectionOutbound means that the transport is used for outgoing traffic.
EnvoyTrafficDirectionOutbound
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mathetake
Copy link
Member

mathetake commented Oct 16, 2023

@boeboe you didn't resolve all my comments. I found a few places to fix, but all good

Comment on lines +86 to +95
type IstioTrafficInterceptionMode int

const (
// None indicates that the workload is not using IPtables for traffic interception.
None IstioTrafficInterceptionMode = iota
// Tproxy implies traffic intercepted by IPtables with TPROXY mode.
Tproxy
// Redirect implies traffic intercepted by IPtables with REDIRECT mode. This is our default mode.
Redirect
)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
type IstioTrafficInterceptionMode int
const (
// None indicates that the workload is not using IPtables for traffic interception.
None IstioTrafficInterceptionMode = iota
// Tproxy implies traffic intercepted by IPtables with TPROXY mode.
Tproxy
// Redirect implies traffic intercepted by IPtables with REDIRECT mode. This is our default mode.
Redirect
)
type IstioTrafficInterceptionMode int
const (
// IstioTrafficInterceptionModeNone indicates that the workload is not using IPtables for traffic interception.
IstioTrafficInterceptionModeNone IstioTrafficInterceptionMode = iota
// IstioTrafficInterceptionModeTproxy implies traffic intercepted by IPtables with TPROXY mode.
IstioTrafficInterceptionModeTproxy
// IstioTrafficInterceptionModeRedirect implies traffic intercepted by IPtables with REDIRECT mode. This is our default mode.
IstioTrafficInterceptionModeRedirect
)

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 most idiomatic Go comments are those that are clear, concise, and directly above the item they're describing. The Go community values clarity and simplicity, and the official Go documentation often uses the style seen in the first example.

Therefore, the first example is the most idiomatic:

const (
	// None indicates that the workload is not using IPtables for traffic interception.
	None IstioTrafficInterceptionMode = iota
	// Tproxy implies traffic intercepted by IPtables with TPROXY mode.
	Tproxy
	// Redirect implies traffic intercepted by IPtables with REDIRECT mode. This is our default mode.
	Redirect
)

The comments are directly above the constants they describe, and they don't repeat the constant's name, which is redundant and not necessary. The other examples are more verbose and less idiomatic.

Online examples can be found in plenty upstream well adopted modules. An example of the net/http package

https://cs.opensource.google/go/go/+/refs/tags/go1.21.3:src/net/http/server.go;l=2873

PS: IstioTrafficInterceptionModeRedirect does not make any sense, as this string would not even be useful in code nor documents (where does the type name start and end?). IstioTrafficInterceptionMode.Redirect (with the dot) would make a bit more sense, but is not as golang idiomatic as mentioned in the initial argument.

Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

After the prefix nits are resolved, will merge it

@mathetake mathetake merged commit 918caa5 into main Oct 16, 2023
14 checks passed
@mathetake mathetake deleted the properties-api branch October 16, 2023 11:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants