Skip to content

fix: Fix the firewall startup failure#8141

Merged
f2c-ci-robot[bot] merged 1 commit into
dev-v2from
pr@dev-v2@fix_firewalld_port
Mar 13, 2025
Merged

fix: Fix the firewall startup failure#8141
f2c-ci-robot[bot] merged 1 commit into
dev-v2from
pr@dev-v2@fix_firewalld_port

Conversation

@ssongliu
Copy link
Copy Markdown
Member

No description provided.

@f2c-ci-robot
Copy link
Copy Markdown

f2c-ci-robot Bot commented Mar 13, 2025

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

};

const search = async () => {
loading.value = true;
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There are several changes and potential improvements suggested in the code snippet:

  1. Replaced LicenseDelete with OpDialog:

    • The <LicenseDelete> component has been replaced by <OpDialog>, which supports both searching and submitting actions through props.
    • @search="search" is shared between two instances of OpDialog.
  2. Removed unnecessary ref declaration for delRef:

    • Since the deleted reference (delRef) is not used anymore, it can be removed.
  3. Renamed instance references:

    • opRef has been renamed to opRef2 to avoid confusion or naming conflicts (though this change is more about clarity rather than optimization).
  4. Added conditional rendering for help text using slots in OpDialog:

    • A conditionally rendered help text template inside the slot content helps improve maintainability and readibility.
  5. Simplified button logic for unbinding:

    • The onUnbind function now passes the necessary parameters directly to opRef2.acceptParams(). This enhances code reusability and simplifies error handling if needed.

Overall, these changes make the code cleaner, easier to understand, and potentially more robust in future development. However, additional context such as the purpose of each component, usage pattern within the broader system, and performance considerations might warrant further review.

}
}
data.Hostname = oldContainer.Config.Hostname
data.DNS = oldContainer.HostConfig.DNS
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The provided code does not contain significant irregularities, potential issues, or major optimization opportunities given the current context and requirements. Specifically:

  1. Logic for IPv4/IPv6 Assignment: The changes ensure that if data.Network is not "bridge", both IPv4 and IPv6 addresses are assigned from the Network's IPAM configuration. This is consistent with normal container behavior in most Docker setups.

  2. Handling Different Networks: If data.Network is other than "bridge," the function assigns both IPv4 and IPv6 address properties to the data structure based on the specified network's IPAM settings.

  3. Consistency: The logic maintains consistency between the original assignment and the modified code, handling different configurations gracefully.

Overall, this change will improve the reliability of mapping IPs correctly across various Docker networks, which aligns well with typical use cases and standard practices for container management through Docker APIs.

downloadAccountID: currentRow.value.downloadAccountID,
type: 'redis',
name: database.value,
detailName: '',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code you provided contains some changes related to translations and handling of account information in an El Table component within a Vue.js application. Here are some recommendations:

Translations Issues

  1. Inconsistent Translation Keys: The $t function is used inconsistently between different conditions within the <template> block.

    • In one instance, you use "setting." + row.source" but in another, it's "setting.LOCAL". Consider using more consistent keys or removing unnecessary conditionals.
  2. Missing Translation String: If there's no translation string defined for setting.LOCKED, it might cause runtime errors.

Code Improvements

  1. Remove Unconditional Check:

    -<span v-if="row.source">

    Remove this conditional as it's redundant when checking for existence directly with Elvis operator (??).

  2. Consistent Formatting:
    Align template expressions for better readability. For example:

    <span>{{ (row.accountType || '-') + ' - ' + row.accountName }}</span>
  3. Optimize Data Structure:
    Ensure that downloadAccountID is consistently named throughout your data structure since both tables reference it under its correct property names. However, if there’s a difference in naming conventions, ensure they match correctly across components.

  4. Use Default Values Properly:
    Although already mentioned above, verify that default values like - are handled properly whenever necessary.

Here's how the improved section could look like:

