Skip to content
Open
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("--" + RENOVATE_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 renovate enterprise");
parser.addArgument("--" + RENOVATE_GITHUB_APP_KEY)
.type(String.class)
.required(false)
.help("Path to the Github app key of renovate enterprise");
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 RENOVATE_GITHUB_APP_ID = "appId";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be more generic than renovate. The logic is to skip DFIU if a particular github app is installed

Copy link
Author

Choose a reason for hiding this comment

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

100%. WIll refactor the entire code to make it more generic.

public static final String RENOVATE_GITHUB_APP_KEY = "appKey";
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
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<>();
RenovateUtil renovateUtil = new RenovateUtil(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());
&& (renovateUtil.isRenovateEnabledOnRepository(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
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
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 RenovateUtil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
public class RenovateUtil {
public class GithubAppCheck {

Util is not a good descriptive name

Copy link
Author

Choose a reason for hiding this comment

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

Done.

private static final Logger log = LoggerFactory.getLogger(RenovateUtil.class);

private static String appId;
Copy link
Collaborator

Choose a reason for hiding this comment

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

these all need to be made final static is not a good practice

Copy link
Author

Choose a reason for hiding this comment

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

Done.

private static String privateKeyPath;
private static String jwt;
private static Instant jwtExpiry;
private static GitHub gitHub;

public RenovateUtil(final Namespace ns){
this.appId = ns.get(Constants.RENOVATE_GITHUB_APP_ID);
this.privateKeyPath = ns.get(Constants.RENOVATE_GITHUB_APP_KEY);
Copy link

Choose a reason for hiding this comment

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

Why do we have to do ns.get() in this case instead of calling directly from Constants?

Copy link
Author

@AvvariSaiBharadwaj AvvariSaiBharadwaj Aug 9, 2024

Choose a reason for hiding this comment

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

This is to call the CLI input to the Java code. The constant is the CLI identifier for the input.

jwt = null;
jwtExpiry = null;
gitHub = null;
try {
generateJWT(this.appId, this.privateKeyPath);
} catch (GeneralSecurityException | IOException exception) {
log.warn("Could not initialise JWT due to exception: {}", exception.getMessage());
}
try {
gitHub = new GitHubBuilder()
.withEndpoint(CommandLine.gitApiUrl(ns))
.withJwtToken(jwt)
.build();
} catch (IOException exception) {
log.warn("Could not initialise github due to exception: {}", exception.getMessage());
}
}

protected boolean isRenovateEnabledOnRepository(String fullRepoName){
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's make this more generic - to check for any app existance

Copy link
Author

Choose a reason for hiding this comment

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

Done.

refreshJwtIfNeeded(appId, privateKeyPath);
try {
gitHub.getApp().getInstallationByRepository(fullRepoName.split("/")[0], fullRepoName.split("/")[1]);
Copy link

Choose a reason for hiding this comment

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

From the owner's open source, looks like it is using GHApp instead of GithubBuilder for this logic: https://github.com/hub4j/github-api/blob/main/src/main/java/org/kohsuke/github/GHApp.java

I don't see such getApp() function be mentioned in GithubBuilder: https://github.com/hub4j/github-api/blob/main/src/main/java/org/kohsuke/github/GitHubBuilder.java

Copy link
Author

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

But github is a GithubBuilder object, not Github though, right?

Copy link
Author

Choose a reason for hiding this comment

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

return true;
} catch (HttpException exception) {
if (exception.getResponseCode() != 404) {
log.warn("Caught a HTTPException {} while trying to get app installation. Defaulting to False", exception.getMessage());
}
return false;
} catch (IOException exception) {
log.warn("Caught a IOException {} while trying to get app installation. Defaulting to False", exception.getMessage());
return false;
}
}

private static 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());
}
}
}

private static 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
.sign(algorithm);
jwtExpiry = now.plusSeconds(600);
}

private static 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);
}
}

}
Loading