Skip to content

feat(website): fix issues with create website with ftp failed #8272

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.

Sign up for GitHub

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

Merged
wanghe-fit2cloud merged 1 commit into from
Mar 28, 2025
Merged

feat(website): fix issues with create website with ftp failed #8272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions agent/app/service/app_install.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,12 @@ func (a *AppInstallService) Operate(req request.AppInstalledOperate) error {
}
return syncAppInstallStatus(&install, false)
case constant.Restart:
if install.App.Key == "openresty" {
websites, _ := websiteRepo.GetBy()
if len(websites) > 0 {
_ = createAllWebsitesWAFConfig(websites)
}
}
out, err := compose.Restart(dockerComposePath)
if err != nil {
return handleErr(install, err, out)
Expand Down
29 changes: 17 additions & 12 deletions agent/app/service/runtime_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,20 +331,25 @@ func buildRuntime(runtime *model.Runtime, oldImageID string, oldEnv string, rebu
if out, err := compose.Up(composePath); err != nil {
runtime.Status = constant.StatusStartErr
runtime.Message = out
} else {
extensions := getRuntimeEnv(runtime.Env, "PHP_EXTENSIONS")
if extensions != "" {
installCmd := fmt.Sprintf("docker exec -i %s %s %s", runtime.ContainerName, "install-ext", extensions)
err = cmd2.ExecWithLogFile(installCmd, 60*time.Minute, logPath)
if err != nil {
runtime.Status = constant.StatusError
runtime.Message = buserr.New("ErrImageBuildErr").Error() + ":" + err.Error()
_ = runtimeRepo.Save(runtime)
return
}
_ = runtimeRepo.Save(runtime)
}
extensions := getRuntimeEnv(runtime.Env, "PHP_EXTENSIONS")
if extensions != "" {
installCmd := fmt.Sprintf("docker exec -i %s %s %s", runtime.ContainerName, "install-ext", extensions)
err = cmd2.ExecWithLogFile(installCmd, 60*time.Minute, logPath)
if err != nil {
runtime.Status = constant.StatusError
runtime.Message = buserr.New("ErrImageBuildErr").Error() + ":" + err.Error()
_ = runtimeRepo.Save(runtime)
return
}
runtime.Status = constant.StatusRunning
}
if out, err := compose.DownAndUp(composePath); err != nil {
runtime.Status = constant.StatusStartErr
runtime.Message = out
_ = runtimeRepo.Save(runtime)
}
runtime.Status = constant.StatusRunning
}
_ = runtimeRepo.Save(runtime)
}
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Mar 28, 2025

Choose a reason for hiding this comment

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

There is an issue with the logic inside the if block before saving the runtime status to running if no errors occur up to that point. Specifically:

  1. The function should not call _runtimeRepo.Save(runtime) inside both branches of the conditional statement (before and after the installation command) since it may overwrite changes made in one branch.

  2. After processing the installation command, there might be some additional cleanup needed (depending on what compose.DownAndUp() does), but this part needs clarification based on the actual implementation.

For optimization, consider moving any operations unrelated to runtime state updates above the conditional statements to reduce redundancy. Additionally, ensure error handling when calling downAndUp() since downgrading and upgrading the container can fail as well.

Expand Down
37 changes: 15 additions & 22 deletions agent/app/service/website.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,26 +410,6 @@ func (w WebsiteService) CreateWebsite(create request.WebsiteCreate) (err error)
website.ParentWebsiteID = parentWebsite.ID
}

if len(create.FtpUser) != 0 && len(create.FtpPassword) != 0 {
createFtpUser := func(t *task.Task) error {
indexDir := GetSitePath(*website, SiteIndexDir)
itemID, err := NewIFtpService().Create(dto.FtpCreate{User: create.FtpUser, Password: create.FtpPassword, Path: indexDir})
if err != nil {
createTask.Log(fmt.Sprintf("create ftp for website failed, err: %v", err))
}
website.FtpID = itemID
return nil
}
deleteFtpUser := func(t *task.Task) {
if website.FtpID > 0 {
if err = NewIFtpService().Delete(dto.BatchDeleteReq{Ids: []uint{website.FtpID}}); err != nil {
createTask.Log(err.Error())
}
}
}
createTask.AddSubTask(i18n.GetWithName("ConfigFTP", create.FtpUser), createFtpUser, deleteFtpUser)
}