<el-table-column :label="$t('commons.table.name')" show-overflow-tooltip prop="fileName" />
<el-table-column :label="$t('app.source')" prop="backupType">
    <template #default="{ row }">
        <span>{{ $t(row.accountType && row.accountType !== 'LOCAL' ? 'setting.' + row.accountType : row.accountInfo.status !== 'LOCKED' ? 'setting.LOCAL' : '-') }}{{ row.accountInfo.status !== 'LOCKED' && '-' + row.accountName }}</span>
        <!-- Simplified version -->
        <!-- <span>[[ (row.accountType || '-') + ' - ' + (row.accountInfo.status === 'LOCKED' && '') + row.accountName ]]</span>  -->
    </template>
</el-table-column>
<!-- ... rest of the table columns remain unchanged -->

const onRecover = async () => {
...
};

Note: Using `[[]] syntax for interpolation can improve readability; however, always test before production to avoid HTML injection vulnerabilities.

These changes help maintain consistency and make debugging easier. Make sure all translated strings are available and up to date in your project's message files to reduce runtime issues.

@ssongliu ssongliu force-pushed the pr@dev-v2@fix_firewalld_port branch from a70e59a to 75b0d92 Compare March 13, 2025 07:05
};

const search = async () => {
loading.value = true;
Copy link
Copy Markdown
Member

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 changes and improvements that can be made:

Changes:

  1. Renamed LicenseDelete Referenced Component to OpDialog: The component <LicenseDelete> was renamed to <OpDialog>. While this is valid, it's generally better to maintain consistent naming conventions throughout the project.

  2. Added New OpDialog with Checkbox Input: A new variant of <OpDialog> (<OpDialog ref="opRef2">) was added. This dialog now includes a checkbox to enforce unbinding and provides additional content using a template slot. This enhances functionality and user experience.

  3. Removed Redundant Reference for Delete Action: Removed the reference to the delete action button, which seems unnecessary since its functionality is already encapsulated within the new OpDialog.

  4. Added submitUnbind Functionality: Introduced a submitUnbind function tied to the second modal that handles the actual deletion logic when confirmed.

Potential Issues:

  • Data Integrity Check on Unbind: It might lead to issues if multiple users try to unbind simultaneously or without proper locking mechanisms, especially if there could be concurrent updates affecting counts.

  • Favicon Initialization Logic in Prod vs Dev: The favicon initialization code is only executed during development (globalStore.isPro). This may cause inconsistencies across different environments unless explicitly modified to include all environments, such as masterProductPro, depending on your product's architecture.

  • Template Reusability: While useful for modularity, ensure that reusing templates efficiently does not increase render overhead unnecessarily, which would impact performance in larger applications.

Here are specific fixes and optimizations based on these points:

  1. Consistent Naming Conventions:

    - <LicenseDelete ref="delRef" @search="search" />
    + <OpDialog ref="delRef" @search="search" @unbind="onUnbind">
  2. Error Handling in Unbind Operation:
    Ensure error handling in the onUnbind method to gracefully manage exceptions and prevent unexpected behavior:

    - else {
  •   loading.value = false;
    
    }
    Should be updated to something like:
    ```typescript
    } catch (error) {
      loading.value = false;
      console.error("Failed to perform unbind operation", error);
      MsgFail(i18n.global.t('commons.msg.operationFailed'));
    }
    
  1. Addendum to Favicon Initialization:
    To address the issue mentioned above, consider adding checks to enable favicon initialization across all applicable conditions:
    // Assuming you want to support all three states (pro, master Pro, non-pro), update accordingly
    globalStore.isMasterProductPro || globalStore.isPro ? initFavicon() : null;

By addressing these areas, the application becomes more robust and efficient, while adhering to best practices.

}
}
data.Hostname = oldContainer.Config.Hostname
data.DNS = oldContainer.HostConfig.DNS
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code snippet you provided is modifying some properties of a container based on its configuration settings. Here are several observations to consider:

  1. MacAddress Mismatch: There seems to be an issue where data.MacAddr gets overwritten by both bridgeNetworkSettings.MacAddress and another value (oldContainer.NetworkSettings.MacAddress). This could lead to unintended behavior when dealing with multiple networks.

  2. IPv4/IPv6 Handling: The handling of IPv4 and IPv6 addresses differs depending on whether the network is "bridge". If the network is not "bridge", it copies either the IP address from IPV4Address or IPAddress into data.Ipv4, but since bridgeNetworkSettings.IPAMConfig might be nil, this logic doesn't directly follow what was intended in previous versions of the function, which may have been copying only if there's an IPAM configuration present.

  3. Consistency: Without additional context, it's hard to determine if using bridgeNetworkSettings.IPAddress regardless of network type makes sense, given that IP Address typically refers to IPv4 addresses unless specified otherwise.

