-
Notifications
You must be signed in to change notification settings - Fork 39
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
Solve the Application Login Issue After Domain Mapping #4
Changes from 29 commits
5be40b9
ccb98fb
66076d1
3ade89e
51f0f22
5f34c14
e29c0c5
486cd10
9024244
6b59ec5
b249b31
3360753
ee8f5f7
58b2bea
f8cd447
22b2e0f
b6e5099
bfb9280
3915c89
eebe154
e4aafa9
3d1fc59
62b8b7c
d554819
b86d02e
3321358
4a3c9d1
06affe0
cfdfba4
c74c513
92a0c75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -4,18 +4,22 @@ metadata: | |||||||||||||||||
name: bankapp-ingress | ||||||||||||||||||
namespace: bankapp-namespace | ||||||||||||||||||
annotations: | ||||||||||||||||||
nginx.ingress.kubernetes.io/rewrite-target: / | ||||||||||||||||||
nginx.ingress.kubernetes.io/proxy-body-size: "50m" | ||||||||||||||||||
nginx.ingress.kubernetes.io/ssl-redirect: "true" # Force HTTPS | ||||||||||||||||||
cert-manager.io/cluster-issuer: letsencrypt-prod # Use Let's Encrypt | ||||||||||||||||||
nginx.ingress.kubernetes.io/proxy-set-headers: "ingress-headers" | ||||||||||||||||||
nginx.ingress.kubernetes.io/rewrite-target: / | ||||||||||||||||||
nginx.ingress.kubernetes.io/affinity: cookie | ||||||||||||||||||
nginx.ingress.kubernetes.io/session-cookie-name: bankapp_session | ||||||||||||||||||
nginx.ingress.kubernetes.io/session-cookie-hash: sha1 | ||||||||||||||||||
Comment on lines
+12
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance cookie security settings. While session affinity is correctly configured, consider adding these security-focused annotations for the session cookie: nginx.ingress.kubernetes.io/affinity: cookie
nginx.ingress.kubernetes.io/session-cookie-name: bankapp_session
nginx.ingress.kubernetes.io/session-cookie-hash: sha1
+ nginx.ingress.kubernetes.io/session-cookie-secure: "true"
+ nginx.ingress.kubernetes.io/session-cookie-samesite: "Strict" 📝 Committable suggestion
Suggested change
|
||||||||||||||||||
spec: | ||||||||||||||||||
tls: | ||||||||||||||||||
- hosts: | ||||||||||||||||||
- amitabh.letsdeployit.com | ||||||||||||||||||
- bankapp.snehaws.com | ||||||||||||||||||
secretName: bankapp-tls-secret | ||||||||||||||||||
ingressClassName: nginx | ||||||||||||||||||
rules: | ||||||||||||||||||
- host: "amitabh.letsdeployit.com" | ||||||||||||||||||
- host: bankapp.snehaws.com | ||||||||||||||||||
http: | ||||||||||||||||||
paths: | ||||||||||||||||||
- path: / | ||||||||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
apiVersion: v1 | ||
kind: ConfigMap | ||
metadata: | ||
name: ingress-headers | ||
namespace: bankapp-namespace | ||
data: | ||
X-Forwarded-Host: "$host" | ||
X-Forwarded-Proto: "$scheme" | ||
X-Forwarded-For: "$proxy_add_x_forwarded_for" |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,17 @@ | ||||||
package com.yourpackage.config; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update the package declaration to match the project structure. The package name should be -package com.yourpackage.config;
+package com.example.bankapp.config; 📝 Committable suggestion
Suggested change
|
||||||
|
||||||
import org.springframework.context.annotation.Bean; | ||||||
import org.springframework.context.annotation.Configuration; | ||||||
import org.springframework.web.servlet.config.annotation.CorsRegistry; | ||||||
import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; | ||||||
|
||||||
@Configuration | ||||||
public class WebConfig implements WebMvcConfigurer { | ||||||
@Override | ||||||
public void addCorsMappings(CorsRegistry registry) { | ||||||
registry.addMapping("/**") | ||||||
.allowedOrigins("https://bankapp.snehaws.com") // Your frontend domain | ||||||
.allowedMethods("GET", "POST", "PUT", "DELETE", "OPTIONS") | ||||||
.allowCredentials(true); | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package com.yourpackage.config; | ||
|
||
import org.springframework.context.annotation.Bean; | ||
import org.springframework.context.annotation.Configuration; | ||
import org.springframework.web.servlet.config.annotation.CorsRegistry; | ||
import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; | ||
|
||
@Configuration | ||
public class WebConfig implements WebMvcConfigurer { | ||
@Override | ||
public void addCorsMappings(CorsRegistry registry) { | ||
registry.addMapping("/**") | ||
.allowedOrigins("https://bankapp.snehaws.com") // Your frontend domain | ||
.allowedMethods("GET", "POST", "PUT", "DELETE", "OPTIONS") | ||
.allowCredentials(true); | ||
Comment on lines
+12
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Fix duplicate CORS configuration files and externalize properties The review comment's suggestion is valid, and additional issues were found:
Action items:
app.cors.allowed-origin=https://bankapp.snehaws.com
@Value("${app.cors.allowed-origin}")
private String allowedOrigin;
@Override
public void addCorsMappings(CorsRegistry registry) {
registry.addMapping("/**")
.allowedOrigins(allowedOrigin)
.allowedMethods("GET", "POST", "PUT", "DELETE", "OPTIONS")
.allowCredentials(true);
} 🔗 Analysis chainReview security implications of CORS with credentials While the CORS configuration is necessary, enabling
Consider extracting the origin URL to a configuration property: - .allowedOrigins("https://bankapp.snehaws.com")
+ @Value("${app.cors.allowed-origin}")
+ private String allowedOrigin;
+ // ...
+ .allowedOrigins(allowedOrigin) 🏁 Scripts executedThe following scripts were executed for the analysis: Error: No shell scripts found Script: #!/bin/bash
# Search for CORS configurations and related properties
echo "=== Searching for CORS configurations ==="
rg -l "addMapping|allowedOrigins|WebMvcConfigurer|CorsRegistry" --type java
echo -e "\n=== Searching for application properties/yaml files ==="
fd "application.*\.(properties|ya?ml)$"
echo -e "\n=== Checking for CORS-related properties ==="
rg -i "cors" --type properties --type yaml
echo -e "\n=== Looking for @Value annotations ==="
ast-grep --pattern '@Value("${$_}")' --lang java
Length of output: 820 Script: #!/bin/bash
echo "=== Content of webconfig.java ==="
cat src/main/java/com/example/bankapp/config/webconfig.java
echo -e "\n=== Content of WebConfig.java ==="
cat src/main/java/com/example/bankapp/config/WebConfig.java
echo -e "\n=== Content of application.properties ==="
cat src/main/resources/application.properties
Length of output: 2328 |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use explicit versioning for container images.
The current image tag
bankappv3
is not immutable and could lead to inconsistency across replicas or during rollbacks.📝 Committable suggestion