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.

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

Fixing the helm templates to support Control Plane SDS #16466

Merged
merged 20 commits into from Aug 24, 2019

Conversation

JimmyCYJ
Copy link
Member

@JimmyCYJ JimmyCYJ commented Aug 22, 2019

Fix the helm templates to support control plane security with SDS. Before this change, the control plane security cannot be enabled with SDS enabled.

[ ] Configuration Infrastructure
[ ] Docs
[ x ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ x ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure

#11434

@JimmyCYJ
Copy link
Member Author

JimmyCYJ commented Aug 23, 2019

@howardjohn could you please take a look? The Pilot agent change is already merged and cherry-picked into 1.3

@howardjohn
Copy link
Member

@JimmyCYJ approved but I am not an owner

@myidpt
Copy link
Contributor

myidpt commented Aug 23, 2019

@howardjohn Thanks for the review.

@JimmyCYJ
Copy link
Member Author

@howardjohn Thanks a lot!

@JimmyCYJ
Copy link
Member Author

@sdake could you please take a look?

Copy link
Member

@sdake sdake left a comment

Choose a reason for hiding this comment

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

looks pretty good. I did have a few questions. WIll approve in 1-2 hours if those questions don't turn up problems.

Cheers
-steve

Copy link
Member Author

@JimmyCYJ JimmyCYJ left a comment

Choose a reason for hiding this comment

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

Thanks a lot @sdake for the review! Please take a look.

@JimmyCYJ
Copy link
Member Author

/retest

1 similar comment
@JimmyCYJ
Copy link
Member Author

/retest

@istio-testing
Copy link
Collaborator

@JimmyCYJ: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
integ-mixer-k8s-presubmit-tests-master 12d1c9e link /test integ-mixer-k8s-presubmit-tests-master

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.

@@ -236,6 +236,8 @@ spec:
valueFrom:
fieldRef:
fieldPath: metadata.namespace
- name: SDS_ENABLED
value: "{{ $.Values.global.sds.enabled }}"
Copy link
Member

@morvencao morvencao Aug 24, 2019

Choose a reason for hiding this comment

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

nit: maybe better to replace "{{ $.Values.global.sds.enabled }}" with {{ $.Values.global.sds.enabled | quote }}

Copy link
Contributor

@mandarjog mandarjog 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 bootstrap

@istio-testing istio-testing merged commit 90ebc30 into istio:master Aug 24, 2019
@istio-testing
Copy link
Collaborator

In response to a cherrypick label: #16466 failed to apply on top of branch "release-1.3":

Using index info to reconstruct a base tree...
M	install/kubernetes/helm/istio/charts/gateways/templates/deployment.yaml
M	install/kubernetes/helm/istio/charts/mixer/templates/deployment.yaml
M	install/kubernetes/helm/istio/files/injection-template.yaml
M	pilot/cmd/pilot-agent/main.go
M	pilot/cmd/pilot-agent/main_test.go
M	tools/packaging/common/envoy_bootstrap_v2.json
Falling back to patching base and 3-way merge...
Auto-merging tools/packaging/common/envoy_bootstrap_v2.json
Auto-merging pilot/cmd/pilot-agent/main_test.go
CONFLICT (content): Merge conflict in pilot/cmd/pilot-agent/main_test.go
Auto-merging pilot/cmd/pilot-agent/main.go
CONFLICT (content): Merge conflict in pilot/cmd/pilot-agent/main.go
Auto-merging install/kubernetes/helm/istio/files/injection-template.yaml
Auto-merging install/kubernetes/helm/istio/charts/mixer/templates/deployment.yaml
Auto-merging install/kubernetes/helm/istio/charts/gateways/templates/deployment.yaml
CONFLICT (content): Merge conflict in install/kubernetes/helm/istio/charts/gateways/templates/deployment.yaml
error: Failed to merge in the changes.
Patch failed at 0001 support control plane SDS

@JimmyCYJ
Copy link
Member Author

/cherrypick release-1.3

@istio-testing
Copy link
Collaborator

@JimmyCYJ: #16466 failed to apply on top of branch "release-1.3":

Using index info to reconstruct a base tree...
M	install/kubernetes/helm/istio/charts/gateways/templates/deployment.yaml
M	install/kubernetes/helm/istio/charts/mixer/templates/deployment.yaml
M	install/kubernetes/helm/istio/files/injection-template.yaml
M	pilot/cmd/pilot-agent/main.go
M	pilot/cmd/pilot-agent/main_test.go
M	tools/packaging/common/envoy_bootstrap_v2.json
Falling back to patching base and 3-way merge...
Auto-merging tools/packaging/common/envoy_bootstrap_v2.json
Auto-merging pilot/cmd/pilot-agent/main_test.go
CONFLICT (content): Merge conflict in pilot/cmd/pilot-agent/main_test.go
Auto-merging pilot/cmd/pilot-agent/main.go
CONFLICT (content): Merge conflict in pilot/cmd/pilot-agent/main.go
Auto-merging install/kubernetes/helm/istio/files/injection-template.yaml
Auto-merging install/kubernetes/helm/istio/charts/mixer/templates/deployment.yaml
Auto-merging install/kubernetes/helm/istio/charts/gateways/templates/deployment.yaml
CONFLICT (content): Merge conflict in install/kubernetes/helm/istio/charts/gateways/templates/deployment.yaml
error: Failed to merge in the changes.
Patch failed at 0001 support control plane SDS

In response to this:

/cherrypick release-1.3

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.

JimmyCYJ added a commit to JimmyCYJ/istio that referenced this pull request Aug 24, 2019
* support control plane SDS

* test control plane SDS

* revise

* revise

* revise

* set env

* revise

* revise

* revise

* revise

* revise

* revise

* template change for SDS control plane

* revise

* update

* fix unit tests

* fix tests

* enable control plane SDS
@JimmyCYJ JimmyCYJ deleted the control-plane-sds-template-change branch August 24, 2019 04:38
istio-testing pushed a commit that referenced this pull request Aug 25, 2019
* Fixing the helm templates to support Control Plane SDS (#16466)

* support control plane SDS

* test control plane SDS

* revise

* revise

* revise

* set env

* revise

* revise

* revise

* revise

* revise

* revise

* template change for SDS control plane

* revise

* update

* fix unit tests

* fix tests

* enable control plane SDS

* fix test golden files

* fix tests
howardjohn added a commit to howardjohn/istio that referenced this pull request Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/security cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants