Merge pull request #19482 from vespa-engine/bjorncs/s3-policy

Bjorncs/s3 policy

4 files changed, 31 insertions, 41 deletions

diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveService.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveService.java
index 04e52c59d7a..5363e8d0150 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveService.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/ArchiveService.java
@@ -5,7 +5,6 @@ import com.yahoo.config.provision.TenantName;
 import com.yahoo.config.provision.zone.ZoneId;
 
 import java.util.Map;
-import java.util.Set;
 
 /**
  * Service that manages archive storage URIs for tenant nodes.
@@ -15,7 +14,7 @@ import java.util.Set;
  */
 public interface ArchiveService {
 
-    ArchiveBucket createArchiveBucketFor(ZoneId zoneId);
+    ArchiveBucket createArchiveBucketFor(ZoneId zoneId, boolean sharded);
 
     void updateBucketAndKeyPolicy(ZoneId zoneId, ArchiveBucket bucket, Map<TenantName, String> authorizeIamRoleByTenantName);
 }
diff --git a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/MockArchiveService.java b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/MockArchiveService.java
index c8e79a84925..5c979ddfc7b 100644
--- a/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/MockArchiveService.java
+++ b/controller-api/src/main/java/com/yahoo/vespa/hosted/controller/api/integration/archive/MockArchiveService.java
@@ -16,7 +16,7 @@ public class MockArchiveService implements ArchiveService {
     public Map<ArchiveBucket, Map<TenantName, String>> authorizedIamRoles = new HashMap<>();
 
     @Override
-    public ArchiveBucket createArchiveBucketFor(ZoneId zoneId) {
+    public ArchiveBucket createArchiveBucketFor(ZoneId zoneId, boolean sharded) {
         return new ArchiveBucket("bucketName", "keyArn");
     }
 
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/archive/CuratorArchiveBucketDb.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/archive/CuratorArchiveBucketDb.java
index a7555307a59..ce5869af1a0 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/archive/CuratorArchiveBucketDb.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/archive/CuratorArchiveBucketDb.java
@@ -17,6 +17,7 @@ import java.net.URI;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Optional;
+import java.util.OptionalInt;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.stream.Collectors;
@@ -29,14 +30,6 @@ import java.util.stream.Collectors;
 public class CuratorArchiveBucketDb {
 
     /**
-     * Due to policy limits, we can't put data for more than this many tenants in a bucket.
-     * Policy size limit is 20kb, about 550 bytes for non-tenant related policies. Each tenant
-     * needs about 500 + len(role_arn) bytes, we limit role_arn to 100 characters, so we can
-     * fit about (20k - 550) / 600 ~ 32 tenants per bucket.
-     */
-    private final static int TENANTS_PER_BUCKET = 30;
-
-    /**
      * Archive URIs are often requested because they are returned in /application/v4 API. Since they
      * never change, it's safe to cache them and only update on misses
      */
@@ -84,7 +77,7 @@ public class CuratorArchiveBucketDb {
                     .orElseGet(() -> {
                         // If not, find an existing bucket with space
                         Optional<ArchiveBucket> unfilledBucket = zoneBuckets.stream()
-                                .filter(bucket -> bucket.tenants().size() < TENANTS_PER_BUCKET)
+                                .filter(bucket -> bucket.tenants().size() < tenantsPerBucket().orElse(Integer.MAX_VALUE))
                                 .findAny();
 
                         // And place the tenant in that bucket.
@@ -99,7 +92,8 @@ public class CuratorArchiveBucketDb {
                         }
 
                         // We'll have to create a new bucket
-                        var newBucket = archiveService.createArchiveBucketFor(zoneId).withTenant(tenant);
+                        var newBucket = archiveService.createArchiveBucketFor(zoneId, tenantsPerBucket().isPresent())
+                                .withTenant(tenant);
                         zoneBuckets.add(newBucket);
                         curatorDb.writeArchiveBuckets(zoneId, zoneBuckets);
                         updateArchiveUriCache(zoneId, zoneBuckets);
@@ -121,6 +115,23 @@ public class CuratorArchiveBucketDb {
         return bucketName;
     }
 
+    private OptionalInt tenantsPerBucket() {
+        if (system.isPublic()) {
+            /*
+             * Due to policy limits, we can't put data for more than this many tenants in a bucket.
+             * Policy size limit is 20kb, about 550 bytes for non-tenant related policies. Each tenant
+             * needs about 500 + len(role_arn) bytes, we limit role_arn to 100 characters, so we can
+             * fit about (20k - 550) / 600 ~ 32 tenants per bucket.
+             */
+            return OptionalInt.of(30);
+        } else {
+            /*
+             * The S3 policies in main/cd have a fixed size.
+             */
+            return OptionalInt.empty();
+        }
+    }
+
     private Optional<String> getBucketNameFromCache(ZoneId zoneId, TenantName tenantName) {
         return Optional.ofNullable(archiveUriCache.get(zoneId)).map(map -> map.get(tenantName));
     }
diff --git a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveAccessMaintainer.java b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveAccessMaintainer.java
index 0ed2e930c57..e333982cc18 100644
--- a/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveAccessMaintainer.java
+++ b/controller-server/src/main/java/com/yahoo/vespa/hosted/controller/maintenance/ArchiveAccessMaintainer.java
@@ -3,24 +3,20 @@ package com.yahoo.vespa.hosted.controller.maintenance;
 
 import com.google.common.collect.Maps;
 import com.yahoo.config.provision.TenantName;
-import com.yahoo.config.provision.zone.ZoneApi;
 import com.yahoo.config.provision.zone.ZoneId;
 import com.yahoo.jdisc.Metric;
 import com.yahoo.vespa.flags.BooleanFlag;
-import com.yahoo.vespa.flags.FetchVector;
 import com.yahoo.vespa.flags.Flags;
 import com.yahoo.vespa.hosted.controller.Controller;
 import com.yahoo.vespa.hosted.controller.api.integration.archive.ArchiveService;
 import com.yahoo.vespa.hosted.controller.api.integration.zone.ZoneRegistry;
 import com.yahoo.vespa.hosted.controller.archive.CuratorArchiveBucketDb;
-import com.yahoo.vespa.hosted.controller.tenant.AthenzTenant;
 import com.yahoo.vespa.hosted.controller.tenant.CloudTenant;
 import com.yahoo.vespa.hosted.controller.tenant.Tenant;
 
 import java.time.Duration;
 import java.util.List;
 import java.util.Map;
-import java.util.function.Function;
 import java.util.stream.Collectors;
 
 /**
@@ -60,7 +56,7 @@ public class ArchiveAccessMaintainer extends ControllerMaintainer {
         zoneRegistry.zones().controllerUpgraded().zones().forEach(z -> {
                     ZoneId zoneId = z.getId();
                     try {
-                        var tenantArchiveAccessRoles = tenantArchiveAccessRoles(z);
+                        var tenantArchiveAccessRoles = cloudTenantArchiveExternalAccessRoles();
                         archiveBucketDb.buckets(zoneId).forEach(archiveBucket ->
                                 archiveService.updateBucketAndKeyPolicy(zoneId, archiveBucket,
                                         Maps.filterEntries(tenantArchiveAccessRoles,
@@ -75,30 +71,14 @@ public class ArchiveAccessMaintainer extends ControllerMaintainer {
         return 1.0;
     }
 
-    private Map<TenantName, String> tenantArchiveAccessRoles(ZoneApi zone) {
+    private Map<TenantName, String> cloudTenantArchiveExternalAccessRoles() {
         List<Tenant> tenants = controller().tenants().asList();
-        if (zoneRegistry.system().isPublic()) {
-            return tenants.stream()
-                    .filter(t -> t instanceof CloudTenant)
-                    .map(t -> (CloudTenant) t)
-                    .filter(t -> t.archiveAccessRole().isPresent())
-                    .collect(Collectors.toUnmodifiableMap(
-                            Tenant::name, cloudTenant -> cloudTenant.archiveAccessRole().orElseThrow()));
-        } else {
-            return tenants.stream()
-                    .filter(t -> t instanceof AthenzTenant
-                            && enabled(archiveEnabled, t, zone) && enabled(developerRoleEnabled, t, zone))
-                    .map(Tenant::name)
-                    .collect(Collectors.toUnmodifiableMap(
-                            Function.identity(), t -> zoneRegistry.tenantDeveloperRoleArn(t).orElseThrow()));
-
-        }
-    }
-
-    private boolean enabled(BooleanFlag flag, Tenant tenant, ZoneApi zone) {
-        return flag.with(FetchVector.Dimension.TENANT_ID, tenant.name().value())
-                .with(FetchVector.Dimension.ZONE_ID, zone.getId().value())
-                .value();
+        return tenants.stream()
+                .filter(t -> t instanceof CloudTenant)
+                .map(t -> (CloudTenant) t)
+                .filter(t -> t.archiveAccessRole().isPresent())
+                .collect(Collectors.toUnmodifiableMap(
+                        Tenant::name, cloudTenant -> cloudTenant.archiveAccessRole().orElseThrow()));
     }
 
 }