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

identity cache: Support looking up reserved identities #2922

Merged
merged 1 commit into from
Feb 26, 2018
Merged

identity cache: Support looking up reserved identities #2922

merged 1 commit into from
Feb 26, 2018

Conversation

tgraf
Copy link
Member

@tgraf tgraf commented Feb 23, 2018 •
edited

  • Support resolving and listing reserved identities
  • Simplify bpf/init.sh by hardcoding the reserved identities
    which are static.
  • Convert cilium identity list and cilium identity get to
    use text/tabwriter and add uniformal -o json support.
  • Always list reserved identities for cilium identity list

Example:

$ cilium identity list
ID      LABELS
4       [reserved:health]
3       [reserved:cluster]
1       [reserved:host]
2       [reserved:world]
58770   [reserved:health]
12536   [container:id.bar]
2558    [container:id.foo]

Fixes: #2857

@tgraf tgraf added kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Feb 23, 2018
@tgraf tgraf requested review from a team as code owners February 23, 2018 17:46
houndci-bot
houndci-bot reviewed Feb 23, 2018
View reviewed changes
pkg/identity/cache.go
)

// IdentityCache is a cache of identity to labels mapping
type IdentityCache map[NumericIdentity]labels.LabelArray
Copy link

Choose a reason for hiding this comment

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

type name will be used as identity.IdentityCache by other packages, and that stutters; consider calling this Cache

@tgraf
Copy link
Member Author

tgraf commented Feb 23, 2018

test-me-please

joestringer
joestringer approved these changes Feb 23, 2018
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM, but would be nice to see a green build. Current failure appears unrelated.

@joestringer
Copy link
Member

I believe that bpf policy get needs some similar treatment too, it uses client.IdentityGet() right now and this isn't providing reserved identities.

@tgraf
Copy link
Member Author

tgraf commented Feb 23, 2018 •
edited

I believe that bpf policy get needs some similar treatment too, it uses client.IdentityGet() right now and this isn't providing reserved identities.

$ sudo cilium bpf policy get 38939
LABELS (source:key[=value])   PORT/PROTO   ACTION    BYTES   PACKETS
reserved:host                 ANY          allowed   0       0
container:id.foo              ANY          allowed   0       0

Are you referring to something else?

@tgraf tgraf force-pushed the resolve-reserved-identities branch from 0164896 to 0f5b1ba Compare February 23, 2018 18:36
@tgraf
Copy link
Member Author

tgraf commented Feb 23, 2018

test-me-please

@joestringer
Copy link
Member

Are you referring to something else?

Looks like it works from your output. I had previously seen it fail to resolve id 1 in #2908.

@tgraf
Copy link
Member Author

tgraf commented Feb 23, 2018

test-me-please

@tgraf tgraf force-pushed the resolve-reserved-identities branch from 0f5b1ba to 2873de0 Compare February 23, 2018 19:30
@tgraf
Copy link
Member Author

tgraf commented Feb 23, 2018

test-me-please

@tgraf tgraf force-pushed the resolve-reserved-identities branch from 2873de0 to e1e3549 Compare February 23, 2018 21:06
@tgraf tgraf requested a review from a team as a code owner February 23, 2018 21:06
@tgraf
Copy link
Member Author

tgraf commented Feb 23, 2018

test-me-please

@tgraf tgraf force-pushed the resolve-reserved-identities branch from e1e3549 to 99e8217 Compare February 24, 2018 06:07
@tgraf tgraf requested review from a team February 24, 2018 06:07
@tgraf tgraf requested a review from a team as a code owner February 24, 2018 06:07
@aanm
Copy link
Member

aanm commented Feb 24, 2018

test-me-please

@tgraf tgraf force-pushed the resolve-reserved-identities branch from 99e8217 to aab7419 Compare February 24, 2018 16:07
@tgraf
Copy link
Member Author

tgraf commented Feb 24, 2018

test-me-please

@tgraf
Copy link
Member Author

tgraf commented Feb 24, 2018

@joestringer @ianvernon PR has changed significantly, please review again

manalibhutiyani
manalibhutiyani reviewed Feb 24, 2018
View reviewed changes
bpf/init.sh
@@ -323,7 +321,7 @@ elif [ "$MODE" = "lb" ]; then
else
echo 1 > /proc/sys/net/ipv6/conf/all/forwarding

CALLS_MAP="cilium_calls_lb_${ID}"
CALLS_MAP="cilium_calls_lb"
Copy link

Choose a reason for hiding this comment

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

Should this be CALLS_MAP="cilium_calls_lb_${ID_HOST}" or ID_WORLD?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, LB is always a single program, this was a copy&paste error but it didn't have any negative effect.

aanm
aanm reviewed Feb 24, 2018
View reviewed changes
pkg/identity/cache.go
}

if gi, ok := allocatorKey.(globalIdentity); ok {
return NewIdentity(id, gi.Labels)
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we store the updated identity from the KVStore into the cache?

Copy link
Member Author

Choose a reason for hiding this comment

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

The lookup is using the cache in the allocator but falls back to kvstore if it can't be found in the cache. I'm just moving the code here, I'm not adding or modifying code.

@tgraf
cache: Support looking up reserved identities
dc3d710 
 * Support resolving and listing reserved identities
 * Simplify bpf/init.sh by hardcoding the reserved identities
   which are static.
 * Convert `cilium identity list` and `cilium identity get` to
   use `text/tabwriter` and add uniformal `-o json` support.
 * Always list reserved identities for `cilium identity list`

Example:
```
$ cilium identity list
ID      LABELS
4       [reserved:health]
3       [reserved:cluster]
1       [reserved:host]
2       [reserved:world]
58770   [reserved:health]
12536   [container:id.bar]
2558    [container:id.foo]
```

Fixes: #2857

Signed-off-by: Thomas Graf <thomas@cilium.io>
@tgraf tgraf force-pushed the resolve-reserved-identities branch from aab7419 to dc3d710 Compare February 26, 2018 18:43
@tgraf
Copy link
Member Author

tgraf commented Feb 26, 2018

test-me-please

joestringer
joestringer approved these changes Feb 26, 2018
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM for bpf / cli.

@tgraf tgraf merged commit 1c18d1d into master Feb 26, 2018
@tgraf tgraf deleted the resolve-reserved-identities branch February 26, 2018 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm left review comments

@houndci-bot houndci-bot houndci-bot left review comments

@joestringer joestringer joestringer approved these changes

@ianvernon ianvernon ianvernon approved these changes

@manalibhutiyani manalibhutiyani manalibhutiyani approved these changes

Assignees
No one assigned
Labels
kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@tgraf @joestringer @aanm @ianvernon @houndci-bot @manalibhutiyani