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

api: add namespace as part of endpoint external identifiers #10038

Merged
merged 2 commits into from Feb 28, 2020

Conversation

fristonio
Copy link
Member

@fristonio fristonio commented Feb 4, 2020

* Fixes #9331

Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>
@fristonio fristonio requested a review from aanm February 4, 2020 09:21
@fristonio fristonio requested review from a team as code owners February 4, 2020 09:21
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.7.0 Feb 4, 2020
@coveralls
Copy link

coveralls commented Feb 4, 2020

Coverage Status

Coverage increased (+1.005%) to 45.509% when pulling 7735103 on fristonio/fix-9331 into 1810709 on master.

@aanm aanm added this to the 1.8 milestone Feb 4, 2020
@aanm aanm added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Feb 4, 2020
@maintainer-s-little-helper
Copy link

Release note label not set, please set the appropriate release note.

@aanm aanm removed this from In progress in 1.7.0 Feb 4, 2020
@aanm aanm modified the milestones: 1.8, 1.7 Feb 18, 2020
@aanm aanm removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Feb 18, 2020
@aanm aanm added pending-review release-note/misc This PR makes changes that have no direct user impact. labels Feb 18, 2020
@aanm
Copy link
Member

aanm commented Feb 18, 2020

test-me-please

1 similar comment
@aanm
Copy link
Member

aanm commented Feb 19, 2020

test-me-please

@@ -215,6 +215,7 @@ func (e *Endpoint) GetModelRLocked() *models.Endpoint {
DockerEndpointID: e.dockerEndpointID,
DockerNetworkID: e.dockerNetworkID,
PodName: e.getK8sNamespaceAndPodName(),
Copy link
Member

Choose a reason for hiding this comment

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

The PodName already contains the namespace (getK8sNamespaceAndPodName() returns the format namespace/podname). I don't quite understand the reason to add the namespace as a separate field.

Copy link
Member

Choose a reason for hiding this comment

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

We have been unnecessarily parsing the namespace from this field in places that use the podname. That's why the reason of adding a dedicated field. This field can be misleading as it specifies that is a pod name which it isn't, it's a namespace + pod name. Maybe this PR should also populate this field with the pod name only.

Copy link
Member

@gandro gandro Feb 21, 2020

Choose a reason for hiding this comment

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

Note that the PodName field is currently parsed by Hubble, thus
removing the namespace from PodName would be a breaking API change (and therefore breaking Hubble), see:
https://github.com/cilium/hubble/blob/5d60255e1190b135959464bfac3954eaec215a51/pkg/parser/endpoint/endpoint.go#L36

It is probably something we can mitigate in Hubble (by checking for the availability of the Namespace field, and falling back to the old behavior if missing), but there might be other consumers that assume PodName contains the namespace.

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, I'm fine with changing the contents of PodName to only contain the pod name. We will just have to do the corresponding PR in Hubble ASAP.

However as the PR is right now, I don't understand what problem it actually solves. Consumers that want the pod name still have to parse the namespace away, and I'm not aware of any consumers that are only interested in the namespace.

In any case, thanks for the PR @fristonio :)

Copy link
Member

Choose a reason for hiding this comment

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

As an alternative proposal, we could deprecate the above pod-name as is (i.e. it will stick to the current ns/pod format and mention in its description that it is deprecated), and introduce k8s-pod-name and k8s-pod-namespace fields that only contain the pod name or namespace respectively.

Copy link
Member

Choose a reason for hiding this comment

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

@gandro That makes sense because we have that concept already in our API

// Kubernetes namespace name
K8sNamespace string `json:"k8s-namespace,omitempty"`
// Kubernetes pod name
K8sPodName string `json:"k8s-pod-name,omitempty"`

Maybe we should call it k8s-namespace to be similar to this field ^^^^

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, this sounds good. I will update the PR with the required changes.

@aanm aanm self-requested a review February 24, 2020 09:56
Signed-off-by: Deepesh Pathak <deepshpathak@gmail.com>
api/v1/openapi.yaml Outdated Show resolved Hide resolved
@aanm
Copy link
Member

aanm commented Feb 24, 2020

test-me-please

2 similar comments
@fristonio
Copy link
Member Author

test-me-please

@fristonio
Copy link
Member Author

test-me-please

@aanm
Copy link
Member

aanm commented Feb 25, 2020

test-me-please

@aanm aanm merged commit 6d583fd into master Feb 28, 2020
@aanm aanm added this to Merged in 1.8.0 via automation Feb 28, 2020
@aanm aanm deleted the fristonio/fix-9331 branch February 28, 2020 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

Add namespace as part of external identifies of an endpoint
5 participants