proposal: encoding/json, encoding/xml: support zero values of structs with omitempty #11939

joeshaw · 2015-07-30T14:14:34Z

Support zero values of structs with omitempty in encoding/json and encoding/xml.

This bites people a lot, especially with time.Time. Open bugs include #4357 (which has many dups) and #10648. There may be others.

Proposal

Check for zero struct values by adding an additional case to the isEmptyValue function:

case reflect.Struct:
        return reflect.Zero(v.Type()).Interface() == v.Interface()

This will solve the vast majority of cases.

(Optional) Introduce a new encoding.IsZeroer interface, and use this to check for emptiness:

Update: I am dropping this part of the proposal, see below.

type IsZeroer interface {
        IsZero() bool
}

Visit this playground link and note that the unmarshaled time.Time value does not have a nil Location field. This prevents the reflection-based emptiness check from working. IsZero() already exists on time.Time, has the correct semantics, and has been adopted as a convention by Go code outside the standard library.

An additional check can be added to the isEmptyValue() functions before checking the value's Kind:

if z, ok := v.Interface().(encoding.IsZeroer); ok {
        return z.IsZero()
}

Compatibility

The encoding.IsZeroer interface could introduce issues with existing non-struct types that may have implemented IsZero() without consideration of omitempty. If this is undesirable, the encoding.IsZeroer interface check could be moved only within the struct case:

case reflect.Struct:
        val := v.Interface()
        if z, ok := val.(encoding.IsZeroer); ok {
                return z.IsZero()
        }
        return reflect.Zero(v.Type()).Interface() == val

Otherwise, this change is backward-compatible with existing valid uses of omitempty. Users who have applied omitempty to struct fields incorrectly will get their originally intended behavior for free.

Implementation

I (@joeshaw) have implemented and tested this change locally, and will send the CL when the Go 1.6 tree opens.

The text was updated successfully, but these errors were encountered:

gopherbot · 2015-08-25T16:00:49Z

CL https://golang.org/cl/13914 mentions this issue.

gopherbot · 2015-08-28T16:01:03Z

CL https://golang.org/cl/13977 mentions this issue.

joeshaw · 2015-09-18T13:16:41Z

The empty struct approach is implemented in CL 13914 and the IsZeroer interface is implemented in CL 13977.

In order for them to be reviewable separately they conflict a bit -- mostly in the documentation -- but I will fix for one if the other is merged.

joeshaw · 2015-10-19T18:20:42Z

In the CLs @rsc said,

I'd really like to stop adding to these packages. I think we need to leave well enough alone at some point.

I see what he's getting at. CL 13977, which implements the IsZeroer interface is clearly an enhancement and adds API to the standard library that needs to be maintained forever. So, I am abandoning that CL and that part of the proposal.

However, I still feel strongly about omitempty with empty structs, and I want to push for CL 13914 to land for Go 1.6.

I use the JSON encoding in Go a lot, as my work is mostly writing services that communicate with other services, in multiple languages, over HTTP. The fact that structs don't obey omitempty is a frequent source of confusion (see #4357 and its many dups and references, and #10648) and working around it is really annoying. Other programming languages do not conform to Go's ideal "zero value" idea, and as a result encoding a zero value is semantically different in JSON than omitting it or encoding it as null. People run into this most commonly with time.Time. (There is also the issue that decoding a zero time.Time does not result in an empty struct, see #4357 (comment) for background on that.)

I think it should be considered a bug that Go does not support omitempty for these types, and although it adds a small amount of additional code, it fixes a bug.

jeromenerf · 2016-03-27T16:21:19Z

This proposal is marked as unplanned, yet the related bug report #10648 is marked as go1.7.
Is it still being worked /thought on?

rsc · 2016-03-28T16:40:34Z

To my knowledge, it is not being worked on. Honestly this seems fairly low
priority and will likely miss Go 1.7.

On Sun, Mar 27, 2016 at 12:22 PM Jérôme Andrieux notifications@github.com
wrote:

This proposal is marked as unplanned, yet the related bug report #10648
#10648 is marked as go1.7.
Is it still being worked /thought on?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#11939 (comment)

jeromenerf · 2016-03-28T22:07:10Z

OK.

This is more of a convenience than a priority indeed.

It can be a pain point when dealing with libs that don't support "embedded structs" as pointer though.

Perelandric · 2016-03-29T23:33:01Z

I wonder if a low-impact alternative to the IsZeroer interface would be to allow one to return an error called json.CanOmit (or similar) from an implementation of the Marshaler interface. That way the dev is in control of determining what constitutes a zero value, and it doesn't impact other code.

It's not a perfect solution, since one can't add methods to types defined in another package, but this can be worked around to a degree.

Taking the time.Time example:

type MyTime struct {
  time.Time
}

// Implement the Marshaler interface
func (mt MyTime) MarshalJSON() ([]byte, error) {
  res, err := json.Marshal(mt.Time)

  if err == nil && mt.IsZero() {
    return res, json.CanOmit // Exclude zero value from fields with `omitempty`
  }
  return res, err
}

I haven't looked into implementation, but on the surface it would seem like a low-overhead solution, assuming the only work would be to check if an error returned equals json.CanOmit on fields where omitempty was included.

Using errors as a flag is not without precedent in the standard library, e.g. filepath#WalkFunc allows one to return filepath.SkipDir to skip recursion into a directory.

joeshaw · 2016-05-12T13:05:43Z

@Perelandric I mentioned a possible sentinel error value in https://golang.org/cl/13914 but I didn't get feedback on the idea or an opportunity to implement it before the Go 1.7 freeze. After Russ's comments on my original CL (and showing the unexpected difficulty in implementing this) I think that's the better way to go.

gopherbot · 2016-05-15T19:00:32Z

CL https://golang.org/cl/23088 mentions this issue.

adg · 2016-07-19T21:48:52Z

While it's clear that we can do this, it's not clear that we want to. I'd like to see a formal proposal document that weighs the advantages and drawbacks of this feature addition. In particular, I am concerned about compatibility and maintenance burden.

joeshaw · 2016-07-19T23:30:57Z

thanks Andrew. I worked on this a little bit at GopherCon. I will look into putting together a formal proposal.

albrow · 2016-08-02T23:51:46Z

@joeshaw we ran into this issue at my place of work and I'm eagerly awaiting your proposal. Feel free to contact me if you would like any help. Email is on my profile.

Perelandric · 2016-09-08T20:39:42Z

@joeshaw Is the proposal you're considering based on the sentinel object idea, or are you considering a different approach? Do you think you'll have time for this before the next release?

joeshaw · 2016-09-09T13:14:57Z

@Perelandric Yes, I think the sentinel object idea is the most straightforward way to go.

Other options include:

The IsZeroer interface (but I think this has potential backward compatibility issues)
"Backspacing" over objects that serialize to {} (but I think this requires too big a change to the JSON encoder code, and doesn't handle json.Marshaler implementers like time.Time)

I don't think I will be able to do this (proposal + implementation) before Go 1.8. If someone else wants to take it on for 1.8, I will gladly pass along my knowledge and partial implementation.

Perelandric · 2016-09-10T14:23:33Z

Thanks @joeshaw. I created an implementation using the sentinel error for the encoding/json package and will start to work on the proposal in a bit. I think I'll focus primarily on this approach.

The Marshaler interface in encoding/xml is different from that in encoding/json, and seems as though a custom zero-value can already be established without needing to return anything special. Did you find that to be true?

After I make a little more progress, I'll post a link to a branch in case you, @albrow or anyone else wishes to review and contribute.

If you have any additional thoughts or info in the meantime, please let me know. Thank you!

Perelandric · 2016-09-19T21:39:01Z

Change of heart on this. If there's resistance to adding to packages, then this won't fly. Maybe someone else wishes to advocate for this.

pschultz · 2016-12-14T21:13:10Z

Mentioned in one of the duplicate issues was the idea to let MarshalJSON return (nil, nil) to skip the field. Borrowing your earlier example:

type MyTime struct { time.Time }

func (mt MyTime) MarshalJSON() ([]byte, error) {
    if mt.IsZero() {
        return nil, nil // Exclude zero value from fields with `omitempty`
    }

    return json.Marshal(mt.Time)
}

In Go 1.7, returning nil is not a valid implementation for MarshalJSON and leads to "unexpected end of JSON input" errors. This approach doesn't require any visible change to the encoding package (not even adding an error value).

For what it's worth, I just intuitively wrote a MarshalJSON method like that, expecting a field to be omitted from the JSON output.

joeshaw · 2016-12-14T21:44:56Z

@pschultz That approach seems reasonable to me, but it can't be used with omitempty.

The reason you get that error is because the JSON encoder checks for the validity of the JSON coming out of MarshalJSON and the result of returning a nil byte slice is (something like) "key":,. If returning a nil byte slice indicated that it should be omitted that'd be a different way to omit something from being encoded in the JSON than omitempty. (That might be fine, it seems ok to me.)

The benefit of the error value is that it could fit in with the existing omitempty because you'd return something like []byte(""), ErrOmitEmpty and it'd obey omitempty yet still return a valid value ("") if not set.

Perelandric · 2016-12-14T21:58:52Z

@pschultz: A nil return wouldn't be able to be used as a flag for omitempty without causing undesired errors when omitempty is not present, since the implementation of MarshalJSON doesn't know when omitempty is actually there.

I don't know if having nil as an alternative to omitempty would be the best either. Seems like the user of a type should be the one deciding when it is omitted. IMO, the implementation of MarshalJSON should always return some useful representation, or an error when that's impossible.

Fixes - linkerd/linkerd2#2962 - linkerd/linkerd2#2545 ### Problem Field omissions for workload objects are not respected while marshaling to JSON. ### Solution After digging a bit into the code, I came to realize that while marshaling, workload objects have empty structs as values for various fields which would rather be omitted. As of now, the standard library`encoding/json` does not support zero values of structs with the `omitemty` tag. The relevant issue can be found [here](golang/go#11939). To tackle this problem, the object declaration should have _pointer-to-struct_ as a field type instead of _struct_ itself. However, this approach would be out of scope as the workload object declaration is handled by the k8s library. I was able to find a drop-in replacement for the `encoding/json` library which supports zero value of structs with the `omitempty` tag. It can be found [here](https://github.com/clarketm/json). I have made use of this library to implement a simple filter like functionality to remove empty tags once a YAML with empty tags is generated, hence leaving the previously existing methods unaffected Signed-off-by: Mayank Shah <mayankshah1614@gmail.com>

* WIP: CNI Plugin (#2071) * Export RootOptions and BuildFirewallConfiguration so that the cni-plugin can use them. * Created the cni-plugin based on istio-cni implementation * Create skeleton files that need to be filled out. * Create the install scripts and finish up plugin to write iptables * Added in an integration test around the install_cni.sh and updated the script to handle the case where it isn't the only plugin. Removed the istio kubernetes.go file in favor of pkg/k8s; initial usage of this package; found and fixed the typo in the ClusterRole and ClusterRoleBinding; found the docker-build-cni-plugin script * Corrected an incorrect name in the docker build file for cni-plugin * Rename linkerd2-cni to linkerd-cni * Fixup Dockerfile and clean up code a bit as well as logging statements. * Update Gopkg.lock after master merge. * Update test file to remove temporary tag. * Fixed the command to run during the test while building up the docker run. * Added attributions to applicable files; in the test file, use a different container for each test scenario and also print the docker logs to stdout when there is an error; * Add the --no-init-container flag to install and inject. This flag will not output the initContainer and will add an annotation assuming that the cni will be used in this case. * Update .travis.yml to build the cni-plugin docker image before running the tests. * Workaround golint warnings. * Create a new command to install the linkerd-cni plugin. * Add the --no-init-container option to linkerd inject * Use the setup ip tables annotation during the proxy auto inject webhook prevent/allow addition of an init container; move cni-plugin tests to the integration-test section of travis * gate the cni-plugin tests with the -integration-tests flag; remove unnecessary deployment .yaml file. * Incorporate PR Cleanup suggestions. * Remove the SetupIPTablesLabel annotation and use config flags and the presence of the init container to determine whether the cni-plugin writes ip tables. * Fix a logic bug in the cni-plugin code that prevented the iptables from being written; Address PR comments; make tests pass. * Update go deps shas * Changed the single file install-cni plugin filename to be .conf vs .conflist; Incorporated latest PR comments around spacing with the new renderer among others. * Fix an issue with renaming .conf to .conflist when needed. * Renamed some of the variables to try to make it more clear what is going on. * Address final PR comments. * Hide cni flags for the time being. Signed-off-by: Cody Vandermyn <cody.vandermyn@nordstrom.com> * Add support for timeouts in service profiles (#2149) Fixes #2042 Adds a new field to service profile routes called `timeout`. Any requests to that route which take longer than the given timeout will be aborted and a 504 response will be returned instead. If the timeout field is not specified, a default timeout of 10 seconds is used. Signed-off-by: Alex Leong <alex@buoyant.io> * Added flags to allow further configuration of destination cni bin and cni conf directories; fixed up spacing in template. (#2181) Signed-off-by: Cody Vandermyn <cody.vandermyn@nordstrom.com> * Introduce go generate to embed static templates (#2189) # Problem In order to switch Linkerd template rendering to use `.yaml` files, static assets must be bundled in the Go binary for use by `linkerd install`. # Solution The solution should not affect the local development process of building and testing. [vfsgen](https://github.com/shurcooL/vfsgen) generates Go code that statically implements the provided `http.FileSystem`. Paired with `go generate` and Go [build tags](https://golang.org/pkg/go/build/), we can continue to use the template files on disk when developing with no change required. In `!prod` Go builds, the `cli/static/templates.go` file provides a `http.FileSystem` to the local templates. In `prod` Go builds, `go generate ./cli` generates `cli/static/generated_templates.gogen.go` that statically provides the template files. When built with `-tags prod`, the executable will be built with the staticlly generated file instead of the local files. # Validation The binaries were compiled locally with `bin/docker-build`. The binaries were then tested with `bin/test-run (pwd)/target/cli/darwin/linkerd`. All tests passed. No change was required to successfully run `bin/go-run cli install`. No change was required to run `bin/linkerd install`. Fixes #2153 Signed-off-by: Kevin Leimkuhler <kevin@kleimkuhler.com> * Introduce Discovery API and endpoints command (#2195) The Proxy API service lacked introspection of its internal state. Introduce a new gRPC Discovery API, implemented by two servers: 1) Proxy API Server: returns a snapshot of discovery state 2) Public API Server: pass-through to the Proxy API Server Also wire up a new `linkerd endpoints` command. Fixes #2165 Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Improve ServiceProfile validation in linkerd check (#2218) The `linkerd check` command was doing limited validation on ServiceProfiles. Make ServiceProfile validation more complete, specifically validate: - types of all fields - presence of required fields - presence of unknown fields - recursive fields Also move all validation code into a new `Validate` function in the profiles package. Validation of field types and required fields is handled via `yaml.UnmarshalStrict` in the `Validate` function. This motivated migrating from github.com/ghodss/yaml to a fork, sigs.k8s.io/yaml. Fixes #2190 * Read service profiles from client or server namespace instead of control namespace (#2200) Fixes #2077 When looking up service profiles, Linkerd always looks for the service profile objects in the Linkerd control namespace. This is limiting because service owners who wish to create service profiles may not have write access to the Linkerd control namespace. Instead, we have the control plane look for the service profile in both the client namespace (as read from the proxy's `proxy_id` field from the GetProfiles request and from the service's namespace. If a service profile exists in both namespaces, the client namespace takes priority. In this way, clients may override the behavior dictated by the service. Signed-off-by: Alex Leong <alex@buoyant.io> * Introduce golangci-lint tooling, fixes (#2239) `golangci-lint` performs numerous checks on Go code, including golint, ineffassign, govet, and gofmt. This change modifies `bin/lint` to use `golangci-lint`, and replaces usage of golint and govet. Also perform a one-time gofmt cleanup: - `gofmt -s -w controller/` - `gofmt -s -w pkg/` Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Upgrade Spinner to fix race condition (#2265) Fixes #2264 Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io> * Generate CLI docs for usage by the website (#2296) * Generate CLI docs for usage by the website * Update description to match existing commands * Remove global * Bump base Docker images (#2241) - `debian:jessie-slim` -> `stretch-20190204-slim` - `golang:1.10.3` -> `1.11.5` - `gcr.io/linkerd-io/base:2017-10-30.01` -> `2019-02-19.01` - bump `golangci-lint` to 1.15.0 - use `GOCACHE` in travis Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Enable `unused` linter (#2357) `unused` checks Go code for unused constants, variables, functions, and types. Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io> * lint: Enable goconst (#2365) goconst finds repeated strings that could be replaced by a constant: https://github.com/jgautheron/goconst Part of #217 Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Authorization-aware control-plane components (#2349) The control-plane components relied on a `--single-namespace` param, passed from `linkerd install` into each individual component, to determine which namespaces they were authorized to access, and whether to support ServiceProfiles. This command-line flag was redundant given the authorization rules encoded in the parent `linkerd install` output, via [Cluster]Role[Binding]s. Modify the control-plane components to query Kubernetes at startup to determine which namespaces they are authorized to access, and whether ServiceProfile support is available. This allows removal of the `--single-namespace` flag on the components. Also update `bin/test-cleanup` to cleanup the ServiceProfile CRD. TODO: - Remove `--single-namespace` flag on `linkerd install`, part of #2164 Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Wire up stats for Jobs (#2416) Support for Jobs in stat/tap/top cli commands Part of #2007 Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * Injection consolidation (#2334) - Created the pkg/inject package to hold the new injection shared lib. - Extracted from `/cli/cmd/inject.go` and `/cli/cmd/inject_util.go` the core methods doing the workload parsing and injection, and moved them into `/pkg/inject/inject.go`. The CLI files should now deal only with strictly CLI concerns, and applying the json patch returned by the new lib. - Proceeded analogously with `/cli/cmd/uninject.go` and `/pkg/inject/uninject.go`. - The `InjectReport` struct and helping methods were moved into `/pkg/inject/report.go` - Refactored webhook to use the new injection lib - Removed linkerd-proxy-injector-sidecar-config ConfigMap - Added the ability to add pod labels and annotations without having to specify the already existing ones Fixes #1748, #2289 Signed-off-by: Alejandro Pedraza <alejandro.pedraza@gmail.com> * Bump Prometheus client to v0.9.2 (#2388) We were depending on an untagged version of prometheus/client_golang from Feb 2018. This bumps our dependency to v0.9.2, from Dec 2018. Also, this is a prerequisite to #1488. Signed-off-by: Andrew Seigner <siggy@buoyant.io> * add preStop and change sleep command; update yaml spacing (#2441) Signed-off-by: Cody Vandermyn <cody.vandermyn@nordstrom.com> * Remove `--tls=optional` and `linkerd-ca` (#2515) The proxy's TLS implementation has changed to use a new _Identity_ controller. In preparation for this, the `--tls=optional` CLI flag has been removed from install and inject; and the `ca` controller has been deleted. Metrics and UI treatments for TLS have **not** been removed, as they will continue to be valuable for the new Identity system. With the removal of the old identity scheme, the Destination service's proxy ID field is now set with an opaque string (e.g. `ns:emojivoto`) to enable locality awareness. * Introduce the Identity controller implementation (#2521) This change introduces a new Identity service implementation for the `io.linkerd.proxy.identity.Identity` gRPC service. The `pkg/identity` contains a core, abstract implementation of the service (generic over both the CA and (Kubernetes) Validator interfaces). `controller/identity` includes a concrete implementation that uses the Kubernetes TokenReview API to validate serviceaccount tokens when issuing certificates. This change does **NOT** alter installation or runtime to include the identity service. This will be included in a follow-up. * config: Store install parameters with global config (#2577) When installing Linkerd, a user may override default settings, or may explicitly configure defaults. Consider install options like `--ha --controller-replicas=4` -- the `--ha` flag sets a new default value for the controller-replicas, and then we override it. When we later upgrade this cluster, how can we know how to configure the cluster? We could store EnableHA and ControllerReplicas configurations in the config, but what if, in a later upgrade, the default value changes? How can we know whether the user specified an override or just used the default? To solve this, we add an `Install` message into a new config. This message includes (at least) the CLI flags used to invoke install. upgrade does not specify defaults for install/proxy-options fields and, instead, uses the persisted install flags to populate default values, before applying overrides from the upgrade invocation. This change breaks the protobuf compatibility by altering the `installation_uuid` field introduced in https://github.com/linkerd/linkerd2/commit/9c442f688575c3ee0261facc7542aa490b89c6cf. Because this change was not yet released (even in an edge release), we feel that it is safe to break. Fixes https://github.com/linkerd/linkerd2/issues/2574 * Add validation webhook for service profiles (#2623) Add validation webhook for service profiles Fixes #2075 Todo in a follow-up PRs: remove the SP check from the CLI check. Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io> * Switch UUID implementation (#2667) The UUID implementation we use to generate install IDs is technically not random enough for secure uses, which ours is not. To prevent security scanners like SNYK from flagging this false-positive, let's just switch to the other UUID implementation (Already in our dependencies). * Don't use spinner in cli when run without a tty (#2716) In some non-tty environments, the `linkerd check` spinner can render unexpected control characters. Disable the spinner when run without a tty. Fixes #2700 Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Consolidate k8s APIs (#2747) Numerous codepaths have emerged that create k8s configs, k8s clients, and make k8s api requests. This branch consolidates k8s client creation and APIs. The primary change migrates most codepaths to call `k8s.NewAPI` to instantiate a `KubernetesAPI` struct from `pkg`. `KubernetesAPI` implements the `kubernetes.Interface` (clientset) interface, and also persists a `client-go` `rest.Config`. Specific list of changes: - removes manual GET requests from `k8s.KubernetesAPI`, in favor of clientsets - replaces most calls to `k8s.GetConfig`+`kubernetes.NewForConfig` with a single `k8s.NewAPI` - introduces a `timeout` param to `k8s.NewAPI`, currently only used by healthchecks - removes `NewClientSet` in `controller/k8s/clientset.go` in favor of `k8s.NewAPI` - removes `httpClient` and `clientset` from `HealthChecker`, use `KubernetesAPI` instead Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Introduce k8s apiextensions support (#2759) CustomResourceDefinition parsing and retrieval is not available via client-go's `kubernetes.Interface`, but rather via a separate `k8s.io/apiextensions-apiserver` package. Introduce support for CustomResourceDefintion object parsing and retrieval. This change facilitates retrieval of CRDs from the k8s API server, and also provides CRD resources as mock objects. Also introduce a `NewFakeAPI` constructor, deprecating `NewFakeClientSets`. Callers need no longer be concerned with discreet clientsets (for k8s resources vs. CRDs vs. (eventually) ServiceProfiles), and can instead use the unified `KubernetesAPI`. Part of #2337, in service to multi-stage check. Signed-off-by: Andrew Seigner <siggy@buoyant.io> * lower the log level of the linkerd-cni output (#2787) Signed-off-by: Cody Vandermyn <cody.vandermyn@nordstrom.com> * Split proxy-init into separate repo (#2824) Split proxy-init into separate repo Fixes #2563 The new repo is https://github.com/linkerd/linkerd2-proxy-init, and I tagged the latest there `v1.0.0`. Here, I've removed the `/proxy-init` dir and pinned the injected proxy-init version to `v1.0.0` in the injector code and tests. `/cni-plugin` depends on proxy-init, so I updated the import paths there, and could verify CNI is still working (there is some flakiness but unrelated to this PR). For consistency, I added a `--init-image-version` flag to `linkerd inject` along with its corresponding override config annotation. Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io> * Refactor destination service (#2786) This is a major refactor of the destination service. The goals of this refactor are to simplify the code for improved maintainability. In particular: * Remove the "resolver" interfaces. These were a holdover from when our decision tree was more complex about how to handle different kinds of authorities. The current implementation only accepts fully qualified kubernetes service names and thus this was an unnecessary level of indirection. * Moved the endpoints and profile watchers into their own package for a more clear separation of concerns. These watchers deal only in Kubernetes primitives and are agnostic to how they are used. This allows a cleaner layering when we use them from our gRPC service. * Renamed the "listener" types to "translator" to make it more clear that the function of these structs is to translate kubernetes updates from the watcher to gRPC messages. Signed-off-by: Alex Leong <alex@buoyant.io> * Add support for TrafficSplits (#2897) Add support for querying TrafficSplit resources through the common API layer. This is done by depending on the TrafficSplit client bindings from smi-sdk-go. Signed-off-by: Alex Leong <alex@buoyant.io> * Add traffic splitting to destination profiles (#2931) This change implements the DstOverrides feature of the destination profile API (aka traffic splitting). We add a TrafficSplitWatcher to the destination service which watches for TrafficSplit resources and notifies subscribers about TrafficSplits for services that they are subscribed to. A new TrafficSplitAdaptor then merges the TrafficSplit logic into the DstOverrides field of the destination profile. Signed-off-by: Alex Leong <alex@buoyant.io> * Add prometheus metrics for watchers (#3022) To give better visibility into the inner workings of the kubernetes watchers in the destination service, we add some prometheus metrics. Signed-off-by: Alex Leong <alex@buoyant.io> * Introduce Go modules support (#2481) The repo relied on `dep` for managing Go dependencies. Go 1.11 shipped with Go modules support. Go 1.13 will be released in August 2019 with module support enabled by default, deprecating GOPATH. This change replaces `dep` with Go modules for dependency management. All scripts, including Docker builds and ci, should work without any dev environment changes. To execute `go` commands directly during development, do one of the following: 1. clone this repo outside of `GOPATH`; or 2. run `export GO111MODULE=on` Summary of changes: - Docker build scripts and ci set `-mod=readonly`, to ensure dependencies defined in `go.mod` are exactly what is used for the builds. - Dependency updates to `go.mod` are accomplished by running `go build` and `go test` directly. - `bin/go-run`, `bin/build-cli-bin`, and `bin/test-run` set `GO111MODULE=on`, permitting usage inside and outside of GOPATH. - `gcr.io/linkerd-io/go-deps` tags hashed from `go.mod`. - `bin/update-codegen.sh` still requires running from GOPATH, instructions added to BUILD.md. Fixes #1488 Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Introduce `linkerd --as` flag for impersonation (#3173) Similar to `kubectl --as`, global flag across all linkerd subcommands which sets a `ImpersonationConfig` in the Kubernetes API config. Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Check in gen deps (#3245) Go dependencies which are only used by generated code had not previously been checked into the repo. Because `go generate` does not respect the `-mod=readonly` flag, running `bin/linkerd` will add these dependencies and dirty the local repo. This can interfere with the way version tags are generated. To avoid this, we simply check these deps in. Note that running `go mod tidy` will remove these again. Thus, it is not recommended to run `go mod tidy`. Signed-off-by: Alex Leong <alex@buoyant.io> * Add a flag to install-cni command to configure iptables wait flag (#3066) Signed-off-by: Charles Pretzer <charles@buoyant.io> * Update CNI integration tests (#3273) Followup to #3066 Signed-off-by: Alejandro Pedraza <alejandro@buoyant.io> * Merge the CLI 'installValues' type with Helm 'Values' type (#3291) * Rename template-values.go * Define new constructor of charts.Values type * Move all Helm values related code to the pkg/charts package * Bump dependency * Use '/' in filepath to remain compatible with VFS requirement * Add unit test to verify Helm YAML output * Alejandro's feedback * Add unit test for Helm YAML validation (HA) Signed-off-by: Ivan Sim <ivan@buoyant.io> * Require go 1.12.9 for controller builds (#3297) Netflix recently announced a security advisory that identified several Denial of Service attack vectors that can affect server implementations of the HTTP/2 protocol, and has issued eight CVEs. [1] Go is affected by two of the vulnerabilities (CVE-2019-9512 and CVE-2019-9514) and so Linkerd components that serve HTTP/2 traffic are also affected. [2] These vulnerabilities allow untrusted clients to allocate an unlimited amount of memory, until the server crashes. The Kubernetes Product Security Committee has assigned this set of vulnerabilities with a CVSS score of 7.5. [3] [1] https://github.com/Netflix/security-bulletins/blob/master/advisories/third-party/2019-002.md [2] https://golang.org/doc/devel/release.html#go1.12 [3] https://www.first.org/cvss/calculator/3.0#CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H * Remove broken thrift dependency (#3370) The repo depended on a (recently broken) thrift package: ``` github.com/linkerd/linkerd2 -> contrib.go.opencensus.io/exporter/ocagent@v0.2.0 -> go.opencensus.io@v0.17.0 -> git.apache.org/thrift.git@v0.0.0-20180902110319-2566ecd5d999 ``` ... via this line in `controller/k8s`: ```go _ "k8s.io/client-go/plugin/pkg/client/auth" ``` ...which created a dependency on go.opencensus.io: ```bash $ go mod why go.opencensus.io ... github.com/linkerd/linkerd2/controller/k8s k8s.io/client-go/plugin/pkg/client/auth k8s.io/client-go/plugin/pkg/client/auth/azure github.com/Azure/go-autorest/autorest github.com/Azure/go-autorest/tracing contrib.go.opencensus.io/exporter/ocagent go.opencensus.io ``` Bump contrib.go.opencensus.io/exporter/ocagent from `v0.2.0` to `v0.6.0`, creating this new dependency chain: ``` github.com/linkerd/linkerd2 -> contrib.go.opencensus.io/exporter/ocagent@v0.6.0 -> google.golang.org/api@v0.7.0 -> go.opencensus.io@v0.21.0 ``` Bumping our go.opencensus.io dependency from `v0.17.0` to `v0.21.0` pulls in this commit: https://github.com/census-instrumentation/opencensus-go/commit/ed3a3f0bf00d34af1ca7056123dae29672ca3b1a#diff-37aff102a57d3d7b797f152915a6dc16 ...which removes our dependency on github.com/apache/thrift Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Decrease proxy and web Docker image sizes (#3384) The `proxy` and `web` Docker images were 161MB and 186MB, respectively. Most of the space was tools installed into the `linkerd.io/base` image. Decrease `proxy` and `web` Docker images to 73MB and 90MB, respectively. Switch these images to be based off of `debian:stretch-20190812-slim`. Also set `-ldflags "-s -w"` for `proxy-identity` and `web`. Modify `linkerd.io/base` to also be based off of `debian:stretch-20190812-slim`, update tag to `2019-09-04.01`. Fixes #3383 Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Bump proxy-init to 1.2.0 (#3397) Pulls in latest proxy-init: https://github.com/linkerd/linkerd2-proxy-init/releases/tag/v1.2.0 This also bumps a dependency on cobra, which provides more complete zsh completion. Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Update to client-go v12.0.0, forked stern (#3387) The repo depended on an old version of client-go. It also depended on stern, which itself depended on an old version of client-go, making client-go upgrade non-trivial. Update the repo to client-go v12.0.0, and also replace stern with a fork. This fork of stern includes the following changes: - updated to use Go Modules - updated to use client-go v12.0.0 - fixed log line interleaving: - https://github.com/wercker/stern/issues/96 - based on: - https://github.com/oandrew/stern/commit/8723308e46b408e239ce369ced12706d01479532 Fixes #3382 Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Trace Control Plane components using OC (#3461) * add exporter config for all components Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * add cmd flags wrt tracing Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * add ochttp tracing to web server Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * add flags to the tap deployment Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * add trace flags to install and upgrade command Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * add linkerd prefix to svc names Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * add ochttp trasport to API Internal Client Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * fix goimport linting errors Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * add ochttp handler to tap http server Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * review and fix tests Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * update test values Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * use common template Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * update tests Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * use Initialize Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * fix sample flag Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * add verbose info reg flags Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * Update base docker image to debian latest stable: buster (#3438) * Update base docker image to debian latest stable: buster Signed-off-by: Charles Pretzer <charles@buoyant.io> * Update all files to use buster image * Revert "Trace Control Plane components using OC (#3461)" (#3484) This reverts commit eaf7460448e33e229d5b5996aafcafe1dbf225e2. This is a temporary revert of #3461 while we sort out some details of how this should configured and how it should interact with configuring a trace collector on the Linkerd proxy. We will reintroduce this change once the config plan is straightened out. Signed-off-by: Alex Leong <alex@buoyant.io> * Revert upgrade to buster based on CNI test failure after merge (#3486) * Add TapEvent headers and trailers to the tap protobuf (#3410) ### Motivation In order to expose arbitrary headers through tap, headers and trailers should be read from the linkerd2-proxy-api `TapEvent`s and set in the public `TapEvent`s. This change should have no user facing changes as it just prepares the events for JSON output in linkerd/linkerd2#3390 ### Solution The public API has been updated with a headers field for `TapEvent_Http_RequestInit_` and `TapEvent_Http_ResponseInit_`, and trailers field for `TapEvent_Http_ResponseEnd_`. These values are set by reading the corresponding fields off of the proxy's tap events. The proto changes are equivalent to the proto changes proposed in linkerd/linkerd2-proxy-api#33 Closes #3262 Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com> * Switch from using golangci fmt to using goimports (#3555) CI currently enforcing formatting rules by using the fmt linter of golang-ci-lint which is invoked from the bin/lint script. However it doesn't seem possible to use golang-ci-lint as a formatter, only as a linter which checks formatting. This means any formatter used by your IDE or invoked manually may or may not use the same formatting rules as golang-ci-lint depending on which formatter you use and which specific revision of that formatter you use. In this change we stop using golang-ci-lint for format checking. We introduce `tools.go` and add goimports to the `go.mod` and `go.sum` files. This allows everyone to easily get the same revision of goimports by running `go install -mod=readonly golang.org/x/tools/cmd/goimports` from inside of the project. We add a step in the CI workflow that uses goimports via the `bin/fmt` script to check formatting. Some shell gymnastics were required in the `bin/fmt` script to work around some limitations of `goimports`: * goimports does not have a built-in mechanism for excluding directories, and we need to exclude the vendor director as well as the generated Go sources * goimports returns a 0 exit code, even when formatting errors are detected Signed-off-by: Alex Leong <alex@buoyant.io> * Trace Control plane Components with OC (#3495) * add trace flags and initialisation * add ocgrpc handler to newgrpc * add ochttp handler to linkerd web * add flags to linkerd web * add ochttp handler to prometheus handler initialisation * add ochttp clients for components * add span for prometheus query * update godep sha * fix reviews * better commenting * add err checking * remove sampling * add check in main * move to pkg/trace Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * Add APIService fake clientset support (#3569) The `linkerd upgrade --from-manifests` command supports reading the manifest output via `linkerd install`. PR #3167 introduced a tap APIService object into `linkerd install`, but the manifest-reading code in fake.go was never updated to support this new object kind. Update the fake clientset code to support APIService objects. Fixes #3559 Signed-off-by: Andrew Seigner <siggy@buoyant.io> * Cert manager support (#3600) * Add support for --identity-issuer-mode flag to install cmd * Change flag to be a bool * Read correct data form identity when external issuer is used * Add ability for identity service to dynamically reload certs * Fix failing tests * Minor refactor * Load trust anchors from identity issuer secret * Make identity service actually watch for issuer certs updates * Add some testing around cmd line identity options validation * Add tests ensuring that identity service loads issuer * Take into account external-issuer flag during upgrade + tests * Fix failing upgrade test * Address initial review feedback * Address further review feedback on cli and helm * Do not persist --identity-external-issuer * Some improvements to identitiy service * Bring back persistane of external issuer flag * Address more feedback * Update dockerfiles shas * Publishing k8s events on issuer certs rotation * Ensure --ignore-cluster+external issuer is not supported * Update go-deps shas * Transition to identity issuer scheme based configuration * Use k8s consts for secret file names Signed-off-by: zaharidichev <zaharidichev@gmail.com> * Upgrade go to 1.13.4 (#3702) Fixes #3566 As explained in #3566, as of go 1.13 there's a strict check that ensures a dependency's timestamp matches it's sha (as declared in go.mod). Our smi-sdk dependency has a problem with that that got resolved later on, but more work would be required to upgrade that dependency. In the meantime a quick pair of replace statements at the bottom of go.mod fix the issue. * Removed calico logutils dependency, incompatible with go 1.13 (#3763) * Removed calico logutils dependency, incompatible with go 1.13 Fixes #1153 Removed dependency on `github.com/projectcalico/libcalico-go/lib/logutils` because it has problems with go modules, as described in projectcalico/libcalico-go#1153 Not a big deal since it was only used for modifying the plugin's log format. * Move CNI template to helm (#3581) * Create helm chart for the CNI plugin Signed-off-by: zaharidichev <zaharidichev@gmail.com> * Add helm install tests for the CNI plugin Signed-off-by: zaharidichev <zaharidichev@gmail.com> * Add readme for the CNI helm chart Signed-off-by: zaharidichev <zaharidichev@gmail.com> * Fix integration tests Signed-off-by: zaharidichev <zaharidichev@gmail.com> * Remove old cni-plugin.yaml Signed-off-by: zaharidichev <zaharidichev@gmail.com> * Add trace partial template Signed-off-by: zaharidichev <zaharidichev@gmail.com> * Address more comments Signed-off-by: Zahari Dichev <zaharidichev@gmail.com> * Upgrade prometheus to v1.2.1 (#3541) Signed-off-by: Dax McDonald <dax@rancher.com> * Cache StatSummary responses in dashboard web server (#3769) Signed-off-by: Sergio Castaño Arteaga <tegioz@icloud.com> * Enable mixed configuration of skip-[inbound|outbound]-ports (#3766) * Enable mixed configuration of skip-[inbound|outbound]-ports using port numbers and ranges (#3752) * included tests for generated output given proxy-ignore configuration options * renamed "validate" method to "parseAndValidate" given mutation * updated documentation to denote inclusiveness of ranges * Updates for expansion of ignored inbound and outbound port ranges to be handled by the proxy-init rather than CLI (#3766) This change maintains the configured ports and ranges as strings rather than unsigned integers, while still providing validation at the command layer. * Bump versions for proxy-init to v1.3.0 Signed-off-by: Paul Balogh <javaducky@gmail.com> * Remove empty fields from generated configs (#3886) Fixes - https://github.com/linkerd/linkerd2/issues/2962 - https://github.com/linkerd/linkerd2/issues/2545 ### Problem Field omissions for workload objects are not respected while marshaling to JSON. ### Solution After digging a bit into the code, I came to realize that while marshaling, workload objects have empty structs as values for various fields which would rather be omitted. As of now, the standard library`encoding/json` does not support zero values of structs with the `omitemty` tag. The relevant issue can be found [here](https://github.com/golang/go/issues/11939). To tackle this problem, the object declaration should have _pointer-to-struct_ as a field type instead of _struct_ itself. However, this approach would be out of scope as the workload object declaration is handled by the k8s library. I was able to find a drop-in replacement for the `encoding/json` library which supports zero value of structs with the `omitempty` tag. It can be found [here](https://github.com/clarketm/json). I have made use of this library to implement a simple filter like functionality to remove empty tags once a YAML with empty tags is generated, hence leaving the previously existing methods unaffected Signed-off-by: Mayank Shah <mayankshah1614@gmail.com> * Add `as-group` CLI flag (#3952) Add CLI flag --as-group that can impersonate group for k8s operations Signed-off-by: Mayank Shah mayankshah1614@gmail.com * Fix CNI config parsing (#3953) This PR addreses the problem introduced after #3766. Fixes #3941 Signed-off-by: Zahari Dichev <zaharidichev@gmail.com> * Use correct go module file syntax (#4021) The correct syntax for the go module file is go MAJOR.MINOR Signed-off-by: Dax McDonald <dax@rancher.com> * Update linkerd/stern to fix go.mod parsing (#4173) ## Motivation I noticed the Go language server stopped working in VS Code and narrowed it down to `go build ./...` failing with the following: ``` ❯ go build ./... go: github.com/linkerd/stern@v0.0.0-20190907020106-201e8ccdff9c: parsing go.mod: go.mod:3: usage: go 1.23 ``` This change updates `linkerd/stern` version with changes made in linkerd/stern#3 to fix this issue. This does not depend on #4170, but it is also needed in order to completely fix `go build ./...` * Bump proxy-init to v1.3.2 (#4170) * Bump proxy-init to v1.3.2 Bumped `proxy-init` version to v1.3.2, fixing an issue with `go.mod` (linkerd/linkerd2-proxy-init#9). This is a non-user-facing fix. * Set auth override (#4160) Set AuthOverride when present on endpoints annotation Signed-off-by: Zahari Dichev <zaharidichev@gmail.com> * Upgrade to client-go 0.17.4 and smi-sdk-go 0.3.0 (#4221) Here we upgrade our dependencies on client-go to 0.17.4 and smi-sdk-go to 0.3.0. Since smi-sdk-go uses client-go 0.17.4, these upgrades must be performed simultaneously. This also requires simultaneously upgrading our dependency on linkerd/stern to a SHA which also uses client-go 0.17.4. This keeps all of our transitive dependencies synchronized on one version of client-go. This ALSO requires updating our codegen scripts to use the 0.17.4 version of code-generator and running it to generate 0.17.4 compatible generated code. I took this opportunity to update our code generation script to properly use the version of code-generater from `go.mod` rather than a hardcoded SHA. Signed-off-by: Alex Leong <alex@buoyant.io> * Upgrade to go 1.14.2 (#4278) Upgrade Linkerd's base docker image to use go 1.14.2 in order to stay modern. The only code change required was to update a test which was checking the error message of a `crypto/x509.CertificateInvalidError`. The error message of this error changed between go versions. We update the test to not check for the specific error string so that this test passes regardless of go version. Signed-off-by: Alex Leong <alex@buoyant.io> * Refactor CNI integration tests to use annotations functions (#4363) Followup to #4341 Replaced all the `t.Error`/`t.Fatal` calls in the integration tests with the new functions defined in `testutil/annotations.go` as described in #4292, in order for the errors to produce Github annotations. This piece takes care of the CNI integration test suite. This also enables the annotations for these and the general integration tests, by setting the `GH_ANNOTATIONS` environment variable in the workflows whose flakiness we're interested on catching: Kind integration, Cloud integration and Release. Re #4176 * install-cni.sh: Fix shellcheck issues (#4405) Where cat and echo are actually not needed, they have been removed. Signed-off-by: Joakim Roubert <joakim.roubert@axis.com> * Add --close-wait-timeout inject flag (#4409) Depends on https://github.com/linkerd/linkerd2-proxy-init/pull/10 Fixes #4276 We add a `--close-wait-timeout` inject flag which configures the proxy-init container to run with `privileged: true` and to set `nf_conntrack_tcp_timeout_close_wait`. Signed-off-by: Alex Leong <alex@buoyant.io> * Fix quotes in shellscripts (#4406) - Add quotes where missing, to handle whitespace & c:o. - Use single quotes for non-expansion strings. - Fix quotes were the current would cause errors. Signed-off-by: Joakim Roubert <joakim.roubert@axis.com> * Use buster for base and web images too (#4567) Requires setting iptables-legacy as the iptables provider. Signed-off-by: Joakim Roubert <joakim.roubert@axis.com> * Improve shellscript portability by using /bin/env (#4628) Using `/bin/env` increases portability for the shell scripts (and often using `/bin/env` is requested by e.g. Mac users). This would also facilitate testing scripts with different Bash versions via the Bash containers, as they have bash in `/usr/local` and not `/bin`. Using `/bin/env`, there is no need to change the script when testing. (I assume the latter was behind https://github.com/linkerd/linkerd2/pull/4593/files/c301ea214b7ccf8d74d7c41cbf8c4cc05fea7d4a#diff-ecec5e3a811f60bc2739019004fa35b0, which would not happen using `/bin/env`.) Signed-off-by: Joakim Roubert <joakimr@axis.com> * Add support for Helm configuration of per-component proxy resources requests and limits (#4226) Signed-off-by: Lutz Behnke <lutz.behnke@finleap.com> * Update proxy-api version to v0.1.13 (#4614) This update includes no API changes, but updates grpc-go to the latest release. * Upgrade generated protobuf files to v1.4.2 (#4673) Regenerated protobuf files, using version 1.4.2 that was upgraded from 1.3.2 with the proxy-api update in #4614. As of v1.4 protobuf messages are disallowed to be copied (because they hold a mutex), so whenever a message is passed to or returned from a function we need to use a pointer. This affects _mostly_ test files. This is required to unblock #4620 which is adding a field to the config protobuf. * add fish shell completion (#4751) fixes #4208 Signed-off-by: Wei Lun <weilun_95@hotmail.com> * Migrate CI to docker buildx and other improvements (#4765) * Migrate CI to docker buildx and other improvements ## Motivation - Improve build times in forks. Specially when rerunning builds because of some flaky test. - Start using `docker buildx` to pave the way for multiplatform builds. ## Performance improvements These timings were taken for the `kind_integration.yml` workflow when we merged and rerun the lodash bump PR (#4762) Before these improvements: - when merging: `24:18` - when rerunning after merge (docker cache warm): `19:00` - when running the same changes in a fork (no docker cache): `32:15` After these improvements: - when merging: `25:38` - when rerunning after merge (docker cache warm): `19:25` - when running the same changes in a fork (docker cache warm): `19:25` As explained below, non-forks and forks now use the same cache, so the important take is that forks will always start with a warm cache and we'll no longer see long build times like the `32:15` above. The downside is a slight increase in the build times for non-forks (up to a little more than a minute, depending on the case). ## Build containers in parallel The `docker_build` job in the `kind_integration.yml`, `cloud_integration.yml` and `release.yml` workflows relied on running `bin/docker-build` which builds all the containers in sequence. Now each container is built in parallel using a matrix strategy. ## New caching strategy CI now uses `docker buildx` for building the container images, which allows using an external cache source for builds, a location in the filesystem in this case. That location gets cached using actions/cache, using the key `{{ runner.os }}-buildx-${{ matrix.target }}-${{ env.TAG }}` and the restore key `${{ runner.os }}-buildx-${{ matrix.target }}-`. For example when building the `web` container, its image and all the intermediary layers get cached under the key `Linux-buildx-web-git-abc0123`. When that has been cached in the `main` branch, that cache will be available to all the child branches, including forks. If a new branch in a fork asks for a key like `Linux-buildx-web-git-def456`, the key won't be found during the first CI run, but the system falls back to the key `Linux-buildx-web-git-abc0123` from `main` and so the build will start with a warm cache (more info about how keys are matched in the [actions/cache docs](https://docs.github.com/en/actions/configuring-and-managing-workflows/caching-dependencies-to-speed-up-workflows#matching-a-cache-key)). ## Packet host no longer needed To benefit from the warm caches both in non-forks and forks like just explained, we're required to ditch doing the builds in Packet and now everything runs in the github runners VMs. As a result there's no longer separate logic for non-forks and forks in the workflow files; `kind_integration.yml` was greatly simplified but `cloud_integration.yml` and `release.yml` got a little bigger in order to use the actions artifacts as a repository for the images built. This bloat will be fixed when support for [composite actions](https://github.com/actions/runner/blob/users/ethanchewy/compositeADR/docs/adrs/0549-composite-run-steps.md) lands in github. ## Local builds You still are able to run `bin/docker-build` or any of the `docker-build.*` scripts. And to make use of buildx, run those same scripts after having set the env var `DOCKER_BUILDKIT=1`. Using buildx supposes you have installed it, as instructed [here](https://github.com/docker/buildx). ## Other - A new script `bin/docker-cache-prune` is used to remove unused images from the cache. Without that the cache grows constantly and we can rapidly hit the 5GB limit (when the limit is attained the oldest entries get evicted). - The `go-deps` dockerfile base image was changed from `golang:1.14.2` (ubuntu based) to `golang-1:14.2-alpine` also to conserve cache space. # Addressed separately in #4875: Got rid of the `go-deps` image and instead added something similar on top of all the Dockerfiles dealing with `go`, as a first stage for those Dockerfiles. That continues to serve as a way to pre-populate go's build cache, which speeds up the builds in the subsequent stages. That build should in theory be rebuilt automatically only when `go.mod` or `go.sum` change, and now we don't require running `bin/update-go-deps-shas`. That script was removed along with all the logic elsewhere that used it, including the `go_dependencies` job in the `static_checks.yml` github workflow. The list of modules preinstalled was moved from `Dockerfile-go-deps` to a new script `bin/install-deps`. I couldn't find a way to generate that list dynamically, so whenever a slow-to-compile dependency is found, we have to make sure it's included in that list. Although this simplifies the dev workflow, note that the real motivation behind this was a limitation in buildx's `docker-container` driver that forbids us from depending on images that haven't been pushed to a registry, so we have to resort to building the dependencies as a first stage in the Dockerfiles. * CI: Remove Base image (#4782) Removed the dependency on the base image, and instead install the needed packages in the Dockerfiles for debug and CNI. Also removed some obsolete info from BUILD.md Signed-off-by: Ali Ariff <ali.ariff12@gmail.com> * Build ARM docker images (#4794) Build ARM docker images in the release workflow. # Changes: - Add a new env key `DOCKER_MULTIARCH` and `DOCKER_PUSH`. When set, it will build multi-arch images and push them to the registry. See https://github.com/docker/buildx/issues/59 for why it must be pushed to the registry. - Usage of `crazy-max/ghaction-docker-buildx ` is necessary as it already configured with the ability to perform cross-compilation (using QEMU) so we can just use it, instead of manually set up it. - Usage of `buildx` now make default global arguments. (See: https://docs.docker.com/engine/reference/builder/#automatic-platform-args-in-the-global-scope) # Follow-up: - Releasing the CLI binary file in ARM architecture. The docker images resulting from these changes already build in the ARM arch. Still, we need to make another adjustment like how to retrieve those binaries and to name it correctly as part of Github Release artifacts. Signed-off-by: Ali Ariff <ali.ariff12@gmail.com> * Push docker images to ghcr.io instead of gcr.io (#4953) * Push docker images to ghcr.io instead of gcr.io The `cloud_integration.yml` and `release.yml` workflows were modified to log into ghcr.io, and remove the `Configure gcloud` step which is no longer necessary. Note that besides the changes to cloud_integration.yml and release.yml, there was a change to the upgrade-stable integration test so that we do linkerd upgrade --addon-overwrite to reset the addons settings because in stable-2.8.1 the Grafana image was pegged to gcr.io/linkerd-io/grafana in linkerd-config-addons. This will need to be mentioned in the 2.9 upgrade notes. Also the egress integration test has a debug container that now is pegged to the edge-20.9.2 tag. Besides that, the other changes are just a global search and replace (s/gcr.io\/linkerd-io/ghcr.io\/linkerd/). * CNI: Use skip ports configuration in CNI (#4974) * CNI: Use skip ports configuration in CNI This PR updates the install and `cmdAdd` workflow (which is called for each new Pod creation) to retrieve and set the configured Skip Ports. This also updates the `cmdAdd` workflow to check if the new pod is a control plane Pod, and adds `443` to OutBoundSkipPort so that 443 (used with k8s API) is skipped as it was causing errors because a resolve lookup was happening for them which is not intended. * Bump k8s client-go to v0.19.2 (#5002) Fixes #4191 #4993 This bumps Kubernetes client-go to the latest v0.19.2 (We had to switch directly to 1.19 because of this issue). Bumping to v0.19.2 required upgrading to smi-sdk-go v0.4.1. This also depends on linkerd/stern#5 This consists of the following changes: - Fix ./bin/update-codegen.sh by adding the template path to the gen commands, as it is needed after we moved to GOMOD. - Bump all k8s related dependencies to v0.19.2 - Generate CRD types, client code using the latest k8s.io/code-generator - Use context.Context as the first argument, in all code paths that touch the k8s client-go interface Signed-off-by: Tarun Pothulapati <tarunpothulapati@outlook.com> * Updated debian image tags (#5249) Signed-off-by: Agnivesh Adhikari <agnivesh.adhikari@gmail.com> * Update debian base images to buster-20210208-slim (#5750) Before the upcoming stable release, we should update our base images to use the most recent Debian images to pick up any security fixes that may have been addressed. This change updates all o four debian images to use the `buster-20210208-slim` tag. * Update Go to 1.14.15 (#5751) The Go-1.14 release branch includes a number of important updates. This change updates our containers' base image to the latest release, 1.14.15 See linkerd/linkerd2-proxy-init#32 Fixes #5655 * docker: Access container images via cr.l5d.io (#5756) We've created a custom domain, `cr.l5d.io`, that redirects to `ghcr.io` (using `scarf.sh`). This custom domain allows us to swap the underlying container registry without impacting users. It also provides us with important metrics about container usage, without collecting PII like IP addresses. This change updates our Helm charts and CLIs to reference this custom domain. The integration test workflow now refers to the new domain, while the release workflow continues to use the `ghcr.io/linkerd` registry for the purpose of publishing images. * cni: add ConfigureFirewall error propagation (#5811) This change adds error propagation for the CNI's ADD command so that any failures in the `ConfigureFirewall` function to configure the Pod's iptables can be bubbled up to be logged and handled. Fixes #5809 Signed-off-by: Frank Gu <frank@voiceflow.com> * update go.mod and docker images to go 1.16.2 (#5890) * update go.mod and docker images to go 1.16.1 Signed-off-by: Dennis Adjei-Baah <dennis@buoyant.io> * update test error messages for ParseDuration * update go version to 1.16.2 * fix: issues affecting code quality (#5827) Fix various lint issues: - Remove unnecessary calls to fmt.Sprint - Fix check for empty string - Fix unnecessary calls to Printf - Combine multiple `append`s into a single call Signed-off-by: shubhendra <withshubh@gmail.com> * Update Go to 1.16.4 (#6170) Go 1.16.4 includes a fix for a denial-of-service in net/http: golang/go#45710 Go's error file-line formatting changed in 1.16.3, so this change updates tests to only do suffix matching on these error strings. * Enable readOnlyFileSystem for cni plugin chart (#6469) Increase container security by making the root file system of the cni install plugin read-only. Change the temporary directory used in the cni install script, add a writable EmptyDir volume and enable readOnlyFileSystem securityContext in cni plugin helm chart. Tested this by building the container image of the cni plugin and installed the chart onto a cluster. Logs looked the same as before this change. Fixes #6468 Signed-off-by: Gerald Pape <gerald@giantswarm.io> * Upgrade CNI to v0.8.1 (#7270) Addresses #7247 and unblocks #7094 Bumped the cni lib version in `go.mod`, which required implementing the new CHECK command through `cmdCHeck`, which for now is no-op. * build(deps): bump github.com/containernetworking/cni from 0.8.1 to 1.0.1 (#7346) * build(deps): bump github.com/containernetworking/cni from 0.8.1 to 1.0.1 Bumps [github.com/containernetworking/cni](https://github.com/containernetworking/cni) from 0.8.1 to 1.0.1. - [Release notes](https://github.com/containernetworking/cni/releases) - [Commits](https://github.com/containernetworking/cni/compare/v0.8.1...v1.0.1) --- updated-dependencies: - dependency-name: github.com/containernetworking/cni dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Alejandro Pedraza <alejandro@buoyant.io> * build: upgrade to Go 1.17 (#7371) * build: upgrade to Go 1.17 This commit introduces three changes: 1. Update the `go` directive in `go.mod` to 1.17 2. Update all Dockerfiles from `golang:1.16.2` to `golang:1.17.3` 3. Update all CI to use Go 1.17 Signed-off-by: Eng Zer Jun <engzerjun@gmail.com> * chore: run `go fmt ./...` This commit synchronizes `//go:build` lines with `// +build` lines. Reference: https://go.googlesource.com/proposal/+/master/design/draft-gobuild.md Signed-off-by: Eng Zer Jun <engzerjun@gmail.com> * bin/shellcheck-all: Add filename/shebang check (#7541) We only run shellcheck for files that contain a #!/usr/bin/env shebang with either bash or sh. If a new shellscript file is added that has the .sh extension but either lacks shebang or has something other than that, shellcheck will not be run for that file. Then there is a risk that by mistake such a file slips into the repo under the radar. This patch adds a check for all .sh files to make sure they have a corresponding shebang in order for them to be passed to shellcheck. Change-Id: I24235e672dd82c7c73df6fe6c8beda2a579bd187 Signed-off-by: Joakim Roubert <joakimr@axis.com> * Fix CNI integration test (#7660) Reverts the change made to `env_vars.sh` in #7541 That file is consumed by `docker run --env-file` which requires the old format, as documented [here](https://docs.docker.com/engine/reference/commandline/run/#set-environment-variables--e---env---env-file). Also renamed it to `env_vars.list` to have it not mistaken to be a shell target. This broke the `ARM64 integration test` as seen here: https://github.com/linkerd/linkerd2/runs/4887813913?check_suite_focus=true#step:7:34 * go: Enable `errorlint` checking (#7885) Since Go 1.13, errors may "wrap" other errors. [`errorlint`][el] checks that error formatting and inspection is wrapping-aware. This change enables `errorlint` in golangci-lint and updates all error handling code to pass the lint. Some comparisons in tests have been left unchanged (using `//nolint:errorlint` comments). [el]: https://github.com/polyfloyd/go-errorlint Signed-off-by: Oliver Gould <ver@buoyant.io> * Add `gosec` and `errcheck` lints (#7954) Closes #7826 This adds the `gosec` and `errcheck` lints to the `golangci` configuration. Most significant lints have been fixed my individual changes, but this enables them by default so that all future changes are caught ahead of time. A significant amount of these lints are been exluced by the various `exclude-rules` rules added to `.golangci.yml`. These include operations are files that generally do not fail such as `Copy`, `Flush`, or `Write`. We also choose to ignore most errors when cleaning up functions via the `defer` keyword. Aside from those, there are several other rules added that all have comments explaining why it's okay to ignore the errors that they cover. Finally, several smaller fixes in the code have been made where it seems necessary to catch errors or at least log them. Signed-off-by: Kevin Leimkuhler <kleimkuhler@icloud.com> * Update debian to bullseye (#8287) Several container images use `debian:buster-20210208-slim`. `bullseye` is now the default version (i.e., referenced by the `latest` tag). This change updates container images that use debian to reference `bullseye` instead of `buster`. The date tags have been dropped so that we pick up the latest patch version on each Linkerd release. Signed-off-by: Oliver Gould <ver@buoyant.io> * Introduce file watch to CNI installer (#8299) Introduce fs watch for cni installer Our CNI installer script is prone to race conditions, especially when a node is rebooted, or restarted. Order of configuration should not matter and our CNI plugin should attach to other plugins (i.e chain to them) or run standalone when applicable. In order to be more flexible, we introduce a filesystem watcher through inotifywait to react to changes in the cni config directory. We react to changes based on SHAs. Linkerd's CNI plugin should append configuration when at least one other file exists, but if multiple files exist, the CNI plugin should not have to make a decision on whether thats the current file to append itself to. As a result, most of the logic in this commit revolves around the assumption that whatever file we detect has been created should be injected with Linkerd's config -- the rest is up to the host. In addition, we also introduce a sleep in the cni preStop hook, changed to using bash and introduce procps to get access to ps and pgrep. Closes #8070 Signed-off-by: Matei David <matei@buoyant.io> Co-authored-by: Oliver Gould <ver@buoyant.io> Co-authored-by: Alejandro Pedraza <alejandro@buoyant.io> * Shellscript housekeeping (#8549) - Replace simple awk commands with shell built-ins - Single quotes instead of double quotes for static strings - No need for -n operator to check that variables are not empty - Use single echo calls instead of several consecutive ones - No quotes are needed for variable assignments - Use the more lightweight echo instead of printf where applicable - No need to use bash's == comparison when there is the POSIX = Signed-off-by: Joakim Roubert <joakim.roubert@axis.com> * Update Go to the latest 1.17 release (#8603) Our docker images hardcode a patch version, 1.17.3, which does not include a variety of important fixes that have been released: > go1.17.4 (released 2021-12-02) includes fixes to the compiler, linker, > runtime, and the go/types, net/http, and time packages. See the Go > 1.17.4 milestone on our issue tracker for details. > go1.17.5 (released 2021-12-09) includes security fixes to the net/http > and syscall packages. See the Go 1.17.5 milestone on our issue tracker > for details. > go1.17.6 (released 2022-01-06) includes fixes to the compiler, linker, > runtime, and the crypto/x509, net/http, and reflect packages. See the Go > 1.17.6 milestone on our issue tracker for details. > go1.17.7 (released 2022-02-10) includes security fixes to the go > command, and the crypto/elliptic and math/big packages, as well as bug > fixes to the compiler, linker, runtime, the go command, and the > debug/macho, debug/pe, and net/http/httptest packages. See the Go 1.17.7 > milestone on our issue tracker for details. > go1.17.8 (released 2022-03-03) includes a security fix to the > regexp/syntax package, as well as bug fixes to the compiler, runtime, > the go command, and the crypto/x509 and net packages. See the Go 1.17.8 > milestone on our issue tracker for details. > go1.17.9 (released 2022-04-12) includes security fixes to the > crypto/elliptic and encoding/pem packages, as well as bug fixes to the > linker and runtime. See the Go 1.17.9 milestone on our issue tracker for > details. > go1.17.10 (released 2022-05-10) includes security fixes to the syscall > package, as well as bug fixes to the compiler, runtime, and the > crypto/x509 and net/http/httptest packages. See the Go 1.17.10 milestone > on our issue tracker for details. > go1.17.11 (released 2022-06-01) includes security fixes to the > crypto/rand, crypto/tls, os/exec, and path/filepath packages, as well as > bug fixes to the crypto/tls package. See the Go 1.17.11 milestone on our > issue tracker for details. This changes our container configs to use the latest 1.17 release on each build so that these patch releases are picked up without manual intervention. Signed-off-by: Oliver Gould <ver@buoyant.io> * Fix CNI plugin event processing (#8778) The CNI plugin watches for file changes and reacts accordingly. To append our CNI plugin configuration to an existing configuration file, we keep a watch on the config file directory, and whenever a new file is created (or modified) we append to it. To avoid redundancy and infinite loops, after a file has been processed, we save its SHA in-memory. Whenever a new update is received, we calculate the file's SHA, and if it differs from the previous one, we update it (since the file hasn't been 'seen' by our script yet). The in-memory SHA is continously overridden as updates are received and processed. In our processing logic, we override the SHA only if the file exists (in short, we want to avoid processing the SHA on 'DELETE' events). However, when a different CNI plugin deletes the file, it typically re-creates it immediately after. Since we do not check for the event type and instead rely only on file existence, we end up calculating the SHA for a new file before the file has had a chance to be processed when its associated 'CREATE' event is picked up. This means that new files will essentially be skipped from being updated, since the script considers them to have been processed already (since their SHA was calculated when the previous file was deleted). This change fixes the bug by introducing a type check for the event in addition to checking the file's existence. This allows us to be sure that new files are only processed when the 'CREATE' event is picked up, ensuring we do not skip them. Signed-off-by: Matei David <matei@buoyant.io> * Bump proxy-init version to v1.6.1 (#8913) Release v1.6.1 of proxy-init adds support for iptables-nft. This change bumps up the proxy-init version used in code, chart values, and golden files. * Update go.mod dep * Update CNI plugin with new opts * Update proxy-init ref in golden files and chart values * Update policy controller CI workflow Signed-off-by: Matei David <matei@buoyant.io> * Update Go to 1.18 (#9019) Go 1.18 features a number of important chanages, notably removing client support for defunct TLS versions: https://tip.golang.org/doc/go1.18 This change updates our Go version in CI and development. Signed-off-by: Oliver Gould <ver@buoyant.io> * Allow running Linkerd CNI plugin stand-alone (#8864) This PR allows Linkerd-CNI to be called in non-chained (stand-alone) mode. Together with a separate controller https://github.com/ErmakovDmitriy/linkerd-multus-attach-operator this PR should allow to run Linkerd-CNI in Kubernetes clusters with Multus CNI. The main issue with Multus-CNI clusters is that Multus does not handle "*.conflist" CNI configuration files, so Linkerd-CNI is ignored. Please, take a look at some details in issue #8553. Short summary about the aforementioned controller: it adds Multus NetworkAttachmentDefinitions to namespaces which have special annotation `linkerd.io/multus=enabled` and patches Pod definitions with `k8s.cni.cncf.io/v1=linkerd-cni`. The result is that Linkerd-CNI binary is called by Multus with configuration from the NetworkAttachmentDefinition. For using with Openshift, one should manually annotate a namespace or a Pod with config.linkerd.io/proxy-uid annotation with some value in the allowed range, for instance: ```yaml apiVersion: v1 kind: Namespace metadata: annotations: # I used UID in the end of the range "openshift.io/sa.scc.uid-range" config.linkerd.io/proxy-uid: "1000739999" linkerd.io/inject: enabled linkerd.io/multus: enabled openshift.io/sa.scc.mcs: s0:c27,c14 openshift.io/sa.scc.supplemental-groups: 1000730000/10000 openshift.io/sa.scc.uid-range: 1000730000/10000 labels: config.linkerd.io/admission-webhooks: enabled kubernetes.io/metadata.name: emojivoto name: emojivoto ``` Signed-off-by: Dmitrii Ermakov <demonihin@gmail.com> * Remove old .conf file from CNI directory when we convert .conf file to .conflist (#9555) * Change the integration test to check that the CNI configuration directory only has a single configuration file * Change the install script to remove the old .conf file when it's rewritten into a .conflist * Replace usage of io/ioutil package (#9613) `io/ioutil` has been deprecated since go 1.16 and the linter started to…

ianlancetaylor · 2023-05-17T22:08:46Z

There are some slow-moving plans for encoding/json/v2.

dsnet · 2023-10-06T05:42:19Z

Hi all, we kicked off a discussion for a possible "encoding/json/v2" package that addresses the spirit of this proposal at least for JSON. See the "omitzero" struct tag option under the "Struct tag options" section.

thw0rted · 2024-02-12T21:04:09Z

My team inherited maintenance duties for a Go application, and we just had to fix a bug caused by the legacy code trying to use the omitempty tag on a nested (inline) struct member for JSON serialization. I've read a lot of related issues here, and started digging through this one, but Github is terrible at presenting a large number of comments on a single issue. I'm trying to understand the current state of things -- if there's a good summary somewhere in the middle of the ~200 comments on this issue, I'd appreciate a link.

As a JS developer, I can tell you that sending an empty object value ({}) over the wire is, to a first approximation, never the desired outcome. I could point to a decade (!) of issues where people are trying to emit sensible JSON. From an outsider's perspective, "all" it would take to accomplish this is to make the existing keyword do what it says on the tin, instead of having no effect (!) -- we just want the package to follow the Principle of Least Surprise. It sounds like there is some implementation detail that makes avoiding empty-object output difficult, or would incur an unacceptable performance penalty. Personally, I would rather have good output slowly than bad output fast.

ianlancetaylor added this to the Unplanned milestone Jul 30, 2015

mikioh added the Proposal label Aug 13, 2015

adg added Proposal and removed Proposal labels Sep 25, 2015

rsc modified the milestones: Proposal, Unplanned Oct 24, 2015

rsc changed the title ~~proposal: encoding: Support zero values of structs with omitempty in encoding/json and encoding/xml~~ proposal: encoding/json, encoding/xml: support zero values of structs with omitempty Oct 24, 2015

joeshaw mentioned this issue May 12, 2016

encoding/json, encoding/xml: self-defined zero types #4357

Closed

adg mentioned this issue Jul 19, 2016

encoding/json: marshal does not obey omitempty for empty structures #10648

Closed

oneumyvakin added a commit to oneumyvakin/jirardeau that referenced this issue Jan 17, 2017

Workaround for golang/go#11939

2da1033

ernado mentioned this issue Nov 7, 2022

JSON Marshal error on generated empty OptString struct ogen-go/ogen#660

Open

majewsky mentioned this issue Nov 18, 2022

add package regexpext sapcc/go-bits#55

Merged

switchupcb mentioned this issue Dec 1, 2022

specification: optional and nullable semantic conflict switchupcb/dasgo#14

Closed

Skarlso mentioned this issue Dec 23, 2022

Add the basics of a test framework open-component-model/ocm-controller#72

Merged

ggicci mentioned this issue Jan 10, 2023

The output JSON should omit Field when Field.Valid is false ggicci/patch#1

Open

michaelbeaumont mentioned this issue Jan 17, 2023

chore(MeshHTTPRoute): add header modification filters kumahq/kuma#5702

Merged

12 tasks

PumpkinSeed mentioned this issue Feb 13, 2023

proposal: encoding/json: Struct omitempty #58489

Closed

briandealwis mentioned this issue Feb 24, 2023

x/tools/gopls/internal/lsp/protocol: CodeAction.Edit is always serialized #58687

Closed

This comment was marked as off-topic.

Sign in to view

zephyrtronium mentioned this issue Mar 21, 2023

encoding/json: rendered empty struct on omitempty #59170

Closed

This comment was marked as duplicate.

Sign in to view

ocervell mentioned this issue Mar 22, 2023

Add JSON lines output edoardottt/cariddi#106

Merged

3 tasks

This comment was marked as duplicate.

Sign in to view

apparentlymart mentioned this issue Dec 4, 2023

proposal: encoding/json: allow struct encoder to handle undefined fields #64515

Open

wking mentioned this issue Dec 5, 2023

OTA-916: add SignatureStores to ClusterVersionSpec openshift/api#1678

Merged

brandtkeller mentioned this issue Mar 22, 2024

fix(generate)!: embedded optional structs changed to pointers defenseunicorns/go-oscal#170

Merged

3 tasks

gbrlcustodio mentioned this issue Mar 28, 2024

✨ support defining a dimension field as multi-valued azaurus1/go-pinot-api#126

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

proposal: encoding/json, encoding/xml: support zero values of structs with omitempty #11939

proposal: encoding/json, encoding/xml: support zero values of structs with omitempty #11939

joeshaw commented Jul 30, 2015

gopherbot commented Aug 25, 2015

gopherbot commented Aug 28, 2015

joeshaw commented Sep 18, 2015

joeshaw commented Oct 19, 2015

jeromenerf commented Mar 27, 2016

rsc commented Mar 28, 2016

jeromenerf commented Mar 28, 2016

Perelandric commented Mar 29, 2016

joeshaw commented May 12, 2016

gopherbot commented May 15, 2016

adg commented Jul 19, 2016

joeshaw commented Jul 19, 2016

albrow commented Aug 2, 2016

Perelandric commented Sep 8, 2016

joeshaw commented Sep 9, 2016

Perelandric commented Sep 10, 2016

Perelandric commented Sep 19, 2016

pschultz commented Dec 14, 2016

joeshaw commented Dec 14, 2016 •

edited

Perelandric commented Dec 14, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as duplicate.

This comment was marked as duplicate.

ianlancetaylor commented May 17, 2023

dsnet commented Oct 6, 2023

thw0rted commented Feb 12, 2024 •

edited

proposal: encoding/json, encoding/xml: support zero values of structs with omitempty #11939

proposal: encoding/json, encoding/xml: support zero values of structs with omitempty #11939

Comments

joeshaw commented Jul 30, 2015

Proposal

Compatibility

Implementation

gopherbot commented Aug 25, 2015

gopherbot commented Aug 28, 2015

joeshaw commented Sep 18, 2015

joeshaw commented Oct 19, 2015

jeromenerf commented Mar 27, 2016

rsc commented Mar 28, 2016

jeromenerf commented Mar 28, 2016

Perelandric commented Mar 29, 2016

joeshaw commented May 12, 2016

gopherbot commented May 15, 2016

adg commented Jul 19, 2016

joeshaw commented Jul 19, 2016

albrow commented Aug 2, 2016

Perelandric commented Sep 8, 2016

joeshaw commented Sep 9, 2016

Perelandric commented Sep 10, 2016

Perelandric commented Sep 19, 2016

pschultz commented Dec 14, 2016

joeshaw commented Dec 14, 2016 • edited

Perelandric commented Dec 14, 2016

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as duplicate.

This comment was marked as duplicate.

ianlancetaylor commented May 17, 2023

dsnet commented Oct 6, 2023

thw0rted commented Feb 12, 2024 • edited

joeshaw commented Dec 14, 2016 •

edited

thw0rted commented Feb 12, 2024 •

edited