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

[DRAFT] Update DFIU to verify whether renovate is installed #1133

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
10 changes: 10 additions & 0 deletions dockerfile-image-update/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,16 @@
<artifactId>logging-interceptor</artifactId>
<version>4.9.1</version>
</dependency>
<dependency>
<groupId>com.auth0</groupId>
<artifactId>java-jwt</artifactId>
<version>3.18.1</version>
</dependency>
<dependency>
<groupId>org.bouncycastle</groupId>
<artifactId>bcprov-jdk15on</artifactId>
<version>1.68</version>
</dependency>
</dependencies>

<build>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,14 @@ static ArgumentParser getArgumentParser() {
.setDefault(false) //To prevent null from being returned by the argument
.required(false)
.help("Enable debug logging, including git wire logs.");
parser.addArgument("--" + SKIP_GITHUB_APP_ID)
.type(String.class)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aren't IDs always integers?

Copy link

Choose a reason for hiding this comment

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

@afalko It does not matter as it would be parsed as a parameter in a request API call to Github API to generate a JWT token. We have tested this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not have the parser do the parsing for you upfront here?

Copy link

Choose a reason for hiding this comment

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

@afalko the reason for that is because appID will be parsed into the JWT token generation API call under the .withIssuer(appId) subcall, and this has to be parsed as a string type. You can see more here: https://www.baeldung.com/java-auth0-jwt

We can change it to Integer type if you truly think it is necessary and purposeful (to comply with the App ID integer type from Github App) and convert it to string when parsed into this API call

Copy link
Collaborator

@afalko afalko Aug 19, 2024

Choose a reason for hiding this comment

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

Gotcha - pass-through might be fine, but it likely be a difficult error message to understand (vs. hey, you need an integer here). I think it is purposeful to make this integer for that release, but not necessary :)

.required(false)
.help("Github app ID of the Github App upon whose presence we skip sending the DFIU PR.");
parser.addArgument("--" + SKIP_GITHUB_APP_KEY)
.type(String.class)
.required(false)
.help("Path to the Github app key of the Github App upon whose presence we skip sending the DFIU PR.");
return parser;
}

Expand Down Expand Up @@ -253,7 +261,7 @@ public static GitHubBuilder shouldAddWireLogger(final GitHubBuilder builder, fin
logger.setLevel(HttpLoggingInterceptor.Level.HEADERS);
logger.redactHeader("Authorization");

builder.withConnector(new OkHttpGitHubConnector(new OkHttpClient.Builder()
builder.withConnector(new OkHttpGitHubConnector(new OkHttpClient.Builder()
.addInterceptor(logger)
.build()));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ private Constants() {
public static final String IGNORE_IMAGE_STRING = "x";
public static final String FILE_NAMES_TO_SEARCH = "filenamestosearch";
public static final String RATE_LIMIT_PR_CREATION = "rate_limit_pr_creations";
public static final String SKIP_GITHUB_APP_ID = "skipAppId";
public static final String SKIP_GITHUB_APP_KEY = "skipAppKey";
public static final String DEBUG = "debug";
//max number of PRs to be sent (or tokens to be added) per DEFAULT_RATE_LIMIT_DURATION(per hour in this case)
public static final long DEFAULT_RATE_LIMIT = 60;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
package com.salesforce.dockerfileimageupdate.utils;

import com.auth0.jwt.JWT;
import com.auth0.jwt.algorithms.Algorithm;
import com.salesforce.dockerfileimageupdate.CommandLine;
import net.sourceforge.argparse4j.inf.Namespace;
import org.bouncycastle.util.io.pem.PemReader;
import org.kohsuke.github.GitHub;
import org.kohsuke.github.GitHubBuilder;
import org.kohsuke.github.HttpException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.File;
import java.io.FileReader;
import java.io.IOException;
import java.security.GeneralSecurityException;
import java.security.KeyFactory;
import java.security.Security;
import java.security.interfaces.RSAPrivateKey;
import java.security.spec.PKCS8EncodedKeySpec;
import java.time.Instant;
import java.util.Date;

public class GithubAppCheck {
private static final Logger log = LoggerFactory.getLogger(GithubAppCheck.class);

private final String appId;
private final String privateKeyPath;
private String jwt;
private Instant jwtExpiry;
private GitHub gitHub;

public GithubAppCheck(final Namespace ns){
this.appId = ns.get(Constants.SKIP_GITHUB_APP_ID);
this.privateKeyPath = ns.get(Constants.SKIP_GITHUB_APP_KEY);
this.jwt = null;
this.jwtExpiry = null;
this.gitHub = null;
if (this.appId != null && this.privateKeyPath != null) {
try {
generateJWT(this.appId, this.privateKeyPath);
} catch (GeneralSecurityException | IOException exception) {
log.warn("Could not initialise JWT due to exception: {}", exception.getMessage());
}
try {
this.gitHub = new GitHubBuilder()
.withEndpoint(CommandLine.gitApiUrl(ns))
.withJwtToken(jwt)
.build();
} catch (IOException exception) {
log.warn("Could not initialise github due to exception: {}", exception.getMessage());
}
}
else {
log.warn("Could not find any Github app ID and Github app Key in the declared list. Hence assuming this class is no longer needed");
}
}

/**
* Method to verify whether the github app is installed on a repository or not.
* @param fullRepoName = The repository full name, i.e, of the format "owner/repoName". Eg: "Salesforce/dockerfile-image-update"
* @return True if github app is installed, false otherwise.
*/
protected boolean isGithubAppEnabledOnRepository(String fullRepoName){
refreshJwtIfNeeded(appId, privateKeyPath);
try {
gitHub.getApp().getInstallationByRepository(fullRepoName.split("/")[0], fullRepoName.split("/")[1]);
return true;
} catch (HttpException exception) {
Copy link
Member

Choose a reason for hiding this comment

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

If we're defaulting to false, there's a risk of false negatives.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but I am trying to be cautious. Since we are dealing with docker patches, isn't it better to get both DFIU and Renovate PRs and have a confusing experience, rather than not receive either PR and fall behind on your patches?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree here

if (exception.getResponseCode() != 404) {
// Log for any HTTP status code other than 404 Not found.
log.warn("Caught a HTTPException {} while trying to get app installation. Defaulting to False", exception.getMessage());
}
return false;
} catch (IOException exception) {
// Most often happens on timeout scenarios.
log.warn("Caught a IOException {} while trying to get app installation. Defaulting to False", exception.getMessage());
return false;
}
}

/**
* Method to refresh the JWT token if needed. Checks the JWT expiry time, and if it is 60s away from expiring, refreshes it.
* @param appId = The id of the Github App to generate the JWT for
* @param privateKeyPath = The path to the private key of the Github App to generate the JWT for
*/
private void refreshJwtIfNeeded(String appId, String privateKeyPath){
if (jwt == null || jwtExpiry.isBefore(Instant.now().minusSeconds(60))) { // Adding a buffer to ensure token validity
try {
generateJWT(appId, privateKeyPath);
} catch (IOException | GeneralSecurityException exception) {
log.warn("Could not refresh the JWT due to exception: {}", exception.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this cause a problem? If we simply ignore this exception and default the value to false in the isGithubAppEnabledOnRepository method, it means that if the JWT refresh fails, we'll end up creating PRs even if the repository isn't onboarded to Renovate.

Copy link
Author

Choose a reason for hiding this comment

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

Same comment as above. I am trying to be cautious. Lmk if you think otherwise.

Copy link

Choose a reason for hiding this comment

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

Yes, but I am trying to be cautious. Since we are dealing with docker patches, isn't it better to get both DFIU and Renovate PRs and have a confusing experience, rather than not receive either PR and fall behind on your patches?

I agree with Sai in this case

}
}
}

/**
* Method to generate the JWT used to access the Github App APIs. We generate the JWT to be valid for 600 seconds.
* Along with the JWT value, the jwtExpiry value is set to the time of 600 sec from now.
* @param appId = The id of the Github App to generate the JWT for
* @param privateKeyPath = The path to the private key of the Github App to generate the JWT for
* @throws IOException
* @throws GeneralSecurityException
*/
private void generateJWT(String appId, String privateKeyPath) throws IOException, GeneralSecurityException {
Security.addProvider(new org.bouncycastle.jce.provider.BouncyCastleProvider());
RSAPrivateKey privateKey = getRSAPrivateKey(privateKeyPath);

Algorithm algorithm = Algorithm.RSA256(null, privateKey);
Instant now = Instant.now();
jwt = JWT.create()
.withIssuer(appId)
.withIssuedAt(Date.from(now))
.withExpiresAt(Date.from(now.plusSeconds(600))) // 10 minutes expiration
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this hardcoded?

Copy link
Collaborator

Choose a reason for hiding this comment

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

what will the behavior be if you set this really short (for example 1 second)

Copy link

Choose a reason for hiding this comment

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

@afalko we would like to avoid as much API calls as possible. Setting to 1 second means that we have to make an API call to regenerate a new JWT token every 1 second. Taking into account that the DFIU takes hours to run, I believe it is not efficient

.sign(algorithm);
jwtExpiry = now.plusSeconds(600);
}

/**
* The method to get the private key in an RSA Encoded format. Makes use of org.bouncycastle.util
* @param privateKeyPath
* @return
* @throws IOException
* @throws GeneralSecurityException
*/
private RSAPrivateKey getRSAPrivateKey(String privateKeyPath) throws IOException, GeneralSecurityException {
try (PemReader pemReader = new PemReader(new FileReader(new File(privateKeyPath)))) {
PKCS8EncodedKeySpec spec = new PKCS8EncodedKeySpec(pemReader.readPemObject().getContent());
KeyFactory keyFactory = KeyFactory.getInstance("RSA");
return (RSAPrivateKey) keyFactory.generatePrivate(spec);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -25,15 +25,16 @@ public void prepareToCreate(final Namespace ns,
pullRequestSender.forkRepositoriesFoundAndGetPathToDockerfiles(contentsFoundWithImage, gitForkBranch);
List<IOException> exceptions = new ArrayList<>();
List<String> skippedRepos = new ArrayList<>();
GithubAppCheck githubAppCheck = new GithubAppCheck(ns);
for (String currUserRepo : pathToDockerfilesInParentRepo.keySet()) {
Optional<GitHubContentToProcess> forkWithContentPaths =
pathToDockerfilesInParentRepo.get(currUserRepo).stream().findFirst();
if (forkWithContentPaths.isPresent()) {
try {
//If the repository has been onboarded to renovate enterprise, skip sending the DFIU PR
if(ns.getBoolean(Constants.CHECK_FOR_RENOVATE)
&& (isRenovateEnabled(Constants.RENOVATE_CONFIG_FILEPATHS, forkWithContentPaths.get()))) {
log.info("Found a renovate configuration file in the repo {}. Skip sending DFIU PRs to this repository.", forkWithContentPaths.get().getParent().getFullName());
&& (githubAppCheck.isGithubAppEnabledOnRepository(forkWithContentPaths.get().getParent().getFullName()))) {
log.info("The repo {} is onboarded onto Renovate.Hence, skip sending DFIU PRs to this repository.", forkWithContentPaths.get().getParent().getFullName());
} else {
dockerfileGitHubUtil.changeDockerfiles(ns,
pathToDockerfilesInParentRepo,
Expand Down
Loading
Loading