Citadel standalone by sdake · Pull Request #5292 · istio/istio

sdake · 2018-04-29T22:47:18Z

No description provided.

googlebot · 2018-04-29T22:47:22Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

istio-testing · 2018-04-29T22:50:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: greghanson

Assign the PR to them by writing /assign @greghanson in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

install/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

googlebot · 2018-04-29T22:50:35Z

CLAs look good, thanks!

sdake · 2018-04-29T22:52:45Z

Reviewers,

This is the standalone citadel template merged into the main helm chart. I don't know if we want this sort of work prior to 0.8, however, I've submitted on the 0.8 branch as master is blocked.

sdake · 2018-04-29T22:54:27Z

install/kubernetes/helm/istio/values.yaml

this part may not be correct.

codecov · 2018-04-29T23:00:25Z

Codecov Report

❗ No coverage uploaded for pull request base (release-0.8@977e942). Click here to learn what that means.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             release-0.8   #5292   +/-   ##
=============================================
  Coverage               ?     74%           
=============================================
  Files                  ?     323           
  Lines                  ?   27231           
  Branches               ?       0           
=============================================
  Hits                   ?   19973           
  Misses                 ?    6473           
  Partials               ?     785

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 977e942...92bb2b2. Read the comment docs.

istio-testing · 2018-04-30T11:33:23Z

@sdake: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
prow/istio-unit-tests.sh	`92bb2b2`	link	`/test istio-unit-tests`
prow/e2e-simpleTests.sh	`92bb2b2`	link	`/test e2e-simple`
prow/istio-pilot-e2e-envoyv2-v1alpha3.sh	`92bb2b2`	link	`/test istio-pilot-e2e-envoyv2-v1alpha3`
prow/istio-pilot-e2e.sh	`92bb2b2`	link	`/test istio-pilot-e2e`
prow/e2e-bookInfoTests-v1alpha3.sh	`92bb2b2`	link	`/test e2e-bookInfo-envoyv2-v1alpha3`

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

myidpt · 2018-04-30T14:22:41Z

Thanks, Steven.
I believe our current istio-citadel-standalone.yaml.tmpl generates istio-citadel-standalone.yaml for V0.8. Your PR changes the generation to using helm.

myidpt · 2018-04-30T14:32:47Z

install/kubernetes/helm/istio/requirements.yaml

    # repository: file://../sidecar-injector
    condition: sidecar-injector.enabled
+    tags:
+      - integrated_citadel


How about using standalone_citadel as a tag? Then you don't need to explicitly set integrated_citadel tags here.

I'm not convinced this PR works for the non-standalone case - need a bit of help from @jascott1.

Unfortunately standalone_citadel, while making more logical sense, is not viable with the way Helm works. The way tags and conditions are processed is covered here: https://github.com/kubernetes/helm/blob/master/docs/charts.md#tags-and-condition-fields-in-requirementsyaml

In short here is my parsing of how conditions/tags work (or don't work) together:
If subchart.condition == undefined
if tag == true
include subchart

There is no way to have the opposite logic:

if subchart.condition == undefined
if tag = true (on the security)
include all charts that are not security

@jascott1 can you weigh in here is this PR correct, or is there a way to specify "standalone_citadel".

I get the impression condition/tags are half-baked after reading the docs, and need some more time in the oven ;-)

myidpt · 2018-04-30T14:40:42Z

BTW, I saw the newly generated file istio-citadel-standalone.yaml does have the ConfigMap, we can avoid it because it's not used.

sdake · 2018-05-01T15:53:42Z

/hold

istio-testing · 2018-05-20T18:12:07Z

@sdake: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

stale · 2018-06-23T12:34:15Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

sdake · 2018-07-03T10:15:24Z

ping leaving open for 1.1 progress.

stale · 2018-07-17T10:25:54Z

This pull request has been automatically marked as stale because it has not had activity in the last 2 weeks. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

sdake · 2018-07-17T13:39:17Z

stalebot ignore

googlebot added the cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. label Apr 29, 2018

sdake force-pushed the citadel-standalone branch from ea82a7d to d370565 Compare April 29, 2018 22:50

googlebot added cla: yes and removed cla: no Set by the Google CLA bot to indicate the author of a PR has not signed the Google CLA. labels Apr 29, 2018

sdake requested review from costinm and myidpt April 29, 2018 22:50

sdake commented Apr 29, 2018

View reviewed changes

install/kubernetes/helm/istio/values.yaml Outdated

Copy link
Copy Markdown

Member Author

sdake Apr 29, 2018

Choose a reason for hiding this comment

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

this part may not be correct.

sdake force-pushed the citadel-standalone branch from d370565 to d2760ae Compare April 29, 2018 23:00

sdake mentioned this pull request Apr 29, 2018

Remove templates #5091

Closed

Helm convergence: create Istio citadel standalone

92bb2b2

sdake force-pushed the citadel-standalone branch from d2760ae to 92bb2b2 Compare April 30, 2018 10:11

myidpt reviewed Apr 30, 2018

View reviewed changes

istio-testing added the do-not-merge/hold Block automatic merging of a PR. label May 1, 2018

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label May 20, 2018

stale bot added the stale label Jun 23, 2018

sdake removed the stale label Jul 3, 2018

stale bot added the stale label Jul 17, 2018

sdake mentioned this pull request Jul 17, 2018

Docs refer to istio-citadel-plugin-certs.yaml but it doesn't exist istio/istio.io#1836

Closed

stale bot removed the stale label Jul 17, 2018

sdake added the do-not-merge/work-in-progress Block merging of a PR because it isn't ready yet. label Jul 17, 2018

rshriram added the stale label Jul 20, 2018

rshriram closed this Jul 20, 2018

Conversation

sdake commented Apr 29, 2018

Uh oh!

googlebot commented Apr 29, 2018

Uh oh!

istio-testing commented Apr 29, 2018

Uh oh!

googlebot commented Apr 29, 2018

Uh oh!

sdake commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sdake Apr 29, 2018

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Apr 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

istio-testing commented Apr 30, 2018

Uh oh!

myidpt commented Apr 30, 2018

Uh oh!

myidpt Apr 30, 2018

Choose a reason for hiding this comment

Uh oh!

sdake May 1, 2018

Choose a reason for hiding this comment

Uh oh!

myidpt commented Apr 30, 2018

Uh oh!

sdake commented May 1, 2018

Uh oh!

istio-testing commented May 20, 2018

Uh oh!

stale bot commented Jun 23, 2018

Uh oh!

sdake commented Jul 3, 2018

Uh oh!

stale bot commented Jul 17, 2018

Uh oh!

sdake commented Jul 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sdake commented Apr 29, 2018 •

edited

Loading

codecov bot commented Apr 29, 2018 •

edited

Loading