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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add more metadata about the target if possible #447

Merged
merged 1 commit into from
May 31, 2022
Merged

Add more metadata about the target if possible #447

merged 1 commit into from
May 31, 2022

Conversation

geobeau
Copy link
Contributor

@geobeau geobeau commented Feb 25, 2021 •
edited
Loading

Changes proposed in this PR:

  • Add more metadata when creating a service in Consul. It adds the name of the node name (on kubernetes) and the references of the entries in the endpoints of the kubernetes service.

How I've tested this PR:

  • manually

Checklist:

  • Tests added
  • CHANGELOG entry added (HashiCorp engineers only, community PRs should not add a changelog entry)

@hashicorp-cla
Copy link

hashicorp-cla commented Feb 25, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

lkysow
lkysow reviewed Mar 1, 2021
View reviewed changes
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Can you write tests for the changes as well please.

catalog/to-consul/resource.go Outdated
Comment on lines 650 to 652
for k, v := range baseService.Meta {
r.Service.Meta[k] = v
}
Copy link
Member

Choose a reason for hiding this comment

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

Consul metadata has some specific rules that must be followed so we can't map directly:

The meta object is a map of max 64 key/values with string semantics. Key can contain only ASCII chars and no special characters (A-Z a-z 0-9 _ and -). For performance and security reasons, values as well as keys are limited to 128 characters for keys, 512 for values. This object has the same limitations as the node meta object in node definition. All those meta data can be retrieved individually per instance of the service and all the instances of a given service have their own copy of it.

From https://www.consul.io/docs/discovery/services

@lkysow lkysow added the waiting-reply Waiting on the issue creator for a response before taking further action label Apr 8, 2021
ndhanushkodi pushed a commit to ndhanushkodi/consul-k8s that referenced this pull request Jul 9, 2021
@lkysow
Merge pull request hashicorp#447 from hashicorp/create-federation-secret
6814d0e 
New create-federation-secret job
@geobeau
Copy link
Contributor Author

geobeau commented May 20, 2022

@lkysow updated the PR with your comments. Sorry for the slight delay 😄

@lkysow
Copy link
Member

lkysow commented May 24, 2022

@geobeau would you be able to rebase off of main? I'm not sure why our github checks aren't running.

@geobeau
Copy link
Contributor Author

geobeau commented May 25, 2022

Done

@geobeau
Copy link
Contributor Author

geobeau commented May 25, 2022

Maybe I can try to close the PR and repoen it if it still doesn't work

lkysow
lkysow reviewed May 25, 2022
View reviewed changes
Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

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

Looks pretty good. Don't worry about the tests, we need to fix them.

control-plane/catalog/to-consul/resource_test.go Outdated
@@ -1601,6 +1634,7 @@ func createNodes(t *testing.T, client *fake.Clientset) (*apiv1.Node, *apiv1.Node
func createEndpoints(t *testing.T, client *fake.Clientset, serviceName string, namespace string) {
node1 := nodeName1
node2 := nodeName2
target_ref := apiv1.ObjectReference{Kind: "pod", Name: "foobar"}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
target_ref := apiv1.ObjectReference{Kind: "pod", Name: "foobar"}
targetRef := apiv1.ObjectReference{Kind: "pod", Name: "foobar"}
Suggested change
target_ref := apiv1.ObjectReference{Kind: "pod", Name: "foobar"}
target_ref := apiv1.ObjectReference{Kind: "pod", Name: "foobar"}

control-plane/catalog/to-consul/resource_test.go Outdated
@@ -1612,7 +1646,7 @@ func createEndpoints(t *testing.T, client *fake.Clientset, serviceName string, n
Subsets: []apiv1.EndpointSubset{
{
Addresses: []apiv1.EndpointAddress{
{NodeName: &node1, IP: "1.1.1.1"},
{NodeName: &node1, IP: "1.1.1.1", TargetRef: &target_ref},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{NodeName: &node1, IP: "1.1.1.1", TargetRef: &target_ref},
{NodeName: &node1, IP: "1.1.1.1", TargetRef: &targetRef},
Suggested change
{NodeName: &node1, IP: "1.1.1.1", TargetRef: &target_ref},
{NodeName: &node1, IP: "1.1.1.1", TargetRef: &target_ref},

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

control-plane/catalog/to-consul/resource_test.go Outdated
@@ -1612,7 +1646,7 @@ func createEndpoints(t *testing.T, client *fake.Clientset, serviceName string, n
Subsets: []apiv1.EndpointSubset{
{
Addresses: []apiv1.EndpointAddress{
{NodeName: &node1, IP: "1.1.1.1"},
{NodeName: &node1, IP: "1.1.1.1", TargetRef: &target_ref},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{NodeName: &node1, IP: "1.1.1.1", TargetRef: &target_ref},
{NodeName: &node1, IP: "1.1.1.1", TargetRef: &targetRef},
Suggested change
{NodeName: &node1, IP: "1.1.1.1", TargetRef: &target_ref},
{NodeName: &node1, IP: "1.1.1.1", TargetRef: &target_ref},

control-plane/catalog/to-consul/resource_test.go Outdated
Comment on lines 1297 to 1298
require.NotContains(r, ConsulK8SRefValue, actual[0].Service.Meta)
require.NotContains(r, ConsulK8SRefKind, actual[0].Service.Meta)
Copy link
Member

Choose a reason for hiding this comment

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

What is this testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually it was a mistake, I redid it and added a comment. It was supposed to test that the second service didn't contain incorrect refs

control-plane/catalog/to-consul/resource_test.go Show resolved Hide resolved
lkysow
lkysow approved these changes May 31, 2022
View reviewed changes
control-plane/catalog/to-consul/resource_test.go Outdated
@@ -1601,6 +1634,7 @@ func createNodes(t *testing.T, client *fake.Clientset) (*apiv1.Node, *apiv1.Node
func createEndpoints(t *testing.T, client *fake.Clientset, serviceName string, namespace string) {
node1 := nodeName1
node2 := nodeName2
target_ref := apiv1.ObjectReference{Kind: "pod", Name: "foobar"}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
target_ref := apiv1.ObjectReference{Kind: "pod", Name: "foobar"}
targetRef := apiv1.ObjectReference{Kind: "pod", Name: "foobar"}
Suggested change
target_ref := apiv1.ObjectReference{Kind: "pod", Name: "foobar"}
target_ref := apiv1.ObjectReference{Kind: "pod", Name: "foobar"}

@lkysow lkysow merged commit 340f7ef into hashicorp:main May 31, 2022
lkysow added a commit that referenced this pull request May 31, 2022
@lkysow
Changelog for #447
a1ce4fe
david-yu pushed a commit that referenced this pull request Jun 3, 2022
@lkysow
Changelog for #447 (#1249)
7142fa9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lkysow lkysow lkysow approved these changes

Assignees
No one assigned
Labels
waiting-reply Waiting on the issue creator for a response before taking further action
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@geobeau @hashicorp-cla @lkysow