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

Emit security configuration by default #19

Merged
merged 1 commit into from
Sep 13, 2021

Conversation

easwars
Copy link
Collaborator

@easwars easwars commented Jul 23, 2021

No description provided.

@easwars easwars requested a review from ejona86 July 23, 2021 22:24
@easwars
Copy link
Collaborator Author

easwars commented Jul 23, 2021

@sanjaypujare

@easwars
Copy link
Collaborator Author

easwars commented Jul 23, 2021

@ejona86
This is in preparation for the upcoming security GA release. We need to be in a position to cut the bootstrap generator release when required. Making this change will ensure that we have test coverage with this change before cutting the release.
Thanks.

@ejona86
Copy link
Collaborator

ejona86 commented Jul 26, 2021

@sanjaypujare, can you confirm this feature is ready to be enabled by default?

@sanjaypujare
Copy link

@sanjaypujare, can you confirm this feature is ready to be enabled by default?

Yes. We have been running security in our staging environment for the past 3-4 months with the flag enabled. The plan is to enable security by default and test the bootstrap generator (from this commit) in all of our staging environments before making a formal release with that change.

@ejona86
Copy link
Collaborator

ejona86 commented Jul 26, 2021

Should this wait until grpc/proposal#243 is merged?

@easwars
Copy link
Collaborator Author

easwars commented Jul 26, 2021

Should this wait until grpc/proposal#243 is merged?

Hmm ... I don't expect anything to change here based on any changes in the gRFC. So, I'm ok going forward with this, and that way we can get the staging tests to use the new version, and we can be more ready to push the new release out when time comes.

@ejona86
Copy link
Collaborator

ejona86 commented Jul 26, 2021

that way we can get the staging tests to use the new version, and we can be more ready to push the new release out when time comes.

That saves ~1 day of calendar time. I think it is weird to enable any flags for security in any language until the gRFC is merged or we agree it is "safe" to do so (for example, if things are held up for unrelated reasons).

I'm a bit concerned about https://github.com/grpc/proposal/pull/243/files/414e01c80dff3ed2ff57a7be582d4e73ed659fd2 and some related SPIFFE trust domain conversations (alluded to at envoyproxy/envoy#17201 (comment)). That said, given how late this discussion is happening we'll have serious trouble making any substantial changes. But I still see limited benefit to rushing this piece.

@ejona86 ejona86 self-requested a review July 26, 2021 20:12
Copy link
Collaborator

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

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

Let's wait.

@sanjaypujare
Copy link

Let's wait.

Is there another way to test out this change in a staging environment? Note that the plan was to just commit the change but not build a binary (but use the commit-triggered build in staging)

@ejona86
Copy link
Collaborator

ejona86 commented Jul 26, 2021

It is a flag. Pass --include-psm-security-experimental=true

@sanjaypujare
Copy link

It is a flag. Pass --include-psm-security-experimental=true

Yes, the idea was to not use the flag and have the security config be present by default and test it everywhere possible.

@easwars
Copy link
Collaborator Author

easwars commented Sep 10, 2021

Resolved merge conflicts. PTAL @ejona86

@easwars easwars merged commit 8765051 into GoogleCloudPlatform:master Sep 13, 2021
@easwars easwars deleted the make_security_default branch September 13, 2021 14:43
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.

None yet

3 participants