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

Ldelossa/phase out sec label pt1 #25057

Merged
merged 1 commit into from Apr 26, 2023
Merged

Ldelossa/phase out sec label pt1 #25057

merged 1 commit into from Apr 26, 2023

Conversation

ldelossa
Copy link
Contributor

@ldelossa ldelossa commented Apr 22, 2023 •
edited

Currently in the datapath we swap around the terminology sec_label and identity.

As an effort to consolidate our terminology, this PR begins the phasing out of the keywords sec_label in favor of sec_identity.

An arbitrary starting point was picked, the remote_endpoint_info struct, and further renames will take place over time, as to not create too many conflicts with other PRs.

Rename the `sec_label` field in remote_endpoint_info structure to `sec_identity`

@ldelossa ldelossa requested a review from a team as a code owner April 22, 2023 01:13
@ldelossa ldelossa requested a review from aspsk April 22, 2023 01:13
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 22, 2023
aspsk
aspsk requested changes Apr 22, 2023
View reviewed changes
bpf/bpf_host.c Outdated Show resolved Hide resolved
bpf/lib/host_firewall.h Show resolved Hide resolved
@ldelossa
Copy link
Contributor Author

Marking as draft, suggestion from out of band convo is to refactor terminology from "sec_label" to "sec_identity". Making it explicit that we are using a security identity.

@ldelossa ldelossa marked this pull request as draft April 22, 2023 12:54
@ldelossa ldelossa force-pushed the ldelossa/phase-out-sec-label-pt1 branch 4 times, most recently from 56e77a4 to 7fbb773 Compare April 24, 2023 21:13
@ldelossa ldelossa added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Apr 24, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 24, 2023
@ldelossa ldelossa force-pushed the ldelossa/phase-out-sec-label-pt1 branch 5 times, most recently from 610b0c3 to 544c1ef Compare April 25, 2023 00:48
@ldelossa ldelossa marked this pull request as ready for review April 25, 2023 13:24
@ldelossa
Copy link
Contributor Author

/test

@ldelossa ldelossa requested a review from a team April 25, 2023 13:43
aspsk
aspsk approved these changes Apr 25, 2023
View reviewed changes
Copy link
Contributor

@aspsk aspsk left a comment

Choose a reason for hiding this comment

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

LGTM from technical perspective.

I am not a big fan of this _sec_ thing though. Do we have other types of identities? If we want to emphasize that this is a "Secutiry Identity" then I would rather use security_id than sec_identity.

@ldelossa
Copy link
Contributor Author

We landed on "sec_identity" since it stays close to the terminology used in our upstream documentation.

I.e. we make explicit call outs to the term "identity" in docs. For instance you can search "identity" in upstream docs and get a lot of info.

I understand this is a bit subjective, but there was some previous discussion on this naming change, and I think we could keep going back and forth on 'security_id' vs 'sec_identity' ad infinitum. My feeling is to try to ground the naming change into something tangible, like what words we use in the upstream docs the most often to explain a "concept".

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 25, 2023
@ldelossa
bpf: rename remote_endpoint_info.sec_label to sec_identity
0015581 
 In the datapath we often swap between calling the `security identity` an
`identity` and a `sec_label`.

As an effort to consolidate our terminology, begin phasing out the
`sec_label` terminology, with the first step focusing on the
`remote_endpoint_info` structure.

Rename the `sec_label` field on this structure to `sec_identity`, and also
update variables to consistently use `*_sec_identity` syntax.

Signed-off-by: Louis DeLosSantos <louis.delos@isovalent.com>
@ldelossa ldelossa force-pushed the ldelossa/phase-out-sec-label-pt1 branch from 544c1ef to 0015581 Compare April 25, 2023 15:52
@ldelossa
Copy link
Contributor Author

ldelossa commented Apr 25, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.25-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN and endpoint routes

Failure Output

FAIL: Failed to reach 192.168.56.11:80 from testclient-host-chmwl

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-5.4/135/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-5.4 so I can create one.

Then please upload the Jenkins artifacts to that issue.

@ldelossa
Copy link
Contributor Author

/test-1.25-5.4

@ldelossa
Copy link
Contributor Author

/mlh new-flake Cilium-PR-K8s-1.25-kernel-5.4

@ldelossa ldelossa merged commit 2ec713e into cilium:main Apr 26, 2023
57 checks passed
@ldelossa ldelossa deleted the ldelossa/phase-out-sec-label-pt1 branch April 26, 2023 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aspsk aspsk aspsk approved these changes

Assignees
No one assigned
Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@ldelossa @aspsk