configNginx := func(t *task.Task) error {
if err = configDefaultNginx(website, domains, appInstall, runtime); err != nil {
return err
Expand Down Expand Up @@ -488,6 +468,19 @@ func (w WebsiteService) CreateWebsite(create request.WebsiteCreate) (err error)
createTask.AddSubTaskWithIgnoreErr(i18n.GetMsgByKey("EnableSSL"), enableSSL)
}

if len(create.FtpUser) != 0 && len(create.FtpPassword) != 0 {
createFtpUser := func(t *task.Task) error {
indexDir := GetSitePath(*website, SiteIndexDir)
itemID, err := NewIFtpService().Create(dto.FtpCreate{User: create.FtpUser, Password: create.FtpPassword, Path: indexDir})
if err != nil {
return err
}
website.FtpID = itemID
return nil
}
createTask.AddSubTaskWithIgnoreErr(i18n.GetWithName("ConfigFTP", create.FtpUser), createFtpUser)
}

return createTask.Execute()
}

Expand Down Expand Up @@ -2061,7 +2054,7 @@ func (w WebsiteService) GetPathAuthBasics(req request.NginxAuthReq) (res []respo
}
directives := config.Directives
location, _ := directives[0].(*components.Location)
pathAuth.Path = location.Match
pathAuth.Path = strings.TrimPrefix(location.Match, "^")
passPath := path.Join(passDir, fmt.Sprintf("%s.pass", name))
authContent, err = fileOp.GetContent(passPath)
if err != nil {
Expand Down Expand Up @@ -2130,7 +2123,7 @@ func (w WebsiteService) UpdatePathAuthBasic(req request.NginxPathAuthUpdate) err
directives := config.Directives
location, _ := directives[0].(*components.Location)
location.UpdateDirective("auth_basic_user_file", []string{fmt.Sprintf("/www/sites/%s/path_auth/pass/%s", website.Alias, fmt.Sprintf("%s.pass", req.Name))})
location.ChangePath("~*", req.Path)
location.ChangePath("~*", fmt.Sprintf("^%s", req.Path))
var passwdHash []byte
passwdHash, err = bcrypt.GenerateFromPassword([]byte(req.Password), bcrypt.DefaultCost)
if err != nil {
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Mar 28, 2025

Choose a reason for hiding this comment

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

The provided code has some areas that can be reviewed for better organization, clarity, and performance.

Code Snippet 1

if len(create.FtpUser) != 0 && len(create.FtpPassword) != 0 {
    createFtpUser := func(t *task.Task) error {
        indexDir := GetSitePath(*website, SiteIndexDir)
        itemID, err := NewIFtpService().Create(dto.FtpCreate{User: create.FtpUser, Password: create.FtpPassword, Path: indexDir})
        if err != nil {
            t.Log(fmt.Sprintf("create ftp for website failed, err: %v", err))
        }
        website.FtpID = itemID
        return nil
    }

    deleteFtpUser := func(t *task.Task) {
        if website.FtpID > 0 {
            if err = NewIFtpService().Delete(dto.BatchDeleteReq{Ids: []uint{website.FtpID}}); err != nil {
                t.Log(err.Error())
            }
        }
    }

    createTask.AddSubTask(i18n.GetWithName("ConfigFTP", create.FtpUser), createFtpUser, deleteFtpUser)
}

Recommendations:

  • Function Simplification: Consider combining createFtpUser and deleteFtpUser into a single function that handles both operations simultaneously.
func ensureFtpUserExists(t context.Context) func(ctx context.Context) error {
    return func(ctx context.Context) error {
        user := dto.FtpCreate{
            User:   create.FtpUser,
            Password: create.FtpPassword,
            Path:     GetSitePath(*website, SiteIndexDir),
        }
        
        itemID, err := NewIFtpService().Create(user)
        if err != nil {
            return err
        }
        
        website.FtpID = itemID
        
        // Cleanup on failure
        defer cleanupFunc(itemID)

        return nil
    }
}

func cleanupFunc(id uint) {
    if id > 0 {
        if err := NewIFtpService().Delete(dto.BatchDeleteReq{Ids: []uint{id}}); err != nil {
            log.Printf("Unable to clean up FTP resource: %v\n", err)
        }
    }
}
  • Context Use: It's generally recommended to use contexts instead of task execution directly inside tasks. Ensure all functions used accept and handle contexts properly.
  1. Log Formatting: Improve logging formatting to ensure consistency across different environments (t.Log() should be replaced with consistent logging methods according to specific project standards).

Code Snippet 2

if len(create.FtpUser) != 0 && len(create.FtpPassword) != 0 {
    createFtpUser := func(t *task.Task) error {
        ...
    }

    createTask.AddSubTask(i18n.GetWithName("ConfigFTP", create.FtpUser), createFtpUser)
}

No significant changes here but the pattern is similar to snippet 1.

General Recommendations

  1. Consistent Structure: Encourage adhering to a standard structure like one of the popular Go coding patterns such as interfaces, structs, or modules.

  2. Unit Test Coverage: Always test edge cases including empty strings, null values, etc., especially where new code introduces logic flow.

  3. Code readability: Avoid deeply nested blocks unless absolutely necessary. Refactor long functions/functions with many parameters if they make it hard to understand at glance.

  4. Optimization Tips:

    • Minimize repeated calls to expensive operation like database queries within loops unless profiling indicates otherwise.
    • Utilize buffered channels or go-routines efficiently to perform parallel operations wherever feasible and safe.

  5. Avoid Global Variables: If you need shared data between functions, consider using package-level variables cautiously after careful analysis to avoid race conditions.

  6. Linter Checks: Regularly run linters (like golint, gosec) to identify and fix style inconsistencies before submitting for review.

Overall Feedback: The code seems well-structured except for minor inefficiencies and inconsistent logging practices which could impact maintainability and debugging effort. Suggestions focus on improving code cleanliness, consistency, and efficiency.

Expand Down
2 changes: 0 additions & 2 deletions agent/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@ import (
"github.com/1Panel-dev/1Panel/agent/init/viper"
"github.com/1Panel-dev/1Panel/agent/utils/encrypt"

_ "net/http/pprof"

"github.com/gin-gonic/gin"
)

Expand Down
12 changes: 12 additions & 0 deletions agent/utils/nginx/dumper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,18 @@ func DumpBlock(b components.IBlock, style *Style, startLine int) string {
}

directives := b.GetDirectives()

var sortDirectives []components.IDirective
var proxyIncludes []components.IDirective
for _, directive := range directives {
if directive.GetName() == "include" && strings.Contains(strings.Join(directive.GetParameters(), " "), "/proxy/") {
proxyIncludes = append(proxyIncludes, directive)
} else {
sortDirectives = append(sortDirectives, directive)
}
}
directives = append(sortDirectives, proxyIncludes...)

for i, directive := range directives {

if directive.GetLine() > line {
Expand Down
8 changes: 4 additions & 4 deletions core/app/service/setting.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,19 +517,19 @@ func (u *SettingService) GenerateApiKey() (string, error) {
}

func (u *SettingService) UpdateApiConfig(req dto.ApiInterfaceConfig) error {
if err := settingRepo.Update("ApiInterfaceStatus", req.ApiInterfaceStatus); err != nil {
if err := settingRepo.UpdateOrCreate("ApiInterfaceStatus", req.ApiInterfaceStatus); err != nil {
return err
}
global.Api.ApiInterfaceStatus = req.ApiInterfaceStatus
if err := settingRepo.Update("ApiKey", req.ApiKey); err != nil {
if err := settingRepo.UpdateOrCreate("ApiKey", req.ApiKey); err != nil {
return err
}
global.Api.ApiKey = req.ApiKey
if err := settingRepo.Update("IpWhiteList", req.IpWhiteList); err != nil {
if err := settingRepo.UpdateOrCreate("IpWhiteList", req.IpWhiteList); err != nil {
return err
}
global.Api.IpWhiteList = req.IpWhiteList
if err := settingRepo.Update("ApiKeyValidityTime", req.ApiKeyValidityTime); err != nil {
if err := settingRepo.UpdateOrCreate("ApiKeyValidityTime", req.ApiKeyValidityTime); err != nil {
return err
}
global.Api.ApiKeyValidityTime = req.ApiKeyValidityTime
Expand Down
4 changes: 3 additions & 1 deletion core/init/router/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ func Proxy() gin.HandlerFunc {
currentNode = c.Request.Header.Get("CurrentNode")
}

if strings.HasPrefix(c.Request.URL.Path, "/api/v2/") && !checkSession(c) {
apiReq := c.GetBool("API_AUTH")

if !apiReq && strings.HasPrefix(c.Request.URL.Path, "/api/v2/") && !checkSession(c) {
data, _ := res.ErrorMsg.ReadFile("html/401.html")
c.Data(401, "text/html; charset=utf-8", data)
c.Abort()
Expand Down
1 change: 1 addition & 0 deletions core/middleware/api_auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ func ApiAuth() gin.HandlerFunc {
helper.BadAuth(c, "ErrApiConfigIPInvalid", nil)
return
}
c.Set("API_AUTH", true)
c.Next()
return
} else {
Expand Down
5 changes: 0 additions & 5 deletions frontend/src/global/mimetype.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,11 +355,6 @@ export const Algorithms = [
value: 'least_conn',
placeHolder: i18n.global.t('website.leastConnHelper'),
},
{
label: i18n.global.t('website.leastTime'),
value: 'least_time',
placeHolder: i18n.global.t('website.leastTimeHelper'),
},
];

export const StatusStrategy = [
Expand Down
18 changes: 16 additions & 2 deletions frontend/src/views/website/website/config/basic/auth-basic/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
/>
</ComplexTable>
</el-tab-pane>
<el-tab-pane :label="$t('website.path')">
<el-tab-pane :label="$t('website.path')" v-if="showPath">
<ComplexTable :data="pathData" @search="searchPath" v-loading="loading" :heightDiff="420">
<template #toolbar>
<el-button type="primary" plain @click="openCreate('path')">
Expand Down Expand Up @@ -54,7 +54,13 @@

<script lang="ts" setup name="proxy">
import { Website } from '@/api/interface/website';
import { operateAuthConfig, getAuthConfig, getPathAuthConfig, operatePathAuthConfig } from '@/api/modules/website';
import {
operateAuthConfig,
getAuthConfig,
getPathAuthConfig,
operatePathAuthConfig,
getWebsite,
} from '@/api/modules/website';
import { computed, onMounted, ref } from 'vue';
import i18n from '@/lang';
import Create from './create/index.vue';
Expand All @@ -80,6 +86,7 @@ const createRef = ref();
const enable = ref(false);
const opRef = ref();
const pathData = ref([]);
const showPath = ref(false);

const buttons = [
{
Expand Down Expand Up @@ -207,7 +214,14 @@ const searchAll = () => {
searchPath();
};

const getSiteDetail = async () => {
getWebsite(id.value).then(async (res) => {
showPath.value = res.data.type !== 'proxy' && res.data.type !== 'deployment';
});
};

onMounted(() => {
getSiteDetail();
searchAll();
});
</script>
Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud Mar 28, 2025

Choose a reason for hiding this comment

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

There are several areas where this code can be improved:

1. Remove Unused Code

  • The createRef is not used anywhere after its initialization.
// remove this line: const createRef = ref();

Additionally, the unused methods operateAuthConfig, getAuthConfig, and possibly others might need to be removed as well if they are not referenced elsewhere.

2. Simplify Variable Declarations

Declare variables more clearly when using ref() or even directly assigning values during setup.

const data = ref([]); // instead of let data = [];

const loading = ref(false);

...

onMounted(() => {
    loadData();
});

3. Refactor Complex Logic

If the complex logic related to checking conditions should only run under certain circumstances, consider wrapping it in a separate function with conditional checks within.

function handleShowPath(websiteType) {
  showPath.value = websiteType !== 'proxy' && websiteType !== 'deployment';
}

...
handleShowPath(siteDetail.type);

This makes the code cleaner and easier to maintain.

4. Add More TypeScript Type Checking

Ensure that types are correctly defined and used throughout the component.

interface WebSiteType { /** Define your interface here */ }

const siteDetail: WebSiteType = computed(() => {/* logic to compute siteDetail*/ });

... other similar uses.

5. Ensure Error Handling

Include error handling for APIs like getWebsite and getPathData.

async function loadWebsites() {
   try {
      const response = await getWebsite(id.value);
      pathData.value = response.data;
   } catch (error) {
      console.error("Error fetching websites", error.message);
   }
}
loadWebsites();

These recommendations will help streamline the codebase by removing unnecessary parts, simplifying variable declarations, improving modularity, adding type definitions explicitly, and ensuring robustness through proper error handling.

1 change: 1 addition & 0 deletions frontend/src/views/website/website/config/basic/load-balance/operate/index.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ const acceptParams = async (req: LoadBalanceOperate) => {
});
item.value.servers = servers;
} else {
item.value.name = '';
item.value.servers = [initServer()];
}
open.value = true;
Expand Down
Loading