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

Correctly handle istio secrets #2277

Merged
merged 3 commits into from
Feb 14, 2020

Conversation

ppeble
Copy link
Contributor

@ppeble ppeble commented Feb 5, 2020

Description

Change to correctly consume istio.io/key-and-cert secret type so that we no longer are required to mount the secret as a volume as specified here.

As background, the mounting solution works but we discovered that when istio secrets change (say, with an istio upgrade) it seems that Ambassador was not correctly picking up the new key-and-cert secret. The solution when using the mounted volume method was to reboot Ambassador. With this change Ambassador correctly reads the secret like any other secret and therefore will pick up any changes.

Related Issues

Testing

I've performed manual testing of on istio versions 1.2 and 1.3. I do not have a more recent (1.4) version to test against but nothing in the documentation leads me to believe that there is a change to the istio/key-and-cert secret format.

I have added a test to verify that the key is correctly consumed into a TLSContext. I am open to guidance on how to add additional tests but to be quite honest I am overwhelmed trying to learn how kat works after a few months away from it. 😬

@ppeble ppeble force-pushed the issue-1475-istio-mtls branch from d63fdd9 to a0ddc25 Compare February 5, 2020 21:29
kflynn
kflynn previously approved these changes Feb 13, 2020
Copy link
Member

@kflynn kflynn left a comment

Choose a reason for hiding this comment

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

I think we're good here! Many thanks.

Copy link
Member

@kflynn kflynn left a comment

Choose a reason for hiding this comment

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

Still looks good after conflict resolution. 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants