Skip to content

[RELEASE 0.1] Change our configurations to work with Istio 1.0 validation.#1725

Merged
mattmoor merged 1 commit intoknative:release-0.1from
tcnghia:0.1-patch
Aug 3, 2018
Merged

[RELEASE 0.1] Change our configurations to work with Istio 1.0 validation.#1725
mattmoor merged 1 commit intoknative:release-0.1from
tcnghia:0.1-patch

Conversation

@tcnghia
Copy link
Copy Markdown
Contributor

@tcnghia tcnghia commented Jul 26, 2018

Istio has tightened several aspects of its validation in 1.0:

  • They now require tls.mode to be set when we use HTTPS, and
  • They now require header names to lowercase.

- Set a default TLS mode.
- Change header names to lowercase.
@google-prow-robot google-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 26, 2018
@tcnghia
Copy link
Copy Markdown
Contributor Author

tcnghia commented Jul 27, 2018

/assign @mattmoor

@mattmoor mattmoor changed the title Change our configurations to work with Istio 1.0 validation. [RELEASE 0.1] Change our configurations to work with Istio 1.0 validation. Jul 27, 2018
@mattmoor
Copy link
Copy Markdown
Member

/lgtm
/approve

This LGTM, thanks for putting this together to quickly.

@google-prow-robot google-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2018
@google-prow-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor, tcnghia

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:

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

@google-prow-robot google-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2018
@mattmoor
Copy link
Copy Markdown
Member

/hold

For a second set of eyes (I suspect I will need to merge anyways given the branch ACL)

@google-prow-robot google-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 27, 2018
Copy link
Copy Markdown
Member

@evankanderson evankanderson left a comment

Choose a reason for hiding this comment

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

/lgtm

Comment thread config/202-gateway.yaml
hosts:
- "*"
tls:
mode: PASSTHROUGH
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this seems to mean that SNI is used to route, is it always there?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Docs for this option are scant: https://istio.io/docs/reference/config/istio.networking.v1alpha3/#Server.TLSOptions.TLSmode

I'm also curious how this value was chosen. Is it intended to have the same behavior as before?

Copy link
Copy Markdown
Contributor

@grantr grantr left a comment

Choose a reason for hiding this comment

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

LGTM but I'm also curious about tls mode setting.

Comment thread config/202-gateway.yaml
hosts:
- "*"
tls:
mode: PASSTHROUGH
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Docs for this option are scant: https://istio.io/docs/reference/config/istio.networking.v1alpha3/#Server.TLSOptions.TLSmode

I'm also curious how this value was chosen. Is it intended to have the same behavior as before?

@tcnghia
Copy link
Copy Markdown
Contributor Author

tcnghia commented Jul 27, 2018

@grantr that is the only mode that doesn't require a SSL cert to be set.

We have instructions to update the cert in knative/docs, but we don't provide any default certs. Once we have some sort of automated process to get cert (like LetsEncrypt) we should change this option here.

I already checked these changes into our master yesterday. We could let that bake for a while before applying this patch to have more surety.

@tcnghia
Copy link
Copy Markdown
Contributor Author

tcnghia commented Aug 1, 2018

Istio released their 1.0 so I'd like to check this in and create a 0.1.1

@mattmoor
Copy link
Copy Markdown
Member

mattmoor commented Aug 1, 2018

@tcnghia find me tomorrow, and unless someone speaks up we can merge this.

@tcnghia
Copy link
Copy Markdown
Contributor Author

tcnghia commented Aug 1, 2018

/hold cancel

@google-prow-robot google-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2018
@tcnghia tcnghia mentioned this pull request Aug 1, 2018
@mattmoor
Copy link
Copy Markdown
Member

mattmoor commented Aug 2, 2018

/retest

@mattmoor mattmoor merged commit b6bed97 into knative:release-0.1 Aug 3, 2018
@knative-prow-robot
Copy link
Copy Markdown
Contributor

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

Test name Commit Details Rerun command
pull-knative-serving-integration-tests f31d81b link /test pull-knative-serving-integration-tests

Full PR test history. Your PR dashboard.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants