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

Feature Request: Support key-value attributes for services #3881

Merged
merged 6 commits into from
Mar 27, 2018

Conversation

pierresouchay
Copy link
Contributor

This is a PR for supporting #1107

It basically allow to add metadata to service instances, so the data is linked to the lifecycle of service and avoid having Hacks such as using the KV (and find a way to cleanup the KV once the service is removed) or using tags to encode data.

It does not support searching for now, but support might be added in the future.

It would be very helpful for lots of new workloads such as adding dynamic provisioning of services in load balancers.

See #1107 (comment) for examples of possible usage

@pierresouchay
Copy link
Contributor Author

Do you think this kind of PR could be merged?
I really think it is a good alternative to people encoding weird conventions in tags (not compatible with DNS)

Copy link
Contributor

@preetapan preetapan left a comment

Choose a reason for hiding this comment

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

This fell off our radar, reviewed now with some suggestions

@@ -579,6 +579,11 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re

// Get the node service.
ns := args.NodeService()
if err := structs.ValidateMetadata(ns.ServiceMeta, false); err != nil {
resp.WriteHeader(http.StatusBadRequest)
fmt.Fprint(resp, fmt.Errorf("Invalid Meta: %v", err))
Copy link
Contributor

Choose a reason for hiding this comment

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

fmt.Fprint(resp, fmt.Errorf("Invalid Service Meta: %v", err))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

Port int `json:",omitempty"`
Address string `json:",omitempty"`
EnableTagOverride bool `json:",omitempty"`
ServiceMeta map[string]string `json:",omitempty"`
Check *AgentServiceCheck
Checks AgentServiceChecks
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to prevent abusing this feature to store arbitarily long metadata values, add some validation.

There is existing logic for node metadata here https://github.com/hashicorp/consul/blob/master/agent/structs/structs.go#L306. To keep things consistent we can use the same rules for service metadata as well. You should validate it in the agent and catalog registration endpoints.

Its fine to allow the reserved consul prefix for service metadata since we don't rely on this for any internal uses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually used this exact function in agent/agent_endpoint.go:582 but I am adding another check in the catalog as well in next patch and a unit test for Catalog insertion.

Fixed Error Message when ServiceMeta is not valid

Added Unit test for adding a Service with badly formatted ServiceMeta
Copy link
Contributor Author

@pierresouchay pierresouchay left a comment

Choose a reason for hiding this comment

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

I did the change, patch as been updated with new Check for MetaData validity into catalog registration of a service

@@ -579,6 +579,11 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re

// Get the node service.
ns := args.NodeService()
if err := structs.ValidateMetadata(ns.ServiceMeta, false); err != nil {
resp.WriteHeader(http.StatusBadRequest)
fmt.Fprint(resp, fmt.Errorf("Invalid Meta: %v", err))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DONE

Port int `json:",omitempty"`
Address string `json:",omitempty"`
EnableTagOverride bool `json:",omitempty"`
ServiceMeta map[string]string `json:",omitempty"`
Check *AgentServiceCheck
Checks AgentServiceChecks
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually used this exact function in agent/agent_endpoint.go:582 but I am adding another check in the catalog as well in next patch and a unit test for Catalog insertion.

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.

2 participants