Skip to content

Commit

Permalink
[PLAT-13150] Move sshUser/Port at bundle details level
Browse files Browse the repository at this point in the history 
Summary:
Previously, we were having sshUser/Port Override information at
bundle->details->region->info level, which would make more sense at bundle->details level.
This diff changes the same, also marking the previous behaviour as deprecated.

Also, brings back old logic of saving arch at region level that will still be used in case
`yb.provider.vm_os_patching` is not set. Will remove that bit once the above flag is turned on by
default.

Adds fine grained check while editing the in use bundles. We will allow only addition of new region AMI details in the bundle, otherwise information like sshUser/Port cannot be modified for in use bundle.

Test Plan:
Created a provider using old/new payload.
Ensured that the details are stored as per the desired format.
Performed OS patching.
Performed Day2 operations.
Tried modifying the used bundle (both via Provider PUT/ Image Bundle PUT) - verified that it failed.

Took portal backup - ran migration against it.

Reviewers: #yba-api-review, sneelakantan, amalyshev

Reviewed By: #yba-api-review, amalyshev

Subscribers: yugaware

Differential Revision: https://phorge.dev.yugabyte.com/D33205
  • Loading branch information
@Vars-07
Vars-07 committed Mar 21, 2024
1 parent 5f47ca3 commit d457fc4
Show file tree
Hide file tree
Showing 12 changed files with 312 additions and 42 deletions.
57 changes: 42 additions & 15 deletions ...rc/main/java/com/yugabyte/yw/commissioner/tasks/subtasks/cloud/CloudImageBundleSetup.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,8 @@ public static void generateYBADefaultImageBundle(
CloudQueryHelper cloudQueryHelper,
Architecture arch,
boolean isDefault,
boolean forceFetchFromMetadata) {
boolean forceFetchFromMetadata,
boolean enableVMOSPatching) {
List<Region> regions = provider.getRegions();
CloudType cloudType = provider.getCloudCode();
if (arch == null) {
Expand Down Expand Up @@ -151,14 +152,16 @@ public static void generateYBADefaultImageBundle(
// We are doing this as, once the bundle is configured with the custom image,
// we should get rid of the image in the region object, so that during bundle
// update it does not pick the image from region.
r.setYbImage(null);
r.save();
if (enableVMOSPatching) {
r.setYbImage(null);
r.save();
}
}
bundleInfo.setYbImage(ybImage);
if (isCustomImage) {
bundleInfo.setSshUserOverride(provider.getDetails().getSshUser());
details.setSshUser(provider.getDetails().getSshUser());
} else {
bundleInfo.setSshUserOverride(cloudType.getSshUser());
details.setSshUser(cloudType.getSshUser());
}
regionsImageInfo.put(r.getCode(), bundleInfo);
}
Expand Down Expand Up @@ -187,14 +190,22 @@ public static void generateYBADefaultImageBundle(
// We are doing this as, once the bundle is configured with the custom image,
// we should get rid of the image in the region object, so that during bundle
// update it does not pick the image from region.
regions.stream()
.forEach(
r -> {
r.setYbImage(null);
r.save();
});
if (enableVMOSPatching) {
regions.stream()
.forEach(
r -> {
r.setYbImage(null);
r.save();
});
}
}
details.setGlobalYbImage(ybImage);
details.setSshUser(provider.getDetails().getSshUser());
}
if (provider.getDetails().getSshPort() != null) {
details.setSshPort(provider.getDetails().getSshPort());
} else {
details.setSshPort(22);
}
// If the bundle is not specified we will create YBA default with the type
// YBA_ACTIVE.
Expand All @@ -217,6 +228,7 @@ public void run() {
List<Region> regions = provider.getRegions();

List<ImageBundle> imageBundles = taskParams().imageBundles;
boolean enableVMOSPatching = confGetter.getGlobalConf(GlobalConfKeys.enableVMOSPatching);
if ((imageBundles == null || imageBundles.size() == 0)
&& provider.getImageBundles().size() == 0
&& !taskParams().updateBundleRequest) {
Expand All @@ -240,7 +252,8 @@ public void run() {
}
}
}
generateYBADefaultImageBundle(provider, cloudQueryHelper, arch, true, false);
generateYBADefaultImageBundle(
provider, cloudQueryHelper, arch, true, false, enableVMOSPatching);
} else if (imageBundles != null) {
Map<UUID, ImageBundle> existingImageBundles =
provider.getImageBundles().stream()
Expand All @@ -252,7 +265,6 @@ public void run() {
// creation failed in first try mid-way, we will delete up existing Bundles
existingImageBundles.forEach((bundleUUID, bundle) -> bundle.delete());
}
boolean enableVMOSPatching = confGetter.getGlobalConf(GlobalConfKeys.enableVMOSPatching);
for (ImageBundle bundle : imageBundles) {
if (taskParams().updateBundleRequest && bundle.getUuid() != null) {
updateBundles(
Expand Down Expand Up @@ -317,7 +329,7 @@ private void updateYBAActiveImageBundles(
String defaultRegionImage =
cloudQueryHelper.getDefaultImage(region, bundle.getDetails().getArch().toString());
info.setYbImage(defaultRegionImage);
info.setSshUserOverride(provider.getDetails().getSshUser());
details.setSshUser(provider.getDetails().getSshUser());

regionBundleInfo.put(region.getCode(), info);
details.setRegions(regionBundleInfo);
Expand Down Expand Up @@ -357,12 +369,21 @@ private void createBundle(Provider provider, List<Region> regions, ImageBundle b
bundle.setName(getDefaultImageBundleName(provider.getCode()));
String defaultImage = cloudQueryHelper.getDefaultImage(region, arch.toString());
bundleInfo.setYbImage(defaultImage);
bundleInfo.setSshUserOverride(cloudType.getSshUser());
details.setSshUser(cloudType.getSshUser());
details.setSshPort(22);
// If we are populating the ybImage, bundle will be YBA_DEFAULT.
metadata.setType(ImageBundleType.YBA_ACTIVE);
metadata.setVersion(CLOUD_OS_MAP.get(provider.getCode()).getVersion());
} else {
// In case user specified the AMI ids bundle will be CUSTOM.
if (StringUtils.isNotBlank(bundleInfo.getSshUserOverride())) {
details.setSshUser(bundleInfo.getSshUserOverride());
bundleInfo.setSshUserOverride(null);
}
if (bundleInfo.getSshPortOverride() != null) {
details.setSshPort(bundleInfo.getSshPortOverride());
bundleInfo.setSshPortOverride(null);
}
metadata.setType(ImageBundleType.CUSTOM);
}

Expand All @@ -386,6 +407,12 @@ private void createBundle(Provider provider, List<Region> regions, ImageBundle b
// In case user specified the image id bundle will be CUSTOM.
metadata.setType(ImageBundleType.CUSTOM);
}
if (details.getSshPort() == null) {
details.setSshPort(22);
}
if (StringUtils.isBlank(details.getSshUser())) {
details.setSshUser(cloudType.getSshUser());
}
}
if (bundle.getUseAsDefault()) {
// Check for the existence of no other default image bundle for the provider.
Expand Down
2 changes: 1 addition & 1 deletion managed/src/main/java/com/yugabyte/yw/common/CloudProviderHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -931,7 +931,7 @@ private void validateProviderEditPayload(Provider provider, Provider editProvide
}
ImageBundle currentImageBundle = currentImageBundles.get(uuid);
if (imageBundle.getUniverseCount() > 0
&& currentImageBundle.isUpdateNeeded(imageBundle)) {
&& !currentImageBundle.allowUpdateDuringUniverseAssociation(imageBundle)) {
throw new PlatformServiceException(
BAD_REQUEST,
String.format(
Expand Down
54 changes: 53 additions & 1 deletion managed/src/main/java/com/yugabyte/yw/common/CloudRegionHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,11 @@
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.google.inject.Inject;
import com.yugabyte.yw.cloud.PublicCloudConstants.Architecture;
import com.yugabyte.yw.commissioner.Common.CloudType;
import com.yugabyte.yw.commissioner.tasks.CloudBootstrap.Params.PerRegionMetadata;
import com.yugabyte.yw.common.config.GlobalConfKeys;
import com.yugabyte.yw.common.config.RuntimeConfGetter;
import com.yugabyte.yw.models.AvailabilityZone;
import com.yugabyte.yw.models.Provider;
import com.yugabyte.yw.models.Region;
Expand All @@ -23,12 +26,15 @@
public class CloudRegionHelper {

private final CloudQueryHelper queryHelper;
private final RuntimeConfGetter confGetter;
private final ConfigHelper configHelper;

@Inject
public CloudRegionHelper(CloudQueryHelper queryHelper, ConfigHelper configHelper) {
public CloudRegionHelper(
CloudQueryHelper queryHelper, ConfigHelper configHelper, RuntimeConfGetter confGetter) {
this.queryHelper = queryHelper;
this.configHelper = configHelper;
this.confGetter = confGetter;
}

public Region createRegion(
Expand Down Expand Up @@ -77,6 +83,52 @@ public Region createRegion(
// image reference.
region.setYbImage(customImageId);
region.update();
} else if (!confGetter.getGlobalConf(GlobalConfKeys.enableVMOSPatching)) {
/*
* Todo: Remove this once flag `yb.provider.vm_os_pathcing` turned on by default, else
* the instances will not be correctly configured for the region b/c of missing arch
* in the region object.
*/
switch (CloudType.valueOf(provider.getCode())) {
// Intentional fallthrough for AWS, Azure & GCP should be covered the same way.
case aws:
case gcp:
case azu:
// Setup default image, if no custom one was specified.
String defaultImage = queryHelper.getDefaultImage(region, architecture);
if (defaultImage == null || defaultImage.isEmpty()) {
throw new RuntimeException("Could not get default image for region: " + regionCode);
}
region.setYbImage(defaultImage);
region.update();
break;
}

// Attempt to find architecture for AWS providers.
if (provider.getCode().equals(CloudType.aws.toString())
&& (region.getArchitecture() == null
|| (customImageId != null && !customImageId.isEmpty()))) {
String arch = queryHelper.getImageArchitecture(region);
if (arch == null || arch.isEmpty()) {
log.warn(
"Could not get architecture for image {} in region {}.",
region.getYbImage(),
region.getCode());

} else {
try {
// explicitly overriding arch name to maintain equivalent type of architecture.
if (arch.equals("arm64")) {
arch = Architecture.aarch64.name();
}
region.setArchitecture(Architecture.valueOf(arch));
region.update();
} catch (IllegalArgumentException e) {
log.warn(
"{} not a valid architecture. Skipping for region {}.", arch, region.getCode());
}
}
}
}
String customSecurityGroupId = metadata.customSecurityGroupId;
if (customSecurityGroupId != null && !customSecurityGroupId.isEmpty()) {
Expand Down
30 changes: 23 additions & 7 deletions managed/src/main/java/com/yugabyte/yw/common/ImageBundleUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.UUID;
import java.util.stream.Collectors;
import lombok.extern.slf4j.Slf4j;
import org.apache.commons.lang3.StringUtils;

@Slf4j
public class ImageBundleUtil {
Expand All @@ -44,6 +45,7 @@ public ImageBundle.NodeProperties getNodePropertiesOrFail(
ImageBundle.NodeProperties properties = new ImageBundle.NodeProperties();
ImageBundle bundle = ImageBundle.getOrBadRequest(imageBundleUUID);
ProviderDetails providerDetails = bundle.getProvider().getDetails();
ImageBundleDetails bundleDetails = bundle.getDetails();
if (Common.CloudType.aws.toString().equals(cloudCode)) {
Map<String, ImageBundleDetails.BundleInfo> regionsBundleInfo =
bundle.getDetails().getRegions();
Expand All @@ -53,8 +55,8 @@ public ImageBundle.NodeProperties getNodePropertiesOrFail(
if (regionsBundleInfo.containsKey(region)) {
bundleInfo = regionsBundleInfo.get(region);
properties.setMachineImage(bundleInfo.getYbImage());
properties.setSshPort(bundleInfo.getSshPortOverride());
properties.setSshUser(bundleInfo.getSshUserOverride());
properties.setSshPort(bundleDetails.getSshPort());
properties.setSshUser(bundleDetails.getSshUser());
} else if (imageBundleValidationDisabled) {
// In case the region object is not present in the imageBundle, & we have
// disabled imageBundleValidation, add the empty BundleInfo object for that region.
Expand Down Expand Up @@ -88,8 +90,16 @@ public ImageBundle.NodeProperties getNodePropertiesOrFail(
}
} else {
properties.setMachineImage(bundle.getDetails().getGlobalYbImage());
properties.setSshUser(providerDetails.getSshUser());
properties.setSshPort(providerDetails.getSshPort());
String sshUser = bundleDetails.getSshUser();
if (StringUtils.isBlank(sshUser)) {
sshUser = providerDetails.getSshUser();
}
Integer sshPort = bundleDetails.getSshPort();
if (sshPort == null) {
sshPort = providerDetails.getSshPort();
}
properties.setSshUser(sshUser);
properties.setSshPort(sshPort);
}

return properties;
Expand All @@ -114,7 +124,6 @@ public void updateImageBundleIfRequired(
}
if (ybImage != null) {
info.setYbImage(ybImage);
info.setSshUserOverride(provider.getDetails().getSshUser());
}
bundle.getDetails().setRegions(bundleInfo);
bundle.update();
Expand Down Expand Up @@ -161,6 +170,7 @@ public static ImageBundle getDefaultBundleForUniverse(
}

public void migrateImageBundlesForProviders(Provider provider) {
boolean enableVMOSPatching = runtimeConfGetter.getGlobalConf(GlobalConfKeys.enableVMOSPatching);
List<ImageBundle> bundles = provider.getImageBundles();
if (bundles.size() == 0) {
return;
Expand Down Expand Up @@ -211,14 +221,20 @@ public void migrateImageBundlesForProviders(Provider provider) {

// Populate the new YBA_ACTIVE bundle for x86 arch.
CloudImageBundleSetup.generateYBADefaultImageBundle(
provider, cloudQueryHelper, Architecture.x86_64, x86YBADefaultBundleMarkedDefault, true);
provider,
cloudQueryHelper,
Architecture.x86_64,
x86YBADefaultBundleMarkedDefault,
true,
enableVMOSPatching);
// Populate the new YBA_ACTIVE bundle for aarch64 arch.
CloudImageBundleSetup.generateYBADefaultImageBundle(
provider,
cloudQueryHelper,
Architecture.aarch64,
aarch64YBADefaultBundleMarkedDefault,
true);
true,
enableVMOSPatching);
}

public Map<UUID, ImageBundle> collectUniversesImageBundles() {
Expand Down
17 changes: 14 additions & 3 deletions managed/src/main/java/com/yugabyte/yw/controllers/ImageBundleController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -144,9 +144,9 @@ public Result index(UUID customerUUID, UUID providerUUID, UUID imageBundleUUID)
@YbaApi(visibility = YbaApiVisibility.PREVIEW, sinceYBAVersion = "2.20.0.0")
public Result edit(UUID customerUUID, UUID providerUUID, UUID iBUUID, Http.Request request) {
final Provider provider = Provider.getOrBadRequest(customerUUID, providerUUID);
checkImageBundleUsageInUniverses(providerUUID, iBUUID);

ImageBundle bundle = parseJsonAndValidate(request, ImageBundle.class);
checkImageBundleUsageInUniverses(providerUUID, iBUUID, bundle);

ImageBundle cBundle = imageBundleHandler.edit(provider, iBUUID, bundle);
auditService()
.createAuditEntryWithReqBody(
Expand Down Expand Up @@ -182,10 +182,21 @@ public Result delete(UUID customerUUID, UUID providerUUID, UUID iBUUID, Http.Req
}

private void checkImageBundleUsageInUniverses(UUID providerUUID, UUID imageBundleUUID) {
checkImageBundleUsageInUniverses(providerUUID, imageBundleUUID, null);
}

private void checkImageBundleUsageInUniverses(
UUID providerUUID, UUID imageBundleUUID, ImageBundle bundle) {
ImageBundle iBundle = ImageBundle.getOrBadRequest(providerUUID, imageBundleUUID);
long universeCount = iBundle.getUniverseCount();

if (universeCount > 0) {
if (universeCount > 0 && bundle == null) {
throw new PlatformServiceException(
FORBIDDEN,
String.format(
"There %s %d universe%s using this imageBundle, cannot delete",
universeCount > 1 ? "are" : "is", universeCount, universeCount > 1 ? "s" : ""));
} else if (universeCount > 0 && !bundle.allowUpdateDuringUniverseAssociation(iBundle)) {
throw new PlatformServiceException(
FORBIDDEN,
String.format(
Expand Down
36 changes: 35 additions & 1 deletion managed/src/main/java/com/yugabyte/yw/models/ImageBundle.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import com.fasterxml.jackson.annotation.JsonBackReference;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.yugabyte.yw.cloud.PublicCloudConstants.Architecture;
import com.yugabyte.yw.commissioner.Common.CloudType;
import com.yugabyte.yw.common.ImageBundleUtil;
import com.yugabyte.yw.common.PlatformServiceException;
import com.yugabyte.yw.forms.UniverseDefinitionTaskParams.Cluster;
Expand All @@ -19,6 +20,7 @@
import jakarta.persistence.Id;
import jakarta.persistence.ManyToOne;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.UUID;
Expand Down Expand Up @@ -202,7 +204,39 @@ public static List<ImageBundle> getYBADefaultBundles(UUID providerUUID) {
@JsonIgnore
public boolean isUpdateNeeded(ImageBundle bundle) {
return !Objects.equals(this.getUseAsDefault(), bundle.getUseAsDefault())
|| !Objects.equals(this.getDetails(), bundle.getDetails());
|| !Objects.equals(this.getDetails(), bundle.getDetails())
|| (this.getDetails().getRegions() != null
&& bundle.getDetails().getRegions() != null
&& !this.getDetails().getRegions().equals(bundle.getDetails().getRegions()));
}

@JsonIgnore
public boolean allowUpdateDuringUniverseAssociation(ImageBundle existingBundle) {
ImageBundleDetails existingDetails = existingBundle.getDetails();
ImageBundleDetails details = this.getDetails();
// We will allow fine grain edit in case the bundle is associated with
// the universe. We will allow addition of new AMI in the newly added
// region but will not allow any other edit.
if (existingBundle.getProvider().getCloudCode() == CloudType.aws) {
// Compare that AMI is not removed for any region in used bundle for AWS.
Map<String, ImageBundleDetails.BundleInfo> infoExistingBundle = existingDetails.getRegions();
Map<String, ImageBundleDetails.BundleInfo> info = details.getRegions();

// We will not allow any region AMI information to be removed from the used bundle.
boolean allMatch =
infoExistingBundle.entrySet().stream()
.allMatch(
entry -> {
String key = entry.getKey();
ImageBundleDetails.BundleInfo bundleInfo = entry.getValue();
return info.containsKey(key) && info.get(key).equals(bundleInfo);
});

if (!allMatch) {
return false;
}
}
return details.equals(existingDetails);
}

@JsonIgnore
Expand Down
Loading

0 comments on commit d457fc4

Please sign in to comment.