Skip to content

Enabling SDS in the gateway#778

Merged
rshriram merged 2 commits intoistio:release-1.1from
rshriram:gateway_sds
Jan 30, 2019
Merged

Enabling SDS in the gateway#778
rshriram merged 2 commits intoistio:release-1.1from
rshriram:gateway_sds

Conversation

@rshriram
Copy link
Copy Markdown
Member

Signed-off-by: Shriram Rajagopalan shriramr@vmware.com

Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@googlebot googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 29, 2019
Signed-off-by: Shriram Rajagopalan <shriramr@vmware.com>
@rshriram
Copy link
Copy Markdown
Member Author

What this API allows is to always have SDS agent in the gateway pod while still using old style file based configs. Then users can incrementally migrate to the SDS server by changing their existing gateway configs to use the CredentialStoreRemoteBackend with the resourceName as secretName. In other words, it allows some servers to have file based paths and others to have SDS without any guess work involving determining whether or not the gateway pod has a SDS agent.

@wenchenglu wenchenglu requested a review from JimmyCYJ January 29, 2019 21:36
@wenchenglu
Copy link
Copy Markdown

@myidpt please help review. I cannot add you to reviewer list, not sure why

message CredentialStoreFileBackend {
// REQUIRED if mode is `SIMPLE` or `MUTUAL`. The path to the file
// holding the server-side TLS certificate to use.
string server_certificate = 3;
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.

Change the field ID to begin with 1?
string server_certificate = 1;
string private_key = 2;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think that's to facilitate the proto field orders when doing the encoding.

@myidpt
Copy link
Copy Markdown

myidpt commented Jan 29, 2019

I'm generally OK with this approach. Wait for Costin's comments.
@costinm

@costinm
Copy link
Copy Markdown
Contributor

costinm commented Jan 29, 2019

+1, it's a good compromise, we can refine it in 1.2 based on user feedback.

I like the idea of using Gateway resource name as key in the SDS storage, it seems to solve all automated cases.

@wenchenglu
Copy link
Copy Markdown

/lgtm
/approve

@istio-testing
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rshriram, wenchenglu

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

Copy link
Copy Markdown
Contributor

@costinm costinm left a comment

Choose a reason for hiding this comment

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

I don't think this is a very nice UX - nor a good reason to deprecate server_certificate and the other fields.

We do need to keep a file-based option - and the existing fields work fine.

I don't agree that users should manually configure server_address or resource_name - it should be environment based, and user settings will prevent us from auto-provisioning and auto-config.

Pretty strong -1 on this change, in particular for 1.1

@costinm
Copy link
Copy Markdown
Contributor

costinm commented Jan 30, 2019

/hold

@istio-testing istio-testing added the do-not-merge/hold Block automatic merging of a PR. label Jan 30, 2019
@costinm costinm requested a review from louiscryan January 30, 2019 00:15
@rshriram rshriram merged commit 3c7e31a into istio:release-1.1 Jan 30, 2019
rshriram added a commit that referenced this pull request Jan 30, 2019
rshriram added a commit that referenced this pull request Jan 30, 2019
@rshriram
Copy link
Copy Markdown
Member Author

FYI, the server certificates is still there.

servers:
- hosts:
   - foo.com
   tls:
    credentialStore:
      files:
        serverCertificate: ...
        privateKey...

Or

servers:
- hosts:
   - foo.com
  tls:
    credentialStore:
     remote:
       resourceName: mysecret

Only the secret name is mandatory. the server path is additional customization.

And systems that are auto provisioning the gateways - they are already generating the Gateway configs and the k8s secrets based on the hostnames. So all they have to do is something like this:

servers:
- hosts:
   - foo.com
tls:
 credentialStore:
   remote:
     resourceName: foo.com

Explicit spec avoids confusions caused by auto inference of that one single field

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

Labels

cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. do-not-merge/hold Block automatic merging of a PR. review/done

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants