Skip to content

Store svc labels in lb, match toservices by them#2332

Merged
tgraf merged 5 commits intocilium:masterfrom
nebril:toservices-labels-match
Jan 25, 2018
Merged

Store svc labels in lb, match toservices by them#2332
tgraf merged 5 commits intocilium:masterfrom
nebril:toservices-labels-match

Conversation

@nebril
Copy link
Copy Markdown
Member

@nebril nebril commented Dec 14, 2017 •
edited
Loading

k8s watcher stores added/updated services labels in LoadBalancer.

Those labels are used by RuleTranslator to check whether the ToService
rule should end up generating ToCIDR rules for service.

fixes #1875

@nebril nebril added the area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. label Dec 14, 2017
@nebril nebril requested a review from a team as a code owner December 14, 2017 16:03
@nebril nebril requested a review from a team December 14, 2017 16:03
@nebril nebril added the wip label Dec 14, 2017
@nebril nebril requested a review from a team as a code owner December 14, 2017 16:51
@nebril nebril force-pushed the toservices-labels-match branch 2 times, most recently from 2ee2426 to 27248d7 Compare December 15, 2017 12:56
@nebril nebril added pending-review and removed wip labels Dec 15, 2017
joestringer
joestringer approved these changes Dec 15, 2017
View reviewed changes
Copy link
Copy Markdown
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.

common/ changes LGTM. I didn't look closely at k8s pieces.

aanm
aanm requested changes Dec 18, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

Discussed offline, needs to match services by namespace

@nebril nebril requested a review from a team as a code owner December 18, 2017 16:20
@nebril nebril force-pushed the toservices-labels-match branch from 243c061 to c884e2c Compare January 2, 2018 10:53
houndci-bot
houndci-bot reviewed Jan 2, 2018
View reviewed changes
Comment thread pkg/k8s/client/clientset/versioned/typed/policy/api/doc.go Outdated
Copy link
Copy Markdown

@houndci-bot houndci-bot Jan 2, 2018

Choose a reason for hiding this comment

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

package comment should be of the form "Package api ..."

Comment thread pkg/k8s/client/clientset/versioned/typed/labels/labels/doc.go Outdated
Copy link
Copy Markdown

@houndci-bot houndci-bot Jan 2, 2018

Choose a reason for hiding this comment

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

package comment should be of the form "Package labels ..."

@nebril nebril force-pushed the toservices-labels-match branch 3 times, most recently from 4e95a55 to 082b1d5 Compare January 3, 2018 10:58
aanm
aanm requested changes Jan 3, 2018
View reviewed changes
Comment thread pkg/k8s/rule_translate.go Outdated
Copy link
Copy Markdown
Member

@aanm aanm Jan 3, 2018

Choose a reason for hiding this comment

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

Put the "matching" logic of the service in a single version to avoid code repetition here and on line 90-99.

Comment thread pkg/policy/api/rule.go Outdated
Copy link
Copy Markdown
Member

@aanm aanm Jan 3, 2018

Choose a reason for hiding this comment

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

By removing k8sService json tag aren't you breaking backwards compatibility?

Copy link
Copy Markdown
Member Author

@nebril nebril Jan 11, 2018

Choose a reason for hiding this comment

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

I guess I am. Do you think it should be left there?

Copy link
Copy Markdown
Member

@aanm aanm Jan 11, 2018

Choose a reason for hiding this comment

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

Do you think it should be left there?

Yes

manalibhutiyani
manalibhutiyani reviewed Jan 5, 2018
View reviewed changes
Comment thread pkg/k8s/rule_translate_test.go Outdated
Copy link
Copy Markdown

@manalibhutiyani manalibhutiyani Jan 5, 2018

Choose a reason for hiding this comment

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

Not sure if relevant, but do you want to add checks and tests with svcLabels == nil?

Copy link
Copy Markdown
Member Author

@nebril nebril Jan 11, 2018

Choose a reason for hiding this comment

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

Thanks for the catch, but I think that empty svcLabels case is tested in TestTranslatorDirect.

@eloycoto
Copy link
Copy Markdown
Member

eloycoto commented Jan 8, 2018

Hi @nebril could you create the Ginkgo test please or create a task to make it in the near future?

Regards

@nebril nebril force-pushed the toservices-labels-match branch 5 times, most recently from 317eefc to c880df5 Compare January 15, 2018 17:39
@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jan 15, 2018

test-me-please

@nebril nebril force-pushed the toservices-labels-match branch 2 times, most recently from 60d0590 to c82fe53 Compare January 16, 2018 14:59
@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jan 16, 2018

test-me-please

@nebril nebril mentioned this pull request Jan 16, 2018
Closed
@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jan 16, 2018

@eloycoto created issue

@tgraf
Copy link
Copy Markdown
Contributor

tgraf commented Jan 17, 2018

@eloycoto created issue

No longer good enough ;-) All new code needs Ginkgo tests.

@nebril nebril force-pushed the toservices-labels-match branch from c82fe53 to 68a0b05 Compare January 17, 2018 10:03
@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jan 17, 2018

test-me-please

eloycoto
eloycoto suggested changes Jan 17, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@eloycoto eloycoto left a comment

Choose a reason for hiding this comment

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

This is going to fail in kubbernetes 1.6. You need to add CNP V1 on manifest/1.6/ folder.

Comment thread test/k8sT/Services.go Outdated
Copy link
Copy Markdown
Member

@eloycoto eloycoto Jan 17, 2018

Choose a reason for hiding this comment

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

You should add a defer function for the endpointPath and policyLabeledPath, if it fails, it'll be no deleted at all.

Copy link
Copy Markdown
Member Author

@nebril nebril Jan 19, 2018

Choose a reason for hiding this comment

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

Makes sense, thanks.

@aanm
Copy link
Copy Markdown
Member

aanm commented Jan 18, 2018

Depends on #2544

@nebril nebril force-pushed the toservices-labels-match branch 3 times, most recently from 7fc9e77 to 729fb07 Compare January 24, 2018 14:16
@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jan 24, 2018

test-me-please

@eloycoto
Copy link
Copy Markdown
Member

eloycoto commented Jan 24, 2018

test-all-ginkgo

@manalibhutiyani
Copy link
Copy Markdown

manalibhutiyani commented Jan 24, 2018

Are all review comments addressed? @nebril : Is this ready to be merged?

@aanm aanm added the wip label Jan 25, 2018
@nebril nebril force-pushed the toservices-labels-match branch from 729fb07 to 1a80d9e Compare January 25, 2018 14:08
nebril added 5 commits January 25, 2018 15:08
@nebril
Store svc labels in lb, match toservices by them
835c0df 
k8s watcher stores added/updated services labels in LoadBalancer.

Those labels are used by RuleTranslator to check whether the ToService
rule should end up generating ToCIDR rules for service.

Signed-off-by: Maciej Kwiek <maciej@covalent.io>
@nebril
Add labels to toservices runtime test
a0153bd 
Signed-off-by: Maciej Kwiek <maciej@covalent.io>
@nebril
Add namespace and selector to api.Service
03a6984 
Signed-off-by: Maciej Kwiek <maciej@covalent.io>
@nebril
Regenerate k8s code to accommodate API changes
f94cf9f 
Signed-off-by: Maciej Kwiek <maciej@covalent.io>
@nebril
Ginkgo test for labeled toservices policies
05e2613 
Signed-off-by: Maciej Kwiek <maciej@covalent.io>
@nebril nebril force-pushed the toservices-labels-match branch from 1a80d9e to 05e2613 Compare January 25, 2018 14:10
@nebril
Copy link
Copy Markdown
Member Author

nebril commented Jan 25, 2018

test-me-please

@nebril nebril removed the wip label Jan 25, 2018
@aanm aanm added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jan 25, 2018
@tgraf tgraf merged commit fe6839b into cilium:master Jan 25, 2018
@nebril nebril mentioned this pull request Jan 29, 2018
Merged
@aanm aanm mentioned this pull request Feb 15, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@joestringer joestringer joestringer approved these changes

@aanm aanm aanm approved these changes

+3 more reviewers

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

@manalibhutiyani manalibhutiyani manalibhutiyani left review comments

@eloycoto eloycoto eloycoto approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. ready-to-merge This PR has passed all tests and received consensus from code owners to merge.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Match services by labels in ToServices egress policy rules.

7 participants

@nebril @eloycoto @tgraf @aanm @manalibhutiyani @joestringer @houndci-bot