Optimization Suggestions:

  • Ensure that data.MacAddr should only be set once and correctly reflect something consistent across all cases.
  • Consider adding checks to handle different network types more explicitly without relying solely on conditional statements like "bridge".

Here’s an updated version incorporating these points:

func (u *ContainerService) ContainerInfo(req dto.OperationWithName) (*dto.ContainerInfo, error) {
	oldContainer, err := u.GetContainerById(req.Id)
	if err != nil && errors.Is(err, ErrResourceNotFound) {
		return nil, fmt.Errorf("container not found: %w", err)
	}

	networkSettings := oldContainer.NetworkSettings

	var (
		ipv4Address   *net.IPNet
		ipv6Address *net.IPNet
	)

	for _, netConf := range networkSettings.Networks {
		if netConf.Name == req.Data.Network {
			ipamConfig := netConf.IPAMConfig
			if ipamConfig != nil {
				ipv4Address = ipamConfig.IPv4Address
				ipv6Address = ipamConfig.IPv6Address
			}
		}
	}

	// Decide whether to use IP_Address for non-Bridge networks,
	// or first available IP in case of Bridge networks.
	ip := bridgeNetworkSettings.IPAddress
	if len(ipv4Addresses)+len(ipv6Addresses) > 0 {
		ip = ipv4Address.IP.String() // Choose the most common one or any valid IP
	} else if data.Network == bridge {
		ip = bridgeNetworkSettings.IPAddress
	}

	data.MacAddr = oldContainer.Config.Hostname
	data.Ipv4 = ip
	data.Ipv6 = nil // Remove or update as needed
	data.Hostname = oldContainer.Config.Hostname
	data.DNS = oldContainer.HostConfig.DNS

	return &data, nil
}

This revised approach ensures clearer conditions regarding MacAddress setting and handles both IPv4 and IPv6 separately according to their availability within each network. Note that determining the correct IP depends on specific business requirements; you might need adjustments to suit your application's needs better.

downloadAccountID: currentRow.value.downloadAccountID,
type: 'redis',
name: database.value,
detailName: '',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The provided file differences involve minor adjustments to table column labels and an update in the onRecover function's parameter downloadAccountID. Here are the changes:

  • In the <el-table-column> for "source", the template now includes two new conditions when row.accountType is not 'LOCAL'. It checks if row.accountName exists before concatenating a dash with it, and provides an empty default value (<span v-if="!row.accountType">-</span>) when no account type is set. This ensures that the display never results in null or undefined values.

  • The onRecover function has been updated to use currentRow.value.downloadAccountID instead of currentRow.value.source, assuming this change aligns with your backend API definition.

No major issues have been identified from these changes. However, consider adding some error handling around the calls to functions like onBackup, onRecover, and possibly others, depending on how they interact with user input and network requests. Also, ensure all dependencies (e.g., vue-i18n) are properly imported in your main app.js file to avoid runtime errors due to missing internationalization translations.

@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown
Member

@wanghe-fit2cloud wanghe-fit2cloud left a comment

Choose a reason for hiding this comment

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

/lgtm

@wanghe-fit2cloud
Copy link
Copy Markdown
Member

/approve

@f2c-ci-robot
Copy link
Copy Markdown

f2c-ci-robot Bot commented Mar 13, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wanghe-fit2cloud

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@f2c-ci-robot f2c-ci-robot Bot merged commit 2ed3aaa into dev-v2 Mar 13, 2025
@f2c-ci-robot f2c-ci-robot Bot deleted the pr@dev-v2@fix_firewalld_port branch March 13, 2025 07:09
ssongliu pushed a commit that referenced this pull request Mar 13, 2025
ssongliu pushed a commit that referenced this pull request Mar 13, 2025
ssongliu pushed a commit that referenced this pull request Mar 14, 2025
zhengkunwang223 pushed a commit that referenced this pull request Mar 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants