Skip to content
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.

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

Refactor other parts of Mesh object to be consistent with CA #704

Merged
merged 6 commits into from
Apr 29, 2020

Conversation

jakubdyszkiewicz
Copy link
Contributor

Summary

This PR refactors other parts of the Mesh to be consistent with new CA format. It looks like this

  type: Mesh
  name: mesh1
  logging:
    defaultBackend logstash
    backends:
      - name: logstash
        type: tcp
        config:
          address: 127.0.0.1:5000
      - config:
          path: /tmp/service.log
        type: file
        name: file
  metrics:
    enabledBackend: prometheus-1
    backends:
      - name: prometheus-1
        type: prometheus
        config:
          path: /non-standard-path
          port: 1234
      - name: prometheus-2
        type: prometheus
        config:
          path: /non-standard-path
          port: 1235
  mtls:
    enabledBackend: builtin-1
    backends:
      - name: builtin-1
        type: builtin
      - name: builtin-2
        type: builtin
  tracing:
    defaultBackend: zipkin-us
    backends:
      - name: zipkin-us
        type: zipkin
        config:
          url: http://zipkin.us:8080/v1/spans
      - name: zipkin-eu
        type: zipkin
        config:
          url: http://zipkin.eu:8080/v1/spans

This format will also give us a space to introduce pluggable backends in the future (for metrics, tracing, logging).

Also as a part of this PR I removed injecting Prometheus annotations

	prometheusScrape = "prometheus.io/scrape"	
	prometheusPath   = "prometheus.io/path"	
	prometheusPort   = "prometheus.io/port"	

as it's pretty useless since we need extra tags from kuma-prometheus-sd for our dashboards.

@jakubdyszkiewicz jakubdyszkiewicz requested review from a team and lobkovilya April 28, 2020 17:12
@@ -77,22 +77,24 @@ message TracingBackend {
// Empty value defaults to 100.0%
google.protobuf.DoubleValue sampling = 2;

// Zipkin defined configuration of Zipkin tracer.
message Zipkin {
// Type of the backend (Kuma ships with 'zipkin')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please feel free to decline any of my suggestions about English.
Maybe a passive voice will sound better here: Kuma is shipped with 'zipkin'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have several expressions "Kuma ships with..." in the docs. I'm not sure which one is correct, I'll stick with the doc version for now.

if tcp.Tcp.Address == "" {
verr.AddViolation("address", "cannot be empty")
cfg := mesh_proto.TcpLoggingBackendConfig{}
if err := proto.ToTyped(cfgStr, &cfg); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

func validateLoggingTcp(cfgStr *structpb.Struct) validators.ValidationError {
	var verr validators.ValidationError
	cfg := mesh_proto.TcpLoggingBackendConfig{}
	if err := proto.ToTyped(cfgStr, &cfg); err != nil {
		verr.AddViolation("", fmt.Sprintf("could not parse config: %s", err.Error()))
		return verr
	} 

	if cfg.Address == "" {
		verr.AddViolation("address", "cannot be empty")
		return verr
	} 
	
	host, port, err := net.SplitHostPort(cfg.Address)
	if host == "" || port == "" || err != nil {
		verr.AddViolation("address", "has to be in format of HOST:PORT")
	}

	return verr
}

If I'm correct this is the same method but feels more readable to me because of less nesting. Please consider using

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 problem is that it's not the same. With your suggestion we validate regardless if the config is completely broken (not convertible to a type). I wanted to avoid that

Copy link
Contributor

Choose a reason for hiding this comment

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

But if it is broken we just return verr:

if err := proto.ToTyped(cfgStr, &cfg); err != nil {
	verr.AddViolation("", fmt.Sprintf("could not parse config: %s", err.Error()))
	return verr
} 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, I see. Ok, let me change it then

}
return verr
}

func validateZipkin(zipkin *mesh_proto.TracingBackend_Zipkin) validators.ValidationError {
func validateZipkin(cfgStr *structpb.Struct) validators.ValidationError {
Copy link
Contributor

Choose a reason for hiding this comment

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

Here I also don't insist, that's up to you, but less nesting looks better to me:

func validateZipkin(cfgStr *structpb.Struct) validators.ValidationError {
	var verr validators.ValidationError
	cfg := mesh_proto.ZipkinTracingBackendConfig{}
	if err := proto.ToTyped(cfgStr, &cfg); err != nil {
		verr.AddViolation("", fmt.Sprintf("could not parse config: %s", err.Error()))
		return verr
	}

	if cfg.ApiVersion != "" && cfg.ApiVersion != "httpJsonV1" && cfg.ApiVersion != "httpJson" && cfg.ApiVersion != "httpProto" {
		verr.AddViolation("apiVersion", fmt.Sprintf(`has invalid value. %s`, AllowedValuesHint("httpJsonV1", "httpJson", "httpProto")))
	}

	if cfg.Url == "" {
		verr.AddViolation("url", "cannot be empty")
		return verr
	} 

	uri, err := url.ParseRequestURI(cfg.Url)
	if err != nil {
		verr.AddViolation("url", "invalid URL")
	} else if uri.Port() == "" {
		verr.AddViolation("url", "port has to be explicitly specified")
	}

	return verr
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

@jakubdyszkiewicz jakubdyszkiewicz merged commit 3deffcb into master Apr 29, 2020
@jakubdyszkiewicz jakubdyszkiewicz deleted the feat/mesh-obj-refactor branch April 29, 2020 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants