Repository: bazelbuild/bazel
Issue: incompatible_tls_enabled_removed: flip it and enable tls by default while deprecating tls_enabled flag
Labels: incompatible-change
team-Remote-Exec

`--tls_enabled` flag is deprecated and should be removed in one of the next releases.
If `--incompatible_tls_enabled_removed` flag set to true, an error is rised in case tls_enabled is still in use.

**Migration**
* gRPC + enabled TLS:
Replace `--tls_enabled=true` and `--remote_cache=localhost:1234` with : `--remote_cache=grpcs://localhost:1234` or `--remote_cache=localhost:1234`
* gRPC + disabled TLS:
Replace `--tls_enabled=false` and `--remote_cache=localhost:1234` with : `--remote_cache=grpc://localhost:1234`

**Note**: the same migration plan applies to all of the following flags: `-remote_cache`, `--remote_executor `or `--bes_backend`. Use `--incompatible_tls_enabled_removed=true` to test.

**Note2**: http cache/executor is not affected

**Expected timeline**
Released with Bazel 0.26 (May 2019), off by default
Enabled by default with Bazel 0.30 (September 2019)

Issue comments:
Hi @ishikhman, just in the improbable case you didn't see it, we have a page with infomation how to roll out breaking changes over at https://bazel.build/breaking-changes-guide.html. There are some minor things missing in this issue (title format, missing migration-ready label)

Thanks!
thanks @hlopko! updated the issue accordingly. 
@ishikhman Any plans to remove incompatible_tls_enabled_removed flag completely? The problem is that with bazel at HEAD when you specify all_incompatible_changes to do a migration the flag incompatible_tls_enabled_removed gets also set displaying the following warning.

`WARNING: Option 'incompatible_tls_enabled_removed' is deprecated: No-op. See #8061 for details.`

Pull request review comments:

Patch that resolved the issue:
diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/AuthAndTLSOptions.java b/src/main/java/com/google/devtools/build/lib/authandtls/AuthAndTLSOptions.java
index 1a0540b4c3f0b2..a92454d8a5ebc1 100644
--- a/src/main/java/com/google/devtools/build/lib/authandtls/AuthAndTLSOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/authandtls/AuthAndTLSOptions.java
@@ -67,25 +67,26 @@ public class AuthAndTLSOptions extends OptionsBase {
       defaultValue = "false",
       documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
       deprecationWarning =
-          "Deprecated. Please specify a valid protocol in the URL (https or grpcs).",
+          "No-op. See #8061 for details.",
       effectTags = {OptionEffectTag.UNKNOWN},
       help =
-          "DEPRECATED. Specifies whether to use TLS for remote execution/caching and "
-              + "the build event service (BES). See #8061 for details.")
+          "No-op. See #8061 for details.")
   public boolean tlsEnabled;
 
+  @Deprecated
   @Option(
       name = "incompatible_tls_enabled_removed",
-      defaultValue = "false",
+      defaultValue = "true",
       documentationCategory = OptionDocumentationCategory.UNCATEGORIZED,
+      deprecationWarning =
+          "No-op. See #8061 for details.",
       effectTags = {OptionEffectTag.UNKNOWN},
       metadataTags = {
         OptionMetadataTag.INCOMPATIBLE_CHANGE,
         OptionMetadataTag.TRIGGERED_BY_ALL_INCOMPATIBLE_CHANGES
       },
       help =
-          "If set to true, bazel will handle --tls_enabled as a not existing flag."
-              + "See #8061 for details.")
+          "No-op. See #8061 for details.")
   public boolean incompatibleTlsEnabledRemoved;
 
   @Option(
diff --git a/src/main/java/com/google/devtools/build/lib/authandtls/GoogleAuthUtils.java b/src/main/java/com/google/devtools/build/lib/authandtls/GoogleAuthUtils.java
index d68f608599cccb..735d65d9b755b9 100644
--- a/src/main/java/com/google/devtools/build/lib/authandtls/GoogleAuthUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/authandtls/GoogleAuthUtils.java
@@ -86,18 +86,11 @@ private static String convertTargetScheme(String target) {
     return target.replace("grpcs://", "").replace("grpc://", "");
   }
 
-  // TODO(ishikhman) remove options.tlsEnabled flag usage when an incompatible flag is flipped
   private static boolean isTlsEnabled(String target, AuthAndTLSOptions options) {
-    if (options.incompatibleTlsEnabledRemoved && options.tlsEnabled) {
-      throw new IllegalArgumentException("flag --tls_enabled was not found");
-    }
-    if (options.incompatibleTlsEnabledRemoved) {
-      // 'grpcs://' or empty prefix => TLS-enabled
-      // when no schema prefix is provided in URL, bazel will treat it as a gRPC request with TLS
-      // enabled
-      return !target.startsWith("grpc://");
-    }
-    return target.startsWith("grpcs") || options.tlsEnabled;
+    // 'grpcs://' or empty prefix => TLS-enabled
+    // when no schema prefix is provided in URL, bazel will treat it as a gRPC request with TLS
+    // enabled
+    return !target.startsWith("grpc://");
   }
 
   private static SslContext createSSlContext(@Nullable String rootCert) throws IOException {


Test patch:
diff --git a/src/test/shell/bazel/remote/remote_execution_tls_test.sh b/src/test/shell/bazel/remote/remote_execution_tls_test.sh
index 9766a25fabb99c..c9205b93caeeee 100755
--- a/src/test/shell/bazel/remote/remote_execution_tls_test.sh
+++ b/src/test/shell/bazel/remote/remote_execution_tls_test.sh
@@ -86,16 +86,15 @@ function test_remote_grpcs_cache() {
       || fail "Failed to build //a:foo with grpcs remote cache"
 }
 
-function test_remote_grpc_cache_with_legacy_tls_enabled() {
-  # Test that if default scheme for --remote_cache flag with --tls_enabled, remote cache works.
+function test_remote_grpc_cache() {
+  # Test that if default scheme for --remote_cache flag, remote cache works.
   _prepareBasicRule
 
   bazel build \
       --remote_cache=localhost:${worker_port} \
-      --tls_enabled=true \
       --tls_certificate="${cert_path}/ca.crt" \
       //a:foo \
-      || fail "Failed to build //a:foo with grpc --tls_enabled remote cache"
+      || fail "Failed to build //a:foo with grpc remote cache"
 }
 
 function test_remote_https_cache() {
@@ -109,50 +108,12 @@ function test_remote_https_cache() {
       || fail "Failed to build //a:foo with https remote cache"
 }
 
-function test_remote_cache_with_incompatible_tls_enabled_removed_default_scheme() {
-  # Test that if default scheme for --remote_cache flag with --incompatible_tls_enabled_removed, remote cache works.
-  _prepareBasicRule
-
-  bazel build \
-      --remote_cache=localhost:${worker_port} \
-      --incompatible_tls_enabled_removed=true \
-      --tls_certificate="${cert_path}/ca.crt" \
-      //a:foo \
-      || fail "Failed to build //a:foo with default(grpcs) remote cache"
-}
-
-function test_remote_cache_with_incompatible_tls_enabled_removed_grpcs_scheme() {
-  # Test that if 'grpcs' scheme for --remote_cache flag with --incompatible_tls_enabled_removed, remote cache works.
-  _prepareBasicRule
-
-  bazel build \
-      --remote_cache=grpcs://localhost:${worker_port} \
-      --incompatible_tls_enabled_removed=true \
-      --tls_certificate="${cert_path}/ca.crt" \
-      //a:foo \
-      || fail "Failed to build //a:foo with grpcs remote cache"
-}
-
 function test_remote_cache_with_incompatible_tls_enabled_removed_grpc_scheme() {
-  # Test that if 'grpc' scheme for --remote_cache flag with --incompatible_tls_enabled_removed, remote cache fails.
-  _prepareBasicRule
-
-  bazel build \
-      --remote_cache=grpc://localhost:${worker_port} \
-      --incompatible_tls_enabled_removed=true \
-      --tls_certificate="${cert_path}/ca.crt" \
-      //a:foo \
-      && fail "Expected test failure" || true
-}
-
-function test_remote_cache_with_incompatible_tls_enabled_removed() {
-  # Test that if --incompatible_tls_enabled_removed=true and --tls_enabled=true an error is thrown
+  # Test that if 'grpc' scheme for --remote_cache flag, remote cache fails.
   _prepareBasicRule
 
   bazel build \
       --remote_cache=grpc://localhost:${worker_port} \
-      --tls_enabled=true \
-      --incompatible_tls_enabled_removed=true \
       --tls_certificate="${cert_path}/ca.crt" \
       //a:foo \
       && fail "Expected test failure" || true