Skip to content

agent: Add CILIUM_ENABLE_POLICY env into the helm chart. #13175

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
Merged
merged 1 commit into from
Sep 16, 2020
Merged

agent: Add CILIUM_ENABLE_POLICY env into the helm chart. #13175

merged 1 commit into from
Sep 16, 2020

Conversation

camilo-schoeningh-sociomantic
Copy link
Contributor

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

Users which are using the official cilium helm repo, can now change the policy enforcement mode without modify templates or patch deployments.

@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.8 kind/backports This PR provides functionality previously merged into master. labels Sep 15, 2020
@camilo-schoeningh-sociomantic camilo-schoeningh-sociomantic force-pushed the add_policy_enforcement branch 2 times, most recently from 235f9cb to f53b72c Compare September 15, 2020 15:13
vadorovsky
vadorovsky requested changes Sep 15, 2020
View reviewed changes
Copy link
Member

@vadorovsky vadorovsky 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 your contribution!

The change itself looks good to me.

Could you change the commit message title to agent: Add CILIUM_ENABLE_POLICY env into the helm chart? We usually use the subsystem: Description format for commit message titles.

And could you wrap lines in the commit message to 72 characters per line?

To sum it up:

agent: Add CILIUM_ENABLE_POLICY env into the helm chart

Users which are using the official cilium helm repo, can now change the
policy enforcement mode without modify templates or patch deployments.

Signed-off-by: Camilo Schoeningh <camilo.schoeningh@dunnhumby.com>

@vadorovsky
Copy link
Member

/cc @aanm

@vadorovsky vadorovsky requested a review from aanm September 15, 2020 16:21
@camilo-schoeningh-sociomantic
Copy link
Contributor Author

@mrostecki : Thanks for your suggestion. I have fixed it in my commit.

@camilo-schoeningh-sociomantic camilo-schoeningh-sociomantic changed the title [AGENT] Add CILIUM_ENABLE_POLICY env into the helm chart. agent: Add CILIUM_ENABLE_POLICY env into the helm chart. Sep 15, 2020
joestringer
joestringer reviewed Sep 15, 2020
View reviewed changes
aanm
aanm requested changes Sep 15, 2020
View reviewed changes
Copy link
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.

Also, please change the base branch to master (and don't forget to rebase against master first). We will do the backport ourselves to 1.8.

@camilo-schoeningh-sociomantic camilo-schoeningh-sociomantic changed the base branch from v1.8 to master September 16, 2020 09:01
@tklauser tklauser removed request for a team September 16, 2020 09:13
@camilo-schoeningh-sociomantic camilo-schoeningh-sociomantic force-pushed the add_policy_enforcement branch 2 times, most recently from 5d2cad6 to dd594bc Compare September 16, 2020 09:27
@aanm aanm added needs-backport/1.8 release-note/misc This PR makes changes that have no direct user impact. labels Sep 16, 2020
@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 Sep 16, 2020
@ghost ghost mentioned this pull request Sep 16, 2020
Merged
aanm
aanm requested changes Sep 16, 2020
View reviewed changes
@aanm
Copy link
Member

aanm commented Sep 16, 2020

@aanm Hey! I have updated my commit and pushing now against master. Now the bot is complaing about a missing release note label - I am guessing there is nothing I can do about ?!?

Correct, I've added the missing release label.

Another question (if you have time for) : Is there a decision-log where I can find out why you want to push everything inside this configMap ? I am just curious about your opinion.

@camilo-schoeningh-sociomantic Cilium config map is mounted in the DaemonSet and it will read that directory and parse all options mounted in the volume. Each option from the cilium-config ConfigMap is a file in the DaemonSet. This avoids having a DaemonSet with numerous environment variables, one for each option from the ConfigMap. The change was introduced in this commit and PR 0866c30

Let me know if that is clear.

@camilo-schoeningh-sociomantic
agent: Add CILIUM_ENABLE_POLICY env into the helm chart.
6eb0830 
Users which are using the official cilium helm repo, can now change the
policy enforcement mode without modify templates or patch deployments.

Signed-off-by: Camilo Schoeningh <camilo.schoeningh@dunnhumby.com>
@camilo-schoeningh-sociomantic
Copy link
Contributor Author

@aanm : Thanks. That had answered all my questions. Cool approach. Anyway, I have updated my commit again.

@aanm
Copy link
Member

aanm commented Sep 16, 2020

test-me-please

@joestringer joestringer merged commit 5fc5ee6 into cilium:master Sep 16, 2020
@kkourt kkourt mentioned this pull request Sep 18, 2020
Merged
@joestringer joestringer mentioned this pull request Oct 1, 2020
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

@vadorovsky vadorovsky vadorovsky approved these changes

Assignees
No one assigned
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@camilo-schoeningh-sociomantic @vadorovsky @aanm @joestringer @kkourt @tklauser