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

"verify_subject_alt_name" should accept an array of strings #569

Merged

Conversation

lookuptable
Copy link
Contributor

This commit cleans up a few leftovers from #559 .

@lookuptable
Copy link
Contributor Author

CC @myidpt @wattli

@wattli
Copy link
Contributor

wattli commented Mar 14, 2017

LGTM, but I can't approve it.

@lookuptable
Copy link
Contributor Author

@mattklein123 PTAL

BTW is there any existing test cases for config_schemas.cc? If yes can you please point me to the test file?

@mattklein123
Copy link
Member

@ccaraman can you take a look at this? I'm a little confused how this passed tests even without the schema change?

@mattklein123
Copy link
Member

@lookuptable please sign CLA also

@lookuptable
Copy link
Contributor Author

@mattklein123 Signed!

@ccaraman
Copy link
Contributor

ccaraman commented Mar 15, 2017

@mattklein123 It passed because the ssl_context are checked in two places Json::Schema::LISTENER_SCHEMA and Json::Schema::CLUSTER_SCHEMA.

A more complete fix would be to pull the SSLContext into its own schema and move the check into https://github.com/lyft/envoy/blob/9dcac8ca111ecc8da059d1f8d42eb766b44bacd6/source/common/ssl/context_config_impl.cc#L17 I can do this later this week.

@lookuptable No, we don't have tests for explicit tests for our schemas other than some places where we load a config that we expect to fail. I've added #572

@mattklein123
Copy link
Member

Yes, but for the change to configgen.py, how did that pass tests previously. That is related to upstream. That should generate a config that does not load. Do we not test that case in the example config tests?

@mattklein123
Copy link
Member

@lookuptable it's not perfect, but you should be able to add a test for a listener that sets verify_subject_alt_name like here: https://github.com/lyft/envoy/blob/master/test/server/configuration_impl_test.cc#L124

I'm guessing in the configgen.py case, we just don't generate an example config with dynamo in it, since I know I fixed this case in our internal configs.

@mattklein123 mattklein123 merged commit fca5cd5 into envoyproxy:master Mar 15, 2017
@lookuptable lookuptable deleted the schema_verify_subject_alt_name branch March 15, 2017 18:29
lambdai pushed a commit to lambdai/envoy-dai that referenced this pull request Jul 21, 2020
jpsim pushed a commit that referenced this pull request Nov 28, 2022
Previously, the artifacts would appear on GitHub Actions as:
- `envoy_android_aar.zip`
- `envoy_ios_framework.zip`

These would then unzip to:
- `envoy_android_aar.zip.zip`
- `envoy_android_aar.zip/envoy.aar`

And:
- `envoy_ios_framework.zip.zip`
- `envoy_ios_framework.zip/Envoy`
- `envoy_ios_framework.zip/Headers`
- `envoy_ios_framework.zip/Modules`

This was confusing and actually ended up mis-naming both the zip files and the framework.

With the changes in this PR, the new artifacts will be:
- `envoy_android_aar`
- `envoy_ios_framework`

And will unzip to:
- `envoy_android_aar.zip`
- `envoy_android_aar/envoy.aar`

And:
- `envoy_ios_framework.zip`
- `envoy_ios_framework/Envoy.framework/*`

Signed-off-by: Michael Rebello <[email protected]>
Signed-off-by: JP Simard <[email protected]>
jpsim pushed a commit that referenced this pull request Nov 29, 2022
Previously, the artifacts would appear on GitHub Actions as:
- `envoy_android_aar.zip`
- `envoy_ios_framework.zip`

These would then unzip to:
- `envoy_android_aar.zip.zip`
- `envoy_android_aar.zip/envoy.aar`

And:
- `envoy_ios_framework.zip.zip`
- `envoy_ios_framework.zip/Envoy`
- `envoy_ios_framework.zip/Headers`
- `envoy_ios_framework.zip/Modules`

This was confusing and actually ended up mis-naming both the zip files and the framework.

With the changes in this PR, the new artifacts will be:
- `envoy_android_aar`
- `envoy_ios_framework`

And will unzip to:
- `envoy_android_aar.zip`
- `envoy_android_aar/envoy.aar`

And:
- `envoy_ios_framework.zip`
- `envoy_ios_framework/Envoy.framework/*`

Signed-off-by: Michael Rebello <[email protected]>
Signed-off-by: JP Simard <[email protected]>
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.

4 participants