Skip to content

doc: Fix clustermesh documentation to set the correct identityMode#12153

Merged
joestringer merged 1 commit intocilium:masterfrom
soumynathan:fix-clustermesh-guide
Jul 14, 2020
Merged

doc: Fix clustermesh documentation to set the correct identityMode#12153
joestringer merged 1 commit intocilium:masterfrom
soumynathan:fix-clustermesh-guide

Conversation

@soumynathan
Copy link
Copy Markdown

This PR fixes the documentation issues and as well adds in the option
through helm when 'etcd' is enabled.

Fixes: #12125
Signed-off-by: Swaminathan Vasudevan svasudevan@suse.com

@soumynathan soumynathan requested review from a team as code owners June 17, 2020 20:08
@soumynathan soumynathan requested a review from a team June 17, 2020 20:08
@maintainer-s-little-helper
Copy link
Copy Markdown

Please set the appropriate release note label.

@coveralls
Copy link
Copy Markdown

coveralls commented Jun 17, 2020

Coverage Status

Coverage increased (+0.001%) to 37.146% when pulling b1f48738e438c7378ba68ff1e5d8e122b0ef6468 on soumynathan:fix-clustermesh-guide into 93d32dd on cilium:master.

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.

Thanks for picking this up. I have a couple of minor comments below.

Comment thread Documentation/gettingstarted/clustermesh.rst Outdated
Comment thread install/kubernetes/cilium/charts/config/templates/configmap.yaml Outdated
@aanm aanm added the release-note/misc This PR makes changes that have no direct user impact. label Jun 18, 2020
@soumynathan soumynathan force-pushed the fix-clustermesh-guide branch 2 times, most recently from 1aa91de to b1f4873 Compare June 19, 2020 18:29
Copy link
Copy Markdown
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Just one minor comment, otherwise everything looks good. I'll let others review the Helm change.

Comment thread Documentation/gettingstarted/clustermesh.rst Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unless we shift around how this is configured, I'm not sure there's a better way. Currently it's by default set to crd via the global config map, so we can't detect that it's not configured then apply only in that case.

I'm fine with forcing kvstore mode in this way even though it overrides the user-specified configuration, because I don't see a reason why users would want to use the kvstore but manage identities via CRDs. 👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

because I don't see a reason why users would want to use the kvstore but manage identities via CRDs.

@joestringer cilium-etcd-operator won't have a security identity because of the chicken-egg problem.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

because I don't see a reason why users would want to use the kvstore but manage identities via CRDs.

@joestringer cilium-etcd-operator won't have a security identity because of the chicken-egg problem.
So @joestringer what do you think, can we leave it this way or do we want to change this.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@aanm isn't that what fixed identities are for?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@joestringer it's not 100% reliable, I mean it would be less reliable than CRD based identity.

This PR fixes the documentation issues and as well adds in the option
through helm when 'etcd' is enabled.

Fixes: cilium#12125
Signed-off-by: Swaminathan Vasudevan <svasudevan@suse.com>
@soumynathan soumynathan force-pushed the fix-clustermesh-guide branch from b1f4873 to 0e2e136 Compare June 22, 2020 21:17
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

because I don't see a reason why users would want to use the kvstore but manage identities via CRDs.

@joestringer cilium-etcd-operator won't have a security identity because of the chicken-egg problem.

@soumynathan
Copy link
Copy Markdown
Author

@joestringer and @aanm Is there anything else left in this PR that need to be addressed.

@joestringer
Copy link
Copy Markdown
Member

@joestringer and @aanm Is there anything else left in this PR that need to be addressed.

During my testing, I found that if identities are not managed by the kvstore, clustermesh would not be able to enforce security policies correctly. The docs changes reflect the changes necessary there so that side is good.

The ConfigMap changes side I'm less sure about. @aanm sounds like you think that we should defer this to the user and not override their setting here if they configure --set global.etcd.enabled. Ie outside of clustermesh scenarios, it would be fine to just default to managing the identities via CRD instead. Is that right? In that case, we should drop the helm changes.

@joestringer
Copy link
Copy Markdown
Member

I spoke with @aanm last week before he went on PTO and he said that as long as this behaviour is expected (clustermesh security policies require --identity-allocation-mode=kvstore), he's fine with the PR. @tgraf confirmed this on Slack (at least until we document how to use http://github.com/cilium/clustermesh-apiserver which changes the story).

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

None yet

Development

Successfully merging this pull request may close these issues.

Clustermesh guide with direct etcd connection requires --set global.identityAllocationMode=kvstore

6 participants